Patchwork [v2] i2c-mpc: generate START condition after STOP caused by read i2c_msg

login
register
mail settings
Submitter Esben Haabendal
Date May 19, 2009, 5:22 a.m.
Message ID <4A124202.4010201@doredevelopment.dk>
Download mbox | patch
Permalink /patch/27387/
State Superseded, archived
Delegated to: Kumar Gala
Headers show

Comments

Esben Haabendal - May 19, 2009, 5:22 a.m.
This fixes MAL (arbitration lost) bug caused by illegal use of
RSTA (repeated START) after STOP condition generated after last byte
of reads. With this patch, it is possible to do an i2c_transfer() with
additional i2c_msg's following the I2C_M_RD messages.

It still needs to be resolved if it is possible to fix this issue
by removing the STOP condition after reads in a robust way.

Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
---
  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
  1 files changed, 7 insertions(+), 2 deletions(-)

  	return (ret < 0) ? ret : num;
Esben Haabendal - May 26, 2009, 11:30 a.m.
On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> This fixes MAL (arbitration lost) bug caused by illegal use of
> RSTA (repeated START) after STOP condition generated after last byte
> of reads. With this patch, it is possible to do an i2c_transfer() with
> additional i2c_msg's following the I2C_M_RD messages.
>
> It still needs to be resolved if it is possible to fix this issue
> by removing the STOP condition after reads in a robust way.
>
> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> ---
>  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)

Any blockers to get this accepted?

/Esben
Ben Dooks - May 26, 2009, 9:33 p.m.
On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> > This fixes MAL (arbitration lost) bug caused by illegal use of
> > RSTA (repeated START) after STOP condition generated after last byte
> > of reads. With this patch, it is possible to do an i2c_transfer() with
> > additional i2c_msg's following the I2C_M_RD messages.
> >
> > It still needs to be resolved if it is possible to fix this issue
> > by removing the STOP condition after reads in a robust way.
> >
> > Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> > ---
> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> 
> Any blockers to get this accepted?

It would be nice to get an ack from someone who can actually test
the driver before getting this merged.
 
> /Esben
> --
> 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
Wolfram Sang - May 28, 2009, 5:17 p.m.
> > Any blockers to get this accepted?
> 
> It would be nice to get an ack from someone who can actually test
> the driver before getting this merged.

I wanted to test it, but it does not apply due to line breaks (check
@@-line). Also, I don't really have the time to dig into the topic, so I
would only test it and give a tested-by-tag if it doesn't break anything
here. I think Joakim would be a good candidate for an acked-by .
Joakim Tjernlund - May 28, 2009, 6:43 p.m.
Wolfram Sang <w.sang@pengutronix.de> wrote on 28/05/2009 19:17:26:
>
> > > Any blockers to get this accepted?
> >
> > It would be nice to get an ack from someone who can actually test
> > the driver before getting this merged.
>
> I wanted to test it, but it does not apply due to line breaks (check
> @@-line). Also, I don't really have the time to dig into the topic, so I
> would only test it and give a tested-by-tag if it doesn't break anything
> here. I think Joakim would be a good candidate for an acked-by .

It sure looks OK, even at a closer look :)
Acked-by:  Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Grant Likely - May 28, 2009, 7:31 p.m.
On Tue, May 26, 2009 at 5:30 AM, Esben Haabendal
<esbenhaabendal@gmail.com> wrote:
> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
>> This fixes MAL (arbitration lost) bug caused by illegal use of
>> RSTA (repeated START) after STOP condition generated after last byte
>> of reads. With this patch, it is possible to do an i2c_transfer() with
>> additional i2c_msg's following the I2C_M_RD messages.
>>
>> It still needs to be resolved if it is possible to fix this issue
>> by removing the STOP condition after reads in a robust way.
>>
>> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
>> ---
>>  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
>>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> Any blockers to get this accepted?

It helps if you cc: developers/maintainers of the device.  ie. Kumar
for mpc8xxx, me for 52xx.

