[PATCH] mtd: Fix mtd not the same name not registered if nvmem

[PATCH] mtd: Fix mtd not the same name not registered if nvmem

[Ricardo Ribalda Delgado]

When the nvmem framework is enabled, a nvmem device is created per mtd
device/partition.

It is not uncommon that a device can have multiple mtd devices with
partitions that have the same name. Eg, when there DT overlay is allowed
and the same device with mtd is attached twice.

Under that circumstances, the mtd fails to register due to a name
duplication on the nvmem framework.

With this patch we add a _1, _2, _X to the subsequent names if there is
a collition, and throw a warning, instead of not starting the mtd
device.

[    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
[    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
[    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
[    8.948994] Call Trace:
[    8.948996]  dump_stack+0x50/0x70
[    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
[    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
[    8.949002]  bus_add_device+0x74/0x140
[    8.949004]  device_add+0x34b/0x850
[    8.949006]  nvmem_register.part.0+0x1bf/0x640
...
[    8.948926] mtd mtd8: Failed to register NVMEM device

Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 drivers/mtd/mtdcore.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5fac4355b9c2..7653d45a470a 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -28,6 +28,7 @@
 #include <linux/leds.h>
 #include <linux/debugfs.h>
 #include <linux/nvmem-provider.h>
+#include <linux/nvmem-consumer.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -545,13 +546,34 @@ static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
 	return retlen == bytes ? 0 : -EIO;
 }
 
+static int nvmem_next_name(const char *init_name, char *name, size_t len)
+{
+	unsigned int i = 0;
+	int ret = 0;
+	struct nvmem_device *dev = NULL;
+
+	strlcpy(name, init_name, len);
+
+	while ((ret < len) &&
+	       !IS_ERR(dev = nvmem_device_find(name, device_match_name))) {
+		nvmem_device_put(dev);
+		ret = snprintf(name, len, "%s_%u", init_name, ++i);
+	}
+
+	if (ret >= len)
+		return -ENOMEM;
+
+	return i;
+}
+
 static int mtd_nvmem_add(struct mtd_info *mtd)
 {
 	struct nvmem_config config = {};
+	char name[128];
+	int ret = 0;
 
 	config.id = -1;
 	config.dev = &mtd->dev;
-	config.name = mtd->name;
 	config.owner = THIS_MODULE;
 	config.reg_read = mtd_nvmem_reg_read;
 	config.size = mtd->size;
@@ -562,6 +584,13 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
 	config.no_of_node = true;
 	config.priv = mtd;
 
+	if (mtd->name) {
+		ret = nvmem_next_name(mtd->name, name, sizeof(name));
+		if (ret < 0)
+			return ret;
+		config.name = name;
+	}
+
 	mtd->nvmem = nvmem_register(&config);
 	if (IS_ERR(mtd->nvmem)) {
 		/* Just ignore if there is no NVMEM support in the kernel */
@@ -569,10 +598,15 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
 			mtd->nvmem = NULL;
 		} else {
 			dev_err(&mtd->dev, "Failed to register NVMEM device\n");
+			mtd->nvmem = NULL;
 			return PTR_ERR(mtd->nvmem);
 		}
 	}
 
+	if (ret)
+		dev_warn(&mtd->dev, "mtdev %s renamed to %s due to name collision",
+				mtd->name, nvmem_dev_name(mtd->nvmem));
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Ricardo Ribalda Delgado]

When the nvmem framework is enabled, a nvmem device is created per mtd
device/partition.

It is not uncommon that a device can have multiple mtd devices with
partitions that have the same name. Eg, when there DT overlay is allowed
and the same device with mtd is attached twice.

Under that circumstances, the mtd fails to register due to a name
duplication on the nvmem framework.

With this patch we add a _1, _2, _X to the subsequent names if there is
a collition, and throw a warning, instead of not starting the mtd
device.

[    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
[    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
[    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
[    8.948994] Call Trace:
[    8.948996]  dump_stack+0x50/0x70
[    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
[    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
[    8.949002]  bus_add_device+0x74/0x140
[    8.949004]  device_add+0x34b/0x850
[    8.949006]  nvmem_register.part.0+0x1bf/0x640
...
[    8.948926] mtd mtd8: Failed to register NVMEM device

Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
v2: I left behind on my patch a 

mtd->nvmem = NULL;

from my tests. Sorry.

 drivers/mtd/mtdcore.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5fac4355b9c2..7a4b520ef3b0 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -28,6 +28,7 @@
 #include <linux/leds.h>
 #include <linux/debugfs.h>
 #include <linux/nvmem-provider.h>
+#include <linux/nvmem-consumer.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -545,13 +546,34 @@ static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
 	return retlen == bytes ? 0 : -EIO;
 }
 
+static int nvmem_next_name(const char *init_name, char *name, size_t len)
+{
+	unsigned int i = 0;
+	int ret = 0;
+	struct nvmem_device *dev = NULL;
+
+	strlcpy(name, init_name, len);
+
+	while ((ret < len) &&
+	       !IS_ERR(dev = nvmem_device_find(name, device_match_name))) {
+		nvmem_device_put(dev);
+		ret = snprintf(name, len, "%s_%u", init_name, ++i);
+	}
+
+	if (ret >= len)
+		return -ENOMEM;
+
+	return i;
+}
+
 static int mtd_nvmem_add(struct mtd_info *mtd)
 {
 	struct nvmem_config config = {};
+	char name[128];
+	int ret = 0;
 
 	config.id = -1;
 	config.dev = &mtd->dev;
-	config.name = mtd->name;
 	config.owner = THIS_MODULE;
 	config.reg_read = mtd_nvmem_reg_read;
 	config.size = mtd->size;
@@ -562,6 +584,13 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
 	config.no_of_node = true;
 	config.priv = mtd;
 
+	if (mtd->name) {
+		ret = nvmem_next_name(mtd->name, name, sizeof(name));
+		if (ret < 0)
+			return ret;
+		config.name = name;
+	}
+
 	mtd->nvmem = nvmem_register(&config);
 	if (IS_ERR(mtd->nvmem)) {
 		/* Just ignore if there is no NVMEM support in the kernel */
@@ -573,6 +602,10 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
 		}
 	}
 
+	if (ret)
+		dev_warn(&mtd->dev, "mtdev %s renamed to %s due to name collision",
+				mtd->name, nvmem_dev_name(mtd->nvmem));
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Ricardo Ribalda Delgado]

Ping?

On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado
<ribalda@kernel.org> wrote:
>
> When the nvmem framework is enabled, a nvmem device is created per mtd
> device/partition.
>
> It is not uncommon that a device can have multiple mtd devices with
> partitions that have the same name. Eg, when there DT overlay is allowed
> and the same device with mtd is attached twice.
>
> Under that circumstances, the mtd fails to register due to a name
> duplication on the nvmem framework.
>
> With this patch we add a _1, _2, _X to the subsequent names if there is
> a collition, and throw a warning, instead of not starting the mtd
> device.
>
> [    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
> [    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
> [    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
> [    8.948994] Call Trace:
> [    8.948996]  dump_stack+0x50/0x70
> [    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
> [    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> [    8.949002]  bus_add_device+0x74/0x140
> [    8.949004]  device_add+0x34b/0x850
> [    8.949006]  nvmem_register.part.0+0x1bf/0x640
> ...
> [    8.948926] mtd mtd8: Failed to register NVMEM device
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> v2: I left behind on my patch a
>
> mtd->nvmem = NULL;
>
> from my tests. Sorry.
>
>  drivers/mtd/mtdcore.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 5fac4355b9c2..7a4b520ef3b0 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -28,6 +28,7 @@
>  #include <linux/leds.h>
>  #include <linux/debugfs.h>
>  #include <linux/nvmem-provider.h>
> +#include <linux/nvmem-consumer.h>
>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
> @@ -545,13 +546,34 @@ static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
>         return retlen == bytes ? 0 : -EIO;
>  }
>
> +static int nvmem_next_name(const char *init_name, char *name, size_t len)
> +{
> +       unsigned int i = 0;
> +       int ret = 0;
> +       struct nvmem_device *dev = NULL;
> +
> +       strlcpy(name, init_name, len);
> +
> +       while ((ret < len) &&
> +              !IS_ERR(dev = nvmem_device_find(name, device_match_name))) {
> +               nvmem_device_put(dev);
> +               ret = snprintf(name, len, "%s_%u", init_name, ++i);
> +       }
> +
> +       if (ret >= len)
> +               return -ENOMEM;
> +
> +       return i;
> +}
> +
>  static int mtd_nvmem_add(struct mtd_info *mtd)
>  {
>         struct nvmem_config config = {};
> +       char name[128];
> +       int ret = 0;
>
>         config.id = -1;
>         config.dev = &mtd->dev;
> -       config.name = mtd->name;
>         config.owner = THIS_MODULE;
>         config.reg_read = mtd_nvmem_reg_read;
>         config.size = mtd->size;
> @@ -562,6 +584,13 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>         config.no_of_node = true;
>         config.priv = mtd;
>
> +       if (mtd->name) {
> +               ret = nvmem_next_name(mtd->name, name, sizeof(name));
> +               if (ret < 0)
> +                       return ret;
> +               config.name = name;
> +       }
> +
>         mtd->nvmem = nvmem_register(&config);
>         if (IS_ERR(mtd->nvmem)) {
>                 /* Just ignore if there is no NVMEM support in the kernel */
> @@ -573,6 +602,10 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>                 }
>         }
>
> +       if (ret)
> +               dev_warn(&mtd->dev, "mtdev %s renamed to %s due to name collision",
> +                               mtd->name, nvmem_dev_name(mtd->nvmem));
> +
>         return 0;
>  }
>
> --
> 2.25.1
>

Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Miquel Raynal]

Hi Ricardo,

Ricardo Ribalda Delgado <ribalda@kernel.org> wrote on Tue, 14 Apr 2020
15:47:23 +0200:

> Ping?
> 
> On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado
> <ribalda@kernel.org> wrote:
> >
> > When the nvmem framework is enabled, a nvmem device is created per mtd
> > device/partition.
> >
> > It is not uncommon that a device can have multiple mtd devices with
> > partitions that have the same name. Eg, when there DT overlay is allowed
> > and the same device with mtd is attached twice.
> >
> > Under that circumstances, the mtd fails to register due to a name
> > duplication on the nvmem framework.
> >
> > With this patch we add a _1, _2, _X to the subsequent names if there is
> > a collition, and throw a warning, instead of not starting the mtd
> > device.
> >
> > [    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
> > [    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
> > [    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
> > [    8.948994] Call Trace:
> > [    8.948996]  dump_stack+0x50/0x70
> > [    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
> > [    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> > [    8.949002]  bus_add_device+0x74/0x140
> > [    8.949004]  device_add+0x34b/0x850
> > [    8.949006]  nvmem_register.part.0+0x1bf/0x640
> > ...
> > [    8.948926] mtd mtd8: Failed to register NVMEM device
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>

Thanks for proposing this change. Indeed we are aware of the problem
and the best solution that we could come up with was to create an
additional "unique_name" field to the mtd_info structure. This new
field would have the form:

    [<parent-unique-name><separator>]<mtd-name>

The separator might be '~' (but I am completely open on that), and that
would give for instance:

    my-controller~my-device~my-part~mysub-part

Then, you might use this mtd->unique_name instead of mtd->name. Would
you give this logic a try?

Thanks,
Miquèl

Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Boris Brezillon]

On Mon, 27 Apr 2020 16:22:22 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Ricardo,
> 
> Ricardo Ribalda Delgado <ribalda@kernel.org> wrote on Tue, 14 Apr 2020
> 15:47:23 +0200:
> 
> > Ping?
> > 
> > On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado
> > <ribalda@kernel.org> wrote:  
> > >
> > > When the nvmem framework is enabled, a nvmem device is created per mtd
> > > device/partition.
> > >
> > > It is not uncommon that a device can have multiple mtd devices with
> > > partitions that have the same name. Eg, when there DT overlay is allowed
> > > and the same device with mtd is attached twice.
> > >
> > > Under that circumstances, the mtd fails to register due to a name
> > > duplication on the nvmem framework.
> > >
> > > With this patch we add a _1, _2, _X to the subsequent names if there is
> > > a collition, and throw a warning, instead of not starting the mtd
> > > device.
> > >
> > > [    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
> > > [    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
> > > [    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
> > > [    8.948994] Call Trace:
> > > [    8.948996]  dump_stack+0x50/0x70
> > > [    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
> > > [    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> > > [    8.949002]  bus_add_device+0x74/0x140
> > > [    8.949004]  device_add+0x34b/0x850
> > > [    8.949006]  nvmem_register.part.0+0x1bf/0x640
> > > ...
> > > [    8.948926] mtd mtd8: Failed to register NVMEM device
> > >
> > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>  
> 
> Thanks for proposing this change. Indeed we are aware of the problem
> and the best solution that we could come up with was to create an
> additional "unique_name" field to the mtd_info structure. This new
> field would have the form:
> 
>     [<parent-unique-name><separator>]<mtd-name>
> 
> The separator might be '~' (but I am completely open on that), and that
> would give for instance:
> 
>     my-controller~my-device~my-part~mysub-part

I'd prefer something slightly more standard for the separator, like '/',
which is what we usually use when we want to represent a path in a tree.
I do agree on the general approach though.

Note that controller name is normally hidden in the root MTD device
name, and it's the driver responsibility to come up with a name that
does not collide with other MTD drivers. We can of course try to pick a
different name if we see another device with the same name, but we
should definitely warn about that so drivers are patched accordingly.

Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
2020 16:37:11 +0200:

> On Mon, 27 Apr 2020 16:22:22 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Ricardo,
> > 
> > Ricardo Ribalda Delgado <ribalda@kernel.org> wrote on Tue, 14 Apr 2020
> > 15:47:23 +0200:
> >   
> > > Ping?
> > > 
> > > On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado
> > > <ribalda@kernel.org> wrote:    
> > > >
> > > > When the nvmem framework is enabled, a nvmem device is created per mtd
> > > > device/partition.
> > > >
> > > > It is not uncommon that a device can have multiple mtd devices with
> > > > partitions that have the same name. Eg, when there DT overlay is allowed
> > > > and the same device with mtd is attached twice.
> > > >
> > > > Under that circumstances, the mtd fails to register due to a name
> > > > duplication on the nvmem framework.
> > > >
> > > > With this patch we add a _1, _2, _X to the subsequent names if there is
> > > > a collition, and throw a warning, instead of not starting the mtd
> > > > device.
> > > >
> > > > [    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
> > > > [    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
> > > > [    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
> > > > [    8.948994] Call Trace:
> > > > [    8.948996]  dump_stack+0x50/0x70
> > > > [    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
> > > > [    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> > > > [    8.949002]  bus_add_device+0x74/0x140
> > > > [    8.949004]  device_add+0x34b/0x850
> > > > [    8.949006]  nvmem_register.part.0+0x1bf/0x640
> > > > ...
> > > > [    8.948926] mtd mtd8: Failed to register NVMEM device
> > > >
> > > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>    
> > 
> > Thanks for proposing this change. Indeed we are aware of the problem
> > and the best solution that we could come up with was to create an
> > additional "unique_name" field to the mtd_info structure. This new
> > field would have the form:
> > 
> >     [<parent-unique-name><separator>]<mtd-name>
> > 
> > The separator might be '~' (but I am completely open on that), and that
> > would give for instance:
> > 
> >     my-controller~my-device~my-part~mysub-part  
> 
> I'd prefer something slightly more standard for the separator, like '/',
> which is what we usually use when we want to represent a path in a tree.
> I do agree on the general approach though.

I am not sure / is a valid separator here we would use this
name to create a sysfs entry. Would it work?

> Note that controller name is normally hidden in the root MTD device
> name, and it's the driver responsibility to come up with a name that
> does not collide with other MTD drivers. We can of course try to pick a
> different name if we see another device with the same name, but we
> should definitely warn about that so drivers are patched accordingly.

Yes absolutely.


Thanks,
Miquèl

Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Boris Brezillon]

On Mon, 27 Apr 2020 16:49:22 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
> 2020 16:37:11 +0200:
> 
> > On Mon, 27 Apr 2020 16:22:22 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Ricardo,
> > > 
> > > Ricardo Ribalda Delgado <ribalda@kernel.org> wrote on Tue, 14 Apr 2020
> > > 15:47:23 +0200:
> > >     
> > > > Ping?
> > > > 
> > > > On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado
> > > > <ribalda@kernel.org> wrote:      
> > > > >
> > > > > When the nvmem framework is enabled, a nvmem device is created per mtd
> > > > > device/partition.
> > > > >
> > > > > It is not uncommon that a device can have multiple mtd devices with
> > > > > partitions that have the same name. Eg, when there DT overlay is allowed
> > > > > and the same device with mtd is attached twice.
> > > > >
> > > > > Under that circumstances, the mtd fails to register due to a name
> > > > > duplication on the nvmem framework.
> > > > >
> > > > > With this patch we add a _1, _2, _X to the subsequent names if there is
> > > > > a collition, and throw a warning, instead of not starting the mtd
> > > > > device.
> > > > >
> > > > > [    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
> > > > > [    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
> > > > > [    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
> > > > > [    8.948994] Call Trace:
> > > > > [    8.948996]  dump_stack+0x50/0x70
> > > > > [    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
> > > > > [    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> > > > > [    8.949002]  bus_add_device+0x74/0x140
> > > > > [    8.949004]  device_add+0x34b/0x850
> > > > > [    8.949006]  nvmem_register.part.0+0x1bf/0x640
> > > > > ...
> > > > > [    8.948926] mtd mtd8: Failed to register NVMEM device
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>      
> > > 
> > > Thanks for proposing this change. Indeed we are aware of the problem
> > > and the best solution that we could come up with was to create an
> > > additional "unique_name" field to the mtd_info structure. This new
> > > field would have the form:
> > > 
> > >     [<parent-unique-name><separator>]<mtd-name>
> > > 
> > > The separator might be '~' (but I am completely open on that), and that
> > > would give for instance:
> > > 
> > >     my-controller~my-device~my-part~mysub-part    
> > 
> > I'd prefer something slightly more standard for the separator, like '/',
> > which is what we usually use when we want to represent a path in a tree.
> > I do agree on the general approach though.  
> 
> I am not sure / is a valid separator here we would use this
> name to create a sysfs entry. Would it work?

Hm, you're probably right, I didn't check how the name was used by
nvmem. I also see that partition names can contain spaces, which is
not that great. So, if we want to use mtd->unique_name we should
probably sanitize it before passing it to nvmem. All this makes me
reconsider your initial proposal: use 'mtdX' as the nvmem name. It's
unique and it's simple enough to not require this extra sanitization
pass, on the other hand, guessing the nvmem partition based on such an
opaque name is not simple, not to mention that the mtd device name can
change depending on the probe order.

I also wonder if creating nvmem devs unconditionally for all mtd
devices is a good idea. Sounds like we should only do that if there's an
explicit request to have one partition exposed as an nvmem.

Note that, no matter what we decide about nvmem, I think having unique
names at the MTD level is a good thing.

Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Ricardo Ribalda Delgado]

Hi

On Mon, Apr 27, 2020 at 9:10 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Mon, 27 Apr 2020 16:49:22 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> > Hi Boris,
> >
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
> > 2020 16:37:11 +0200:
> >
> > > On Mon, 27 Apr 2020 16:22:22 +0200
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > > Hi Ricardo,
> > > >
> > > > Ricardo Ribalda Delgado <ribalda@kernel.org> wrote on Tue, 14 Apr 2020
> > > > 15:47:23 +0200:
> > > >
> > > > > Ping?
> > > > >
> > > > > On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado
> > > > > <ribalda@kernel.org> wrote:
> > > > > >
> > > > > > When the nvmem framework is enabled, a nvmem device is created per mtd
> > > > > > device/partition.
> > > > > >
> > > > > > It is not uncommon that a device can have multiple mtd devices with
> > > > > > partitions that have the same name. Eg, when there DT overlay is allowed
> > > > > > and the same device with mtd is attached twice.
> > > > > >
> > > > > > Under that circumstances, the mtd fails to register due to a name
> > > > > > duplication on the nvmem framework.
> > > > > >
> > > > > > With this patch we add a _1, _2, _X to the subsequent names if there is
> > > > > > a collition, and throw a warning, instead of not starting the mtd
> > > > > > device.
> > > > > >
> > > > > > [    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
> > > > > > [    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
> > > > > > [    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
> > > > > > [    8.948994] Call Trace:
> > > > > > [    8.948996]  dump_stack+0x50/0x70
> > > > > > [    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
> > > > > > [    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> > > > > > [    8.949002]  bus_add_device+0x74/0x140
> > > > > > [    8.949004]  device_add+0x34b/0x850
> > > > > > [    8.949006]  nvmem_register.part.0+0x1bf/0x640
> > > > > > ...
> > > > > > [    8.948926] mtd mtd8: Failed to register NVMEM device
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > > >
> > > > Thanks for proposing this change. Indeed we are aware of the problem
> > > > and the best solution that we could come up with was to create an
> > > > additional "unique_name" field to the mtd_info structure. This new
> > > > field would have the form:
> > > >
> > > >     [<parent-unique-name><separator>]<mtd-name>
> > > >
> > > > The separator might be '~' (but I am completely open on that), and that
> > > > would give for instance:
> > > >
> > > >     my-controller~my-device~my-part~mysub-part
> > >
> > > I'd prefer something slightly more standard for the separator, like '/',
> > > which is what we usually use when we want to represent a path in a tree.
> > > I do agree on the general approach though.
> >
> > I am not sure / is a valid separator here we would use this
> > name to create a sysfs entry. Would it work?
>
> Hm, you're probably right, I didn't check how the name was used by
> nvmem. I also see that partition names can contain spaces, which is
> not that great. So, if we want to use mtd->unique_name we should
> probably sanitize it before passing it to nvmem. All this makes me
> reconsider your initial proposal: use 'mtdX' as the nvmem name. It's
> unique and it's simple enough to not require this extra sanitization
> pass, on the other hand, guessing the nvmem partition based on such an
> opaque name is not simple, not to mention that the mtd device name can
> change depending on the probe order.
>
> I also wonder if creating nvmem devs unconditionally for all mtd
> devices is a good idea. Sounds like we should only do that if there's an
> explicit request to have one partition exposed as an nvmem.
>
> Note that, no matter what we decide about nvmem, I think having unique
> names at the MTD level is a good thing.

I have no preference one way or another.

The issue is that our current master leads to mtds not working fine
and making the system unusable. Whatever we decide it must be fast and
the patch backported.

My original patch follows the same logic as led does where there is a
duplicated name. I can send a separate patch that uses mtdX and then
you decide what to pick. Or we can go with a third way, but it needs
to be soon ;)

Best regards!

Re: [PATCH v2] mtd: Fix mtd not the same name not registered if nvmem

[Boris Brezillon]

On Thu, 30 Apr 2020 14:26:51 +0200
Ricardo Ribalda Delgado <ribalda@kernel.org> wrote:

> Hi
> 
> On Mon, Apr 27, 2020 at 9:10 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Mon, 27 Apr 2020 16:49:22 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >  
> > > Hi Boris,
> > >
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
> > > 2020 16:37:11 +0200:
> > >  
> > > > On Mon, 27 Apr 2020 16:22:22 +0200
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >  
> > > > > Hi Ricardo,
> > > > >
> > > > > Ricardo Ribalda Delgado <ribalda@kernel.org> wrote on Tue, 14 Apr 2020
> > > > > 15:47:23 +0200:
> > > > >  
> > > > > > Ping?
> > > > > >
> > > > > > On Thu, Apr 2, 2020 at 8:59 AM Ricardo Ribalda Delgado
> > > > > > <ribalda@kernel.org> wrote:  
> > > > > > >
> > > > > > > When the nvmem framework is enabled, a nvmem device is created per mtd
> > > > > > > device/partition.
> > > > > > >
> > > > > > > It is not uncommon that a device can have multiple mtd devices with
> > > > > > > partitions that have the same name. Eg, when there DT overlay is allowed
> > > > > > > and the same device with mtd is attached twice.
> > > > > > >
> > > > > > > Under that circumstances, the mtd fails to register due to a name
> > > > > > > duplication on the nvmem framework.
> > > > > > >
> > > > > > > With this patch we add a _1, _2, _X to the subsequent names if there is
> > > > > > > a collition, and throw a warning, instead of not starting the mtd
> > > > > > > device.
> > > > > > >
> > > > > > > [    8.948991] sysfs: cannot create duplicate filename '/bus/nvmem/devices/Production Data'
> > > > > > > [    8.948992] CPU: 7 PID: 246 Comm: systemd-udevd Not tainted 5.5.0-qtec-standard #13
> > > > > > > [    8.948993] Hardware name: AMD Dibbler/Dibbler, BIOS 05.22.04.0019 10/26/2019
> > > > > > > [    8.948994] Call Trace:
> > > > > > > [    8.948996]  dump_stack+0x50/0x70
> > > > > > > [    8.948998]  sysfs_warn_dup.cold+0x17/0x2d
> > > > > > > [    8.949000]  sysfs_do_create_link_sd.isra.0+0xc2/0xd0
> > > > > > > [    8.949002]  bus_add_device+0x74/0x140
> > > > > > > [    8.949004]  device_add+0x34b/0x850
> > > > > > > [    8.949006]  nvmem_register.part.0+0x1bf/0x640
> > > > > > > ...
> > > > > > > [    8.948926] mtd mtd8: Failed to register NVMEM device
> > > > > > >
> > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>  
> > > > >
> > > > > Thanks for proposing this change. Indeed we are aware of the problem
> > > > > and the best solution that we could come up with was to create an
> > > > > additional "unique_name" field to the mtd_info structure. This new
> > > > > field would have the form:
> > > > >
> > > > >     [<parent-unique-name><separator>]<mtd-name>
> > > > >
> > > > > The separator might be '~' (but I am completely open on that), and that
> > > > > would give for instance:
> > > > >
> > > > >     my-controller~my-device~my-part~mysub-part  
> > > >
> > > > I'd prefer something slightly more standard for the separator, like '/',
> > > > which is what we usually use when we want to represent a path in a tree.
> > > > I do agree on the general approach though.  
> > >
> > > I am not sure / is a valid separator here we would use this
> > > name to create a sysfs entry. Would it work?  
> >
> > Hm, you're probably right, I didn't check how the name was used by
> > nvmem. I also see that partition names can contain spaces, which is
> > not that great. So, if we want to use mtd->unique_name we should
> > probably sanitize it before passing it to nvmem. All this makes me
> > reconsider your initial proposal: use 'mtdX' as the nvmem name. It's
> > unique and it's simple enough to not require this extra sanitization
> > pass, on the other hand, guessing the nvmem partition based on such an
> > opaque name is not simple, not to mention that the mtd device name can
> > change depending on the probe order.
> >
> > I also wonder if creating nvmem devs unconditionally for all mtd
> > devices is a good idea. Sounds like we should only do that if there's an
> > explicit request to have one partition exposed as an nvmem.
> >
> > Note that, no matter what we decide about nvmem, I think having unique
> > names at the MTD level is a good thing.  
> 
> I have no preference one way or another.
> 
> The issue is that our current master leads to mtds not working fine
> and making the system unusable. Whatever we decide it must be fast and
> the patch backported.
> 
> My original patch follows the same logic as led does where there is a
> duplicated name. I can send a separate patch that uses mtdX and then
> you decide what to pick. Or we can go with a third way, but it needs
> to be soon ;).

Please send a patch using dev_name(&mtd->dev), and let's hope it
doesn't break someone else use case.