lkml.org 
[lkml]   [2017]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH RFC V2] fpga: socfpga: handle interrupted case as non-success
On Sun, May 7, 2017 at 3:59 AM, Nicholas Mc Guire <der.herr@hofr.at> wrote:

HI Nicholas,

Thanks for catching this.

> wait_for_completion_interruptible_timeout() can return 0 (timeout)
> or -ERESTARTSYS when interrupted - the current code treats the
> interrupted case as success which is wrong and could lead to hard
> to reproduce errors. Fix this by returning non-0 in both cases
> to the calling side. This also changes the timeout variable to the
> proper type as wait_for_completion_interruptible_timeout() returns
> long not int.
>
> Fixes: commit fab6266e82a8 ("fpga manager: add driver for socfpga fpga manager")
> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> ---
>
> V2: the patch was not yet compile tested as I wanted to clarify the
> below issue first - but it got picked up by kbuild-test anyway
> so here is a compile-tested (and fixed) version - but the key
> issue still is to clarify if the use of the interruptible version
> actually makes sense here at all - I think it should be dropped.
>
> It is not really clear to me why the _interruptible_ version of
> wait_for_completion is in use here - so maybe the proper mitigation
> is to simply change the call to wait_for_completion_timeout() and
> drop the interrupted case all together as the timeout of 10ms is
> not that long that waiting for timeout would be problematic. Someone
> that knows the intent of selecting the _interruptible_ call would
> need to check if this is a more reasonable solution.

It's just waiting for the isr to complete.

Re: using wait_for_completion_timeout instead: programming the FPGA
may be started by userspace. If userspace wants to signal the task,
10mSec may not be a bad wait, I"m not sure. I haven't found clear
guidelines are regarding this.

Alan

>
> Patch was compile tested with: socfpga_defconfig (implies CONFIG_FPGA_MGR_SOCFPGA)
>
> Patch is against v4.11 (localversion-next is next-20170505)
>
> drivers/fpga/socfpga.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index b6672e6..532abd6 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -312,7 +312,8 @@ static irqreturn_t socfpga_fpga_isr(int irq, void *dev_id)
>
> static int socfpga_fpga_wait_for_config_done(struct socfpga_fpga_priv *priv)
> {
> - int timeout, ret = 0;
> + int ret = 0;
> + long timeout;
>
> socfpga_fpga_disable_irqs(priv);
> init_completion(&priv->status_complete);
> @@ -323,6 +324,8 @@ static int socfpga_fpga_wait_for_config_done(struct socfpga_fpga_priv *priv)
> msecs_to_jiffies(10));
> if (timeout == 0)
> ret = -ETIMEDOUT;
> + else if (timeout == -ERESTARTSYS)
> + ret = -ERESTARTSYS;
>
> socfpga_fpga_disable_irqs(priv);
> return ret;
> --
> 2.1.4
>

\
 
 \ /
  Last update: 2017-05-10 21:22    [W:0.034 / U:36.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site