This is the first time I noticed your posting.  It will take me a few
days before I get a chance to review it.

g.
Esben Haabendal - May 28, 2009, 8:10 p.m.
On Thu, May 28, 2009 at 7:17 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> > Any blockers to get this accepted?
>>
>> It would be nice to get an ack from someone who can actually test
>> the driver before getting this merged.
>
> I wanted to test it, but it does not apply due to line breaks (check
> @@-line). Also, I don't really have the time to dig into the topic, so I
> would only test it and give a tested-by-tag if it doesn't break anything
> here. I think Joakim would be a good candidate for an acked-by .

I've checked both my copy in my "Sent" folder and the copy received
from the list,
and I cannot see any "line break" breakage of the patch.

It must be your mail client that is messing with it, so I cannot really do it
much better in that way.

If necessary, I can push a branch to a public repo you can pull from.

/Esben
Esben Haabendal - May 28, 2009, 8:15 p.m.
On Thu, May 28, 2009 at 9:31 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, May 26, 2009 at 5:30 AM, Esben Haabendal wrote:
>> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal wrote:
>>> This fixes MAL (arbitration lost) bug caused by illegal use of
>>> RSTA (repeated START) after STOP condition generated after last byte
>>> of reads. With this patch, it is possible to do an i2c_transfer() with
>>> additional i2c_msg's following the I2C_M_RD messages.
>>>
>>> It still needs to be resolved if it is possible to fix this issue
>>> by removing the STOP condition after reads in a robust way.
>>>
>>> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
>>> ---
>>>  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
>>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>
>> Any blockers to get this accepted?
>
> It helps if you cc: developers/maintainers of the device.  ie. Kumar
> for mpc8xxx, me for 52xx.
>
> This is the first time I noticed your posting.  It will take me a few
> days before I get a chance to review it.

Kumar, will you take a look at this patch?

/Esben
Peter Korsgaard - May 28, 2009, 8:34 p.m.
>>>>> "Esben" == Esben Haabendal <esbenhaabendal@gmail.com> writes:

Hi,

 >> I wanted to test it, but it does not apply due to line breaks (check
 >> @@-line). Also, I don't really have the time to dig into the topic, so I
 >> would only test it and give a tested-by-tag if it doesn't break anything
 >> here. I think Joakim would be a good candidate for an acked-by .

 Esben> I've checked both my copy in my "Sent" folder and the copy
 Esben> received from the list, and I cannot see any "line break"
 Esben> breakage of the patch.

I guess Wolfram referred to the context line which was clearly word wrapped:

@@ -456,17 +456,22 @@ static int mpc_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)

The other lines look fine.
Esben Haabendal - May 28, 2009, 8:41 p.m.
>  Esben> I've checked both my copy in my "Sent" folder and the copy
>  Esben> received from the list, and I cannot see any "line break"
>  Esben> breakage of the patch.
>
> I guess Wolfram referred to the context line which was clearly word wrapped:
>
> @@ -456,17 +456,22 @@ static int mpc_xfer(struct i2c_adapter *adap,
> struct i2c_msg *msgs, int num)
>
> The other lines look fine.

It's strange, that line looks perfectly fine when I check the mail in my GMail
inbox and the outbox from the account I sent it from.

/Esben
Peter Korsgaard - May 28, 2009, 9:08 p.m.
>>>>> "Esben" == Esben Haabendal <esbenhaabendal@gmail.com> writes:

Hi,

 Esben> It's strange, that line looks perfectly fine when I check the
 Esben> mail in my GMail inbox and the outbox from the account I sent
 Esben> it from.

Well, it is here and in the archive:
http://ozlabs.org/pipermail/linuxppc-dev/2009-May/072274.html

