nvmem: include bit index in cell sysfs file name

  From: Arnd Bergmann <arnd@arndb.de>

Creating sysfs files for all Cells caused a boot failure for linux-6.8-rc1 on
Apple M1, which (in downstream dts files) has multiple nvmem cells that use the
same byte address. This causes the device probe to fail with

[    0.605336] sysfs: cannot create duplicate filename '/devices/platform/soc@200000000/2922bc000.efuse/apple_efuses_nvmem0/cells/efuse@a10'
[    0.605347] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G S                 6.8.0-rc1-arnd-5+ #133
[    0.605355] Hardware name: Apple Mac Studio (M1 Ultra, 2022) (DT)
[    0.605362] Call trace:
[    0.605365]  show_stack+0x18/0x2c
[    0.605374]  dump_stack_lvl+0x60/0x80
[    0.605383]  dump_stack+0x18/0x24
[    0.605388]  sysfs_warn_dup+0x64/0x80
[    0.605395]  sysfs_add_bin_file_mode_ns+0xb0/0xd4
[    0.605402]  internal_create_group+0x268/0x404
[    0.605409]  sysfs_create_groups+0x38/0x94
[    0.605415]  devm_device_add_groups+0x50/0x94
[    0.605572]  nvmem_populate_sysfs_cells+0x180/0x1b0
[    0.605682]  nvmem_register+0x38c/0x470
[    0.605789]  devm_nvmem_register+0x1c/0x6c
[    0.605895]  apple_efuses_probe+0xe4/0x120
[    0.606000]  platform_probe+0xa8/0xd0

As far as I can tell, this is a problem for any device with multiple cells on
different bits of the same address. Avoid the issue by changing the file name
to include the first bit number.

Fixes: 0088cbc19276 ("nvmem: core: Expose cells through sysfs")
Link: https://github.com/AsahiLinux/linux/blob/bd0a1a7d4/arch/arm64/boot/dts/apple/t600x-dieX.dtsi#L156
Cc: regressions@lists.linux.dev
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Rafał Miłecki <rafal@milecki.pl>
Cc: Chen-Yu Tsai <wenst@chromium.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: asahi@lists.linux.dev
Cc: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
This might not be the fix we want upstream, but it illustrates the issue and
I have successfully tested that this lets me boot v6.8-rc1 on my workstation
again.

Feel free to just treat this as a bug report and suggest a different fix.
As the ABI was only introduced in 6.8-rc1, it can still be changed without
causing other regressions for users, but time is running out for that.
---
 Documentation/ABI/testing/sysfs-nvmem-cells | 16 ++++++++--------
 drivers/nvmem/core.c                        |  5 +++--
 2 files changed, 11 insertions(+), 10 deletions(-)
  

On Mon, 22 Jan 2024 16:34:10 +0100, Arnd Bergmann wrote:
> Creating sysfs files for all Cells caused a boot failure for linux-6.8-rc1 on
> Apple M1, which (in downstream dts files) has multiple nvmem cells that use the
> same byte address. This causes the device probe to fail with
> 
> [    0.605336] sysfs: cannot create duplicate filename '/devices/platform/soc@200000000/2922bc000.efuse/apple_efuses_nvmem0/cells/efuse@a10'
> [    0.605347] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G S                 6.8.0-rc1-arnd-5+ #133
> [    0.605355] Hardware name: Apple Mac Studio (M1 Ultra, 2022) (DT)
> [    0.605362] Call trace:
> [    0.605365]  show_stack+0x18/0x2c
> [    0.605374]  dump_stack_lvl+0x60/0x80
> [    0.605383]  dump_stack+0x18/0x24
> [    0.605388]  sysfs_warn_dup+0x64/0x80
> [    0.605395]  sysfs_add_bin_file_mode_ns+0xb0/0xd4
> [    0.605402]  internal_create_group+0x268/0x404
> [    0.605409]  sysfs_create_groups+0x38/0x94
> [    0.605415]  devm_device_add_groups+0x50/0x94
> [    0.605572]  nvmem_populate_sysfs_cells+0x180/0x1b0
> [    0.605682]  nvmem_register+0x38c/0x470
> [    0.605789]  devm_nvmem_register+0x1c/0x6c
> [    0.605895]  apple_efuses_probe+0xe4/0x120
> [    0.606000]  platform_probe+0xa8/0xd0
> 
> [...]

