[PATCH] i2c-davinci: Handle signals gracefully

Discussion:

Mike Looijmans

2014-01-09 11:11:25 UTC

When a signal is caught while the i2c-davinci bus driver is transferring,
the drive just "abandons" the transfer and leaves the controller to fend
for itself. The next I2C transaction will find the controller in an
undefined state and often results in a stream of "initiating i2c bus recovery"
messages until the controller arrives in a defined state. This behaviour
also sends out "half" or possibly even mixed messages to I2C client
devices which may put them in an undesired state as well.

This patch fixes this issue by always attempting to finish the current
transaction, and then check on a pending signal. It either reports
success if all data has been transferred, or it returns failure when
the transaction was aborted. This keeps the controller in a defined
state, and is also much friendlier towards client devices, because
it will only send complete messages.

Before this patch, reading an I2C device in a loop and interrupting it
often resulted in a "initiating i2c bus recovery" storm and not being
able to communicate via I2C for several seconds. With this patch, the
userspace call simply returns EINTR and the next I2C transaction
succeeds without errors.

Signed-off-by: Mike Looijmans <mike.looijmans at topic.nl>
---
drivers/i2c/busses/i2c-davinci.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 132369f..102673b 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -372,9 +372,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
flag |= DAVINCI_I2C_MDR_STP;
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);