Please consider using git send-email for patches.
Esben Haabendal - May 28, 2009, 9:22 p.m.
Peter Korsgaard wrote:
>>>>>> "Esben" == Esben Haabendal <esbenhaabendal@gmail.com> writes:
>>>>>>             
>
> Hi,
>
>  Esben> It's strange, that line looks perfectly fine when I check the
>  Esben> mail in my GMail inbox and the outbox from the account I sent
>  Esben> it from.
>
> Well, it is here and in the archive:
> http://ozlabs.org/pipermail/linuxppc-dev/2009-May/072274.html
>   
If you look at
http://www.mail-archive.com/linux-i2c@vger.kernel.org/msg01050.html
instead, the patch is not broken.  I find it more likely that the ozlabs.org
archive is breaking my lines, than mail-archive.com putting broken lines
together again.
> Please consider using git send-email for patches.
>   
I used git imap-send and thunderbird to do it.

I will consider git send-email in the future, although I don't think
my original e-mail were broken ;-)

But
Ben Dooks - June 2, 2009, 10:25 p.m.
On Thu, May 28, 2009 at 10:15:22PM +0200, Esben Haabendal wrote:
> On Thu, May 28, 2009 at 9:31 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Tue, May 26, 2009 at 5:30 AM, Esben Haabendal wrote:
> >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal wrote:
> >>> This fixes MAL (arbitration lost) bug caused by illegal use of
> >>> RSTA (repeated START) after STOP condition generated after last byte
> >>> of reads. With this patch, it is possible to do an i2c_transfer() with
> >>> additional i2c_msg's following the I2C_M_RD messages.
> >>>
> >>> It still needs to be resolved if it is possible to fix this issue
> >>> by removing the STOP condition after reads in a robust way.
> >>>
> >>> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> >>> ---
> >>> ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> >>> ?1 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> Any blockers to get this accepted?
> >
> > It helps if you cc: developers/maintainers of the device. ?ie. Kumar
> > for mpc8xxx, me for 52xx.
> >
> > This is the first time I noticed your posting. ?It will take me a few
> > days before I get a chance to review it.
> 
> Kumar, will you take a look at this patch?

is anyone else likely to review it, or should I merge?
Grant Likely - June 2, 2009, 11:12 p.m.
Hi Esben and Ben,

Sorry for the lateness in reviewing this.  FWIW, here are my comments.

g.

On Mon, May 18, 2009 at 11:22 PM, Esben Haabendal
<eha@doredevelopment.dk> wrote:
> This fixes MAL (arbitration lost) bug caused by illegal use of
> RSTA (repeated START) after STOP condition generated after last byte
> of reads. With this patch, it is possible to do an i2c_transfer() with
> additional i2c_msg's following the I2C_M_RD messages.
>
> It still needs to be resolved if it is possible to fix this issue
> by removing the STOP condition after reads in a robust way.
>
> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> ---
>  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 4af5c09..0199f9a 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -456,17 +456,22 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>        }
>
>        for (i = 0; ret >= 0 && i < num; i++) {
> +               int restart = i;
>                pmsg = &msgs[i];
>                dev_dbg(i2c->dev,
>                        "Doing %s %d bytes to 0x%02x - %d of %d messages\n",
>                        pmsg->flags & I2C_M_RD ? "read" : "write",
>                        pmsg->len, pmsg->addr, i + 1, num);
> +               if (i > 0 && ((pmsg - 1)->flags & I2C_M_RD))
> +                       restart = 0;
>                if (pmsg->flags & I2C_M_RD)
>                        ret =
> -                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> i);
> +                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> +                                    restart);
>                else
>                        ret =
> -                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> i);
> +                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> +                                     restart);
>        }

Hmmm, seems to be a rather convoluted code path.  Surely this could be
handled in a clearer way.  The whole (pmsg - 1) bit raises my eyebrows
(I'd rather see msgs[i-1]).  At the very least this deserves an
comment describing what it is doing.  The following might be better
for the next person who has to read this code:

+      int restart = 0;
       for (i = 0; ret >= 0 && i < num; i++) {
               pmsg = &msgs[i];
               dev_dbg(i2c->dev,
                       "Doing %s %d bytes to 0x%02x - %d of %d messages\n",
                       pmsg->flags & I2C_M_RD ? "read" : "write",
                       pmsg->len, pmsg->addr, i + 1, num);
               if (pmsg->flags & I2C_M_RD)
                       ret =
-                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
i);
+                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
+                                    restart);
               else
                       ret =
