diff mbox

[RFC] i2c algo, Add i2c-algo-i801 driver [v1]

Message ID 1397060563-30431-1-git-send-email-prarit@redhat.com (mailing list archive)
State RFC, archived
Headers show

Commit Message

Prarit Bhargava April 9, 2014, 4:22 p.m. UTC
RFC and a work in progress ... I need to go through and do a bunch of error
condition checking, more testing, etc.  I'm just throwing this out there to see
if anyone has any major concerns about doing something like this.

TODO:

- decipher error returns from ACPI AML operations (and implement those too)
- additional error checking from i2c and acpi function calls (this code is
not robust enough)
- testing

P.

----8<----

Some ACPI tables on new systems implement an SBUS device in ACPI.  This results
in a conflict with the ACPI tables and the i2c-i801 driver over the address
space.  As a result the i2c-i801 driver will not load.  To workaround this, we
have users use the kernel parameter "acpi_enforce_resources=lax".  This,
isn't a good solution as I've seen other conflicts on some systems that are
also overriden.  I thought about implementing an i2c-core kernel parameter and
a wrapper function for acpi_check_resource_conflict() but that seems rather
clunky and doesn't resolve the issue of the shared resource between the ACPI
and the device.

There isn't any documentaton on Intel's website about the SBUS device but from
reading the acpidump it looks like the operations are straightforward.

SSXB
     transmit one byte
SRXB
     receive one byte
SWRB
     write one byte
SRDB
     read one byte
SWRW
     write one word
SRDW
     read one word
SBLW
     write block
SBRW
     read block

I've implemented these as an i2c algorithm so that users who cannot load the
regular i801 i2c driver can at least get the functionality they are looking
for.

Cc: Jean Delvare <khali@linux-fr.org>
Cc: linux-acpi@vger.kernel.org
Cc: Seth Heasley <seth.heasley@intel.com>
Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Janet Morgan <janet.morgan@intel.com>
Cc: Myron Stowe <mstowe@redhat.com>
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/i2c/algos/Kconfig         |    3 +
 drivers/i2c/algos/Makefile        |    1 +
 drivers/i2c/algos/i2c-algo-i801.c |  211 +++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig        |    1 +
 4 files changed, 216 insertions(+)
 create mode 100644 drivers/i2c/algos/i2c-algo-i801.c

Comments

Matthew Garrett April 9, 2014, 4:36 p.m. UTC | #1
On Wed, 2014-04-09 at 12:22 -0400, Prarit Bhargava wrote:
> RFC and a work in progress ... I need to go through and do a bunch of error

> condition checking, more testing, etc.  I'm just throwing this out there to see

> if anyone has any major concerns about doing something like this.


This isn't really a good approach. These aren't standardised ACPI
methods, so you have no guarantee that they exist. The fact that a
method with one of these names exists is no guarantee that it has the
same behaviour as the ones on your board. There's no guarantee that
you're not racing against the firmware.

These systems simply don't safely support OS-level i2c access.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
Jean Delvare April 9, 2014, 4:37 p.m. UTC | #2
Hi Prarit,

On Wed,  9 Apr 2014 12:22:43 -0400, Prarit Bhargava wrote:
> RFC and a work in progress ... I need to go through and do a bunch of error
> condition checking, more testing, etc.  I'm just throwing this out there to see
> if anyone has any major concerns about doing something like this.
> 
> TODO:
> 
> - decipher error returns from ACPI AML operations (and implement those too)
> - additional error checking from i2c and acpi function calls (this code is
> not robust enough)
> - testing
> 
> P.
> 
> ----8<----
> 
> Some ACPI tables on new systems implement an SBUS device in ACPI.  This results
> in a conflict with the ACPI tables and the i2c-i801 driver over the address
> space.  As a result the i2c-i801 driver will not load.  To workaround this, we
> have users use the kernel parameter "acpi_enforce_resources=lax".  This,
> isn't a good solution as I've seen other conflicts on some systems that are
> also overriden.  I thought about implementing an i2c-core kernel parameter and
> a wrapper function for acpi_check_resource_conflict() but that seems rather
> clunky and doesn't resolve the issue of the shared resource between the ACPI
> and the device.
> 
> There isn't any documentaton on Intel's website about the SBUS device but from
> reading the acpidump it looks like the operations are straightforward.
> 
> SSXB
>      transmit one byte
> SRXB
>      receive one byte
> SWRB
>      write one byte
> SRDB
>      read one byte
> SWRW
>      write one word
> SRDW
>      read one word
> SBLW
>      write block
> SBRW
>      read block
> 
> I've implemented these as an i2c algorithm so that users who cannot load the
> regular i801 i2c driver can at least get the functionality they are looking
> for.

Writing a driver for the ACPI interface to the SMBus is IMHO a very
good idea, thanks for doing that.

However I have two technical concerns with your approach:

1* What you wrote is NOT an "i2c algo". i2c "algorithms" are abstracted
I2C implementations, not correlated with any hardware or BIOS specific
interfaces. If you check other drivers under i2c/algos you'll see they
do NOT call i2c_add_adapter (although they may implement helper
wrappers around that function.) Hardware-specific drivers which build
on top of the algo driver are responsible for that.

What you wrote is much more like i2c/busses/i2c-scmi.c, so it is
actually an i2c _bus_ driver.

2* You are creating a link between i2c-i801 and your driver, where IMHO
there should not be any. Both drivers are independent, and from the
kernel's perspective, they are not even for the same hardware. In your
case the ACPI interface is backed by an i801-like chip, but the same
interface could totally be implemented on top of any other chip.

