Re: [PATCH] I2C update for 2.6.9

[Eugene Surovegin]

On Tue, Oct 19, 2004 at 05:18:26PM -0700, Greg Kroah-Hartman wrote:
> ChangeSet 1.2072, 2004/10/19 15:21:15-07:00, nacc@xxxxxxxxxx
> 
> [PATCH] I2C: replace schedule_timeout() with msleep_interruptible() in i2c-ibm_iic.c
> 
> Use msleep_interruptible() instead of schedule_timeout() to
> guarantee the task delays as expected. Remove the unnecessary
> set_current_state() following the if, as schedule_timeout() [and thus,
> mlseep_interruptible()] is guaranteed to return in TASK_RUNNING.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@xxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <greg@xxxxxxxxx>
> 
> 
>  drivers/i2c/busses/i2c-ibm_iic.c |    4 +---
>  1 files changed, 1 insertion(+), 3 deletions(-)
> 
> 
> diff -Nru a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
> --- a/drivers/i2c/busses/i2c-ibm_iic.c	2004-10-19 16:53:58 -07:00
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c	2004-10-19 16:53:58 -07:00
> @@ -416,10 +416,8 @@
>      		init_waitqueue_entry(&wait, current);
>  		
>  		add_wait_queue(&dev->wq, &wait);
> -		set_current_state(TASK_INTERRUPTIBLE);
>  		if (in_8(&iic->sts) & STS_PT)
> -			schedule_timeout(dev->adap.timeout * HZ);
> -		set_current_state(TASK_RUNNING);
> +			msleep_interruptible(dev->adap.timeout * 1000);
>  		remove_wait_queue(&dev->wq, &wait);
>  		
>  		if (unlikely(signal_pending(current))){

It looks like this change added race I tried to avoid here.

This code is modeled after __wait_event_interruptible_timeout, where 
"prepare_to_wait" is done _before_ checking completion status. This 
change breaks this, e.g. if IRQ happens right after we check iic->sts, 
but before calling msleep_interruptible(). In this case we'll sleep 
much more than required (seconds instead of microseconds)

Greg, if my analysis is correct, please rollback this change.

Nishanth, I'd be nice if you CC'ed me with this patch, my e-mail is at 
the top of that source file.

--
Eugene


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/