Applied, thanks!

[1/1] nvmem: include bit index in cell sysfs file name
      commit: b40fed13870045731e374e6bb48800cde0feb4e2

Best regards,
  

Hi Arnd,

arnd@kernel.org wrote on Mon, 22 Jan 2024 16:34:10 +0100:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> Creating sysfs files for all Cells caused a boot failure for linux-6.8-rc1 on
> Apple M1, which (in downstream dts files) has multiple nvmem cells that use the
> same byte address. This causes the device probe to fail with

o_O	I didn't even know this was allowed...

> [    0.605336] sysfs: cannot create duplicate filename '/devices/platform/soc@200000000/2922bc000.efuse/apple_efuses_nvmem0/cells/efuse@a10'
> [    0.605347] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G S                 6.8.0-rc1-arnd-5+ #133
> [    0.605355] Hardware name: Apple Mac Studio (M1 Ultra, 2022) (DT)
> [    0.605362] Call trace:
> [    0.605365]  show_stack+0x18/0x2c
> [    0.605374]  dump_stack_lvl+0x60/0x80
> [    0.605383]  dump_stack+0x18/0x24
> [    0.605388]  sysfs_warn_dup+0x64/0x80
> [    0.605395]  sysfs_add_bin_file_mode_ns+0xb0/0xd4
> [    0.605402]  internal_create_group+0x268/0x404
> [    0.605409]  sysfs_create_groups+0x38/0x94
> [    0.605415]  devm_device_add_groups+0x50/0x94
> [    0.605572]  nvmem_populate_sysfs_cells+0x180/0x1b0
> [    0.605682]  nvmem_register+0x38c/0x470
> [    0.605789]  devm_nvmem_register+0x1c/0x6c
> [    0.605895]  apple_efuses_probe+0xe4/0x120
> [    0.606000]  platform_probe+0xa8/0xd0
> 
> As far as I can tell, this is a problem for any device with multiple cells on
> different bits of the same address. Avoid the issue by changing the file name
> to include the first bit number.

There is only one bit number right? We are talking about byte offsets
so this value can only range from 0 to 7? If we understand each other
correctly then why not, I'm fine with the extra ",0" thing.

> Fixes: 0088cbc19276 ("nvmem: core: Expose cells through sysfs")
> Link: https://github.com/AsahiLinux/linux/blob/bd0a1a7d4/arch/arm64/boot/dts/apple/t600x-dieX.dtsi#L156
> Cc: regressions@lists.linux.dev
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Rafał Miłecki <rafal@milecki.pl>
> Cc: Chen-Yu Tsai <wenst@chromium.org>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: asahi@lists.linux.dev
> Cc: Sven Peter <sven@svenpeter.dev>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Thanks,
Miquèl
  

On Wed, Jan 24, 2024, at 18:22, Miquel Raynal wrote:
> arnd@kernel.org wrote on Mon, 22 Jan 2024 16:34:10 +0100:
>
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> 
>> As far as I can tell, this is a problem for any device with multiple cells on
>> different bits of the same address. Avoid the issue by changing the file name
>> to include the first bit number.
>
> There is only one bit number right? We are talking about byte offsets
> so this value can only range from 0 to 7? If we understand each other
> correctly then why not, I'm fine with the extra ",0" thing.

On the Apple M1, the nvmem registers are 32 bit wide, so the
bit numbers can go up to 31. I can imagine some system using
64-bit registers, but it's unlikely to be higher than that.

      Arnd
  

Hi Arnd,

arnd@arndb.de wrote on Wed, 24 Jan 2024 20:49:53 +0100:

