[PATCH 1/2] i2c-au1550: send i2c stop on error #2

[PATCH 1/2] i2c-au1550: send i2c stop on error #2

[Manuel Lauss]

When the au1550 i2c driver encounteres an error while addressing a slave
or has no data to send to a slave in the last i2c message, it returns to
the upper layers without issuing a i2c stop condition.  This for example
resulted in the minute register of the RTC on my board to be overwritten
with a random value on a following transfer.

Fix the driver to send a stop over the i2c bus if one of the following
2 conditions are met:
* error when addressing a slave
* no data to send in the last i2c message

Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>

--- a/drivers/i2c/busses/i2c-au1550.c	2007-04-26 05:08:32.000000000 +0200
+++ b/drivers/i2c/busses/i2c-au1550.c	2007-05-15 20:19:56.000000000 +0200
@@ -260,13 +260,20 @@ static int
 au1550_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs, int num)
 {
 	struct i2c_au1550_data *adap = i2c_adap->algo_data;
+	volatile psc_smb_t *sp = (volatile psc_smb_t *)(adap->psc_base);
 	struct i2c_msg *p;
 	int i, err = 0;
 
 	for (i = 0; !err && i < num; i++) {
 		p = &msgs[i];
 		err = do_address(adap, p->addr, p->flags & I2C_M_RD);
-		if (err || !p->len)
+		if (err || ((!p->len) && (i == (num - 1)))) {
+			sp->psc_smbtxrx = PSC_SMBTXRX_STP;
+			au_sync();
+			wait_master_done(adap);
+			continue;
+		}
+		if (!p->len)
 			continue;
 		if (p->flags & I2C_M_RD)
 			err = i2c_read(adap, p->buf, p->len);

Re: [PATCH 1/2] i2c-au1550: send i2c stop on error #2

[Jean Delvare]

Hi Manuel,

On Wed, 16 May 2007 07:31:13 +0200, Manuel Lauss wrote:
> When the au1550 i2c driver encounteres an error while addressing a slave
> or has no data to send to a slave in the last i2c message, it returns to
> the upper layers without issuing a i2c stop condition.  This for example
> resulted in the minute register of the RTC on my board to be overwritten
> with a random value on a following transfer.
> 
> Fix the driver to send a stop over the i2c bus if one of the following
> 2 conditions are met:
> * error when addressing a slave
> * no data to send in the last i2c message
> 
> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
> 
> --- a/drivers/i2c/busses/i2c-au1550.c	2007-04-26 05:08:32.000000000 +0200
> +++ b/drivers/i2c/busses/i2c-au1550.c	2007-05-15 20:19:56.000000000 +0200
> @@ -260,13 +260,20 @@ static int
>  au1550_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs, int num)
>  {
>  	struct i2c_au1550_data *adap = i2c_adap->algo_data;
> +	volatile psc_smb_t *sp = (volatile psc_smb_t *)(adap->psc_base);
>  	struct i2c_msg *p;
>  	int i, err = 0;
>  
>  	for (i = 0; !err && i < num; i++) {
>  		p = &msgs[i];
>  		err = do_address(adap, p->addr, p->flags & I2C_M_RD);
> -		if (err || !p->len)
> +		if (err || ((!p->len) && (i == (num - 1)))) {
> +			sp->psc_smbtxrx = PSC_SMBTXRX_STP;
> +			au_sync();
> +			wait_master_done(adap);
> +			continue;
> +		}
> +		if (!p->len)
>  			continue;
>  		if (p->flags & I2C_M_RD)
>  			err = i2c_read(adap, p->buf, p->len);

Good catch. I'd have two comments though:

1* It looks to me like there are other error conditions which also
cause the driver to leave without issuing a stop condition on the bus:
if not all bytes of a write are acked by the target slave (in
i2c_write) or if the master receives less bytes than expected (in
i2c_read). I understand these are less likely to happen than the quick
write case which bit you, but shouldn't these bugs be fixed as well?

2* In i2c_write and i2c_read, the stop bit is always sent together with
the last byte, while your new code sends the stop bit on its own after
the address byte. Is it OK? I am wondering if your code isn't sending
an extra (0) byte after the address when asked to send a zero-byte
message. That would be bad. Do you have a bus analyzer or scope to
check what exactly is being sent on the bus in this case?

Domen, care to comment on this patch and/or my own comments?

Thanks,
-- 
Jean Delvare

Re: [PATCH 1/2] i2c-au1550: send i2c stop on error #2

[Manuel Lauss]

Hi Jean,

On Wed, May 16, 2007 at 03:38:22PM +0200, Jean Delvare wrote:
> Hi Manuel,
> 
> 1* It looks to me like there are other error conditions which also
> cause the driver to leave without issuing a stop condition on the bus:
> if not all bytes of a write are acked by the target slave (in
> i2c_write) or if the master receives less bytes than expected (in
> i2c_read). I understand these are less likely to happen than the quick
> write case which bit you, but shouldn't these bugs be fixed as well?

Hm, didn't think about those (because I didn't hit them yet)
I'll fix them too of course. (I even started a complete rewrite of this
driver [using IRQ and DMA instead of polling] a few months back but got
stuck and instead fixed the bug that annoyed me the most)
 
> 2* In i2c_write and i2c_read, the stop bit is always sent together with
> the last byte, while your new code sends the stop bit on its own after
> the address byte. Is it OK? 

Well, no. However the quick probe does an i2c read IIRC so it should be
safe. I'll fix that too.

>                             I am wondering if your code isn't sending
> an extra (0) byte after the address when asked to send a zero-byte
> message. That would be bad. Do you have a bus analyzer or scope to
> check what exactly is being sent on the bus in this case?

Yes I have a scope. I'll improve the driver some more and then check
the actual data sent over the wires.

Thank you for your review, much appreciated!
	Manuel Lauss

Re: [PATCH 1/2] i2c-au1550: send i2c stop on error #2

[Jean Delvare]

On Wed, 16 May 2007 16:20:38 +0200, Manuel Lauss wrote:
> > 2* In i2c_write and i2c_read, the stop bit is always sent together with
> > the last byte, while your new code sends the stop bit on its own after
> > the address byte. Is it OK? 
> 
> Well, no. However the quick probe does an i2c read IIRC so it should be
> safe. I'll fix that too.

From an I2C perspective, zero-byte transactions can be both reads and
writes. So if you accidentally send one more byte on the bus after the
address, you are actually doing a 1-byte read or write. The variant
used by the Linux kernel for probing purposes is the "write" variant.

> >                             I am wondering if your code isn't sending
> > an extra (0) byte after the address when asked to send a zero-byte
> > message. That would be bad. Do you have a bus analyzer or scope to
> > check what exactly is being sent on the bus in this case?
> 
> Yes I have a scope. I'll improve the driver some more and then check
> the actual data sent over the wires.

OK, thanks. I have to admit I am a bit curious how (if) you can send a
stop transaction on error without sending an extra byte with this chip.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c-au1550: send i2c stop on error #2

[Jean Delvare]

On Wed, 16 May 2007 16:20:38 +0200, Manuel Lauss wrote:
> > 2* In i2c_write and i2c_read, the stop bit is always sent together with
> > the last byte, while your new code sends the stop bit on its own after
> > the address byte. Is it OK? 
> 
> Well, no. However the quick probe does an i2c read IIRC so it should be
> safe. I'll fix that too.