Calling wait_event_interruptible_timeout() in I2C wait functions

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

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

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

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

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.

Ditto for the Freescale PowerPC I2C driver.

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