So I invite you to make your driver an independent i2c bus driver much
like i2c-scmi.

Thanks,
Prarit Bhargava April 9, 2014, 4:57 p.m. UTC | #3
On 04/09/2014 12:37 PM, Jean Delvare wrote:
> Hi Prarit,
> 
> On Wed,  9 Apr 2014 12:22:43 -0400, Prarit Bhargava wrote:
>> RFC and a work in progress ... I need to go through and do a bunch of error
>> condition checking, more testing, etc.  I'm just throwing this out there to see
>> if anyone has any major concerns about doing something like this.
>>
>> TODO:
>>
>> - decipher error returns from ACPI AML operations (and implement those too)
>> - additional error checking from i2c and acpi function calls (this code is
>> not robust enough)
>> - testing
>>
>> P.
>>
>> ----8<----
>>
>> Some ACPI tables on new systems implement an SBUS device in ACPI.  This results
>> in a conflict with the ACPI tables and the i2c-i801 driver over the address
>> space.  As a result the i2c-i801 driver will not load.  To workaround this, we
>> have users use the kernel parameter "acpi_enforce_resources=lax".  This,
>> isn't a good solution as I've seen other conflicts on some systems that are
>> also overriden.  I thought about implementing an i2c-core kernel parameter and
>> a wrapper function for acpi_check_resource_conflict() but that seems rather
>> clunky and doesn't resolve the issue of the shared resource between the ACPI
>> and the device.
>>
>> There isn't any documentaton on Intel's website about the SBUS device but from
>> reading the acpidump it looks like the operations are straightforward.
>>
>> SSXB
>>      transmit one byte
>> SRXB
>>      receive one byte
>> SWRB
>>      write one byte
>> SRDB
>>      read one byte
>> SWRW
>>      write one word
>> SRDW
>>      read one word
>> SBLW
>>      write block
>> SBRW
>>      read block
>>
>> I've implemented these as an i2c algorithm so that users who cannot load the
>> regular i801 i2c driver can at least get the functionality they are looking
>> for.
> 
> Writing a driver for the ACPI interface to the SMBus is IMHO a very
> good idea, thanks for doing that.
> 
> However I have two technical concerns with your approach:
> 
> 1* What you wrote is NOT an "i2c algo". i2c "algorithms" are abstracted
> I2C implementations, not correlated with any hardware or BIOS specific
> interfaces. If you check other drivers under i2c/algos you'll see they
> do NOT call i2c_add_adapter (although they may implement helper
> wrappers around that function.) Hardware-specific drivers which build
> on top of the algo driver are responsible for that.
> 
> What you wrote is much more like i2c/busses/i2c-scmi.c, so it is
> actually an i2c _bus_ driver.
> 
> 2* You are creating a link between i2c-i801 and your driver, where IMHO
> there should not be any. Both drivers are independent, and from the
> kernel's perspective, they are not even for the same hardware. In your
> case the ACPI interface is backed by an i801-like chip, but the same
> interface could totally be implemented on top of any other chip.
> 
> So I invite you to make your driver an independent i2c bus driver much
> like i2c-scmi.

Hey Jean -- thanks for the valuable feedback.  This is exactly why I RFC'd it :)
before I got in too deep :)

I'll rework the patch with your suggestions.  I may have (possibly dumb)
questions and I hope that's okay ..

P.
> 
> Thanks,
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Prarit Bhargava April 9, 2014, 5:02 p.m. UTC | #4
On 04/09/2014 12:36 PM, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 12:22 -0400, Prarit Bhargava wrote:
>> RFC and a work in progress ... I need to go through and do a bunch of error
>> condition checking, more testing, etc.  I'm just throwing this out there to see
>> if anyone has any major concerns about doing something like this.
> 
> This isn't really a good approach. These aren't standardised ACPI
> methods, so you have no guarantee that they exist. The fact that a

I've looked at the ACPI dump across six different systems (3 different vendors,
and an intel whitebox), and the data appears to be identical.  Could Intel
change these?  Yes, of course they could, but that's no different from any other
undocumented driver in the kernel.

> method with one of these names exists is no guarantee that it has the
> same behaviour as the ones on your board. There's no guarantee that
> you're not racing against the firmware.
> 

I think there is -- AFAICT the operations are serialized; if they aren't that is
an associated risk.  Hopefully someone from Intel will lend a hand here and let
me know if I'm doing something horrible ;)

> These systems simply don't safely support OS-level i2c access.

Possibly -- my interpretation of the ACPI AML could be wrong but it seems like
these operations are here for the OS.

I'm not denying there is a lot of risk in doing this...

P.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett April 9, 2014, 5:09 p.m. UTC | #5
On Wed, 2014-04-09 at 13:02 -0400, Prarit Bhargava wrote:
> 

> On 04/09/2014 12:36 PM, Matthew Garrett wrote:

> > On Wed, 2014-04-09 at 12:22 -0400, Prarit Bhargava wrote:

> >> RFC and a work in progress ... I need to go through and do a bunch of error

> >> condition checking, more testing, etc.  I'm just throwing this out there to see

> >> if anyone has any major concerns about doing something like this.

> > 

> > This isn't really a good approach. These aren't standardised ACPI

> > methods, so you have no guarantee that they exist. The fact that a

> 

> I've looked at the ACPI dump across six different systems (3 different vendors,

> and an intel whitebox), and the data appears to be identical.  Could Intel

> change these?  Yes, of course they could, but that's no different from any other

> undocumented driver in the kernel.


Not really. These are an internal implementation detail, not an exported
interface. We try to write drivers for exported interfaces, even if
they're not documented.

> > method with one of these names exists is no guarantee that it has the

