| Message ID | 1352222317.15558.1584.camel@cliu38-desktop-build (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 07, 2012 at 01:18:37AM +0800, Chuansheng Liu wrote: > > The return value of wait_for_completion_timeout() is always > >= 0 with unsigned int type. > > So the condition "ret < 0" or "ret >= 0" is pointless. > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> You forgot to add the i2c-list, so I nearly overlooked this patch. Luckily I haven't, so applied to for-next, thanks! Did you use a script to find all occurences? Would be nice to know. Also, please take care of the subject line. "I2c" is misspelled. @Linus: I am not pushing out today, so if you want to add acks or so... Thanks, Wolfram > --- > drivers/i2c/busses/i2c-nomadik.c | 14 -------------- > 1 files changed, 0 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c > index 02c3115..8b2ffcf 100644 > --- a/drivers/i2c/busses/i2c-nomadik.c > +++ b/drivers/i2c/busses/i2c-nomadik.c > @@ -435,13 +435,6 @@ static int read_i2c(struct nmk_i2c_dev *dev, u16 flags) > timeout = wait_for_completion_timeout( > &dev->xfer_complete, dev->adap.timeout); > > - if (timeout < 0) { > - dev_err(&dev->adev->dev, > - "wait_for_completion_timeout " > - "returned %d waiting for event\n", timeout); > - status = timeout; > - } > - > if (timeout == 0) { > /* Controller timed out */ > dev_err(&dev->adev->dev, "read from slave 0x%x timed out\n", > @@ -523,13 +516,6 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags) > timeout = wait_for_completion_timeout( > &dev->xfer_complete, dev->adap.timeout); > > - if (timeout < 0) { > - dev_err(&dev->adev->dev, > - "wait_for_completion_timeout " > - "returned %d waiting for event\n", timeout); > - status = timeout; > - } > - > if (timeout == 0) { > /* Controller timed out */ > dev_err(&dev->adev->dev, "write to slave 0x%x timed out\n", > -- > 1.7.0.4 > > >
> -----Original Message----- > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > Sent: Tuesday, November 13, 2012 1:27 AM > To: Liu, Chuansheng > Cc: linus.walleij@linaro.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org > Subject: Re: [PATCH 1/7] I2c-nomadik: Fix the usage of > wait_for_completion_timeout > > On Wed, Nov 07, 2012 at 01:18:37AM +0800, Chuansheng Liu wrote: > > > > The return value of wait_for_completion_timeout() is always > > >= 0 with unsigned int type. > > > > So the condition "ret < 0" or "ret >= 0" is pointless. > > > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > > You forgot to add the i2c-list, so I nearly overlooked this patch. > Luckily I haven't, so applied to for-next, thanks! Did you use a script > to find all occurences? Would be nice to know. Also, please take care of > the subject line. "I2c" is misspelled. Thanks your pointing out. I just manually reviewed the code when found this point. Would like to try some auto-script but failed:)
> Thanks your pointing out. I just manually reviewed the code when found > this point. Would like to try some auto-script but failed:) I can suggest 'coccinelle' for that kind of job.
> -----Original Message----- > From: Wolfram Sang [mailto:w.sang@pengutronix.de] > Sent: Tuesday, November 13, 2012 3:58 PM > To: Liu, Chuansheng > Cc: linus.walleij@linaro.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org > Subject: Re: [PATCH 1/7] I2c-nomadik: Fix the usage of > wait_for_completion_timeout > > > > Thanks your pointing out. I just manually reviewed the code when found > > this point. Would like to try some auto-script but failed:) > > I can suggest 'coccinelle' for that kind of job. Thanks, let me try, if possible anyone can give some help? > > -- > Pengutronix e.K. | Wolfram Sang > | > Industrial Linux Solutions | http://www.pengutronix.de/ > |
[...] > From: Chuansheng Liu <chuansheng.liu@intel.com> > Date: Tue, Nov 6, 2012 at 6:18 PM > Subject: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout > To: linus.walleij@linaro.org, w.sang@pengutronix.de > Cc: linux-arm-kernel@lists.infradead.org, > linux-kernel@vger.kernel.org, chuansheng.liu@intel.com > > > > The return value of wait_for_completion_timeout() is always > >= 0 with unsigned int type. > > So the condition "ret < 0" or "ret >= 0" is pointless. > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > --- > drivers/i2c/busses/i2c-nomadik.c | 14 -------------- > 1 files changed, 0 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c > index 02c3115..8b2ffcf 100644 > --- a/drivers/i2c/busses/i2c-nomadik.c > +++ b/drivers/i2c/busses/i2c-nomadik.c > @@ -435,13 +435,6 @@ static int read_i2c(struct nmk_i2c_dev *dev, u16 flags) > timeout = wait_for_completion_timeout( > &dev->xfer_complete, dev->adap.timeout); > > - if (timeout < 0) { > - dev_err(&dev->adev->dev, > - "wait_for_completion_timeout " > - "returned %d waiting for event\n", timeout); > - status = timeout; > - } > - No, it is wrong. You need to update the status variable in the case of timeout. It is used further in nmk_i2c_xfer_one. You could perhaps use if (timeout == 0) { ...and the rest of the code as is } regards/srinidhi
> > - if (timeout < 0) { > > - dev_err(&dev->adev->dev, > > - "wait_for_completion_timeout " > > - "returned %d waiting for event\n", timeout); > > - status = timeout; > > - } > > - > No, it is wrong. You need to update the status variable in the case of timeout. Looking at the patch context, such code comes later.
On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote: > > > > - if (timeout < 0) { > > > - dev_err(&dev->adev->dev, > > > - "wait_for_completion_timeout " > > > - "returned %d waiting for event\n", timeout); > > > - status = timeout; > > > - } > > > - > > No, it is wrong. You need to update the status variable in the case of timeout. > > Looking at the patch context, such code comes later. But it causes regressions; without looking at the "later" code, we can't afford merging this code now. regards/srinidhi
On Thu, Nov 15, 2012 at 03:27:42PM +0530, Srinidhi Kasagar wrote: > On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote: > > > > > > - if (timeout < 0) { > > > > - dev_err(&dev->adev->dev, > > > > - "wait_for_completion_timeout " > > > > - "returned %d waiting for event\n", timeout); > > > > - status = timeout; > > > > - } > > > > - > > > No, it is wrong. You need to update the status variable in the case of timeout. > > > > Looking at the patch context, such code comes later. > But it causes regressions; without looking at the "later" code, we can't afford merging > this code now. Later as in "a few lines later" not "some time later". Or am I missing something else?
timeout is never < 0, so status = timeout is never getting run. This patch delete pointless code. Could you show the regression? Thanks. ? 2012-11-15?17:57?Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> ??? > On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote: >> >>>> - if (timeout < 0) { >>>> - dev_err(&dev->adev->dev, >>>> - "wait_for_completion_timeout " >>>> - "returned %d waiting for event\n", timeout); >>>> - status = timeout; >>>> - } >>>> - >>> No, it is wrong. You need to update the status variable in the case of timeout. >> >> Looking at the patch context, such code comes later. > But it causes regressions; without looking at the "later" code, we can't afford merging > this code now. > > regards/srinidhi > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Thu, Nov 15, 2012 at 11:18:20 +0100, Wolfram Sang wrote: > On Thu, Nov 15, 2012 at 03:27:42PM +0530, Srinidhi Kasagar wrote: > > On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote: > > > > > > > > - if (timeout < 0) { > > > > > - dev_err(&dev->adev->dev, > > > > > - "wait_for_completion_timeout " > > > > > - "returned %d waiting for event\n", timeout); > > > > > - status = timeout; > > > > > - } > > > > > - > > > > No, it is wrong. You need to update the status variable in the case of timeout. > > > > > > Looking at the patch context, such code comes later. > > But it causes regressions; without looking at the "later" code, we can't afford merging > > this code now. > > Later as in "a few lines later" not "some time later". Or am I missing > something else? I was too fast in reading emails after my short vacation...Sorry. Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com> regards/srinidhi
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index 02c3115..8b2ffcf 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -435,13 +435,6 @@ static int read_i2c(struct nmk_i2c_dev *dev, u16 flags) timeout = wait_for_completion_timeout( &dev->xfer_complete, dev->adap.timeout); - if (timeout < 0) { - dev_err(&dev->adev->dev, - "wait_for_completion_timeout " - "returned %d waiting for event\n", timeout); - status = timeout; - } - if (timeout == 0) { /* Controller timed out */ dev_err(&dev->adev->dev, "read from slave 0x%x timed out\n", @@ -523,13 +516,6 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags) timeout = wait_for_completion_timeout( &dev->xfer_complete, dev->adap.timeout); - if (timeout < 0) { - dev_err(&dev->adev->dev, - "wait_for_completion_timeout " - "returned %d waiting for event\n", timeout); - status = timeout; - } - if (timeout == 0) { /* Controller timed out */ dev_err(&dev->adev->dev, "write to slave 0x%x timed out\n",