- r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+ r = wait_for_completion_timeout(&dev->cmd_complete,
dev->adapter.timeout);
- if (r == 0) {
+ if (unlikely(r == 0)) {
dev_err(dev->dev, "controller timed out\n");
davinci_i2c_recover_bus(dev);
i2c_davinci_init(dev);
@@ -384,7 +384,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
if (dev->buf_len) {
/* This should be 0 if all bytes were transferred
* or dev->cmd_err denotes an error.
- * A signal may have aborted the transfer.
*/
if (r >= 0) {
dev_err(dev->dev, "abnormal termination buf_len=%i\n",
@@ -436,22 +435,30 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
ret = i2c_davinci_wait_bus_not_busy(dev, 1);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto error;
}

for (i = 0; i < num; i++) {
+ if (signal_pending(current)) {
+ dev_dbg(dev->dev, "%s [%d/%d] %#x ERESTARTSYS\n",
+ __func__, i + 1, num, msgs[i].addr);
+ ret = -ERESTARTSYS;
+ goto error;
+ }
ret = i2c_davinci_xfer_msg(adap, &msgs[i], (i == (num - 1)));
- dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
- ret);
+ dev_dbg(dev->dev, "%s [%d/%d] %#x ret: %d\n", __func__, i + 1,
+ num, msgs[i].addr, ret);
if (ret < 0)
- return ret;
+ goto error;
}
+ ret = num;

+error:
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif

- return num;
+ return ret;
}

static u32 i2c_davinci_func(struct i2c_adapter *adap)

--
1.7.9.5

Wolfram Sang

2014-03-09 20:21:07 UTC

Permalink

Post by Mike Looijmans
When a signal is caught while the i2c-davinci bus driver is transferring,
the drive just "abandons" the transfer and leaves the controller to fend
for itself. The next I2C transaction will find the controller in an
undefined state and often results in a stream of "initiating i2c bus recovery"
messages until the controller arrives in a defined state. This behaviour
also sends out "half" or possibly even mixed messages to I2C client
devices which may put them in an undesired state as well.
This patch fixes this issue by always attempting to finish the current
transaction, and then check on a pending signal. It either reports
success if all data has been transferred, or it returns failure when
the transaction was aborted. This keeps the controller in a defined
state, and is also much friendlier towards client devices, because
it will only send complete messages.

Even more, you should complete the whole transfer. There are devices
where things can really go wrong if you send a half-complete command and
then start with the next one. So, not checking signals at all is the way
to go for I2C drivers. There is some cruft left, so I am happy about
patches fixing that, with testing on real HW. Like yours here.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://linux.davincidsp.com/pipermail/davinci-linux-open-source/attachments/20140309/887b4c45/attachment.bin>

Mike Looijmans

2014-03-10 10:24:55 UTC

Permalink

Post by Wolfram Sang

I agree.

I know the Zynq (using a cadence controller) also lets signals interrupt I2C
transfers, so I'll propose a patch to Xilinx and CC to you and linux-i2c to
completely remove signal handling from that driver as well.

Mike

Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax: (+31) (0) 499 33 69 70
E-mail: mike.looijmans at topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Wolfram Sang

2014-03-10 10:59:52 UTC

Permalink

Post by Mike Looijmans

Post by Wolfram Sang
Even more, you should complete the whole transfer. There are devices
where things can really go wrong if you send a half-complete command and
then start with the next one. So, not checking signals at all is the way
to go for I2C drivers. There is some cruft left, so I am happy about
patches fixing that, with testing on real HW. Like yours here.

Cool, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://linux.davincidsp.com/pipermail/davinci-linux-open-source/attachments/20140310/1c76711b/attachment.bin>

Wolfram Sang

2014-03-10 15:24:06 UTC

Permalink

Post by Wolfram Sang

Post by Mike Looijmans

Cool, thanks!

Are you going to update the davinci patch as well?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://linux.davincidsp.com/pipermail/davinci-linux-open-source/attachments/20140310/21dfe991/attachment-0001.bin>

Mike Looijmans

2014-03-14 06:43:25 UTC

Permalink

Post by Wolfram Sang

Post by Mike Looijmans

Cool, thanks!

Are you going to update the davinci patch as well?

An amended patch is on its way now. I forgot to set the subject to "PATCHv2"
though.

Mike.

Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax: (+31) (0) 499 33 69 70
E-mail: mike.looijmans at topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Mike Looijmans

2014-03-14 06:42:28 UTC

Permalink

When a signal is caught while the i2c-davinci bus driver is transferring,
the drive just "abandons" the transfer and leaves the controller to fend
for itself. The next I2C transaction will find the controller in an
undefined state and often results in a stream of "initiating i2c bus recovery"
messages until the controller arrives in a defined state. This behaviour
also sends out "half" or possibly even mixed messages to I2C client
devices which may put them in an undesired state as well.

This patch fixes this issue by always attempting to finish the current
transaction, and only abort on bus errors. This keeps the controller in a
defined state, and is also much friendlier towards client devices, because
it will only send complete messages.

Before this patch, reading an I2C device in a loop and interrupting it
often resulted in a "initiating i2c bus recovery" storm and not being
able to communicate via I2C for several seconds. With this patch, I2C
transactions will not be interrupted or otherwise halfway completed.

v2: Completely ignore signals.

Signed-off-by: Mike Looijmans <mike.looijmans at topic.nl>
---
drivers/i2c/busses/i2c-davinci.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index af0b583..254d897 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -372,9 +372,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
flag |= DAVINCI_I2C_MDR_STP;
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);

- r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+ r = wait_for_completion_timeout(&dev->cmd_complete,
dev->adapter.timeout);
- if (r == 0) {
+ if (unlikely(r == 0)) {
dev_err(dev->dev, "controller timed out\n");
davinci_i2c_recover_bus(dev);
i2c_davinci_init(dev);
@@ -384,7 +384,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
if (dev->buf_len) {
/* This should be 0 if all bytes were transferred
* or dev->cmd_err denotes an error.
- * A signal may have aborted the transfer.
*/
if (r >= 0) {
dev_err(dev->dev, "abnormal termination buf_len=%i\n",
@@ -436,22 +435,24 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
ret = i2c_davinci_wait_bus_not_busy(dev, 1);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto error;
}

for (i = 0; i < num; i++) {
ret = i2c_davinci_xfer_msg(adap, &msgs[i], (i == (num - 1)));
- dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
- ret);
+ dev_dbg(dev->dev, "%s [%d/%d] %#x ret: %d\n", __func__, i + 1,
+ num, msgs[i].addr, ret);
if (ret < 0)
- return ret;
+ goto error;
}
+ ret = num;

+error:
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif

- return num;
+ return ret;
}

static u32 i2c_davinci_func(struct i2c_adapter *adap)

--
1.7.9.5

Wolfram Sang

2014-03-24 16:14:32 UTC

Permalink

Post by Mike Looijmans
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index af0b583..254d897 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -372,9 +372,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
flag |= DAVINCI_I2C_MDR_STP;
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
- r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+ r = wait_for_completion_timeout(&dev->cmd_complete,
dev->adapter.timeout);
- if (r == 0) {
+ if (unlikely(r == 0)) {

Not really needed, but well yeah...

Post by Mike Looijmans
dev_err(dev->dev, "controller timed out\n");
davinci_i2c_recover_bus(dev);
i2c_davinci_init(dev);
@@ -384,7 +384,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
if (dev->buf_len) {
/* This should be 0 if all bytes were transferred
* or dev->cmd_err denotes an error.
- * A signal may have aborted the transfer.
*/
if (r >= 0) {
dev_err(dev->dev, "abnormal termination buf_len=%i\n",
@@ -436,22 +435,24 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
ret = i2c_davinci_wait_bus_not_busy(dev, 1);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto error;

You are fixing the error path here to include the completion? This is a
seperate patch IMO.

Post by Mike Looijmans
}
for (i = 0; i < num; i++) {
ret = i2c_davinci_xfer_msg(adap, &msgs[i], (i == (num - 1)));
- dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
- ret);
+ dev_dbg(dev->dev, "%s [%d/%d] %#x ret: %d\n", __func__, i + 1,
+ num, msgs[i].addr, ret);

No need for this change. We have other debug output/tracing already. But
no need to clean up either.

Post by Mike Looijmans
if (ret < 0)
- return ret;
+ goto error;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif
- return num;
+ return ret;

Thanks,

Wolfram

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://linux.davincidsp.com/pipermail/davinci-linux-open-source/attachments/20140324/278377e3/attachment.bin>

Wolfram Sang

2014-05-21 08:17:27 UTC

Permalink

Post by Wolfram Sang

You are fixing the error path here to include the completion? This is a
seperate patch IMO.

Is my remark true? I still prefer the seperate patch, but we may also
simply update the commit message.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://linux.davincidsp.com/pipermail/davinci-linux-open-source/attachments/20140521/10f122de/attachment.bin>

Mike Looijmans

2014-05-24 11:53:53 UTC

Permalink

Post by Wolfram Sang

You are fixing the error path here to include the completion? This is a
seperate patch IMO.

Is my remark true? I still prefer the seperate patch, but we may also
simply update the commit message.

Your remarks is correct. All your remarks were. Problem currently is
that I'm not assigned to a project related to the davinci so I cannot
spend time to fix and port it (the actual platform still runs 2.6.37).

Feel free to adap my patch or comments and commit. Or wait a few weeks
for when I have a sponsor to split and update the patch.

--
Mike Looijmans

Wolfram Sang

2014-06-01 20:11:34 UTC

Permalink

Feel free to adap my patch or comments and commit. Or wait a few weeks for
when I have a sponsor to split and update the patch.

OK, I'll hope you can make it for 3.17. Thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://linux.davincidsp.com/pipermail/davinci-linux-open-source/attachments/20140601/b58e65df/attachment.bin>