> > same behaviour as the ones on your board. There's no guarantee that

> > you're not racing against the firmware.

> > 

> 

> I think there is -- AFAICT the operations are serialized; if they aren't that is

> an associated risk.  Hopefully someone from Intel will lend a hand here and let

> me know if I'm doing something horrible ;)


Imagine an i2c chip with indexed register access. What stops:

CPU0 (i2c):		CPU1 (ACPI):
SBWB register address
			SBWB register address
SBRB register value
			SBRB register value

and CPU0 getting back the wrong value?

-- 
Matthew Garrett <matthew.garrett@nebula.com>
Prarit Bhargava April 9, 2014, 5:34 p.m. UTC | #6
On 04/09/2014 01:09 PM, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 13:02 -0400, Prarit Bhargava wrote:
>>
>> On 04/09/2014 12:36 PM, Matthew Garrett wrote:
>>> On Wed, 2014-04-09 at 12:22 -0400, Prarit Bhargava wrote:
>>>> RFC and a work in progress ... I need to go through and do a bunch of error
>>>> condition checking, more testing, etc.  I'm just throwing this out there to see
>>>> if anyone has any major concerns about doing something like this.
>>>
>>> This isn't really a good approach. These aren't standardised ACPI
>>> methods, so you have no guarantee that they exist. The fact that a
>>
>> I've looked at the ACPI dump across six different systems (3 different vendors,
>> and an intel whitebox), and the data appears to be identical.  Could Intel
>> change these?  Yes, of course they could, but that's no different from any other
>> undocumented driver in the kernel.
> 
> Not really. These are an internal implementation detail, not an exported
> interface. We try to write drivers for exported interfaces, even if
> they're not documented.
> 
>>> method with one of these names exists is no guarantee that it has the
>>> same behaviour as the ones on your board. There's no guarantee that
>>> you're not racing against the firmware.
>>>
>>
>> I think there is -- AFAICT the operations are serialized; if they aren't that is
>> an associated risk.  Hopefully someone from Intel will lend a hand here and let
>> me know if I'm doing something horrible ;)
> 
> Imagine an i2c chip with indexed register access. What stops:
> 
> CPU0 (i2c):		CPU1 (ACPI):
> SBWB register address
> 			SBWB register address
> SBRB register value
> 			SBRB register value
> 

Your example is no different from what we've told people to do right now when
they see the ACPI resource conflict message and use a kernel parameter to
override the error condition.  I'm not disputing that this could be a problem --
see my previous comment about hoping that someone @ Intel will let us know if
we're doing something horrible.

P.

> and CPU0 getting back the wrong value?
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett April 9, 2014, 5:37 p.m. UTC | #7
On Wed, 2014-04-09 at 13:34 -0400, Prarit Bhargava wrote:
> On 04/09/2014 01:09 PM, Matthew Garrett wrote:

> > Imagine an i2c chip with indexed register access. What stops:

> > 

> > CPU0 (i2c):		CPU1 (ACPI):

> > SBWB register address

> > 			SBWB register address

> > SBRB register value

> > 			SBRB register value

> > 

> 

> Your example is no different from what we've told people to do right now when

> they see the ACPI resource conflict message and use a kernel parameter to

> override the error condition.  I'm not disputing that this could be a problem --

> see my previous comment about hoping that someone @ Intel will let us know if

> we're doing something horrible.


Right. It's dangerous, which is why we forbid it by default. How do we
benefit from having a driver that's no safer?

-- 
Matthew Garrett <matthew.garrett@nebula.com>
Prarit Bhargava April 9, 2014, 5:55 p.m. UTC | #8
On 04/09/2014 01:37 PM, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 13:34 -0400, Prarit Bhargava wrote:
>> On 04/09/2014 01:09 PM, Matthew Garrett wrote:
>>> Imagine an i2c chip with indexed register access. What stops:
>>>
>>> CPU0 (i2c):		CPU1 (ACPI):
>>> SBWB register address
>>> 			SBWB register address
>>> SBRB register value
>>> 			SBRB register value
>>>
>>
>> Your example is no different from what we've told people to do right now when
>> they see the ACPI resource conflict message and use a kernel parameter to
>> override the error condition.  I'm not disputing that this could be a problem --
>> see my previous comment about hoping that someone @ Intel will let us know if
>> we're doing something horrible.
> 
> Right. It's dangerous, which is why we forbid it by default. How do we
> benefit from having a driver that's no safer?

We have yet to see where the existing case exhibits the behaviour of a race.  In
fact, AFAICT, all we've seen is stability.  So it's no safer?  Yep.  It's as
equally not racy as the existing workaround.