-                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
i);
+                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
+                                     restart);
+               /* Only set the restart flag if this was not an
I2C_M_RD transaction
+                * because it causes an illegal use of
+                * RSTA (repeated START) after STOP condition
generated after last byte
+                * of reads  */
+               restart = (pmsg->flags & I2C_M_RD == 0);
       }

g.
Grant Likely - June 3, 2009, 6:01 a.m.
On Tue, Jun 2, 2009 at 5:12 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> Hi Esben and Ben,
>
> Sorry for the lateness in reviewing this.  FWIW, here are my comments.
>
> g.
>
> On Mon, May 18, 2009 at 11:22 PM, Esben Haabendal
> <eha@doredevelopment.dk> wrote:
>> This fixes MAL (arbitration lost) bug caused by illegal use of
>> RSTA (repeated START) after STOP condition generated after last byte
>> of reads. With this patch, it is possible to do an i2c_transfer() with
>> additional i2c_msg's following the I2C_M_RD messages.
>>
>> It still needs to be resolved if it is possible to fix this issue
>> by removing the STOP condition after reads in a robust way.
>>
>> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
>> ---
>>  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
>> index 4af5c09..0199f9a 100644
>> --- a/drivers/i2c/busses/i2c-mpc.c
>> +++ b/drivers/i2c/busses/i2c-mpc.c
>> @@ -456,17 +456,22 @@ static int mpc_xfer(struct i2c_adapter *adap, struct
>> i2c_msg *msgs, int num)
>>        }
>>
>>        for (i = 0; ret >= 0 && i < num; i++) {
>> +               int restart = i;
>>                pmsg = &msgs[i];
>>                dev_dbg(i2c->dev,
>>                        "Doing %s %d bytes to 0x%02x - %d of %d messages\n",
>>                        pmsg->flags & I2C_M_RD ? "read" : "write",
>>                        pmsg->len, pmsg->addr, i + 1, num);
>> +               if (i > 0 && ((pmsg - 1)->flags & I2C_M_RD))
>> +                       restart = 0;
>>                if (pmsg->flags & I2C_M_RD)
>>                        ret =
>> -                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
>> i);
>> +                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
>> +                                    restart);
>>                else
>>                        ret =
>> -                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
>> i);
>> +                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
>> +                                     restart);
>>        }
>
> Hmmm, seems to be a rather convoluted code path.  Surely this could be
> handled in a clearer way.  The whole (pmsg - 1) bit raises my eyebrows
> (I'd rather see msgs[i-1]).  At the very least this deserves an
> comment describing what it is doing.  The following might be better
> for the next person who has to read this code:
>
> +      int restart = 0;
>       for (i = 0; ret >= 0 && i < num; i++) {
>               pmsg = &msgs[i];
>               dev_dbg(i2c->dev,
>                       "Doing %s %d bytes to 0x%02x - %d of %d messages\n",
>                       pmsg->flags & I2C_M_RD ? "read" : "write",
>                       pmsg->len, pmsg->addr, i + 1, num);
>               if (pmsg->flags & I2C_M_RD)
>                       ret =
> -                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> i);
> +                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> +                                    restart);
>               else
>                       ret =
> -                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> i);
> +                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> +                                     restart);
> +               /* Only set the restart flag if this was not an
> I2C_M_RD transaction
> +                * because it causes an illegal use of
> +                * RSTA (repeated START) after STOP condition
> generated after last byte
> +                * of reads  */
> +               restart = (pmsg->flags & I2C_M_RD == 0);
>       }

