Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1555217 > unrolled thread

[PATCH] i2c: piix4: Avoid race conditions with IMC

Back to article view | Back to linux.kernel

Contents

  [PATCH] i2c: piix4: Avoid race conditions with IMC Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2017-01-10 13:20 +0100
    Re: [PATCH] i2c: piix4: Avoid race conditions with IMC Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-11 02:50 +0100
      Re: [PATCH] i2c: piix4: Avoid race conditions with IMC Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2017-01-11 10:20 +0100
      Re: [PATCH] i2c: piix4: Avoid race conditions with IMC Jean Delvare <jdelvare@suse.de> - 2017-01-12 10:50 +0100
        Re: [PATCH] i2c: piix4: Avoid race conditions with IMC Wolfram Sang <wsa@the-dreams.de> - 2017-01-12 11:10 +0100
          Re: [PATCH] i2c: piix4: Avoid race conditions with IMC Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-12 21:10 +0100

#1555217 — [PATCH] i2c: piix4: Avoid race conditions with IMC

On AMD's SB800 and upwards, the SMBus is shared with the Integrated
Micro Controller (IMC).

The platform provides a hardware semaphore to avoid race conditions
among them. (Check page 288 of the SB800-Series Southbridges Register
Reference Guide http://support.amd.com/TechDocs/45482.pdf)

Without this patch, many access to the SMBus end with an invalid
transaction or even with the bus stalled.

Credit-to: Alexandre Desnoyers <alex@qtec.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/i2c/busses/i2c-piix4.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index f0563f7ce01b..0286cfee6d36 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -585,9 +585,28 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		 u8 command, int size, union i2c_smbus_data *data)
 {
 	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
+	unsigned short piix4_smba = adapdata->smba;
 	u8 smba_en_lo;
 	u8 port;
 	int retval;
+	int timeout = 0;
+	int smbslvcnt;
+
+	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
+	smbslvcnt  = inb_p(SMBSLVCNT);
+	while (++timeout < MAX_TIMEOUT) {
+		outb_p(smbslvcnt | 0x10, SMBSLVCNT);
+
+		/* Check the semaphore status */
+		smbslvcnt  = inb_p(SMBSLVCNT);
+		if (smbslvcnt & 0x10)
+			break;
+
+		usleep_range(1000, 2000);
+	}
+	/* SMBus is still owned by the IMC, we give up */
+	if (timeout == MAX_TIMEOUT)
+		return -EBUSY;
 
 	mutex_lock(&piix4_mutex_sb800);
 
@@ -606,6 +625,9 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 
 	mutex_unlock(&piix4_mutex_sb800);
 
+	/* Release the semaphore */
+	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
+
 	return retval;
 }
 
-- 
2.11.0

[toc] | [next] | [standalone]

#1556139

