Re: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

Re: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Liu, Chuansheng]

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

Re: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]


> 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.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Liu, Chuansheng]

> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> Sent: Tuesday, November 13, 2012 3:58 PM
> To: Liu, Chuansheng
> Cc: linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org;
> linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.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/
> |

Re: Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Srinidhi Kasagar]

[...]

> 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

Re: Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]


> > -       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.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Srinidhi Kasagar]

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

Re: Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

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?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Chuansheng Liu]

[-- Attachment #1.1: Type: text/plain, Size: 1118 bytes --]

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/

[-- Attachment #1.2: Type: text/html, Size: 3802 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Srinidhi Kasagar]

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-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>

regards/srinidhi