> On Wed, Jan 24, 2024, at 18:22, Miquel Raynal wrote:
> > arnd@kernel.org wrote on Mon, 22 Jan 2024 16:34:10 +0100:
> >  
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> 
> >> As far as I can tell, this is a problem for any device with multiple cells on
> >> different bits of the same address. Avoid the issue by changing the file name
> >> to include the first bit number.  
> >
> > There is only one bit number right? We are talking about byte offsets
> > so this value can only range from 0 to 7? If we understand each other
> > correctly then why not, I'm fine with the extra ",0" thing.  
> 
> On the Apple M1, the nvmem registers are 32 bit wide, so the
> bit numbers can go up to 31. I can imagine some system using
> 64-bit registers, but it's unlikely to be higher than that.

In this case we will soon or later have a problem again. Can we include
the full offset of the bit and not just the first digit?

Thanks,
Miquèl
  

On Wed, Jan 24, 2024, at 23:11, Miquel Raynal wrote:
> Hi Arnd,
>
> arnd@arndb.de wrote on Wed, 24 Jan 2024 20:49:53 +0100:
>
>> On Wed, Jan 24, 2024, at 18:22, Miquel Raynal wrote:
>> > arnd@kernel.org wrote on Mon, 22 Jan 2024 16:34:10 +0100:
>> >  
>> >> From: Arnd Bergmann <arnd@arndb.de>
>> >> 
>> >> 
>> >> As far as I can tell, this is a problem for any device with multiple cells on
>> >> different bits of the same address. Avoid the issue by changing the file name
>> >> to include the first bit number.  
>> >
>> > There is only one bit number right? We are talking about byte offsets
>> > so this value can only range from 0 to 7? If we understand each other
>> > correctly then why not, I'm fine with the extra ",0" thing.  
>> 
>> On the Apple M1, the nvmem registers are 32 bit wide, so the
>> bit numbers can go up to 31. I can imagine some system using
>> 64-bit registers, but it's unlikely to be higher than that.
>
> In this case we will soon or later have a problem again. Can we include
> the full offset of the bit and not just the first digit?

I thought that is what my patch does, maybe I don't
undestand the problem you are referring to. This is what
I see on my system with the patch applied:

$ cd /sys/devices/platform/soc@200000000/2922bc000.efuse
$ find . -name efuse\*
/apple_efuses_nvmem0/cells/efuse@a24,11
/apple_efuses_nvmem0/cells/efuse@a24,9
/apple_efuses_nvmem0/cells/efuse@a1c,f
/apple_efuses_nvmem0/cells/efuse@a20,17
/apple_efuses_nvmem0/cells/efuse@a20,1e
/apple_efuses_nvmem0/cells/efuse@a18,0
/apple_efuses_nvmem0/cells/efuse@a14,b
/apple_efuses_nvmem0/cells/efuse@a1c,1f
/apple_efuses_nvmem0/cells/efuse@a1c,d
/apple_efuses_nvmem0/cells/efuse@a20,1c
/apple_efuses_nvmem0/cells/efuse@a18,15
/apple_efuses_nvmem0/cells/efuse@a14,0
/apple_efuses_nvmem0/cells/efuse@a1c,14
/apple_efuses_nvmem0/cells/efuse@a24,3
/apple_efuses_nvmem0/cells/efuse@a20,7
/apple_efuses_nvmem0/cells/efuse@a18,5
/apple_efuses_nvmem0/cells/efuse@a10,16
/apple_efuses_nvmem0/cells/efuse@a1c,12
/apple_efuses_nvmem0/cells/efuse@a20,5
/apple_efuses_nvmem0/cells/efuse@a18,3
/apple_efuses_nvmem0/cells/efuse@a18,a
/apple_efuses_nvmem0/cells/efuse@a10,1b
/apple_efuses_nvmem0/cells/efuse@a14,5
/apple_efuses_nvmem0/cells/efuse@a1c,19
/apple_efuses_nvmem0/cells/efuse@a24,f
/apple_efuses_nvmem0/cells/efuse@a18,1d
/apple_efuses_nvmem0/cells/efuse@a14,13
/apple_efuses_nvmem0/cells/efuse@a18,8
/apple_efuses_nvmem0/cells/efuse@a18,f
/apple_efuses_nvmem0/cells/efuse@a20,14
/apple_efuses_nvmem0/cells/efuse@a10,19
/apple_efuses_nvmem0/cells/efuse@a18,1b
/apple_efuses_nvmem0/cells/efuse@a14,11
/apple_efuses_nvmem0/cells/efuse@a1c,a
/apple_efuses_nvmem0/cells/efuse@a10,1e
/apple_efuses_nvmem0/cells/efuse@a20,19

      Arnd
  

