| Message ID | 20170110121600.30649-1-ricardo.ribalda@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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);
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!
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.
> > > + 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.
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.
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; }
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(+)