Re: [PATCH RFC V2] fpga: socfpga: handle interrupted case as non-success

[Alan Tull]

On Sun, May 7, 2017 at 3:59 AM, Nicholas Mc Guire <der.herr@xxxxxxx> 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@xxxxxxx>
> ---
>
> 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
>