[alsa-devel] [RFC] i2c: Revert back to old device naming for ACPI enumerated I2C slaves
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
lib/sysfs.c:665: if ((!subsys || !strcmp(subsys, "i2c")) && sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr, &entry.chip.addr) == 2) {
This patch fixes this by reverting back the old device naming in i2c-core but at the same avoids regression to ALSA SoC drivers that depend on stable device binding.
Reverted I2C slave device naming is handled in ALSA SoC core by using the ACPI name as a component name if device is ACPI probed. This keeps the component binding in ALSA SoC stable and requires only minimal changes to affected machine drivers.
Fixes: 70762abb9f89 i2c: Use stable dev_name for ACPI enumerated I2C slaves Reported-by: Dustin Byford dustin@cumulusnetworks.com Signed-off-by: Jarkko Nikula jarkko.nikula@linux.intel.com --- This is on top of 4.2.0-rc8. I didn't check exact kernel versions where this still applies but I know we would need a few different versions for older stable versions. Mainly because of added and moved drivers in sound/soc/intel/.
This is for discussion so I didn't cc stable@vger.kernel.org yet. I was thinking would it work if we'd keep the stable name but have an another symlink in /sys/bus/i2c/devices/ that uses "x-00yz" name. However this feels ill-use of devices directory and probably causes more troubles elsewhere.
I don't know how common ACPI enumerated I2C hwmon devices are but I guess they exists since Dustin notices this issue and wrote "With that change, /sys/bus/i2c/devices/i2c-0-004a, for example, became /sys/bus/i2c/devices/i2c-PNPXXXX:xx".
It could be possible there is use for the new device naming. I found a comment from Documentation/hid/hid-sensor.txt introduced by commit b2eafd7282fd ("HID: sensor: Update document for custom sensor") that seems to indicate current i2c-INTABCD:xy naming may be used from userspace too but I didn't find examples of that. --- drivers/i2c/i2c-core.c | 21 ++++----------------- sound/soc/intel/boards/broadwell.c | 6 +++--- sound/soc/intel/boards/byt-max98090.c | 2 +- sound/soc/intel/boards/byt-rt5640.c | 2 +- sound/soc/intel/boards/bytcr_rt5640.c | 2 +- sound/soc/intel/boards/cht_bsw_max98090_ti.c | 4 ++-- sound/soc/intel/boards/cht_bsw_rt5645.c | 4 ++-- sound/soc/intel/boards/cht_bsw_rt5672.c | 6 +++--- sound/soc/intel/boards/haswell.c | 2 +- sound/soc/soc-core.c | 10 ++++++++++ 10 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index c83e4d13cfc5..e9c227b9dc92 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -913,22 +913,6 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } 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) - ? 0xa000 : 0)); -} - /** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device @@ -987,7 +971,10 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; client->dev.fwnode = info->fwnode;
- i2c_dev_set_name(adap, client); + /* 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) + ? 0xa000 : 0)); status = device_register(&client->dev); if (status) goto out_err; diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c index 8bafaf6ceab1..3abcf0d7682e 100644 --- a/sound/soc/intel/boards/broadwell.c +++ b/sound/soc/intel/boards/broadwell.c @@ -205,7 +205,7 @@ static struct snd_soc_dai_link broadwell_rt286_dais[] = { .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1, - .codec_name = "i2c-INT343A:00", + .codec_name = "INT343A:00", .codec_dai_name = "rt286-aif1", .init = broadwell_rt286_codec_init, .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | @@ -223,7 +223,7 @@ static int broadwell_suspend(struct snd_soc_card *card){ struct snd_soc_codec *codec;
list_for_each_entry(codec, &card->codec_dev_list, card_list) { - if (!strcmp(codec->component.name, "i2c-INT343A:00")) { + if (!strcmp(codec->component.name, "INT343A:00")) { dev_dbg(codec->dev, "disabling jack detect before going to suspend.\n"); rt286_mic_detect(codec, NULL); break; @@ -236,7 +236,7 @@ static int broadwell_resume(struct snd_soc_card *card){ struct snd_soc_codec *codec;
list_for_each_entry(codec, &card->codec_dev_list, card_list) { - if (!strcmp(codec->component.name, "i2c-INT343A:00")) { + if (!strcmp(codec->component.name, "INT343A:00")) { dev_dbg(codec->dev, "enabling jack detect for resume.\n"); rt286_mic_detect(codec, &broadwell_headset); break; diff --git a/sound/soc/intel/boards/byt-max98090.c b/sound/soc/intel/boards/byt-max98090.c index 7ab8cc9fbfd5..ae336b8522a8 100644 --- a/sound/soc/intel/boards/byt-max98090.c +++ b/sound/soc/intel/boards/byt-max98090.c @@ -116,7 +116,7 @@ static struct snd_soc_dai_link byt_max98090_dais[] = { .stream_name = "Audio", .cpu_dai_name = "baytrail-pcm-audio", .codec_dai_name = "HiFi", - .codec_name = "i2c-193C9890:00", + .codec_name = "193C9890:00", .platform_name = "baytrail-pcm-audio", .init = byt_max98090_init, .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | diff --git a/sound/soc/intel/boards/byt-rt5640.c b/sound/soc/intel/boards/byt-rt5640.c index ae89b9b966d9..275b056c495c 100644 --- a/sound/soc/intel/boards/byt-rt5640.c +++ b/sound/soc/intel/boards/byt-rt5640.c @@ -186,7 +186,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = { .stream_name = "Audio", .cpu_dai_name = "baytrail-pcm-audio", .codec_dai_name = "rt5640-aif1", - .codec_name = "i2c-10EC5640:00", + .codec_name = "10EC5640:00", .platform_name = "baytrail-pcm-audio", .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index 7f55d59024a8..973f07548b08 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -171,7 +171,7 @@ static struct snd_soc_dai_link byt_dailink[] = { .platform_name = "sst-mfld-platform", .no_pcm = 1, .codec_dai_name = "rt5640-aif1", - .codec_name = "i2c-10EC5640:00", + .codec_name = "10EC5640:00", .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, .be_hw_params_fixup = byt_codec_fixup, diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c index 70f832114a5a..1a5348771e03 100644 --- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c +++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c @@ -232,7 +232,7 @@ static struct snd_soc_ops cht_be_ssp2_ops = { static struct snd_soc_aux_dev cht_max98090_headset_dev = { .name = "Headset Chip", .init = cht_max98090_headset_init, - .codec_name = "i2c-104C227E:00", + .codec_name = "104C227E:00", };
static struct snd_soc_dai_link cht_dailink[] = { @@ -265,7 +265,7 @@ static struct snd_soc_dai_link cht_dailink[] = { .platform_name = "sst-mfld-platform", .no_pcm = 1, .codec_dai_name = "HiFi", - .codec_name = "i2c-193C9890:00", + .codec_name = "193C9890:00", .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, .init = cht_codec_init, diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c index bdcaf467842a..c05841264dc7 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5645.c +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c @@ -290,7 +290,7 @@ static struct snd_soc_dai_link cht_dailink[] = { .platform_name = "sst-mfld-platform", .no_pcm = 1, .codec_dai_name = "rt5645-aif1", - .codec_name = "i2c-10EC5645:00", + .codec_name = "10EC5645:00", .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_CBS_CFS, .init = cht_codec_init, @@ -365,7 +365,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev) } } card->dev = &pdev->dev; - sprintf(codec_name, "i2c-%s:00", drv->acpi_card->codec_id); + sprintf(codec_name, "%s:00", drv->acpi_card->codec_id); /* set correct codec name */ strcpy((char *)card->dai_link[2].codec_name, codec_name); snd_soc_card_set_drvdata(card, drv); diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c index 2c9cc5be439e..632926d0c595 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5672.c +++ b/sound/soc/intel/boards/cht_bsw_rt5672.c @@ -280,7 +280,7 @@ static struct snd_soc_dai_link cht_dailink[] = { .no_pcm = 1, .nonatomic = true, .codec_dai_name = "rt5670-aif1", - .codec_name = "i2c-10EC5670:00", + .codec_name = "10EC5670:00", .dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_CBS_CFS, .init = cht_codec_init, @@ -296,7 +296,7 @@ static int cht_suspend_pre(struct snd_soc_card *card) struct snd_soc_codec *codec;
list_for_each_entry(codec, &card->codec_dev_list, card_list) { - if (!strcmp(codec->component.name, "i2c-10EC5670:00")) { + if (!strcmp(codec->component.name, "10EC5670:00")) { dev_dbg(codec->dev, "disabling jack detect before going to suspend.\n"); rt5670_jack_suspend(codec); break; @@ -310,7 +310,7 @@ static int cht_resume_post(struct snd_soc_card *card) struct snd_soc_codec *codec;
list_for_each_entry(codec, &card->codec_dev_list, card_list) { - if (!strcmp(codec->component.name, "i2c-10EC5670:00")) { + if (!strcmp(codec->component.name, "10EC5670:00")) { dev_dbg(codec->dev, "enabling jack detect for resume.\n"); rt5670_jack_resume(codec); break; diff --git a/sound/soc/intel/boards/haswell.c b/sound/soc/intel/boards/haswell.c index 22558572cb9c..6fd6491e368d 100644 --- a/sound/soc/intel/boards/haswell.c +++ b/sound/soc/intel/boards/haswell.c @@ -160,7 +160,7 @@ static struct snd_soc_dai_link haswell_rt5640_dais[] = { .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1, - .codec_name = "i2c-INT33CA:00", + .codec_name = "INT33CA:00", .codec_dai_name = "rt5640-aif1", .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 0e1e69c7abd5..811d63e31a7a 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -34,6 +34,7 @@ #include <linux/ctype.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/acpi.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/pcm.h> @@ -2476,10 +2477,18 @@ static char *fmt_single_name(struct device *dev, int *id) { char *found, name[NAME_SIZE]; int id1, id2; + struct acpi_device *adev;
if (dev_name(dev) == NULL) return NULL;
+ if (ACPI_HANDLE(dev) && + !acpi_bus_get_device(ACPI_HANDLE(dev), &adev)) { + /* use ACPI name as a component name for ACPI probed devices */ + strlcpy(name, acpi_dev_name(adev), NAME_SIZE); + goto out; + } + strlcpy(name, dev_name(dev), NAME_SIZE);
/* are we a "%s.%d" name (platform and SPI components) */ @@ -2508,6 +2517,7 @@ static char *fmt_single_name(struct device *dev, int *id) *id = 0; }
+out: return kstrdup(name, GFP_KERNEL); }
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
lib/sysfs.c:665: if ((!subsys || !strcmp(subsys, "i2c")) && sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr, &entry.chip.addr) == 2) {
This patch fixes this by reverting back the old device naming in i2c-core but at the same avoids regression to ALSA SoC drivers that depend on stable device binding.
This way we fix some userspace by going back. However, this name change was effective for 18 months, so enough time for some other userspace to adapt. We would break the latter by reverting.
I tend to create the symlinks to prevent any further breakage. I am open for other suggestions, though. This ugly situation has surely happened before somewhere somewhen in the kernel :/
Wolfram
On Monday, August 24, 2015 03:26:13 PM Wolfram Sang wrote:
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
lib/sysfs.c:665: if ((!subsys || !strcmp(subsys, "i2c")) && sscanf(dev_name, "%hd-%x", &entry.chip.bus.nr, &entry.chip.addr) == 2) {
This patch fixes this by reverting back the old device naming in i2c-core but at the same avoids regression to ALSA SoC drivers that depend on stable device binding.
This way we fix some userspace by going back. However, this name change was effective for 18 months, so enough time for some other userspace to adapt. We would break the latter by reverting.
Right.
Generally, reverts of such things need to be made relatively quickly.
I tend to create the symlinks to prevent any further breakage. I am open for other suggestions, though. This ugly situation has surely happened before somewhere somewhen in the kernel :/
It happens on a regular basis.
In this particular case it looks like we need a mechanism to use the old naming scheme for hwmon and the new one for everything else or something similar. I'm afraid I am not familiar enough with the i2c core to suggest anything specific ATM, though.
Thanks, Rafael
In this particular case it looks like we need a mechanism to use the old naming scheme for hwmon and the new one for everything else or something similar. I'm afraid I am not familiar enough with the i2c core to suggest anything specific ATM, though.
Well, it boils down to directory names in sysfs. So, to support both naming schemes, I'd simply suggest symlinks.
On Mon Aug 24 13:52, Jarkko Nikula wrote:
I don't know how common ACPI enumerated I2C hwmon devices are but I guess they exists since Dustin notices this issue and wrote "With that change, /sys/bus/i2c/devices/i2c-0-004a, for example, became /sys/bus/i2c/devices/i2c-PNPXXXX:xx".
I wouldn't take my particular use case as evidence they are common, or even they really exist. I got here because I loaded a hwmon driver using the _DSD/PRP0001 mechanism in ACPI. In that case, those devices have never worked with 'sensors' Therefore, in that context, this change is just fixing a bug.
I can't say much about other apps writing against the current sysfs names. 18 months seems like a long time.
--Dustin
On 08/25/2015 08:03 AM, Dustin Byford wrote:
On Mon Aug 24 13:52, Jarkko Nikula wrote:
I don't know how common ACPI enumerated I2C hwmon devices are but I guess they exists since Dustin notices this issue and wrote "With that change, /sys/bus/i2c/devices/i2c-0-004a, for example, became /sys/bus/i2c/devices/i2c-PNPXXXX:xx".
I wouldn't take my particular use case as evidence they are common, or even they really exist. I got here because I loaded a hwmon driver using the _DSD/PRP0001 mechanism in ACPI. In that case, those devices have never worked with 'sensors' Therefore, in that context, this change is just fixing a bug.
So does this then fall more into new use case rather than being a regression as such?
What I'm trying to understand what kind of regression we actually do have here? Is it about listing the devices or does it also cease to show for instance temperature sensor readings?
If it requires to add a match table to existing driver in order to trigger this then at least stable tree is not worry here.
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
Acked-by: Mark Brown broonie@kernel.org
On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
Acked-by: Mark Brown broonie@kernel.org
Don't you think there will be regressions given that the new naming scheme was around for 18 months?
On 08/25/2015 07:57 AM, Wolfram Sang wrote:
On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
Acked-by: Mark Brown broonie@kernel.org
Don't you think there will be regressions given that the new naming scheme was around for 18 months?
acpi is pretty long term. New bindings don't show up quickly. So I am not surprised that this only shows up now.
Will there be regressions ? Who knows. What we do know is that there are regressions today due to the original change.
Guenter
On Tue, Aug 25, 2015 at 08:18:30AM -0700, Guenter Roeck wrote:
On 08/25/2015 07:57 AM, Wolfram Sang wrote:
On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
Acked-by: Mark Brown broonie@kernel.org
Don't you think there will be regressions given that the new naming scheme was around for 18 months?
acpi is pretty long term. New bindings don't show up quickly.
The key is, there might be userspace applications that got used to our new "export" of ACPI bindings. The bindings were already existing, our "export" did change.
So I am not surprised that this only shows up now.
I am, to be honest. It shows running lm-sensors with ACPI on a kernel newer than 18 months. Not a rare scenario, so I thought.
Will there be regressions ? Who knows. What we do know is that there are regressions today due to the original change.
Because we didn't pay enough attention. I wouldn't like to do the same mistake again. Or?
So I am not surprised that this only shows up now.
I am, to be honest. It shows running lm-sensors with ACPI on a kernel newer than 18 months. Not a rare scenario, so I thought.
And thanks to Mark's recent post I understand now that this is only relevant for ACPI5 enumerated devices. I lost that detail.
On 08/25/2015 09:18 AM, Wolfram Sang wrote:
On Tue, Aug 25, 2015 at 08:18:30AM -0700, Guenter Roeck wrote:
On 08/25/2015 07:57 AM, Wolfram Sang wrote:
On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
Acked-by: Mark Brown broonie@kernel.org
Don't you think there will be regressions given that the new naming scheme was around for 18 months?
acpi is pretty long term. New bindings don't show up quickly.
The key is, there might be userspace applications that got used to our new "export" of ACPI bindings. The bindings were already existing, our "export" did change.
So I am not surprised that this only shows up now.
I am, to be honest. It shows running lm-sensors with ACPI on a kernel newer than 18 months. Not a rare scenario, so I thought.
With i2c (sensor) devices enumerated in acpi ? Yes, I think that is quite rare. This is not about the kernel, it is about such device bindings showing up in acpi data.
Guenter
On Tue, Aug 25, 2015 at 04:57:56PM +0200, Wolfram Sang wrote:
On Tue, Aug 25, 2015 at 06:25:13AM +0100, Mark Brown wrote:
On Mon, Aug 24, 2015 at 01:52:02PM +0300, Jarkko Nikula wrote:
Commit 70762abb9f89 ("i2c: Use stable dev_name for ACPI enumerated I2C slaves") broke the lm-sensors which relies on I2C hwmon slave devices under /sys/bus/i2c/devices/ to be named as "x-00yz". However if those hwmon devices are ACPI 5 enumerated their name became "i2c-INTABCD:ij" and sysfs code in lm-sensors does not find them anymore:
Acked-by: Mark Brown broonie@kernel.org
Don't you think there will be regressions given that the new naming scheme was around for 18 months?
That's a "whatever" from the ASoC point of view. I don't particularly care about the userspace ABI, on the one hand there aren't that many ACPI 5 devices that might be affected and there's other devices that won't work anyway. On the other hand I guess there's other devices that would have worked with the old kernel.
This is for discussion so I didn't cc stable@vger.kernel.org yet. I was thinking would it work if we'd keep the stable name but have an another symlink in /sys/bus/i2c/devices/ that uses "x-00yz" name. However this feels ill-use of devices directory and probably causes more troubles elsewhere.
Do you foresee troubles already? I am still in favour of a symlink.
On 10/01/2015 11:37 PM, Wolfram Sang wrote:
This is for discussion so I didn't cc stable@vger.kernel.org yet. I was thinking would it work if we'd keep the stable name but have an another symlink in /sys/bus/i2c/devices/ that uses "x-00yz" name. However this feels ill-use of devices directory and probably causes more troubles elsewhere.
Do you foresee troubles already? I am still in favour of a symlink.
I haven't looked at this for a while but one problem was that devices/ directory belongs to private structure of struct bus_type and in order to create a symlink there it needs to done in drivers/base/bus.c: bus_add_device() which felt quite hackish to me.
On Fri, Oct 02, 2015 at 12:27:16PM +0300, Jarkko Nikula wrote:
On 10/01/2015 11:37 PM, Wolfram Sang wrote:
This is for discussion so I didn't cc stable@vger.kernel.org yet. I was thinking would it work if we'd keep the stable name but have an another symlink in /sys/bus/i2c/devices/ that uses "x-00yz" name. However this feels ill-use of devices directory and probably causes more troubles elsewhere.
Do you foresee troubles already? I am still in favour of a symlink.
I haven't looked at this for a while but one problem was that devices/ directory belongs to private structure of struct bus_type and in order to create a symlink there it needs to done in drivers/base/bus.c: bus_add_device() which felt quite hackish to me.
This is just a quick prototype and untested; but I did something similar in the i2c-mux code:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 5f89f1e3c2f24f..715dca57ba68fd 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -970,13 +970,15 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, { struct acpi_device *adev = ACPI_COMPANION(&client->dev);
- if (adev) { - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); - return; - } - dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), i2c_encode_flags_to_addr(client)); + + if (adev) { + char symlink_name[256]; + + snprintf(symlink_name, sizeof(symlink_name), "i2c-%s", acpi_dev_name(adev)); + sysfs_create_link(&client->dev.kobj, &adap->dev.kobj, symlink_name); + } }
/**
Shouldn't something like this be enough?
Hi On 10/10/2015 12:47 AM, Wolfram Sang wrote:
On Fri, Oct 02, 2015 at 12:27:16PM +0300, Jarkko Nikula wrote:
On 10/01/2015 11:37 PM, Wolfram Sang wrote:
Do you foresee troubles already? I am still in favour of a symlink.
I haven't looked at this for a while but one problem was that devices/ directory belongs to private structure of struct bus_type and in order to create a symlink there it needs to done in drivers/base/bus.c: bus_add_device() which felt quite hackish to me.
This is just a quick prototype and untested; but I did something similar in the i2c-mux code:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 5f89f1e3c2f24f..715dca57ba68fd 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -970,13 +970,15 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, { struct acpi_device *adev = ACPI_COMPANION(&client->dev);
- if (adev) {
dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));return;- }
- dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), i2c_encode_flags_to_addr(client));
if (adev) {
char symlink_name[256];snprintf(symlink_name, sizeof(symlink_name), "i2c-%s", acpi_dev_name(adev));sysfs_create_link(&client->dev.kobj, &adap->dev.kobj, symlink_name);} }
/**
Shouldn't something like this be enough?
Not really. It would create the symlink under the device not under /sys/bus/i2c/devices/. Please note the sysfs_create_link() must be called after device_register(). I moved the symlink creation into i2c_new_device() for the example below.
Now:
/sys/bus/i2c/devices/i2c-ATML3432:00 -> ../../../devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-9/i2c-ATML3432:00
After modified patch:
/sys/bus/i2c/devices/9-004c -> ../../../devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-9/9-004c
which has now the symlink pointing to adapter
/sys/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-9/9-004c/ i2c-ATML3432:00 -> ../../i2c-9
We would need a following symlink but I didn't figure out how to do it without touching drivers/base/bus.c.
/sys/bus/i2c/devices/i2c-ATML3432:00 -> 9-004c
Symlinks there are added/removed by using &bus->p->devices_kset->kobj where p is a private for driver core only.
participants (6)
-
Dustin Byford -
Guenter Roeck -
Jarkko Nikula -
Mark Brown -
Rafael J. Wysocki -
Wolfram Sang