[RFC 0/1] i2c: acpi: revert setting a "stable" device name

[RFC 0/1] i2c: acpi: revert setting a "stable" device name

[Dustin Byford]

70762ab from 11/2014 (i2c: Use stable dev_name for ACPI enumerated I2C
slaves) modified the sysfs-visible dev_name() for ACPI enumerated I2C
devices.  With that change, /sys/bus/i2c/devices/i2c-0-004a, for
example, became /sys/bus/i2c/devices/i2c-PNPXXXX:xx

That causes problems for userspace code such as 'sensors' which does
this:

lib/sysfs.c:665:
if ((!subsys || !strcmp(subsys, "i2c")) &&
    sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr,
           &entry.chip.addr) == 2) {
...

Therefore, in theory, sensors that were previously visible by running
'sensors' no longer show up.  On the other hand, there are probably few,
if any, cases of this because ACPI enumerated I2C hwmon devices are not
common.

I'm not defending the 'sensors' code, I'm sure there are better ways to
discover a hwmon I2C device from userspace.  But, I'm also not sure
70762ab achieved its stated goal in a meaningful way.  Won't
"i2c-<acpi_dev_name>" also vary with ACPI scan order, BIOS settings,
firmware upgrades, etc...?

Hence the RFC patch.  To submit a change like this I would need to
consider the fallout for ALSA SoC.  The other option is to see what can
be done in 'sensors' to include the ACPI enumerated hwmon devices.

Any opinions on which way to go?

   --Dustin

Dustin Byford (1):
  i2c: acpi: revert setting a "stable" device name

 drivers/i2c/i2c-core.c | 7 -------
 1 file changed, 7 deletions(-)

-- 
2.1.4

[RFC 1/1] i2c: acpi: revert setting a "stable" device name

[Dustin Byford]

70762ab from 11/2014 (i2c: Use stable dev_name for ACPI enumerated I2C
slaves) modified the sysfs-visible dev_name() for ACPI enumerated I2C
devices.  With that change, /sys/bus/i2c/devices/i2c-0-004a, for
example, became /sys/bus/i2c/devices/i2c-PNPXXXX:xx

That causes problems for userspace code such as 'sensors' which does
this:

lib/sysfs.c:665:
if ((!subsys || !strcmp(subsys, "i2c")) &&
    sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr,
           &entry.chip.addr) == 2) {
...

Fix 'sensors' by reverting the kernel change.

Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com>
---
 drivers/i2c/i2c-core.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c83e4d1..fb77031 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -916,13 +916,6 @@ EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
 static void i2c_dev_set_name(struct i2c_adapter *adap,
 			     struct i2c_client *client)
 {
-	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
-
-	if (adev) {
-		dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
-		return;
-	}
-
 	/* For 10-bit clients, add an arbitrary offset to avoid collisions */
 	dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap),
 		     client->addr | ((client->flags & I2C_CLIENT_TEN)
-- 
2.1.4

Re: [RFC 0/1] i2c: acpi: revert setting a "stable" device name

[Guenter Roeck]

On Fri, Aug 14, 2015 at 12:37:13PM -0700, Dustin Byford wrote:
> 70762ab from 11/2014 (i2c: Use stable dev_name for ACPI enumerated I2C
> slaves) modified the sysfs-visible dev_name() for ACPI enumerated I2C
> devices.  With that change, /sys/bus/i2c/devices/i2c-0-004a, for
> example, became /sys/bus/i2c/devices/i2c-PNPXXXX:xx
> 
> That causes problems for userspace code such as 'sensors' which does
> this:
> 
> lib/sysfs.c:665:
> if ((!subsys || !strcmp(subsys, "i2c")) &&
>     sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr,
>            &entry.chip.addr) == 2) {
> ...
> 
> Therefore, in theory, sensors that were previously visible by running
> 'sensors' no longer show up.  On the other hand, there are probably few,
> if any, cases of this because ACPI enumerated I2C hwmon devices are not
> common.
> 
> I'm not defending the 'sensors' code, I'm sure there are better ways to
> discover a hwmon I2C device from userspace.  But, I'm also not sure

Is it necessary to defend user space applications nowadays if a kernel change
breaks a well established ABI ? WHat happened to "Thou Shalt Not Break
Userspace" ?

I absolutely agree that i2c bus renumbering across reboots is a problem.
However, it seems to me that 70762ab doesn't solve that problem, it just
paints it over. And, as you have noticed, it introduces new problems along
the way.

Guenter

> 70762ab achieved its stated goal in a meaningful way.  Won't
> "i2c-<acpi_dev_name>" also vary with ACPI scan order, BIOS settings,
> firmware upgrades, etc...?
> 
> Hence the RFC patch.  To submit a change like this I would need to
> consider the fallout for ALSA SoC.  The other option is to see what can
> be done in 'sensors' to include the ACPI enumerated hwmon devices.
> 
> Any opinions on which way to go?
> 
>    --Dustin
> 
> Dustin Byford (1):
>   i2c: acpi: revert setting a "stable" device name
> 
>  drivers/i2c/i2c-core.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> -- 
> 2.1.4
> 

Re: [RFC 0/1] i2c: acpi: revert setting a "stable" device name

[Jarkko Nikula]

On 15.08.2015 18:13, Guenter Roeck wrote:
> On Fri, Aug 14, 2015 at 12:37:13PM -0700, Dustin Byford wrote:
>> 70762ab from 11/2014 (i2c: Use stable dev_name for ACPI enumerated I2C
>> slaves) modified the sysfs-visible dev_name() for ACPI enumerated I2C
>> devices.  With that change, /sys/bus/i2c/devices/i2c-0-004a, for
>> example, became /sys/bus/i2c/devices/i2c-PNPXXXX:xx
>>
>> That causes problems for userspace code such as 'sensors' which does
>> this:
>>
>> lib/sysfs.c:665:
>> if ((!subsys || !strcmp(subsys, "i2c")) &&
>>      sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr,
>>             &entry.chip.addr) == 2) {
>> ...
>>
>> Therefore, in theory, sensors that were previously visible by running
>> 'sensors' no longer show up.  On the other hand, there are probably few,
>> if any, cases of this because ACPI enumerated I2C hwmon devices are not
>> common.
>>
>> I'm not defending the 'sensors' code, I'm sure there are better ways to
>> discover a hwmon I2C device from userspace.  But, I'm also not sure
>
> Is it necessary to defend user space applications nowadays if a kernel change
> breaks a well established ABI ? WHat happened to "Thou Shalt Not Break
> Userspace" ?
>
> I absolutely agree that i2c bus renumbering across reboots is a problem.
> However, it seems to me that 70762ab doesn't solve that problem, it just
> paints it over. And, as you have noticed, it introduces new problems along
> the way.
>
This indeed is unwanted and needs to be fixed. Looking at the 
lib/sysfs.c also SPI chip detection breaks because of commit 
e13ac47bec20 ("spi: Use stable dev_name for ACPI enumerated SPI slaves").

However plain revert won't work as it breaks those ALSA SoC drivers that 
depend on this. Good thing is that matching there is purely kernel space 
and at quick look "git grep '"i2c-.*:[0-9]'" and "git grep 
'"spi-.*:[0-9]'" don't find other in-kernel users.

>> 70762ab achieved its stated goal in a meaningful way.  Won't
>> "i2c-<acpi_dev_name>" also vary with ACPI scan order, BIOS settings,
>> firmware upgrades, etc...?
>>
If I remember correctly ACPI ID should not ever change and instance id 
:xy after INTABCD:xy should also be visible and keep the order even if 
device is disabled or not plugged. But I'm not absolute sure about this.

At least on a test platform that allow disable devices will show those 
devices off-line (/sys/bus/acpi/devices/INTABCD:xy/status == 0).

-- 
Jarkko

Re: [RFC 0/1] i2c: acpi: revert setting a "stable" device name

[Dustin Byford]

On Mon Aug 17 11:00, Jarkko Nikula wrote:

> If I remember correctly ACPI ID should not ever change and instance id :xy
> after INTABCD:xy should also be visible and keep the order even if device is
> disabled or not plugged. But I'm not absolute sure about this.
> 
> At least on a test platform that allow disable devices will show those
> devices off-line (/sys/bus/acpi/devices/INTABCD:xy/status == 0).

I'm always surprised how hard it is to tell what *can't* happen in ACPI.
How about a conditional call to LoadTable()?  Seems like that would mess
up the :xy.  Moot point, I think you're on the right track in your RFC
patch.

		--Dustin