P.

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett April 9, 2014, 5:59 p.m. UTC | #9
T24gV2VkLCAyMDE0LTA0LTA5IGF0IDEzOjU1IC0wNDAwLCBQcmFyaXQgQmhhcmdhdmEgd3JvdGU6
DQo+IA0KPiBPbiAwNC8wOS8yMDE0IDAxOjM3IFBNLCBNYXR0aGV3IEdhcnJldHQgd3JvdGU6DQo+
ID4gUmlnaHQuIEl0J3MgZGFuZ2Vyb3VzLCB3aGljaCBpcyB3aHkgd2UgZm9yYmlkIGl0IGJ5IGRl
ZmF1bHQuIEhvdyBkbyB3ZQ0KPiA+IGJlbmVmaXQgZnJvbSBoYXZpbmcgYSBkcml2ZXIgdGhhdCdz
IG5vIHNhZmVyPw0KPiANCj4gV2UgaGF2ZSB5ZXQgdG8gc2VlIHdoZXJlIHRoZSBleGlzdGluZyBj
YXNlIGV4aGliaXRzIHRoZSBiZWhhdmlvdXIgb2YgYSByYWNlLiAgSW4NCj4gZmFjdCwgQUZBSUNU
LCBhbGwgd2UndmUgc2VlbiBpcyBzdGFiaWxpdHkuICBTbyBpdCdzIG5vIHNhZmVyPyAgWWVwLiAg
SXQncyBhcw0KPiBlcXVhbGx5IG5vdCByYWN5IGFzIHRoZSBleGlzdGluZyB3b3JrYXJvdW5kLg0K
DQpTby4uLiB3aHkgYWRkIHRoZSBkcml2ZXIgYXQgYWxsPyBSZWZ1c2luZyB0byBwZXJtaXQgdGhl
IGtlcm5lbCB0byB0b3VjaA0KdGhlc2UgcmVzb3VyY2VzIGlzIGEgZGVsaWJlcmF0ZSBkZXNpZ24g
Y2hvaWNlLCBiZWNhdXNlIG9mIHRoZSBwb3RlbnRpYWwNCmZvciB0aGVzZSByYWNlcy4gSXQnbGwg
d29yayBhYnNvbHV0ZWx5IGZpbmUgcmlnaHQgdXAgdW50aWwgdGhlIHBvaW50DQp3aGVyZSB5b3Ug
ZW5kIHVwIHJlYWRpbmcgdGhlIHdyb25nIHRlbXBlcmF0dXJlIHZhbHVlIGZyb20gYW4gaTJjIGh3
bW9uDQpjaGlwIGFuZCBwZXJmb3JtIGEgY3JpdGljYWwgdGhlcm1hbCBzaHV0ZG93bi4NCg0KLS0g
DQpNYXR0aGV3IEdhcnJldHQgPG1hdHRoZXcuZ2FycmV0dEBuZWJ1bGEuY29tPg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Prarit Bhargava April 9, 2014, 6:02 p.m. UTC | #10
On 04/09/2014 01:59 PM, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 13:55 -0400, Prarit Bhargava wrote:
>>
>> On 04/09/2014 01:37 PM, Matthew Garrett wrote:
>>> Right. It's dangerous, which is why we forbid it by default. How do we
>>> benefit from having a driver that's no safer?
>>
>> We have yet to see where the existing case exhibits the behaviour of a race.  In
>> fact, AFAICT, all we've seen is stability.  So it's no safer?  Yep.  It's as
>> equally not racy as the existing workaround.
> 
> So... why add the driver at all? Refusing to permit the kernel to touch
> these resources is a deliberate design choice, because of the potential
> for these races. It'll work absolutely fine right up until the point
> where you end up reading the wrong temperature value from an i2c hwmon
> chip and perform a critical thermal shutdown.

Because it seems like the "right" thing to do is use the ACPI interface rather
than the i801 pci driver.  At least to me that makes sense.  OTOH, if Jean
thinks there isn't a need I can drop it.

P.

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett April 9, 2014, 6:27 p.m. UTC | #11
On Wed, 2014-04-09 at 14:02 -0400, Prarit Bhargava wrote:
> 

> On 04/09/2014 01:59 PM, Matthew Garrett wrote:

> > So... why add the driver at all? Refusing to permit the kernel to touch

> > these resources is a deliberate design choice, because of the potential

> > for these races. It'll work absolutely fine right up until the point

> > where you end up reading the wrong temperature value from an i2c hwmon

> > chip and perform a critical thermal shutdown.

> 

> Because it seems like the "right" thing to do is use the ACPI interface rather

> than the i801 pci driver.  At least to me that makes sense.  OTOH, if Jean

> thinks there isn't a need I can drop it.


We should use any ACPI interface that allows us to perform appropriate
synchronisation with the hardware, but there's no real advantage in
using an ACPI interface that's still racy.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
Jean Delvare April 9, 2014, 6:56 p.m. UTC | #12
Hi Matthew,

On Wed, 9 Apr 2014 16:36:32 +0000, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 12:22 -0400, Prarit Bhargava wrote:
> > RFC and a work in progress ... I need to go through and do a bunch of error
> > condition checking, more testing, etc.  I'm just throwing this out there to see
> > if anyone has any major concerns about doing something like this.
> 
> This isn't really a good approach. These aren't standardised ACPI
> methods, so you have no guarantee that they exist. The fact that a
> method with one of these names exists is no guarantee that it has the
> same behaviour as the ones on your board. There's no guarantee that
> you're not racing against the firmware.
> 
> These systems simply don't safely support OS-level i2c access.

But people need that. And they have been for almost a decade. It's
about time that hardware vendors realize that. Yes, I mean Intel,
amongst others.

Prarit's approach may not be perfect but it has the merit of bringing
the topic for discussion, at last. And using his driver would still be
much safer than using i2c-i801 with acpi_enforce_resources=lax. If we
can come up with a detection method that is reliable enough, it looks
completely acceptable to me.

Of course it would be much better if vendors could just implement the
standard SMBus CMI interface, but having a way to deal with existing
hardware and BIOSes is still good.
Matthew Garrett April 9, 2014, 6:58 p.m. UTC | #13
On Wed, 2014-04-09 at 20:56 +0200, Jean Delvare wrote:
> Hi Matthew,

> > 

> > These systems simply don't safely support OS-level i2c access.

> 

> But people need that. And they have been for almost a decade. It's

> about time that hardware vendors realize that. Yes, I mean Intel,

> amongst others.


