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

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

[Prarit Bhargava]

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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Seth Heasley <seth.heasley-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Rafael J. Wysocki <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org>
Cc: Janet Morgan <janet.morgan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Myron Stowe <mstowe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

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
-- 
1.7.9.3

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Jean Delvare]

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,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Prarit Bhargava]

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,
> 

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Prarit Bhargava]

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.

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Prarit Bhargava]

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?
> 

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Prarit Bhargava]

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.

> 

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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.

-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Prarit Bhargava]

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.

> 

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Jean Delvare]

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?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Jean Delvare]

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 :(

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

On Wed, 2014-04-09 at 21:22 +0200, Jean Delvare wrote:
> On Wed, 9 Apr 2014 17:59:03 +0000, 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. (...)
> 
> 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.

Sure, and if you have made a conscious decision that it's safe to do so
in your specific use case then you can pass the kernel parameter that
allows you to. But it's unsafe in the general case, and so the
appropriate thing is for the kernel to forbid it.

-- 
Matthew Garrett <matthew.garrett@nebula.com>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Jean Delvare]

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
SUSE L3 Support

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Jean Delvare]

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 :)

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Prarit Bhargava]

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.

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

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>

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Prarit Bhargava]

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.

Re: [PATCH RFC] i2c algo, Add i2c-algo-i801 driver [v1]

[Matthew Garrett]

On Fri, 2014-04-11 at 13:47 -0400, Prarit Bhargava wrote:

> 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?

Your ACPI driver is loaded. The user attempts to read a value from a
thermal hwmon chip. You execute SBWB in order to write the register
address to the chip. You then take an ACPI interrupt. The ACPI core
executes a method that executes SBWB and writes a different register
address to the chip. Control passes back to you. You call SBRB and read
back a value from the wrong register.

-- 
Matthew Garrett <matthew.garrett@nebula.com>