* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
@ 2021-06-02 17:08 Alexander Fomichev
2021-06-04 10:00 ` Bartosz Golaszewski
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Fomichev @ 2021-06-02 17:08 UTC (permalink / raw)
To: Jon Hunter, linux-i2c; +Cc: Bartosz Golaszewski, linux
On Thu, Sep 24, 2020 at 03:20:39PM +0200, Jon Hunter wrote:
> By using the label property, a more descriptive name can be populated
> for AT24 EEPROMs NVMEM device. Update the AT24 driver to check to see
> if the label property is present and if so, use this as the name for
> NVMEM device. Please note that when the 'label' property is present for
> the AT24 EEPROM, we do not want the NVMEM driver to append the 'devid'
> to the name and so the nvmem_config.id is initialised to
> NVMEM_DEVID_NONE.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 2fde53dcfc97..4aa96d8e78ef 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -713,8 +713,28 @@ static int at24_probe(struct i2c_client *client)
> return err;
> }
>
> + /*
> + * If the 'label' property is not present for the AT24 EEPROM,
> + * then nvmem_config.id is initialised to NVMEM_DEVID_AUTO,
> + * and this will append the 'devid' to the name of the NVMEM
> + * device. This is purely legacy and the AT24 driver has always
> + * defaulted to this. However, if the 'label' property is
> + * present then this means that the name is specified by the
> + * firmware and this name should be used verbatim and so it is
> + * not necessary to append the 'devid'.
> + */
> + if (device_property_present(dev, "label")) {
> + nvmem_config.id = NVMEM_DEVID_NONE;
> + err = device_property_read_string(dev, "label",
> + &nvmem_config.name);
> + if (err)
> + return err;
> + } else {
> + nvmem_config.id = NVMEM_DEVID_AUTO;
> + nvmem_config.name = dev_name(dev);
> + }
> +
> nvmem_config.type = NVMEM_TYPE_EEPROM;
> - nvmem_config.name = dev_name(dev);
> nvmem_config.dev = dev;
> nvmem_config.id = NVMEM_DEVID_AUTO;
> nvmem_config.read_only = !writable;
This change has a serious defect, as it doesn't guarantee a name
uniqueness. For my case there are a bunch of NVMEM devices with
'dimm-spd' name. So the module initialization fails with several error
dumps in dmesg, like following:
[ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
[ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
[ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
[ 4.784787] Call Trace:
[ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
[ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
[ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
[ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
[ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
[ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
[ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
[ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
[ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
[ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
[ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
[ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
[ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
[ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
[ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
[ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
[ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
[ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
[ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
[ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
[ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
[ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
[ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
[ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
[ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
[ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
[ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
[ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
[ 4.784976] IRQMASK: 0
GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
[ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
[ 4.785036] LR [0000000000000000] 0x0
[ 4.785040] --- interrupt: 3000
[ 4.785146] at24: probe of 3-0051 failed with error -17
It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
property or to add a sort of counter suffix to the name field.
Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
--
Regards,
Alexander
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-02 17:08 [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs Alexander Fomichev
@ 2021-06-04 10:00 ` Bartosz Golaszewski
2021-06-07 11:09 ` Jon Hunter
0 siblings, 1 reply; 9+ messages in thread
From: Bartosz Golaszewski @ 2021-06-04 10:00 UTC (permalink / raw)
To: Alexander Fomichev, Jon Hunter; +Cc: linux-i2c, linux
On Wed, Jun 2, 2021 at 7:08 PM Alexander Fomichev <fomichev.ru@gmail.com> wrote:
>
> On Thu, Sep 24, 2020 at 03:20:39PM +0200, Jon Hunter wrote:
> > By using the label property, a more descriptive name can be populated
> > for AT24 EEPROMs NVMEM device. Update the AT24 driver to check to see
> > if the label property is present and if so, use this as the name for
> > NVMEM device. Please note that when the 'label' property is present for
> > the AT24 EEPROM, we do not want the NVMEM driver to append the 'devid'
> > to the name and so the nvmem_config.id is initialised to
> > NVMEM_DEVID_NONE.
> >
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 2fde53dcfc97..4aa96d8e78ef 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -713,8 +713,28 @@ static int at24_probe(struct i2c_client *client)
> > return err;
> > }
> >
> > + /*
> > + * If the 'label' property is not present for the AT24 EEPROM,
> > + * then nvmem_config.id is initialised to NVMEM_DEVID_AUTO,
> > + * and this will append the 'devid' to the name of the NVMEM
> > + * device. This is purely legacy and the AT24 driver has always
> > + * defaulted to this. However, if the 'label' property is
> > + * present then this means that the name is specified by the
> > + * firmware and this name should be used verbatim and so it is
> > + * not necessary to append the 'devid'.
> > + */
> > + if (device_property_present(dev, "label")) {
> > + nvmem_config.id = NVMEM_DEVID_NONE;
> > + err = device_property_read_string(dev, "label",
> > + &nvmem_config.name);
> > + if (err)
> > + return err;
> > + } else {
> > + nvmem_config.id = NVMEM_DEVID_AUTO;
> > + nvmem_config.name = dev_name(dev);
> > + }
> > +
> > nvmem_config.type = NVMEM_TYPE_EEPROM;
> > - nvmem_config.name = dev_name(dev);
> > nvmem_config.dev = dev;
> > nvmem_config.id = NVMEM_DEVID_AUTO;
> > nvmem_config.read_only = !writable;
>
> This change has a serious defect, as it doesn't guarantee a name
> uniqueness. For my case there are a bunch of NVMEM devices with
> 'dimm-spd' name. So the module initialization fails with several error
> dumps in dmesg, like following:
>
> [ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
> [ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
> [ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
> [ 4.784787] Call Trace:
> [ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
> [ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
> [ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
> [ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
> [ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
> [ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
> [ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
> [ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
> [ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
> [ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
> [ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
> [ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
> [ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
> [ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
> [ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
> [ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
> [ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
> [ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
> [ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
> [ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
> [ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
> [ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
> [ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
> [ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
> [ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
> [ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
> [ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
> [ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
> [ 4.784976] IRQMASK: 0
> GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
> GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
> GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
> GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
> GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
> GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
> [ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
> [ 4.785036] LR [0000000000000000] 0x0
> [ 4.785040] --- interrupt: 3000
> [ 4.785146] at24: probe of 3-0051 failed with error -17
>
>
> It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
> property or to add a sort of counter suffix to the name field.
>
>
> Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
> CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> --
> Regards,
> Alexander
Alexander: Thanks for your bug report. The counter suffix you suggest
is precisely what NVMEM_DEVID_AUTO would do so I think we'll need to
use it. On the other hand, a non-unique label is bad design but
obviously we can't break working setups.
Jon: As the author of this patch - do you have any objections/better ideas?
If not, I'll send out a fix soon.
Bart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-04 10:00 ` Bartosz Golaszewski
@ 2021-06-07 11:09 ` Jon Hunter
2021-06-07 13:31 ` Bartosz Golaszewski
2021-06-16 16:55 ` Alexander Fomichev
0 siblings, 2 replies; 9+ messages in thread
From: Jon Hunter @ 2021-06-07 11:09 UTC (permalink / raw)
To: Bartosz Golaszewski, Alexander Fomichev
Cc: linux-i2c, linux, linux-tegra, Thierry Reding
On 04/06/2021 11:00, Bartosz Golaszewski wrote:
> On Wed, Jun 2, 2021 at 7:08 PM Alexander Fomichev <fomichev.ru@gmail.com> wrote:
...
>> This change has a serious defect, as it doesn't guarantee a name
>> uniqueness. For my case there are a bunch of NVMEM devices with
>> 'dimm-spd' name. So the module initialization fails with several error
>> dumps in dmesg, like following:
>>
>> [ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
>> [ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
>> [ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
>> [ 4.784787] Call Trace:
>> [ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
>> [ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
>> [ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
>> [ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
>> [ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
>> [ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
>> [ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
>> [ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
>> [ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
>> [ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
>> [ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
>> [ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
>> [ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
>> [ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
>> [ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
>> [ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
>> [ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
>> [ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
>> [ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
>> [ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
>> [ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
>> [ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
>> [ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
>> [ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
>> [ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
>> [ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
>> [ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
>> [ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
>> [ 4.784976] IRQMASK: 0
>> GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
>> GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
>> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
>> GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
>> GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
>> GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
>> GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
>> [ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
>> [ 4.785036] LR [0000000000000000] 0x0
>> [ 4.785040] --- interrupt: 3000
>> [ 4.785146] at24: probe of 3-0051 failed with error -17
>>
>>
>> It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
>> property or to add a sort of counter suffix to the name field.
>>
>>
>> Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
>> CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> --
>> Regards,
>> Alexander
>
> Alexander: Thanks for your bug report. The counter suffix you suggest
> is precisely what NVMEM_DEVID_AUTO would do so I think we'll need to
> use it. On the other hand, a non-unique label is bad design but
> obviously we can't break working setups.
Yes the intention was that the label itself would be unique. In our case
we wanted to have something specifically named 'system' or 'module' to
identify a specific eeprom.
BTW I did a quick grep from 'dimm-spd' and did not find it in the
kernel. Where is the device-tree you are using?
> Jon: As the author of this patch - do you have any objections/better ideas?
I would need to check if appending a suffix then has ramifications for
what we were trying to achieve.
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-07 11:09 ` Jon Hunter
@ 2021-06-07 13:31 ` Bartosz Golaszewski
2021-06-07 14:28 ` Jon Hunter
2021-06-16 17:00 ` Alexander Fomichev
2021-06-16 16:55 ` Alexander Fomichev
1 sibling, 2 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2021-06-07 13:31 UTC (permalink / raw)
To: Jon Hunter, Alexander Fomichev
Cc: linux-i2c, linux, linux-tegra, Thierry Reding
On Mon, Jun 7, 2021 at 1:09 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 04/06/2021 11:00, Bartosz Golaszewski wrote:
> > On Wed, Jun 2, 2021 at 7:08 PM Alexander Fomichev <fomichev.ru@gmail.com> wrote:
>
> ...
>
> >> This change has a serious defect, as it doesn't guarantee a name
> >> uniqueness. For my case there are a bunch of NVMEM devices with
> >> 'dimm-spd' name. So the module initialization fails with several error
> >> dumps in dmesg, like following:
> >>
> >> [ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
> >> [ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
> >> [ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
> >> [ 4.784787] Call Trace:
> >> [ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
> >> [ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
> >> [ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
> >> [ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
> >> [ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
> >> [ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
> >> [ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
> >> [ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
> >> [ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
> >> [ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
> >> [ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
> >> [ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
> >> [ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
> >> [ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
> >> [ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
> >> [ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
> >> [ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
> >> [ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
> >> [ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
> >> [ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
> >> [ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
> >> [ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
> >> [ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
> >> [ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
> >> [ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
> >> [ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
> >> [ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
> >> [ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
> >> [ 4.784976] IRQMASK: 0
> >> GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
> >> GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
> >> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> >> GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
> >> GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
> >> GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
> >> GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
> >> GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
> >> [ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
> >> [ 4.785036] LR [0000000000000000] 0x0
> >> [ 4.785040] --- interrupt: 3000
> >> [ 4.785146] at24: probe of 3-0051 failed with error -17
> >>
> >>
> >> It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
> >> property or to add a sort of counter suffix to the name field.
> >>
> >>
> >> Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
> >> CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>
> >> --
> >> Regards,
> >> Alexander
> >
> > Alexander: Thanks for your bug report. The counter suffix you suggest
> > is precisely what NVMEM_DEVID_AUTO would do so I think we'll need to
> > use it. On the other hand, a non-unique label is bad design but
> > obviously we can't break working setups.
>
> Yes the intention was that the label itself would be unique. In our case
> we wanted to have something specifically named 'system' or 'module' to
> identify a specific eeprom.
>
> BTW I did a quick grep from 'dimm-spd' and did not find it in the
> kernel. Where is the device-tree you are using?
>
> > Jon: As the author of this patch - do you have any objections/better ideas?
>
> I would need to check if appending a suffix then has ramifications for
> what we were trying to achieve.
>
> Jon
>
> --
> nvpublic
One alternative would be to use the label "as is" for the first device
and then append ".x" for each subsequent device with a repeating
label. Does this make sense?
Bart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-07 13:31 ` Bartosz Golaszewski
@ 2021-06-07 14:28 ` Jon Hunter
2021-06-16 17:00 ` Alexander Fomichev
1 sibling, 0 replies; 9+ messages in thread
From: Jon Hunter @ 2021-06-07 14:28 UTC (permalink / raw)
To: Bartosz Golaszewski, Alexander Fomichev
Cc: linux-i2c, linux, linux-tegra, Thierry Reding
On 07/06/2021 14:31, Bartosz Golaszewski wrote:
...
>> I would need to check if appending a suffix then has ramifications for
>> what we were trying to achieve.
>
> One alternative would be to use the label "as is" for the first device
> and then append ".x" for each subsequent device with a repeating
> label. Does this make sense?
Sounds like a good option to me (assuming this is really needed).
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-07 13:31 ` Bartosz Golaszewski
2021-06-07 14:28 ` Jon Hunter
@ 2021-06-16 17:00 ` Alexander Fomichev
2021-06-16 19:21 ` Bartosz Golaszewski
1 sibling, 1 reply; 9+ messages in thread
From: Alexander Fomichev @ 2021-06-16 17:00 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Jon Hunter, linux-i2c, linux, linux-tegra, Thierry Reding
On Mon, Jun 07, 2021 at 03:31:55PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 7, 2021 at 1:09 PM Jon Hunter <jonathanh@nvidia.com> wrote:
> >
> >
> > On 04/06/2021 11:00, Bartosz Golaszewski wrote:
> > > On Wed, Jun 2, 2021 at 7:08 PM Alexander Fomichev <fomichev.ru@gmail.com> wrote:
> >
> > ...
> >
> > >> This change has a serious defect, as it doesn't guarantee a name
> > >> uniqueness. For my case there are a bunch of NVMEM devices with
> > >> 'dimm-spd' name. So the module initialization fails with several error
> > >> dumps in dmesg, like following:
> > >>
> > >> [ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
> > >> [ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
> > >> [ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
> > >> [ 4.784787] Call Trace:
> > >> [ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
> > >> [ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
> > >> [ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
> > >> [ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
> > >> [ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
> > >> [ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
> > >> [ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
> > >> [ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
> > >> [ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
> > >> [ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
> > >> [ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
> > >> [ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
> > >> [ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
> > >> [ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
> > >> [ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
> > >> [ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
> > >> [ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
> > >> [ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
> > >> [ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
> > >> [ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
> > >> [ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
> > >> [ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
> > >> [ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
> > >> [ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
> > >> [ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
> > >> [ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
> > >> [ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
> > >> [ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
> > >> [ 4.784976] IRQMASK: 0
> > >> GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
> > >> GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
> > >> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > >> GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
> > >> GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
> > >> GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
> > >> GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
> > >> GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
> > >> [ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
> > >> [ 4.785036] LR [0000000000000000] 0x0
> > >> [ 4.785040] --- interrupt: 3000
> > >> [ 4.785146] at24: probe of 3-0051 failed with error -17
> > >>
> > >>
> > >> It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
> > >> property or to add a sort of counter suffix to the name field.
> > >>
> > >>
> > >> Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
> > >> CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >>
> > >> --
> > >> Regards,
> > >> Alexander
> > >
> > > Alexander: Thanks for your bug report. The counter suffix you suggest
> > > is precisely what NVMEM_DEVID_AUTO would do so I think we'll need to
> > > use it. On the other hand, a non-unique label is bad design but
> > > obviously we can't break working setups.
> >
> > Yes the intention was that the label itself would be unique. In our case
> > we wanted to have something specifically named 'system' or 'module' to
> > identify a specific eeprom.
> >
> > BTW I did a quick grep from 'dimm-spd' and did not find it in the
> > kernel. Where is the device-tree you are using?
> >
> > > Jon: As the author of this patch - do you have any objections/better ideas?
> >
> > I would need to check if appending a suffix then has ramifications for
> > what we were trying to achieve.
> >
> > Jon
> >
> > --
> > nvpublic
>
> One alternative would be to use the label "as is" for the first device
> and then append ".x" for each subsequent device with a repeating
> label. Does this make sense?
>
This was my second suggestion, which I called 'counter suffix'. But it
requires additional effort.
--
Regards,
Alexander
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-16 17:00 ` Alexander Fomichev
@ 2021-06-16 19:21 ` Bartosz Golaszewski
2021-06-17 8:06 ` Alexander Fomichev
0 siblings, 1 reply; 9+ messages in thread
From: Bartosz Golaszewski @ 2021-06-16 19:21 UTC (permalink / raw)
To: Alexander Fomichev
Cc: Jon Hunter, linux-i2c, linux, linux-tegra, Thierry Reding
On Wed, Jun 16, 2021 at 7:00 PM Alexander Fomichev
<fomichev.ru@gmail.com> wrote:
>
> On Mon, Jun 07, 2021 at 03:31:55PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Jun 7, 2021 at 1:09 PM Jon Hunter <jonathanh@nvidia.com> wrote:
> > >
> > >
> > > On 04/06/2021 11:00, Bartosz Golaszewski wrote:
> > > > On Wed, Jun 2, 2021 at 7:08 PM Alexander Fomichev <fomichev.ru@gmail.com> wrote:
> > >
> > > ...
> > >
> > > >> This change has a serious defect, as it doesn't guarantee a name
> > > >> uniqueness. For my case there are a bunch of NVMEM devices with
> > > >> 'dimm-spd' name. So the module initialization fails with several error
> > > >> dumps in dmesg, like following:
> > > >>
> > > >> [ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
> > > >> [ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
> > > >> [ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
> > > >> [ 4.784787] Call Trace:
> > > >> [ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
> > > >> [ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
> > > >> [ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
> > > >> [ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
> > > >> [ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
> > > >> [ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
> > > >> [ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
> > > >> [ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
> > > >> [ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
> > > >> [ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
> > > >> [ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
> > > >> [ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
> > > >> [ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
> > > >> [ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
> > > >> [ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
> > > >> [ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
> > > >> [ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
> > > >> [ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
> > > >> [ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
> > > >> [ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
> > > >> [ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
> > > >> [ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
> > > >> [ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
> > > >> [ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
> > > >> [ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
> > > >> [ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
> > > >> [ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
> > > >> [ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
> > > >> [ 4.784976] IRQMASK: 0
> > > >> GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
> > > >> GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
> > > >> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > >> GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
> > > >> GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
> > > >> GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
> > > >> GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
> > > >> GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
> > > >> [ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
> > > >> [ 4.785036] LR [0000000000000000] 0x0
> > > >> [ 4.785040] --- interrupt: 3000
> > > >> [ 4.785146] at24: probe of 3-0051 failed with error -17
> > > >>
> > > >>
> > > >> It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
> > > >> property or to add a sort of counter suffix to the name field.
> > > >>
> > > >>
> > > >> Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
> > > >> CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >>
> > > >> --
> > > >> Regards,
> > > >> Alexander
> > > >
> > > > Alexander: Thanks for your bug report. The counter suffix you suggest
> > > > is precisely what NVMEM_DEVID_AUTO would do so I think we'll need to
> > > > use it. On the other hand, a non-unique label is bad design but
> > > > obviously we can't break working setups.
> > >
> > > Yes the intention was that the label itself would be unique. In our case
> > > we wanted to have something specifically named 'system' or 'module' to
> > > identify a specific eeprom.
> > >
> > > BTW I did a quick grep from 'dimm-spd' and did not find it in the
> > > kernel. Where is the device-tree you are using?
> > >
> > > > Jon: As the author of this patch - do you have any objections/better ideas?
> > >
> > > I would need to check if appending a suffix then has ramifications for
> > > what we were trying to achieve.
> > >
> > > Jon
> > >
> > > --
> > > nvpublic
> >
> > One alternative would be to use the label "as is" for the first device
> > and then append ".x" for each subsequent device with a repeating
> > label. Does this make sense?
> >
>
> This was my second suggestion, which I called 'counter suffix'. But it
> requires additional effort.
>
It looks like we can reuse the id field of nvmem_config and just track
the suffix ourselves in the driver. It's not pretty but I've seen
worse. I'll send a patch tomorrow for you to test - does this sound
ok?
Bart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-16 19:21 ` Bartosz Golaszewski
@ 2021-06-17 8:06 ` Alexander Fomichev
0 siblings, 0 replies; 9+ messages in thread
From: Alexander Fomichev @ 2021-06-17 8:06 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Jon Hunter, linux-i2c, linux, linux-tegra, Thierry Reding
On Wed, Jun 16, 2021 at 09:21:14PM +0200, Bartosz Golaszewski wrote:
> On Wed, Jun 16, 2021 at 7:00 PM Alexander Fomichev
> <fomichev.ru@gmail.com> wrote:
> >
> > On Mon, Jun 07, 2021 at 03:31:55PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Jun 7, 2021 at 1:09 PM Jon Hunter <jonathanh@nvidia.com> wrote:
> > > >
> > > >
> > > > On 04/06/2021 11:00, Bartosz Golaszewski wrote:
> > > > > On Wed, Jun 2, 2021 at 7:08 PM Alexander Fomichev <fomichev.ru@gmail.com> wrote:
> > > >
> > > > ...
> > > >
> > > > >> This change has a serious defect, as it doesn't guarantee a name
> > > > >> uniqueness. For my case there are a bunch of NVMEM devices with
> > > > >> 'dimm-spd' name. So the module initialization fails with several error
> > > > >> dumps in dmesg, like following:
> > > > >>
> > > > >> [ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
> > > > >> [ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
> > > > >> [ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
> > > > >> [ 4.784787] Call Trace:
> > > > >> [ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
> > > > >> [ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
> > > > >> [ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
> > > > >> [ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
> > > > >> [ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
> > > > >> [ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
> > > > >> [ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
> > > > >> [ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
> > > > >> [ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
> > > > >> [ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
> > > > >> [ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
> > > > >> [ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
> > > > >> [ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
> > > > >> [ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
> > > > >> [ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
> > > > >> [ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
> > > > >> [ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
> > > > >> [ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
> > > > >> [ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
> > > > >> [ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
> > > > >> [ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
> > > > >> [ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
> > > > >> [ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
> > > > >> [ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
> > > > >> [ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
> > > > >> [ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
> > > > >> [ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
> > > > >> [ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
> > > > >> [ 4.784976] IRQMASK: 0
> > > > >> GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
> > > > >> GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
> > > > >> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > > >> GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
> > > > >> GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
> > > > >> GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
> > > > >> GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
> > > > >> GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
> > > > >> [ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
> > > > >> [ 4.785036] LR [0000000000000000] 0x0
> > > > >> [ 4.785040] --- interrupt: 3000
> > > > >> [ 4.785146] at24: probe of 3-0051 failed with error -17
> > > > >>
> > > > >>
> > > > >> It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
> > > > >> property or to add a sort of counter suffix to the name field.
> > > > >>
> > > > >>
> > > > >> Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
> > > > >> CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >>
> > > > >> --
> > > > >> Regards,
> > > > >> Alexander
> > > > >
> > > > > Alexander: Thanks for your bug report. The counter suffix you suggest
> > > > > is precisely what NVMEM_DEVID_AUTO would do so I think we'll need to
> > > > > use it. On the other hand, a non-unique label is bad design but
> > > > > obviously we can't break working setups.
> > > >
> > > > Yes the intention was that the label itself would be unique. In our case
> > > > we wanted to have something specifically named 'system' or 'module' to
> > > > identify a specific eeprom.
> > > >
> > > > BTW I did a quick grep from 'dimm-spd' and did not find it in the
> > > > kernel. Where is the device-tree you are using?
> > > >
> > > > > Jon: As the author of this patch - do you have any objections/better ideas?
> > > >
> > > > I would need to check if appending a suffix then has ramifications for
> > > > what we were trying to achieve.
> > > >
> > > > Jon
> > > >
> > > > --
> > > > nvpublic
> > >
> > > One alternative would be to use the label "as is" for the first device
> > > and then append ".x" for each subsequent device with a repeating
> > > label. Does this make sense?
> > >
> >
> > This was my second suggestion, which I called 'counter suffix'. But it
> > requires additional effort.
> >
>
> It looks like we can reuse the id field of nvmem_config and just track
> the suffix ourselves in the driver. It's not pretty but I've seen
> worse. I'll send a patch tomorrow for you to test - does this sound
> ok?
>
Sounds good. Thank you.
--
Regards,
Alexander
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs
2021-06-07 11:09 ` Jon Hunter
2021-06-07 13:31 ` Bartosz Golaszewski
@ 2021-06-16 16:55 ` Alexander Fomichev
1 sibling, 0 replies; 9+ messages in thread
From: Alexander Fomichev @ 2021-06-16 16:55 UTC (permalink / raw)
To: Jon Hunter
Cc: Bartosz Golaszewski, linux-i2c, linux, linux-tegra, Thierry Reding
On Mon, Jun 07, 2021 at 12:09:10PM +0100, Jon Hunter wrote:
>
> On 04/06/2021 11:00, Bartosz Golaszewski wrote:
> > On Wed, Jun 2, 2021 at 7:08 PM Alexander Fomichev <fomichev.ru@gmail.com> wrote:
>
> ...
>
> >> This change has a serious defect, as it doesn't guarantee a name
> >> uniqueness. For my case there are a bunch of NVMEM devices with
> >> 'dimm-spd' name. So the module initialization fails with several error
> >> dumps in dmesg, like following:
> >>
> >> [ 4.784679] at24 3-0051: supply vcc not found, using dummy regulator
> >> [ 4.784781] sysfs: cannot create duplicate filename '/bus/nvmem/devices/dimm-spd'
> >> [ 4.784783] CPU: 24 PID: 1354 Comm: systemd-udevd Not tainted 5.13.0-rc4-at24-catch+ #25
> >> [ 4.784787] Call Trace:
> >> [ 4.784789] [c00000003f3eb010] [c000000000914700] dump_stack+0xc4/0x114 (unreliable)
> >> [ 4.784797] [c00000003f3eb060] [c00000000061c5c8] sysfs_warn_dup+0x88/0xc0
> >> [ 4.784803] [c00000003f3eb0e0] [c00000000061ccec] sysfs_do_create_link_sd+0x17c/0x190
> >> [ 4.784809] [c00000003f3eb130] [c000000000ac3014] bus_add_device+0x94/0x1d0
> >> [ 4.784817] [c00000003f3eb1b0] [c000000000abe7b8] device_add+0x428/0xb90
> >> [ 4.784822] [c00000003f3eb2a0] [c000000000debbd0] nvmem_register+0x220/0xe00
> >> [ 4.784829] [c00000003f3eb390] [c000000000dec80c] devm_nvmem_register+0x5c/0xc0
> >> [ 4.784835] [c00000003f3eb3d0] [c008000016f40c20] at24_probe+0x668/0x940 [at24]
> >> [ 4.784845] [c00000003f3eb650] [c000000000cfecd4] i2c_device_probe+0x194/0x650
> >> [ 4.784850] [c00000003f3eb6f0] [c000000000ac4d3c] really_probe+0x1cc/0x790
> >> [ 4.784855] [c00000003f3eb790] [c000000000ac545c] driver_probe_device+0x15c/0x200
> >> [ 4.784861] [c00000003f3eb810] [c000000000ac5ecc] device_driver_attach+0x11c/0x130
> >> [ 4.784866] [c00000003f3eb850] [c000000000ac5fd0] __driver_attach+0xf0/0x200
> >> [ 4.784873] [c00000003f3eb8d0] [c000000000ac1158] bus_for_each_dev+0xa8/0x130
> >> [ 4.784879] [c00000003f3eb930] [c000000000ac4104] driver_attach+0x34/0x50
> >> [ 4.784885] [c00000003f3eb950] [c000000000ac35f0] bus_add_driver+0x1b0/0x2f0
> >> [ 4.784893] [c00000003f3eb9e0] [c000000000ac70b4] driver_register+0xb4/0x1c0
> >> [ 4.784900] [c00000003f3eba50] [c000000000cfe498] i2c_register_driver+0x78/0x120
> >> [ 4.784905] [c00000003f3ebad0] [c008000016f41260] at24_init+0x6c/0x88 [at24]
> >> [ 4.784914] [c00000003f3ebb30] [c0000000000122c0] do_one_initcall+0x60/0x2c0
> >> [ 4.784920] [c00000003f3ebc00] [c0000000002537bc] do_init_module+0x7c/0x350
> >> [ 4.784926] [c00000003f3ebc90] [c000000000257904] __do_sys_finit_module+0xd4/0x160
> >> [ 4.784932] [c00000003f3ebdb0] [c00000000002c104] system_call_exception+0xf4/0x200
> >> [ 4.784938] [c00000003f3ebe10] [c00000000000cf70] system_call_vectored_common+0xf0/0x268
> >> [ 4.784944] --- interrupt: 3000 at 0x7c1adac4b4c4
> >> [ 4.784948] NIP: 00007c1adac4b4c4 LR: 0000000000000000 CTR: 0000000000000000
> >> [ 4.784951] REGS: c00000003f3ebe80 TRAP: 3000 Not tainted (5.13.0-rc4-at24-catch+)
> >> [ 4.784955] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 48222844 XER: 00000000
> >> [ 4.784976] IRQMASK: 0
> >> GPR00: 0000000000000161 00007fffefc78b90 00007c1adad37000 0000000000000006
> >> GPR04: 00000f6614d56be0 0000000000000000 0000000000000006 0000000000000000
> >> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> >> GPR12: 0000000000000000 00007c1adafde680 0000000020000000 0000000000000000
> >> GPR16: 0000000000000000 00000f66118b1980 00000f66118b1a18 00000f66118b1948
> >> GPR20: 0000000000000000 00000f6614d60500 00007fffefc78df0 00000f6614d535c0
> >> GPR24: 00000f6614d56be0 00000f6614d60500 000000000000000c 00000f6614d49cb0
> >> GPR28: 00000f6614d56be0 0000000000020000 0000000000000000 00000f6614d60500
> >> [ 4.785033] NIP [00007c1adac4b4c4] 0x7c1adac4b4c4
> >> [ 4.785036] LR [0000000000000000] 0x0
> >> [ 4.785040] --- interrupt: 3000
> >> [ 4.785146] at24: probe of 3-0051 failed with error -17
> >>
> >>
> >> It needs either to use NVMEM_DEVID_AUTO flag irrespective of the 'label'
> >> property or to add a sort of counter suffix to the name field.
> >>
> >>
> >> Reported-by: Alexander Fomichev <fomichev.ru@gmail.com>
> >> CC: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>
> >> --
> >> Regards,
> >> Alexander
> >
> > Alexander: Thanks for your bug report. The counter suffix you suggest
> > is precisely what NVMEM_DEVID_AUTO would do so I think we'll need to
> > use it. On the other hand, a non-unique label is bad design but
> > obviously we can't break working setups.
>
> Yes the intention was that the label itself would be unique. In our case
> we wanted to have something specifically named 'system' or 'module' to
> identify a specific eeprom.
>
> BTW I did a quick grep from 'dimm-spd' and did not find it in the
> kernel. Where is the device-tree you are using?
>
> > Jon: As the author of this patch - do you have any objections/better ideas?
>
> I would need to check if appending a suffix then has ramifications for
> what we were trying to achieve.
>
On POWER8/POWER9 platforms (affected by this issue) the devicetree is
built on-the-boot by firmware. Here are labels of the current
eeprom/nvmem devices:
$ l=$(find /sys/firmware/devicetree/base/xscom@603fc00000000/ -name label); for i in `echo "$l"`; do echo $i ':'; cat $i; echo; done
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a1000/i2c-bus@2/eeprom@50/label :
module-vpd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a1000/i2c-bus@0/eeprom@50/label :
module-vpd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@0/eeprom@52/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@0/eeprom@50/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@0/eeprom@53/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@0/eeprom@51/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@1/eeprom@54/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@1/eeprom@57/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@1/eeprom@55/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a3000/i2c-bus@1/eeprom@56/label :
dimm-spd
/sys/firmware/devicetree/base/xscom@603fc00000000/i2cm@a2000/i2c-bus@0/eeprom@50/label :
system-vpd
There are two 'module-vpd' labels and multiple 'dimm-spd' labels. Even
more can exist.
Possible devicetree template, used by OpenPower Firmware:
https://raw.githubusercontent.com/open-power/nicole-xml/master/nicole.xml
So this is not a MUST for label to be unique.
--
Regards,
Alexander
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-06-17 8:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 17:08 [PATCH] eeprom: at24: Support custom device names for AT24 EEPROMs Alexander Fomichev
2021-06-04 10:00 ` Bartosz Golaszewski
2021-06-07 11:09 ` Jon Hunter
2021-06-07 13:31 ` Bartosz Golaszewski
2021-06-07 14:28 ` Jon Hunter
2021-06-16 17:00 ` Alexander Fomichev
2021-06-16 19:21 ` Bartosz Golaszewski
2021-06-17 8:06 ` Alexander Fomichev
2021-06-16 16:55 ` Alexander Fomichev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.