Hi Arnd,

arnd@arndb.de wrote on Thu, 25 Jan 2024 13:15:26 +0100:

> On Wed, Jan 24, 2024, at 23:11, Miquel Raynal wrote:
> > Hi Arnd,
> >
> > arnd@arndb.de wrote on Wed, 24 Jan 2024 20:49:53 +0100:
> >  
> >> On Wed, Jan 24, 2024, at 18:22, Miquel Raynal wrote:  
> >> > arnd@kernel.org wrote on Mon, 22 Jan 2024 16:34:10 +0100:
> >> >    
> >> >> From: Arnd Bergmann <arnd@arndb.de>
> >> >> 
> >> >> 
> >> >> As far as I can tell, this is a problem for any device with multiple cells on
> >> >> different bits of the same address. Avoid the issue by changing the file name
> >> >> to include the first bit number.    
> >> >
> >> > There is only one bit number right? We are talking about byte offsets
> >> > so this value can only range from 0 to 7? If we understand each other
> >> > correctly then why not, I'm fine with the extra ",0" thing.    
> >> 
> >> On the Apple M1, the nvmem registers are 32 bit wide, so the
> >> bit numbers can go up to 31. I can imagine some system using
> >> 64-bit registers, but it's unlikely to be higher than that.  
> >
> > In this case we will soon or later have a problem again. Can we include
> > the full offset of the bit and not just the first digit?  
> 
> I thought that is what my patch does, maybe I don't
> undestand the problem you are referring to. This is what
> I see on my system with the patch applied:
> 
> $ cd /sys/devices/platform/soc@200000000/2922bc000.efuse
> $ find . -name efuse\*
> ./apple_efuses_nvmem0/cells/efuse@a24,11

Sorry for the misunderstanding, I thought the above situation would
be:

  ./apple_efuses_nvmem0/cells/efuse@a24,1

But the below output is actually fine.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

> ./apple_efuses_nvmem0/cells/efuse@a24,9
> ./apple_efuses_nvmem0/cells/efuse@a1c,f
> ./apple_efuses_nvmem0/cells/efuse@a20,17
> ./apple_efuses_nvmem0/cells/efuse@a20,1e
> ./apple_efuses_nvmem0/cells/efuse@a18,0
> ./apple_efuses_nvmem0/cells/efuse@a14,b
> ./apple_efuses_nvmem0/cells/efuse@a1c,1f
> ./apple_efuses_nvmem0/cells/efuse@a1c,d
> ./apple_efuses_nvmem0/cells/efuse@a20,1c
> ./apple_efuses_nvmem0/cells/efuse@a18,15
> ./apple_efuses_nvmem0/cells/efuse@a14,0
> ./apple_efuses_nvmem0/cells/efuse@a1c,14
> ./apple_efuses_nvmem0/cells/efuse@a24,3
> ./apple_efuses_nvmem0/cells/efuse@a20,7
> ./apple_efuses_nvmem0/cells/efuse@a18,5
> ./apple_efuses_nvmem0/cells/efuse@a10,16
> ./apple_efuses_nvmem0/cells/efuse@a1c,12
> ./apple_efuses_nvmem0/cells/efuse@a20,5
> ./apple_efuses_nvmem0/cells/efuse@a18,3
> ./apple_efuses_nvmem0/cells/efuse@a18,a
> ./apple_efuses_nvmem0/cells/efuse@a10,1b
> ./apple_efuses_nvmem0/cells/efuse@a14,5
> ./apple_efuses_nvmem0/cells/efuse@a1c,19
> ./apple_efuses_nvmem0/cells/efuse@a24,f
> ./apple_efuses_nvmem0/cells/efuse@a18,1d
> ./apple_efuses_nvmem0/cells/efuse@a14,13
> ./apple_efuses_nvmem0/cells/efuse@a18,8
> ./apple_efuses_nvmem0/cells/efuse@a18,f
> ./apple_efuses_nvmem0/cells/efuse@a20,14
> ./apple_efuses_nvmem0/cells/efuse@a10,19
> ./apple_efuses_nvmem0/cells/efuse@a18,1b
> ./apple_efuses_nvmem0/cells/efuse@a14,11
> ./apple_efuses_nvmem0/cells/efuse@a1c,a
> ./apple_efuses_nvmem0/cells/efuse@a10,1e
> ./apple_efuses_nvmem0/cells/efuse@a20,19
> 
>       Arnd


