From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754062AbdEJQGa (ORCPT ); Wed, 10 May 2017 12:06:30 -0400 Received: from mail.kernel.org ([198.145.29.136]:53228 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752755AbdEJQG3 (ORCPT ); Wed, 10 May 2017 12:06:29 -0400 MIME-Version: 1.0 In-Reply-To: <1494147580-1391-1-git-send-email-der.herr@hofr.at> References: <1494147580-1391-1-git-send-email-der.herr@hofr.at> From: Alan Tull Date: Wed, 10 May 2017 11:05:43 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH RFC V2] fpga: socfpga: handle interrupted case as non-success To: Nicholas Mc Guire Cc: Moritz Fischer , linux-fpga@vger.kernel.org, linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, May 7, 2017 at 3:59 AM, Nicholas Mc Guire 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 > --- > > 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 >