* [PATCH] i2c: piix4: Avoid race conditions with IMC
@ 2017-01-10 12:16 Ricardo Ribalda Delgado
2017-01-11 1:49 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Ricardo Ribalda Delgado @ 2017-01-10 12:16 UTC (permalink / raw)
To: Jean Delvare, Wolfram Sang, linux-i2c, linux-kernel
Cc: Ricardo Ribalda Delgado
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
2017-01-10 12:16 [PATCH] i2c: piix4: Avoid race conditions with IMC Ricardo Ribalda Delgado
@ 2017-01-11 1:49 ` Andy Shevchenko
2017-01-11 9:11 ` Ricardo Ribalda Delgado
2017-01-12 9:40 ` Jean Delvare
0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2017-01-11 1:49 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Jean Delvare, Wolfram Sang, linux-i2c,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
2017-01-11 1:49 ` Andy Shevchenko
@ 2017-01-11 9:11 ` Ricardo Ribalda Delgado
2017-01-12 9:40 ` Jean Delvare
1 sibling, 0 replies; 6+ messages in thread
From: Ricardo Ribalda Delgado @ 2017-01-11 9:11 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jean Delvare, Wolfram Sang, linux-i2c,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
2017-01-11 1:49 ` Andy Shevchenko
2017-01-11 9:11 ` Ricardo Ribalda Delgado
@ 2017-01-12 9:40 ` Jean Delvare
2017-01-12 10:00 ` Wolfram Sang
1 sibling, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2017-01-12 9:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Ricardo Ribalda Delgado, Wolfram Sang, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
2017-01-12 9:40 ` Jean Delvare
@ 2017-01-12 10:00 ` Wolfram Sang
2017-01-12 19:37 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Wolfram Sang @ 2017-01-12 10:00 UTC (permalink / raw)
To: Jean Delvare
Cc: Andy Shevchenko, Ricardo Ribalda Delgado, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]
> > > + 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: piix4: Avoid race conditions with IMC
2017-01-12 10:00 ` Wolfram Sang
@ 2017-01-12 19:37 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2017-01-12 19:37 UTC (permalink / raw)
To: Wolfram Sang
Cc: Jean Delvare, Ricardo Ribalda Delgado, linux-i2c,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-12 19:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-10 12:16 [PATCH] i2c: piix4: Avoid race conditions with IMC Ricardo Ribalda Delgado
2017-01-11 1:49 ` Andy Shevchenko
2017-01-11 9:11 ` Ricardo Ribalda Delgado
2017-01-12 9:40 ` Jean Delvare
2017-01-12 10:00 ` Wolfram Sang
2017-01-12 19:37 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox