Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752613AbdGEIHh (ORCPT ); Wed, 5 Jul 2017 04:07:37 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:9353 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751444AbdGEIHc (ORCPT ); Wed, 5 Jul 2017 04:07:32 -0400 Date: Wed, 5 Jul 2017 16:06:47 +0800 From: Jonathan Cameron To: Nicholas Mc Guire CC: Jonathan Cameron , Geert Uytterhoeven , Hartmut Knaack , "Lars-Peter Clausen" , Peter Meerwald-Stadler , simran singhal , Arnd Bergmann , Gregor Boirie , , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH RFC] iio: pressure: zpa2326: report interrupted case as failure Message-ID: <20170705160647.00001913@huawei.com> In-Reply-To: <20170705073705.GA8145@osadl.at> References: <1494751435-14189-1-git-send-email-der.herr@hofr.at> <20170704200853.16dc9e3a@kernel.org> <20170705073705.GA8145@osadl.at> Organization: Huawei X-Mailer: Claws Mail 3.15.0 (GTK+ 2.24.31; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.206.48.115] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A0B0204.595C9E2D.0128,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 9379edbd1970ab817c781ddf6fe14ff7 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4380 Lines: 115 On Wed, 5 Jul 2017 07:37:05 +0000 Nicholas Mc Guire wrote: > On Tue, Jul 04, 2017 at 08:08:53PM +0100, Jonathan Cameron wrote: > > On Tue, 4 Jul 2017 12:40:33 +0200 > > Geert Uytterhoeven wrote: > > > > > Hi Nicholas, > > > > > > On Sun, May 14, 2017 at 10:43 AM, Nicholas Mc Guire > > > wrote: > > > > If the timeout-case prints a warning message then probably the > > > > interrupted case should also. Further, > > > > wait_for_completion_interruptible_timeout() returns long not > > > > int. > > > > > > > > Fixes: commit 03b262f2bbf4 ("iio:pressure: initial zpa2326 > > > > barometer support") Signed-off-by: Nicholas Mc Guire > > > > --- > > > > > > > > The original control-flow was technically not wrong just > > > > confusing and a bit complicated. Not clear if reporting the > > > > interrupted case actually is useful, but given that the timeout > > > > is relatively long (200ms) it is not that unlikely so > > > > differentiating the cases seems helpful. > > > > > > > > Patch was compile-tested with: x86_64_defconfig + CONFIG_IIO=m, > > > > CONFIG_ZPA2326=m > > > > > > > > Patch is against v4.11 (localversion-next is next-20170512) > > > > > > > > drivers/iio/pressure/zpa2326.c | 17 ++++++++++------- > > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/iio/pressure/zpa2326.c > > > > b/drivers/iio/pressure/zpa2326.c index e58a0ad..617926f 100644 > > > > --- a/drivers/iio/pressure/zpa2326.c > > > > +++ b/drivers/iio/pressure/zpa2326.c > > > > @@ -867,12 +867,13 @@ static int > > > > zpa2326_wait_oneshot_completion(const struct iio_dev > > > > *indio_dev, { int ret; > > > > unsigned int val; > > > > + long timeout; > > > > > > > > zpa2326_dbg(indio_dev, "waiting for one shot completion > > > > interrupt"); > > > > > > > > - ret = wait_for_completion_interruptible_timeout( > > > > + timeout = wait_for_completion_interruptible_timeout( > > > > &private->data_ready, > > > > ZPA2326_CONVERSION_JIFFIES); > > > > - if (ret > 0) > > > > + if (timeout > 0) > > > > > > Check for strict positive timeout. > > > > > > > /* > > > > * Interrupt handler completed before timeout: > > > > return operation > > > > * status. > > > > @@ -882,13 +883,15 @@ static int > > > > zpa2326_wait_oneshot_completion(const struct iio_dev > > > > *indio_dev, /* Clear all interrupts just to be sure. */ > > > > regmap_read(private->regmap, ZPA2326_INT_SOURCE_REG, &val); > > > > > > > > - if (!ret) > > > > + if (!timeout) { > > > > > > Check for zero timeout. > > > > > > > /* Timed out. */ > > > > + zpa2326_warn(indio_dev, "no one shot interrupt > > > > occurred (%ld)", > > > > + timeout); > > > > ret = -ETIME; > > > > - > > > > - if (ret != -ERESTARTSYS) > > > > - zpa2326_warn(indio_dev, "no one shot interrupt > > > > occurred (%d)", > > > > - ret); > > > > + } else if (timeout < 0) { > > > > > > So if we get here, timeout is always strict negative, so the > > > check can be removed. > > > > > > > + zpa2326_warn(indio_dev, "wait for one shot > > > > interrupt canceled"); > > > > + ret = -ERESTARTSYS; > > > > + } > > > > > > > > return ret; > > > > > > But gcc-4.1.2 is not smart enough: > > > > > > drivers/iio/pressure/zpa2326.c:868: warning: ???ret??? may be used > > > uninitialized in this function > > Good analysis. Care to send the obvious patch? > > > Thanks Geert for finding that - yes ret needs to be > initialized to 0 here, success case as documented in > the header of zpa2326_wait_oneshot_completion - > interestingly enough gcc gcc (Debian 4.9.2-10) 4.9.2 > does not flag this uninitioalized variable ! I think Geert's analysis also suggests that you can just drop the last conditional without changing the code. gcc should be fine with that Jonathan > > thx! > hofrat > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" > in the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html