On Tue, Jan 10, 2017 at 2:16 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> On AMD's SB800 and upwards, the SMBus is shared with the Integrated
> Micro Controller (IMC).
>
> The platform provides a hardware semaphore to avoid race conditions
> among them. (Check page 288 of the SB800-Series Southbridges Register
> Reference Guide http://support.amd.com/TechDocs/45482.pdf)

It would be nice to understand what kind of devices are accessing and to where.

Hans seems discovered one pretty nice issue on Intel
BayTrail/CherryTrail platforms where I2C semaphore is used to prevent
simultaneous access to P-Unit, but we have two paths there which are
not synchronized (yet). It brings a set of interesting (and
unfortunately "famous") bugs.

>
> Without this patch, many access to the SMBus end with an invalid
> transaction or even with the bus stalled.
>

> Credit-to: Alexandre Desnoyers <alex@qtec.com>

Never saw before. Did he suggested the solution or what?

> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -585,9 +585,28 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>                  u8 command, int size, union i2c_smbus_data *data)
>  {
>         struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
> +       unsigned short piix4_smba = adapdata->smba;
>         u8 smba_en_lo;
>         u8 port;
>         int retval;
> +       int timeout = 0;
> +       int smbslvcnt;

Keep them just after your another added variable.

> +       /* Request the SMBUS semaphore, avoid conflicts with the IMC */
> +       smbslvcnt  = inb_p(SMBSLVCNT);

> +       while (++timeout < MAX_TIMEOUT) {

Usual pattern is countdown.

do {
...
} while (--timeout);

> +               outb_p(smbslvcnt | 0x10, SMBSLVCNT);
> +
> +               /* Check the semaphore status */
> +               smbslvcnt  = inb_p(SMBSLVCNT);
> +               if (smbslvcnt & 0x10)
> +                       break;
> +
> +               usleep_range(1000, 2000);
> +       }

> +       /* SMBus is still owned by the IMC, we give up */
> +       if (timeout == MAX_TIMEOUT)
> +               return -EBUSY;

Would caller do it again? Perhaps -EAGAIN?

Since the returned value is not -ETIMEDOUT, I suppose the name of
counter variable is a bit confusing. Basically it's amount of attempts
with some gap between them. Though, it's up to you and maintainer.

> +       /* Release the semaphore */
> +       outb_p(smbslvcnt | 0x20, SMBSLVCNT);

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1556309

Hi Andy

Thanks for your review!

On Wed, Jan 11, 2017 at 2:49 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jan 10, 2017 at 2:16 PM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
>> On AMD's SB800 and upwards, the SMBus is shared with the Integrated
>> Micro Controller (IMC).
>>
>> The platform provides a hardware semaphore to avoid race conditions
>> among them. (Check page 288 of the SB800-Series Southbridges Register
>> Reference Guide http://support.amd.com/TechDocs/45482.pdf)
>
> It would be nice to understand what kind of devices are accessing and to where.

On my platform I found out that the IMC is polling address 0x98 and
0x99 every 14 and 177 msec.

Check out:

https://postimg.org/image/bssxhlg9d
https://postimg.org/image/g37ld6lch

But be aware that the firmware on the IMC might be different on other
platforms so the addresses and interval will be different.

>
> Hans seems discovered one pretty nice issue on Intel
> BayTrail/CherryTrail platforms where I2C semaphore is used to prevent
> simultaneous access to P-Unit, but we have two paths there which are
> not synchronized (yet). It brings a set of interesting (and
> unfortunately "famous") bugs.

AFAIK on AMD the smbus is just used on this driver.


>
>>
>> Without this patch, many access to the SMBus end with an invalid
>> transaction or even with the bus stalled.
>>
>
>> Credit-to: Alexandre Desnoyers <alex@qtec.com>
>
> Never saw before. Did he suggested the solution or what?

He is the hardware engineer where I work (qtec), when I showed him the
the logic analyzer output and told him that I was pretty sure that the
kernel/userpace was not doing those transactions he came up with the
theory of the IMC. He found up the semaphore on the documentation
also, so he deserves a lot of credit :).

>
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -585,9 +585,28 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>>                  u8 command, int size, union i2c_smbus_data *data)
>>  {
>>         struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
>> +       unsigned short piix4_smba = adapdata->smba;
>>         u8 smba_en_lo;
>>         u8 port;
>>         int retval;
>> +       int timeout = 0;
>> +       int smbslvcnt;
>
>> while (++timeout < MAX_TIMEOUT) {
>
> Usual pattern is countdown.

I was trying to follow the pattern on the file.

>
>> +       /* SMBus is still owned by the IMC, we give up */
>> +       if (timeout == MAX_TIMEOUT)
>> +               return -EBUSY;
>
> Would caller do it again? Perhaps -EAGAIN?

I think in this case we should return -EBUSY. If after 500 attempts
the bus is still hold by the IMC the bus is stalled or the IMC is
crashed, we should not retry. It is also the same errcode returned by
piix4_transaction().

>
> Since the returned value is not -ETIMEDOUT, I suppose the name of
> counter variable is a bit confusing. Basically it's amount of attempts
> with some gap between them. Though, it's up to you and maintainer.
>
>> +       /* Release the semaphore */
>> +       outb_p(smbslvcnt | 0x20, SMBSLVCNT);
>
> --
> With Best Regards,
> Andy Shevchenko

Thanks again, I will send a v2 with your comments!



-- 
Ricardo Ribalda

[toc] | [prev] | [next] | [standalone]

#1557254

On Wed, 11 Jan 2017 03:49:21 +0200, Andy Shevchenko wrote:
> On Tue, Jan 10, 2017 at 2:16 PM, Ricardo Ribalda Delgado
> > --- a/drivers/i2c/busses/i2c-piix4.c
> > +++ b/drivers/i2c/busses/i2c-piix4.c
> > @@ -585,9 +585,28 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
> >                  u8 command, int size, union i2c_smbus_data *data)
> >  {
> >         struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap);
> > +       unsigned short piix4_smba = adapdata->smba;
> >         u8 smba_en_lo;
> >         u8 port;
> >         int retval;
> > +       int timeout = 0;
> > +       int smbslvcnt;
> 
> Keep them just after your another added variable.

FWIW, I don't think this makes sense as a general rule. I'd rather have
the variables in an order which makes sense (for human readers or for
stack size optimization - unless gcc does it for us?), rather than
always adding at the same place. Is there a rationale for doing that? I
don't think shrinking the patch size is good enough a reason.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1557267

[Multipart message — attachments visible in raw view] — view raw

> > > +       unsigned short piix4_smba = adapdata->smba;
> > >         u8 smba_en_lo;
> > >         u8 port;
> > >         int retval;
> > > +       int timeout = 0;
> > > +       int smbslvcnt;
> > 
> > Keep them just after your another added variable.
> 
> FWIW, I don't think this makes sense as a general rule. I'd rather have
> the variables in an order which makes sense (for human readers or for
> stack size optimization - unless gcc does it for us?), rather than
> always adding at the same place. Is there a rationale for doing that? I
> don't think shrinking the patch size is good enough a reason.

Not really. Some say "Reorder to save bytes", some say "reorder to
utilize cache lines most". Unless I get some numbers showing the desired
effect, I go for "most readable" approach which is subjective, of
course. I'd be totally fine with the above.

[toc] | [prev] | [next] | [standalone]

#1557760

On Thu, Jan 12, 2017 at 12:00 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > > +       unsigned short piix4_smba = adapdata->smba;
>> > >         u8 smba_en_lo;
>> > >         u8 port;
>> > >         int retval;
>> > > +       int timeout = 0;
>> > > +       int smbslvcnt;
>> >
>> > Keep them just after your another added variable.
>>
>> FWIW, I don't think this makes sense as a general rule. I'd rather have
>> the variables in an order which makes sense (for human readers or for
>> stack size optimization - unless gcc does it for us?), rather than
>> always adding at the same place. Is there a rationale for doing that? I
>> don't think shrinking the patch size is good enough a reason.
>
> Not really. Some say "Reorder to save bytes", some say "reorder to
> utilize cache lines most". Unless I get some numbers showing the desired
> effect,

> I go for "most readable" approach which is subjective, of
> course. I'd be totally fine with the above.

My motivation was pure readability.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web