Message ID | 1397060563-30431-1-git-send-email-prarit@redhat.com (mailing list archive) |
---|---|
State | RFC, archived |
Headers | show |
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>
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,
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
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
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>
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
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>
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
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
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
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>
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.
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>
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?
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>
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 :(
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
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.
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 :)
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
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>
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
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 --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
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