Patchwork i2c-davinci: Handle signals gracefully

login
register
mail settings
Submitter Mike Looijmans
Date March 14, 2014, 6:42 a.m.
Message ID <1394779348-4084-1-git-send-email-mike.looijmans@topic.nl>
Download mbox | patch
Permalink /patch/330215/
State Accepted
Headers show

Comments

Mike Looijmans - March 14, 2014, 6:42 a.m.
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@topic.nl>
---
 drivers/i2c/busses/i2c-davinci.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
Wolfram Sang - March 24, 2014, 4:14 p.m.
> 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...

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

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

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

Thanks,

   Wolfram
Wolfram Sang - May 21, 2014, 8:17 a.m.
> >  		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.

Is my remark true? I still prefer the seperate patch, but we may also
simply update the commit message.
Mike Looijmans - May 24, 2014, 11:53 a.m.
On 05/21/2014 10:17 AM, Wolfram Sang wrote:
>
>>>   		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.
>
> 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.
Wolfram Sang - June 1, 2014, 8:11 p.m.
> 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!
Wolfram Sang - Nov. 8, 2014, 9:51 p.m.
On Sun, Jun 01, 2014 at 10:11:34PM +0200, Wolfram Sang wrote:
> 
> > 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!

I now picked the part removing the interruptible-call. The part with
the new exit-path would need to be resend and retested in case anyone
is still caring.
Mike Looijmans - Nov. 10, 2014, 2:49 p.m.
´╗┐On 11/08/2014 10:51 PM, Wolfram Sang wrote:
> On Sun, Jun 01, 2014 at 10:11:34PM +0200, Wolfram Sang wrote:
>>
>>> 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!
>
> I now picked the part removing the interruptible-call. The part with
> the new exit-path would need to be resend and retested in case anyone
> is still caring.
>

I think that's the right thing to do. There seems to be little interest in the 
rest of the I2C controller, and it's a tough one to test (you'd have to 
trigger an I2C error at exactly the moment that a clock frequency change is 
about to occur...).




Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


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@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)