Thanks,
Miquèl
  

On 22.01.24 17:55, Srinivas Kandagatla wrote:
> On Mon, 22 Jan 2024 16:34:10 +0100, Arnd Bergmann wrote:
>> Creating sysfs files for all Cells caused a boot failure for linux-6.8-rc1 on
>> Apple M1, which (in downstream dts files) has multiple nvmem cells that use the
>> same byte address. This causes the device probe to fail with
>>
>> [    0.605336] sysfs: cannot create duplicate filename '/devices/platform/soc@200000000/2922bc000.efuse/apple_efuses_nvmem0/cells/efuse@a10'
>> [    0.605347] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G S                 6.8.0-rc1-arnd-5+ #133
>> [    0.605355] Hardware name: Apple Mac Studio (M1 Ultra, 2022) (DT)
>> [    0.605362] Call trace:
>> [...]
> 
> Applied, thanks!
> 
> [1/1] nvmem: include bit index in cell sysfs file name
>       commit: b40fed13870045731e374e6bb48800cde0feb4e2

The problem description from Arnd to an outsider like me sounded like
this is something that should be fixed rather sooner than later in
mainline. Am I wrong with that? If not: will this be heading to Linus
soon? Just wondering, as the fix seems to be a in "for-next" branch[1]
of the nvmem repo and not in a "fixes" branch.

Or am I missing something here?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke
  

On Fri, Feb 9, 2024, at 10:09, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 22.01.24 17:55, Srinivas Kandagatla wrote:
>> On Mon, 22 Jan 2024 16:34:10 +0100, Arnd Bergmann wrote:
>>> Creating sysfs files for all Cells caused a boot failure for linux-6.8-rc1 on
>>> Apple M1, which (in downstream dts files) has multiple nvmem cells that use the
>>> same byte address. This causes the device probe to fail with
>>>
>>> [    0.605336] sysfs: cannot create duplicate filename '/devices/platform/soc@200000000/2922bc000.efuse/apple_efuses_nvmem0/cells/efuse@a10'
>>> [    0.605347] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G S                 6.8.0-rc1-arnd-5+ #133
>>> [    0.605355] Hardware name: Apple Mac Studio (M1 Ultra, 2022) (DT)
>>> [    0.605362] Call trace:
>>> [...]
>> 
>> Applied, thanks!
>> 
>> [1/1] nvmem: include bit index in cell sysfs file name
>>       commit: b40fed13870045731e374e6bb48800cde0feb4e2
>
> The problem description from Arnd to an outsider like me sounded like
> this is something that should be fixed rather sooner than later in
> mainline. Am I wrong with that? If not: will this be heading to Linus
> soon? Just wondering, as the fix seems to be a in "for-next" branch[1]
> of the nvmem repo and not in a "fixes" branch.

Yes, this that this needs to be fixed before v6.8 is
out. I assumed it had gone upstream already.

If anyone is still unsure about the ABI, we could also revert
the original commit 0088cbc19276 ("nvmem: core: Expose cells
through sysfs") for v6.8 and try again for v6.9 with the
fixed ABI, but I think we already had an agreement on the
changes that I sent.

     Arnd