Messages in this thread | | | From | Alan Tull <> | Date | Wed, 10 May 2017 11:05:43 -0500 | Subject | Re: [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 >
| |