[RFC,V2] fpga: socfpga: handle interrupted case as non-success
diff mbox

Message ID 1494147580-1391-1-git-send-email-der.herr@hofr.at
State New
Headers show

Commit Message

Nicholas Mc Guire May 7, 2017, 8:59 a.m. UTC
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.

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(-)

Comments

Alan Tull May 10, 2017, 4:05 p.m. UTC | #1
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;