There is no safe way to do it. If the user is happy to accept the
inherent dangers, the user can pass the kernel parameter.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
Jean Delvare April 9, 2014, 7:01 p.m. UTC | #14
On Wed, 9 Apr 2014 17:09:41 +0000, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 13:02 -0400, Prarit Bhargava wrote:
> > On 04/09/2014 12:36 PM, Matthew Garrett wrote:
> > > method with one of these names exists is no guarantee that it has the
> > > same behaviour as the ones on your board. There's no guarantee that
> > > you're not racing against the firmware.
> > 
> > I think there is -- AFAICT the operations are serialized; if they aren't that is
> > an associated risk.  Hopefully someone from Intel will lend a hand here and let
> > me know if I'm doing something horrible ;)
> 
> Imagine an i2c chip with indexed register access. What stops:
> 
> CPU0 (i2c):		CPU1 (ACPI):
> SBWB register address
> 			SBWB register address
> SBRB register value
> 			SBRB register value
> 
> and CPU0 getting back the wrong value?

Certainly there is some ACPI lock to prevent ACPI from racing with
itself. Can't we just use it too?
Matthew Garrett April 9, 2014, 7:05 p.m. UTC | #15
On Wed, 2014-04-09 at 21:01 +0200, Jean Delvare wrote:

> Certainly there is some ACPI lock to prevent ACPI from racing with

> itself. Can't we just use it too?


There may be, but it's up to the firmware vendor to implement that and
there's no standard. Some systems may just skip it because there's only
a single entry point.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
Jean Delvare April 9, 2014, 7:22 p.m. UTC | #16
On Wed, 9 Apr 2014 17:59:03 +0000, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 13:55 -0400, Prarit Bhargava wrote:
> > 
> > On 04/09/2014 01:37 PM, Matthew Garrett wrote:
> > > Right. It's dangerous, which is why we forbid it by default. How do we
> > > benefit from having a driver that's no safer?
> > 
> > We have yet to see where the existing case exhibits the behaviour of a race.  In
> > fact, AFAICT, all we've seen is stability.  So it's no safer?  Yep.  It's as
> > equally not racy as the existing workaround.
> 
> So... why add the driver at all? Refusing to permit the kernel to touch
> these resources is a deliberate design choice, because of the potential
> for these races. (...)

So it is a deliberate design choice to prevent the user from accessing
his/her own hardware? I want to be able to read the SPD EEPROMs and the
temperature sensors on my memory modules, and this requires SMBus
access.

The ACPI interface to hardware monitoring chips is horribly limited.
Until the ACPI people come up with something which really exposes most
of the features of the hardware monitoring chips currently provide,
native access it required. At the moment, when a full-featured hardware
monitoring chip monitors 5 to 10 voltage inputs, 5 fans and 3
temperature sensors, ACPI will show 2 thermal zones at best, not always
updated in real-time, with limited resolution, and the fans are shown
without their speed. That simply sucks.

So we need either proper ACPI interfaces to all the useful features, or
low-level, safe access to the registers through ACPI. Until we have
that, we are condemned to live dangerously in an hostile world :(
Matthew Garrett April 9, 2014, 7:24 p.m. UTC | #17
T24gV2VkLCAyMDE0LTA0LTA5IGF0IDIxOjIyICswMjAwLCBKZWFuIERlbHZhcmUgd3JvdGU6DQo+
IE9uIFdlZCwgOSBBcHIgMjAxNCAxNzo1OTowMyArMDAwMCwgTWF0dGhldyBHYXJyZXR0IHdyb3Rl
Og0KPiA+IFNvLi4uIHdoeSBhZGQgdGhlIGRyaXZlciBhdCBhbGw/IFJlZnVzaW5nIHRvIHBlcm1p
dCB0aGUga2VybmVsIHRvIHRvdWNoDQo+ID4gdGhlc2UgcmVzb3VyY2VzIGlzIGEgZGVsaWJlcmF0
ZSBkZXNpZ24gY2hvaWNlLCBiZWNhdXNlIG9mIHRoZSBwb3RlbnRpYWwNCj4gPiBmb3IgdGhlc2Ug
cmFjZXMuICguLi4pDQo+IA0KPiBTbyBpdCBpcyBhIGRlbGliZXJhdGUgZGVzaWduIGNob2ljZSB0
byBwcmV2ZW50IHRoZSB1c2VyIGZyb20gYWNjZXNzaW5nDQo+IGhpcy9oZXIgb3duIGhhcmR3YXJl
PyBJIHdhbnQgdG8gYmUgYWJsZSB0byByZWFkIHRoZSBTUEQgRUVQUk9NcyBhbmQgdGhlDQo+IHRl
bXBlcmF0dXJlIHNlbnNvcnMgb24gbXkgbWVtb3J5IG1vZHVsZXMsIGFuZCB0aGlzIHJlcXVpcmVz
IFNNQnVzDQo+IGFjY2Vzcy4NCg0KU3VyZSwgYW5kIGlmIHlvdSBoYXZlIG1hZGUgYSBjb25zY2lv
dXMgZGVjaXNpb24gdGhhdCBpdCdzIHNhZmUgdG8gZG8gc28NCmluIHlvdXIgc3BlY2lmaWMgdXNl
IGNhc2UgdGhlbiB5b3UgY2FuIHBhc3MgdGhlIGtlcm5lbCBwYXJhbWV0ZXIgdGhhdA0KYWxsb3dz
IHlvdSB0by4gQnV0IGl0J3MgdW5zYWZlIGluIHRoZSBnZW5lcmFsIGNhc2UsIGFuZCBzbyB0aGUN
CmFwcHJvcHJpYXRlIHRoaW5nIGlzIGZvciB0aGUga2VybmVsIHRvIGZvcmJpZCBpdC4NCg0KLS0g
DQpNYXR0aGV3IEdhcnJldHQgPG1hdHRoZXcuZ2FycmV0dEBuZWJ1bGEuY29tPg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Delvare April 9, 2014, 8:25 p.m. UTC | #18
On Wed, 9 Apr 2014 18:58:12 +0000, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 20:56 +0200, Jean Delvare wrote:
> > Hi Matthew,
> > > 
> > > These systems simply don't safely support OS-level i2c access.
> > 
> > But people need that. And they have been for almost a decade. It's
> > about time that hardware vendors realize that. Yes, I mean Intel,
> > amongst others.
> 
> There is no safe way to do it. If the user is happy to accept the
> inherent dangers, the user can pass the kernel parameter.

Oh, there is. There always is. It would just take a few engineers, an
equal amount of chairs and pencils, a table and a few sheets of paper.
There's no technical reason why this problem can't be solved.
Jean Delvare April 9, 2014, 8:27 p.m. UTC | #19
On Wed, 09 Apr 2014 12:57:36 -0400, Prarit Bhargava wrote:
> I'll rework the patch with your suggestions.  I may have (possibly dumb)
> questions and I hope that's okay ..

No problem, I'll help as much as I can :)
Prarit Bhargava April 10, 2014, 7:15 p.m. UTC | #20
On 04/09/2014 01:09 PM, Matthew Garrett wrote:
> On Wed, 2014-04-09 at 13:02 -0400, Prarit Bhargava wrote:
>>
>> On 04/09/2014 12:36 PM, Matthew Garrett wrote:
>>> On Wed, 2014-04-09 at 12:22 -0400, Prarit Bhargava wrote:
>>>> RFC and a work in progress ... I need to go through and do a bunch of error
>>>> condition checking, more testing, etc.  I'm just throwing this out there to see
>>>> if anyone has any major concerns about doing something like this.
>>>
>>> This isn't really a good approach. These aren't standardised ACPI
>>> methods, so you have no guarantee that they exist. The fact that a
>>
>> I've looked at the ACPI dump across six different systems (3 different vendors,
>> and an intel whitebox), and the data appears to be identical.  Could Intel
>> change these?  Yes, of course they could, but that's no different from any other
>> undocumented driver in the kernel.
> 
> Not really. These are an internal implementation detail, not an exported
> interface. We try to write drivers for exported interfaces, even if
> they're not documented.

