[PATCH 2/2] nvmem: core: append offset to cell name in sysfs

[PATCH 2/2] nvmem: core: append offset to cell name in sysfs

[Daniel Golle]

The device tree node names are not required to be unique. Append the
offset to the name to make cell nodes with identical names become
accessible via sysfs and avoid kernel stackdump caused by
sysfs: cannot create duplicate filename '...'

Fixes: 757f8b3835c9 ("nvmem: core: Expose cells through sysfs")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/nvmem/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 70e951088826d..90fe9dc30f8ba 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -480,7 +480,9 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 	/* Initialize each attribute to take the name and size of the cell */
 	list_for_each_entry(entry, &nvmem->cells, node) {
 		sysfs_bin_attr_init(&attrs[i]);
-		attrs[i].attr.name = devm_kstrdup(&nvmem->dev, entry->name, GFP_KERNEL);
+		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
+						    "%s@%d", entry->name,
+						    entry->offset);
 		attrs[i].attr.mode = 0444;
 		attrs[i].size = entry->bytes;
 		attrs[i].read = &nvmem_cell_attr_read;
-- 
2.41.0

Re: [PATCH 2/2] nvmem: core: append offset to cell name in sysfs

[Greg Kroah-Hartman]

On Tue, Jul 18, 2023 at 02:55:57PM +0100, Daniel Golle wrote:
> The device tree node names are not required to be unique. Append the
> offset to the name to make cell nodes with identical names become
> accessible via sysfs and avoid kernel stackdump caused by
> sysfs: cannot create duplicate filename '...'
> 
> Fixes: 757f8b3835c9 ("nvmem: core: Expose cells through sysfs")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/nvmem/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 70e951088826d..90fe9dc30f8ba 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -480,7 +480,9 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
>  	/* Initialize each attribute to take the name and size of the cell */
>  	list_for_each_entry(entry, &nvmem->cells, node) {
>  		sysfs_bin_attr_init(&attrs[i]);
> -		attrs[i].attr.name = devm_kstrdup(&nvmem->dev, entry->name, GFP_KERNEL);
> +		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
> +						    "%s@%d", entry->name,
> +						    entry->offset);

No documenatation update to show the new naming scheme?

And again, I don't see that git id in Linus's tree, am I just not up to
date properly?

thanks,

greg k-h

Re: [PATCH 2/2] nvmem: core: append offset to cell name in sysfs

[Miquel Raynal]

Hi Daniel,

daniel@makrotopia.org wrote on Tue, 18 Jul 2023 14:55:57 +0100:

> The device tree node names are not required to be unique. Append the
> offset to the name to make cell nodes with identical names become
> accessible via sysfs and avoid kernel stackdump caused by
> sysfs: cannot create duplicate filename '...'
> 
> Fixes: 757f8b3835c9 ("nvmem: core: Expose cells through sysfs")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/nvmem/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 70e951088826d..90fe9dc30f8ba 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -480,7 +480,9 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
>  	/* Initialize each attribute to take the name and size of the cell */
>  	list_for_each_entry(entry, &nvmem->cells, node) {
>  		sysfs_bin_attr_init(&attrs[i]);
> -		attrs[i].attr.name = devm_kstrdup(&nvmem->dev, entry->name, GFP_KERNEL);
> +		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
> +						    "%s@%d", entry->name,
> +						    entry->offset);

Shouldn't we use %s@%x instead to match the DT descriptions?

>  		attrs[i].attr.mode = 0444;
>  		attrs[i].size = entry->bytes;
>  		attrs[i].read = &nvmem_cell_attr_read;

I plan another version of the series which triggered these patches, if
you don't mind I will squash patch 1 into my patches and pick this one
as a preparation change (likely with the above fix if you agree and
without the Fixes tag).

Thanks,
Miquèl

Re: [PATCH 2/2] nvmem: core: append offset to cell name in sysfs

[Daniel Golle]

Hi Miquel,

On Mon, Jul 31, 2023 at 05:45:06PM +0200, Miquel Raynal wrote:
> Hi Daniel,
> 
> daniel@makrotopia.org wrote on Tue, 18 Jul 2023 14:55:57 +0100:
> 
> > The device tree node names are not required to be unique. Append the
> > offset to the name to make cell nodes with identical names become
> > accessible via sysfs and avoid kernel stackdump caused by
> > sysfs: cannot create duplicate filename '...'
> > 
> > Fixes: 757f8b3835c9 ("nvmem: core: Expose cells through sysfs")
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  drivers/nvmem/core.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 70e951088826d..90fe9dc30f8ba 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -480,7 +480,9 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> >  	/* Initialize each attribute to take the name and size of the cell */
> >  	list_for_each_entry(entry, &nvmem->cells, node) {
> >  		sysfs_bin_attr_init(&attrs[i]);
> > -		attrs[i].attr.name = devm_kstrdup(&nvmem->dev, entry->name, GFP_KERNEL);
> > +		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
> > +						    "%s@%d", entry->name,
> > +						    entry->offset);
> 
> Shouldn't we use %s@%x instead to match the DT descriptions?

True, I agree.

> 
> >  		attrs[i].attr.mode = 0444;
> >  		attrs[i].size = entry->bytes;
> >  		attrs[i].read = &nvmem_cell_attr_read;
> 
> I plan another version of the series which triggered these patches, if
> you don't mind I will squash patch 1 into my patches and pick this one
> as a preparation change (likely with the above fix if you agree and
> without the Fixes tag).

Yes, sure, please go ahead.


Cheers


Daniel

> 
> Thanks,
> Miquèl