BTW, since you're touching these lines anyway, please clean up the
whitespace usage.  Since you have to break the line anyway, it no
longer makes sense for the "ret = " to be on a separate line anymore.

g.
Ben Dooks - June 14, 2009, 1:16 p.m.
is there a new version of this patch available?
Esben Haabendal - June 14, 2009, 2:04 p.m.
Ben Dooks wrote:
> is there a new version of this patch available?
>
>   
I will catch up on it ASAP.

/Esben
Michael Lawnick - Dec. 3, 2009, 3:09 p.m.
Ben Dooks said the following:
> On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
>> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
>> > This fixes MAL (arbitration lost) bug caused by illegal use of
>> > RSTA (repeated START) after STOP condition generated after last byte
>> > of reads. With this patch, it is possible to do an i2c_transfer() with
>> > additional i2c_msg's following the I2C_M_RD messages.
>> >
>> > It still needs to be resolved if it is possible to fix this issue
>> > by removing the STOP condition after reads in a robust way.
>> >
>> > Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
>> > ---
>> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
>> > ?1 files changed, 7 insertions(+), 2 deletions(-)
>> 
>> Any blockers to get this accepted?
> 
> It would be nice to get an ack from someone who can actually test
> the driver before getting this merged.
>  
What is the state of this patch?
Shouldn't we attack the problem on a more general way by inventing a
Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
This way the client driver is able to decide what it needs. If we do the
choice within adapter, chance is about 50% to be wrong.

Just my 2 Cents.
Ben Dooks - Dec. 3, 2009, 3:29 p.m.
On Thu, Dec 03, 2009 at 04:09:57PM +0100, Michael Lawnick wrote:
> Ben Dooks said the following:
> > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> >> > This fixes MAL (arbitration lost) bug caused by illegal use of
> >> > RSTA (repeated START) after STOP condition generated after last byte
> >> > of reads. With this patch, it is possible to do an i2c_transfer() with
> >> > additional i2c_msg's following the I2C_M_RD messages.
> >> >
> >> > It still needs to be resolved if it is possible to fix this issue
> >> > by removing the STOP condition after reads in a robust way.
> >> >
> >> > Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> >> > ---
> >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> >> 
> >> Any blockers to get this accepted?
> > 
> > It would be nice to get an ack from someone who can actually test
> > the driver before getting this merged.
> >  
> What is the state of this patch?
> Shouldn't we attack the problem on a more general way by inventing a
> Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
> This way the client driver is able to decide what it needs. If we do the
> choice within adapter, chance is about 50% to be wrong.

The documentation for 'struct i2c_msg' already says the STOP should only
be generated for the last message of the transfer. If STOP is being
generated for a message that isn't the last in the transfer than this
is incorrect behaviour.

Unless otherwise indicated, I'll put the patch in the series that I'll
send to Linus early next week.
Joakim Tjernlund - Dec. 3, 2009, 3:42 p.m.
>
> Ben Dooks said the following:
> > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> >> > This fixes MAL (arbitration lost) bug caused by illegal use of
> >> > RSTA (repeated START) after STOP condition generated after last byte
> >> > of reads. With this patch, it is possible to do an i2c_transfer() with
> >> > additional i2c_msg's following the I2C_M_RD messages.
> >> >
> >> > It still needs to be resolved if it is possible to fix this issue
> >> > by removing the STOP condition after reads in a robust way.
> >> >
> >> > Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> >> > ---
> >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> Any blockers to get this accepted?
> >
> > It would be nice to get an ack from someone who can actually test
> > the driver before getting this merged.
> >
> What is the state of this patch?
> Shouldn't we attack the problem on a more general way by inventing a
> Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
> This way the client driver is able to decide what it needs. If we do the
> choice within adapter, chance is about 50% to be wrong.

I have sent another patch (which I think is in Linus tree already) that
fixes the problem properly.

Can't remember if the driver handles M_RESTART or not. That is
a separate issue though.

      Jocke