Aren't the methods the exported interface?  I'm obviously missing something :)

> 
>>> method with one of these names exists is no guarantee that it has the
>>> same behaviour as the ones on your board. There's no guarantee that
>>> you're not racing against the firmware.
>>>
>>
>> I think there is -- AFAICT the operations are serialized; if they aren't that is
>> an associated risk.  Hopefully someone from Intel will lend a hand here and let
>> me know if I'm doing something horrible ;)
> 
> Imagine an i2c chip with indexed register access. What stops:
> 
> CPU0 (i2c):		CPU1 (ACPI):
> SBWB register address
> 			SBWB register address
> SBRB register value
> 			SBRB register value
> 
> and CPU0 getting back the wrong value?

I thought that the "Serialized" keyword in the methods specifically indicate
that this cannot happen.

/me could be confused but from the ACPI spec:

SerializeRule is optional and is a flag that defines whether the method is
serialized or not and is one of the following: Serialized or NotSerialized. A
method that is serialized cannot be reentered by additional threads. If not
specified, the default is NotSerialized.

P.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett April 10, 2014, 8:26 p.m. UTC | #21
On Thu, 2014-04-10 at 15:15 -0400, Prarit Bhargava wrote:
> 

> On 04/09/2014 01:09 PM, Matthew Garrett wrote:

> > Not really. These are an internal implementation detail, not an exported

> > interface. We try to write drivers for exported interfaces, even if

> > they're not documented.

> 

> Aren't the methods the exported interface?  I'm obviously missing something :)


No. The device doesn't have a _HID(), so they're internal methods.

> > Imagine an i2c chip with indexed register access. What stops:

> > 

> > CPU0 (i2c):		CPU1 (ACPI):

> > SBWB register address

> > 			SBWB register address

> > SBRB register value

> > 			SBRB register value

> > 

> > and CPU0 getting back the wrong value?

> 

> I thought that the "Serialized" keyword in the methods specifically indicate

> that this cannot happen.


It means you can't run the same method twice at the same time, but in
the above case you're not doing that. Nothing ensures that SBWB can't be
run again before SBRB is run.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
Prarit Bhargava April 11, 2014, 5:47 p.m. UTC | #22
On 04/10/2014 04:26 PM, Matthew Garrett wrote:
> On Thu, 2014-04-10 at 15:15 -0400, Prarit Bhargava wrote:
>>
>> On 04/09/2014 01:09 PM, Matthew Garrett wrote:
>>> Not really. These are an internal implementation detail, not an exported
>>> interface. We try to write drivers for exported interfaces, even if
>>> they're not documented.
>>
>> Aren't the methods the exported interface?  I'm obviously missing something :)
> 
> No. The device doesn't have a _HID(), so they're internal methods.
> 
>>> Imagine an i2c chip with indexed register access. What stops:
>>>

I think I missed something in this example (and if I have this wrong, please say
so).

Are you saying *both* the old (pci-style) driver and my new driver (ACPI) are
loaded?  Or something else?

>
>>> CPU0 (i2c):		CPU1 (ACPI):
>>> SBWB register address
>>> 			SBWB register address
>>> SBRB register value
>>> 			SBRB register value
>>>
>>> and CPU0 getting back the wrong value?

