Calling wait_event_interruptible_timeout() in I2C wait functions

Calling wait_event_interruptible_timeout() in I2C wait functions

[Timur Tabi]

In i2c-mpc.c, the i2c_wait() function has this:

	} else {
		/* Interrupt mode */
		result = wait_event_interruptible_timeout(i2c->queue,
			(i2c->interrupt & CSR_MIF), timeout * HZ);

		if (unlikely(result < 0)) {
			pr_debug("I2C: wait interrupted\n");
			writeccr(i2c, 0);
		} else if (unlikely(!(i2c->interrupt & CSR_MIF))) {

That is, the driver calls wait_event_interruptible_timeout() to wait for
a response from the I2C controller after a read or write operation.

However, it appears that this is not common behavior for I2C driver.  In
fact, only these six drivers ever call wait_event_interruptible_timeout():

i2c-cpm.c
i2c-ibm_iic.c
i2c-mpc.c
i2c-taos-evm.c
i2c-iop3xx.c
i2c-mv64xxx.c

Although one would think that calling wait_event_interruptible_timeout()
is a good idea, it fails in one situation: when the abrupt termination
of a process causes an I2C operation to occur.  That is, you press ^C in
your application, and the driver issues a final I2C operation to shut
down the device.  In this situation, wait_event_interruptible_timeout()
returns -ERESTARTSYS, which is then passed up through
i2c_smbus_write_byte_data().

So my question is, is i2c-mpc.c wrong in using
wait_event_interruptible_timeout()?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Calling wait_event_interruptible_timeout() in I2C wait functions

[Mike Ditto]

Timur Tabi wrote:
> However, it appears that this is not common behavior for I2C driver.  In
> fact, only these six drivers ever call wait_event_interruptible_timeout():
> 
> i2c-cpm.c

I don't know about the others, but in i2c-cpm.c the use of interruptible
wait seems incorrect.  Maybe it could be made correct, but as is, it
does not correctly clean up the hardware state or return a useful
value when interrupted by a signal.  It's not clear what to do, anyway -
it's hard to know which messages of the interrupted transaction have
actually taken effect in the hardware.  I think it's better to use
uninterruptible wait here and just live with the delayed signal
handling (one second delay in the unlikely worst case for i2c-cpm).

In general, I think it's best to consider I2C I/O to be uninterruptible,
like disk I/O.  The only reason to make it interruptible is to make
sure you don't end up with an unkillable process due to an I/O error,
and that is adequately handled by a timeout (and re-initialization of
the I2C interface in that case).

					-=] Mike [=-

Re: Calling wait_event_interruptible_timeout() in I2C wait functions

[Mike Ditto]

Timur Tabi wrote:
> However, it appears that this is not common behavior for I2C driver.  In
> fact, only these six drivers ever call wait_event_interruptible_timeout():
> 
> i2c-cpm.c

I don't know about the others, but in i2c-cpm.c the use of interruptible
wait seems incorrect.  Maybe it could be made correct, but as is, it
does not correctly clean up the hardware state or return a useful
value when interrupted by a signal.  It's not clear what to do, anyway -
it's hard to know which messages of the interrupted transaction have
actually taken effect in the hardware.  I think it's better to use
uninterruptible wait here and just live with the delayed signal
handling (one second delay in the unlikely worst case for i2c-cpm).

In general, I think it's best to consider I2C I/O to be uninterruptible,
like disk I/O.  The only reason to make it interruptible is to make
sure you don't end up with an unkillable process due to an I/O error,
and that is adequately handled by a timeout (and re-initialization of
the I2C interface in that case).

					-=] Mike [=-

Re: Calling wait_event_interruptible_timeout() in I2C wait functions

[Mark Brown]

On Wed, Feb 04, 2009 at 12:00:52PM -0800, Mike Ditto wrote:
> Timur Tabi wrote:
> > However, it appears that this is not common behavior for I2C driver.  In
> > fact, only these six drivers ever call wait_event_interruptible_timeout():

> > i2c-cpm.c

> I don't know about the others, but in i2c-cpm.c the use of interruptible
> wait seems incorrect.  Maybe it could be made correct, but as is, it
> does not correctly clean up the hardware state or return a useful
> value when interrupted by a signal.  It's not clear what to do, anyway -

This is exactly the problem for users that caused Timur to run into this
- further up the stack we're trying to do cleanup that involves writing
via I2C but the I2C writes error out due to the signal.

> handling (one second delay in the unlikely worst case for i2c-cpm).

Ditto for the Freescale PowerPC I2C driver.

Re: Calling wait_event_interruptible_timeout() in I2C wait functions

[Timur Tabi]

On Thu, Feb 5, 2009 at 5:51 AM, Mark Brown <broonie-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> wrote:

> This is exactly the problem for users that caused Timur to run into this
> - further up the stack we're trying to do cleanup that involves writing
> via I2C but the I2C writes error out due to the signal.

Well, there's not much discussion on this issue, so I'm going to make
the change to an uninterruptible wait and see if it fixes my problem.
If so, I'll post a patch for i2c-mpc.c and see how far I get.

-- 
Timur Tabi
Linux kernel developer at Freescale