Joakim Tjernlund - Dec. 3, 2009, 3:49 p.m.
>
> On Thu, Dec 03, 2009 at 04:09:57PM +0100, Michael Lawnick wrote:
> > Ben Dooks said the following:
> > > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> > >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> > >> > This fixes MAL (arbitration lost) bug caused by illegal use of
> > >> > RSTA (repeated START) after STOP condition generated after last byte
> > >> > of reads. With this patch, it is possible to do an i2c_transfer() with
> > >> > additional i2c_msg's following the I2C_M_RD messages.
> > >> >
> > >> > It still needs to be resolved if it is possible to fix this issue
> > >> > by removing the STOP condition after reads in a robust way.
> > >> >
> > >> > Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> > >> > ---
> > >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> > >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> > >>
> > >> Any blockers to get this accepted?
> > >
> > > It would be nice to get an ack from someone who can actually test
> > > the driver before getting this merged.
> > >
> > What is the state of this patch?
> > Shouldn't we attack the problem on a more general way by inventing a
> > Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
> > This way the client driver is able to decide what it needs. If we do the
> > choice within adapter, chance is about 50% to be wrong.
>
> The documentation for 'struct i2c_msg' already says the STOP should only
> be generated for the last message of the transfer. If STOP is being
> generated for a message that isn't the last in the transfer than this
> is incorrect behaviour.
>
> Unless otherwise indicated, I'll put the patch in the series that I'll
> send to Linus early next week.

Don't, already fixed properly and in Linus tree already.
Michael Lawnick - Dec. 4, 2009, 6:58 a.m.
Ben Dooks said the following:
> On Thu, Dec 03, 2009 at 04:09:57PM +0100, Michael Lawnick wrote:
>> Ben Dooks said the following:
>> > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
>> >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
>> >> > This fixes MAL (arbitration lost) bug caused by illegal use of
>> >> > RSTA (repeated START) after STOP condition generated after last byte
>> >> > of reads. With this patch, it is possible to do an i2c_transfer() with
>> >> > additional i2c_msg's following the I2C_M_RD messages.
>> >> >
>> >> > It still needs to be resolved if it is possible to fix this issue
>> >> > by removing the STOP condition after reads in a robust way.
>> >> >
>> >> > Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
>> >> > ---
>> >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
>> >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
>> >> 
>> >> Any blockers to get this accepted?
>> > 
>> > It would be nice to get an ack from someone who can actually test
>> > the driver before getting this merged.
>> >  
>> What is the state of this patch?
>> Shouldn't we attack the problem on a more general way by inventing a
>> Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
>> This way the client driver is able to decide what it needs. If we do the
>> choice within adapter, chance is about 50% to be wrong.
> 
> The documentation for 'struct i2c_msg' already says the STOP should only
> be generated for the last message of the transfer. If STOP is being
> generated for a message that isn't the last in the transfer than this
> is incorrect behaviour.

Ah, now I see, this is a mpc-only problem of implementation
(automatically generating stop in read function).
I couldn't find the above mentioned specification of STOP/RESTART (seems
I need new glasses) and was worried about whether my own implementation
for OCTEON is correct.

Thank you.

Patch

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 4af5c09..0199f9a 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -456,17 +456,22 @@  static int mpc_xfer(struct i2c_adapter *adap, 
struct i2c_msg *msgs, int num)
  	}

  	for (i = 0; ret >= 0 && i < num; i++) {
+		int restart = i;
  		pmsg = &msgs[i];
  		dev_dbg(i2c->dev,
  			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
  			pmsg->flags & I2C_M_RD ? "read" : "write",
  			pmsg->len, pmsg->addr, i + 1, num);
+		if (i > 0 && ((pmsg - 1)->flags & I2C_M_RD))
+			restart = 0;
  		if (pmsg->flags & I2C_M_RD)
  			ret =
-			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
+				     restart);
  		else
  			ret =
-			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
+				      restart);
  	}
  	mpc_i2c_stop(i2c);