P.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett April 11, 2014, 6:55 p.m. UTC | #23
T24gRnJpLCAyMDE0LTA0LTExIGF0IDEzOjQ3IC0wNDAwLCBQcmFyaXQgQmhhcmdhdmEgd3JvdGU6
DQoNCj4gSSB0aGluayBJIG1pc3NlZCBzb21ldGhpbmcgaW4gdGhpcyBleGFtcGxlIChhbmQgaWYg
SSBoYXZlIHRoaXMgd3JvbmcsIHBsZWFzZSBzYXkNCj4gc28pLg0KPiANCj4gQXJlIHlvdSBzYXlp
bmcgKmJvdGgqIHRoZSBvbGQgKHBjaS1zdHlsZSkgZHJpdmVyIGFuZCBteSBuZXcgZHJpdmVyIChB
Q1BJKSBhcmUNCj4gbG9hZGVkPyAgT3Igc29tZXRoaW5nIGVsc2U/DQoNCllvdXIgQUNQSSBkcml2
ZXIgaXMgbG9hZGVkLiBUaGUgdXNlciBhdHRlbXB0cyB0byByZWFkIGEgdmFsdWUgZnJvbSBhDQp0
aGVybWFsIGh3bW9uIGNoaXAuIFlvdSBleGVjdXRlIFNCV0IgaW4gb3JkZXIgdG8gd3JpdGUgdGhl
IHJlZ2lzdGVyDQphZGRyZXNzIHRvIHRoZSBjaGlwLiBZb3UgdGhlbiB0YWtlIGFuIEFDUEkgaW50
ZXJydXB0LiBUaGUgQUNQSSBjb3JlDQpleGVjdXRlcyBhIG1ldGhvZCB0aGF0IGV4ZWN1dGVzIFNC
V0IgYW5kIHdyaXRlcyBhIGRpZmZlcmVudCByZWdpc3Rlcg0KYWRkcmVzcyB0byB0aGUgY2hpcC4g
Q29udHJvbCBwYXNzZXMgYmFjayB0byB5b3UuIFlvdSBjYWxsIFNCUkIgYW5kIHJlYWQNCmJhY2sg
YSB2YWx1ZSBmcm9tIHRoZSB3cm9uZyByZWdpc3Rlci4NCg0KLS0gDQpNYXR0aGV3IEdhcnJldHQg
PG1hdHRoZXcuZ2FycmV0dEBuZWJ1bGEuY29tPg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/i2c/algos/Kconfig b/drivers/i2c/algos/Kconfig
index f1cfe7e..98b382c 100644
--- a/drivers/i2c/algos/Kconfig
+++ b/drivers/i2c/algos/Kconfig
@@ -14,4 +14,7 @@  config I2C_ALGOPCF
 config I2C_ALGOPCA
 	tristate "I2C PCA 9564 interfaces"
 
+config I2C_ALGOI801
+	tristate "I2C I801 interfaces"
+
 endmenu
diff --git a/drivers/i2c/algos/Makefile b/drivers/i2c/algos/Makefile
index 215303f..2c7cc99 100644
--- a/drivers/i2c/algos/Makefile
+++ b/drivers/i2c/algos/Makefile
@@ -5,5 +5,6 @@ 
 obj-$(CONFIG_I2C_ALGOBIT)	+= i2c-algo-bit.o
 obj-$(CONFIG_I2C_ALGOPCF)	+= i2c-algo-pcf.o
 obj-$(CONFIG_I2C_ALGOPCA)	+= i2c-algo-pca.o
+obj-$(CONFIG_I2C_ALGOI801)	+= i2c-algo-i801.o
 
 ccflags-$(CONFIG_I2C_DEBUG_ALGO) := -DDEBUG
