Received-SPF: pass (google.com: domain of linux-kernel+bounces-59671-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240209163454.98051-1-srinivas.kandagatla@linaro.org>
In-Reply-To: <20240209163454.98051-1-srinivas.kandagatla@linaro.org>
From: Eric Curtin <ecurtin@redhat.com>
Date: Fri, 9 Feb 2024 16:49:22 +0000
Message-ID: <CAOgh=FwFMaYvTwmqeOQrqHq2XANxghZbTuY3+SgFx_ozpysBOg@mail.gmail.com>
Subject: Re: [PATCH] nvmem: include bit index in cell sysfs file name
To: srinivas.kandagatla@linaro.org
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, 
	Arnd Bergmann <arnd@arndb.de>, regressions@lists.linux.dev, 
	Miquel Raynal <miquel.raynal@bootlin.com>, =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>, 
	Chen-Yu Tsai <wenst@chromium.org>, asahi@lists.linux.dev, Sven Peter <sven@svenpeter.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, 9 Feb 2024 at 16:43, <srinivas.kandagatla@linaro.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> Creating sysfs files for all Cells caused a boot failure for linux-6.8-rc=
1 on
> Apple M1, which (in downstream dts files) has multiple nvmem cells that u=
se 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 cell=
s on
> different bits of the same address. Avoid the issue by changing the file =
name
> to include the first bit number.
>
> Fixes: 0331c611949f ("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=C5=82 Mi=C5=82ecki <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>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Seems reasonable to me.

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

Is mise le meas/Regards,

Eric Curtin

> ---
> Hi Greg,
>
> Here is a fix in nvmem for 6.8, could you queue these for next possible r=
c.
>
> Did not cc Stable as this is only targeted for 6.8 and no backporting is
> required.
>
> Thanks,
> Srini
>
>  Documentation/ABI/testing/sysfs-nvmem-cells | 16 ++++++++--------
>  drivers/nvmem/core.c                        |  5 +++--
>  2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-nvmem-cells b/Documentation/=
ABI/testing/sysfs-nvmem-cells
> index 7af70adf3690..c7c9444f92a8 100644
> --- a/Documentation/ABI/testing/sysfs-nvmem-cells
> +++ b/Documentation/ABI/testing/sysfs-nvmem-cells
> @@ -4,18 +4,18 @@ KernelVersion:        6.5
>  Contact:       Miquel Raynal <miquel.raynal@bootlin.com>
>  Description:
>                 The "cells" folder contains one file per cell exposed by =
the
> -               NVMEM device. The name of the file is: <name>@<where>, wi=
th
> -               <name> being the cell name and <where> its location in th=
e NVMEM
> -               device, in hexadecimal (without the '0x' prefix, to mimic=
 device
> -               tree node names). The length of the file is the size of t=
he cell
> -               (when known). The content of the file is the binary conte=
nt of
> -               the cell (may sometimes be ASCII, likely without trailing
> -               character).
> +               NVMEM device. The name of the file is: "<name>@<byte>,<bi=
t>",
> +               with <name> being the cell name and <where> its location =
in
> +               the NVMEM device, in hexadecimal bytes and bits (without =
the
> +               '0x' prefix, to mimic device tree node names). The length=
 of
> +               the file is the size of the cell (when known). The conten=
t of
> +               the file is the binary content of the cell (may sometimes=
 be
> +               ASCII, likely without trailing character).
>                 Note: This file is only present if CONFIG_NVMEM_SYSFS
>                 is enabled.
>
>                 Example::
>
> -                 hexdump -C /sys/bus/nvmem/devices/1-00563/cells/product=
-name@d
> +                 hexdump -C /sys/bus/nvmem/devices/1-00563/cells/product=
-name@d,0
>                   00000000  54 4e 34 38 4d 2d 50 2d  44 4e         |TN48M=
-P-DN|
>                   0000000a
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 980123fb4dde..eb357ac2e54a 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -460,8 +460,9 @@ static int nvmem_populate_sysfs_cells(struct nvmem_de=
vice *nvmem)
>         list_for_each_entry(entry, &nvmem->cells, node) {
>                 sysfs_bin_attr_init(&attrs[i]);
>                 attrs[i].attr.name =3D devm_kasprintf(&nvmem->dev, GFP_KE=
RNEL,
> -                                                   "%s@%x", entry->name,
> -                                                   entry->offset);
> +                                                   "%s@%x,%x", entry->na=
me,
> +                                                   entry->offset,
> +                                                   entry->bit_offset);
>                 attrs[i].attr.mode =3D 0444;
>                 attrs[i].size =3D entry->bytes;
>                 attrs[i].read =3D &nvmem_cell_attr_read;
> --
> 2.25.1
>
>