diff --git a/drivers/i2c/algos/i2c-algo-i801.c b/drivers/i2c/algos/i2c-algo-i801.c
new file mode 100644
index 0000000..c7e615c3
--- /dev/null
+++ b/drivers/i2c/algos/i2c-algo-i801.c
@@ -0,0 +1,211 @@ 
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+
+/* From an ACPI dump the supported ACPI SBUS operations are:
+ *
+ * SSXB
+ *	transmit one byte
+ * SRXB
+ *	receive one byte
+ * SWRB
+ *	write one byte
+ * SRDB
+ *	read one byte
+ * SWRW
+ *	write one word
+ * SRDW
+ *	read one word
+ * SBLW
+ *	write block
+ * SBRW
+ *	read block
+ *
+ * A lot of this code is based on what the existing i2c-i801.c driver is
+ * doing.  Note that the i2c-i801.c driver also implements I2C_SMBUS_QUICK,
+ * and I2C_SMBUS_I2C_BLOCK_DATA which do not appear to be implemented in the
+ * ACPI driver.
+ */
+
+struct i2c_adapter i2c_acpi_sbus_adapter;
+
+/* all operations are noted as being serialized in the acpidump so no
+ * locking should be required.
+ */
+static s32 i2c_acpi_sbus_access(struct i2c_adapter *adap, u16 addr,
+				unsigned short flags, char read_write,
+				u8 command, int size,
+				union i2c_smbus_data *data)
+{
+	acpi_status status;
+	acpi_handle handle = adap->algo_data;
+	char *op;
+	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_object_list args;
+	union acpi_object *buffer_obj;
+	union acpi_object objects[4];
+	unsigned long long value;
+
+	args.pointer = objects;
+	objects[0].type = ACPI_TYPE_INTEGER;
+	/* taken directly from i2c-smbus.c */
+	objects[0].integer.value = ((addr & 0x7f) << 1) | (read_write & 0x01);
+
+	switch (size) {
+	case I2C_SMBUS_BYTE:
+		if (read_write == I2C_SMBUS_WRITE) {
+			op = "SSXB";
+			args.count = 2;
+			objects[1].type = ACPI_TYPE_INTEGER;
+			objects[1].integer.value = command;
+		} else {
+			op = "SRXB";
+			args.count = 1;
+		}
+		status = acpi_evaluate_integer(handle, op, &args, &value);
+		data->byte = value;
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		objects[1].type = ACPI_TYPE_INTEGER;
+		objects[1].integer.value = command;
+		if (read_write == I2C_SMBUS_WRITE) {
+			op = "SWRB";
+			args.count = 3;
+			objects[2].type = ACPI_TYPE_INTEGER;
+			objects[2].integer.value = data->byte;
+		} else {
+			op = "SRDB";
+			args.count = 2;
+		}
+		status = acpi_evaluate_integer(handle, op, &args, &value);
+		data->byte = value;
+		break;
+	case I2C_SMBUS_WORD_DATA:
+		objects[1].type = ACPI_TYPE_INTEGER;
+		objects[1].integer.value = command;
+		if (read_write == I2C_SMBUS_WRITE) {
+			op = "SWRW";
+			args.count = 3;
+			objects[2].type = ACPI_TYPE_INTEGER;
+			objects[2].integer.value = data->word;
+		} else {
+			op = "SRDW";
+			args.count = 2;
+		}
+		status = acpi_evaluate_integer(handle, op, &args, &value);
+		data->word = value;
+		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		objects[1].type = ACPI_TYPE_INTEGER;
+		objects[1].integer.value = command;
+		if (read_write == I2C_SMBUS_WRITE) {
+			op = "SBLW";
+			args.count = 3;
+			objects[2].buffer.length = data->block[0];
+			objects[2].buffer.pointer = data->block + 1;
+			status = acpi_evaluate_integer(handle, op, &args,
+						       &value);
+			data->byte = value;
+		} else {
+			op = "SBLR";
+			args.count = 2;
+			status = acpi_evaluate_object(handle, op, &args,
+						      &buffer);
+			if (ACPI_SUCCESS(status)) {
+				buffer_obj = buffer.pointer;
+				memcpy(data->block,
+				       buffer_obj->buffer.pointer,
+				       buffer_obj->buffer.length);
+			}
+		}
+		break;
+	default:
+		pr_warn("%s: Unsupported transaction %d\n",
+			i2c_acpi_sbus_adapter.name, size);
+		return -EOPNOTSUPP;
+	}
+
+	if (ACPI_FAILURE(status)) {
+		pr_warn("%s: Transaction failure.\n",
+			i2c_acpi_sbus_adapter.name);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static u32 i2c_acpi_sbus_func(struct i2c_adapter *adapter)
+{
+	return (I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BLOCK_DATA);
+}
+
+static const struct i2c_algorithm i2c_acpi_sbus_algorithm = {
+	.smbus_xfer	= i2c_acpi_sbus_access,
+	.functionality	= i2c_acpi_sbus_func,
+};
+
+struct i2c_adapter i2c_acpi_sbus_adapter = {
+	.owner	= THIS_MODULE,
+	.class	= I2C_CLASS_HWMON,
+	.algo	= &i2c_acpi_sbus_algorithm,
+	.name	= "i2c ACPI(SBUS) SMBus",
+};
+
+static acpi_status i2c_acpi_sbus_find_device(acpi_handle handle, u32 level,
+					     void *context, void **return_value)
+{
+	acpi_status status;
+	char node_name[5];
+	struct acpi_buffer buffer = {sizeof(node_name), node_name};
+	struct acpi_device *device = NULL;
+	int ret;
+
+	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
+	if (ACPI_FAILURE(status) || strcmp("SBUS", node_name) != 0)
+		return AE_OK;
+
+	acpi_bus_get_device(handle, &device);
+	if (!device) {
+		pr_err("%s: SBUS name found but no matching device\n",
+		       i2c_acpi_sbus_adapter.name);
+		return AE_NOT_FOUND;
+	}
+
+	i2c_acpi_sbus_adapter.dev.parent = &device->dev;
+	i2c_acpi_sbus_adapter.algo_data = handle;
+
+	ret = i2c_add_adapter(&i2c_acpi_sbus_adapter);
+	if (ret)
+		pr_err("%s: i2c core failed to add i2c_sbus\n",
+		       i2c_acpi_sbus_adapter.name);
+		return AE_ERROR;
+	}
+
+	pr_info("%s device found.\n", i2c_acpi_sbus_adapter.name);
+	return AE_OK;
+}
+
+static int __init i2c_acpi_sbus_init(void)
+{
+	acpi_status status;
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX, i2c_acpi_sbus_find_device,
+				     NULL, NULL, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_debug("%s no device found\n", i2c_acpi_sbus_adapter.name);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void __exit i2c_acpi_sbus_exit(void)
+{
+	i2c_del_adapter(&i2c_acpi_sbus_adapter);
+}
+
+module_init(i2c_acpi_sbus_init);
+module_exit(i2c_acpi_sbus_exit);
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 014afab..a4d97ae 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -81,6 +81,7 @@  config I2C_I801
 	tristate "Intel 82801 (ICH/PCH)"
 	depends on PCI
 	select CHECK_SIGNATURE if X86 && DMI
+	select I2C_ALGOI801
 	help
 	  If you say yes to this option, support will be included for the Intel
 	  801 family of mainboard I2C interfaces.  Specifically, the following