[PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Zhuohao Lee]

Currently, we don't have sysfs nodes for querying the underlying flash
name and flash id. This information is important especially when we
want to know the flash detail of the defective system. In order to
support the query, we add two pointers (*flashname, *id) into the
mtd_info structure and create two sysfs nodes (flashname, id). This
patch is modified based on the SPI-NOR flash system as we only have
that system now. But the idea should be applied to the other flash
driver like NAND flash.

The output of new sysfs nodes on my device are:
cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
w25q64dw
cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
ef6017

Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
---
 drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
 drivers/mtd/spi-nor/spi-nor.c |  3 +++
 include/linux/mtd/mtd.h       |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 3ef01baef9b6..dcbe6719ad67 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
 }
 static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
 
+static ssize_t mtd_flashname_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+
+	if (!mtd->flashname)
+		return 0;
+	return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
+}
+static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);
+
+static ssize_t mtd_id_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+
+	if (!mtd->id)
+		return 0;
+	return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);
+}
+static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);
+
 static ssize_t mtd_ecc_strength_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
@@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
 	&dev_attr_oobavail.attr,
 	&dev_attr_numeraseregions.attr,
 	&dev_attr_name.attr,
+	&dev_attr_flashname.attr,
+	&dev_attr_id.attr,
 	&dev_attr_ecc_strength.attr,
 	&dev_attr_ecc_step_size.attr,
 	&dev_attr_corrected_bits.attr,
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 6e13bbd1aaa5..0e10858e532c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 
 	if (!mtd->name)
 		mtd->name = dev_name(dev);
+	mtd->flashname = info->name;
+	mtd->id = info->id;
+	mtd->id_size = info->id_len;
 	mtd->priv = nor;
 	mtd->type = MTD_NORFLASH;
 	mtd->writesize = 1;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 677768b21a1d..0a81569fa4f6 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -210,6 +210,9 @@ struct mtd_info {
 	uint32_t flags;
 	uint32_t orig_flags; /* Flags as before running mtd checks */
 	uint64_t size;	 // Total size of the MTD
+	const char *flashname; /* The underlying flash name */
+	const char *id; /* The ID of the flash */
+	int id_size; /* Number of bytes of id array */
 
 	/* "Major" erase size for the device. Naïve users may take this
 	 * to be the only erase size available, or may use the more detailed
-- 
2.21.0.392.gf8f6787159e-goog


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Zhuohao Lee]

Dear reviewers,

May i have your professional feedback for this patch? Could we land
this patch? Do you have any concern?
Please let me know what you think. Thank you.


On Thu, Mar 28, 2019 at 12:59 PM Zhuohao Lee <zhuohao@chromium.org> wrote:
>
> Currently, we don't have sysfs nodes for querying the underlying flash
> name and flash id. This information is important especially when we
> want to know the flash detail of the defective system. In order to
> support the query, we add two pointers (*flashname, *id) into the
> mtd_info structure and create two sysfs nodes (flashname, id). This
> patch is modified based on the SPI-NOR flash system as we only have
> that system now. But the idea should be applied to the other flash
> driver like NAND flash.
>
> The output of new sysfs nodes on my device are:
> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> w25q64dw
> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> ef6017
>
> Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
> ---
>  drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
>  drivers/mtd/spi-nor/spi-nor.c |  3 +++
>  include/linux/mtd/mtd.h       |  3 +++
>  3 files changed, 30 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 3ef01baef9b6..dcbe6719ad67 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
>  }
>  static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
>
> +static ssize_t mtd_flashname_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct mtd_info *mtd = dev_get_drvdata(dev);
> +
> +       if (!mtd->flashname)
> +               return 0;
> +       return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
> +}
> +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);
> +
> +static ssize_t mtd_id_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct mtd_info *mtd = dev_get_drvdata(dev);
> +
> +       if (!mtd->id)
> +               return 0;
> +       return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);
> +}
> +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);
> +
>  static ssize_t mtd_ecc_strength_show(struct device *dev,
>                                      struct device_attribute *attr, char *buf)
>  {
> @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
>         &dev_attr_oobavail.attr,
>         &dev_attr_numeraseregions.attr,
>         &dev_attr_name.attr,
> +       &dev_attr_flashname.attr,
> +       &dev_attr_id.attr,
>         &dev_attr_ecc_strength.attr,
>         &dev_attr_ecc_step_size.attr,
>         &dev_attr_corrected_bits.attr,
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 6e13bbd1aaa5..0e10858e532c 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>
>         if (!mtd->name)
>                 mtd->name = dev_name(dev);
> +       mtd->flashname = info->name;
> +       mtd->id = info->id;
> +       mtd->id_size = info->id_len;
>         mtd->priv = nor;
>         mtd->type = MTD_NORFLASH;
>         mtd->writesize = 1;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 677768b21a1d..0a81569fa4f6 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -210,6 +210,9 @@ struct mtd_info {
>         uint32_t flags;
>         uint32_t orig_flags; /* Flags as before running mtd checks */
>         uint64_t size;   // Total size of the MTD
> +       const char *flashname; /* The underlying flash name */
> +       const char *id; /* The ID of the flash */
> +       int id_size; /* Number of bytes of id array */
>
>         /* "Major" erase size for the device. Naïve users may take this
>          * to be the only erase size available, or may use the more detailed
> --
> 2.21.0.392.gf8f6787159e-goog
>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Boris Brezillon]

On Thu, 28 Mar 2019 12:59:10 +0800
Zhuohao Lee <zhuohao@chromium.org> wrote:

> Currently, we don't have sysfs nodes for querying the underlying flash
> name and flash id. This information is important especially when we
> want to know the flash detail of the defective system. In order to
> support the query, we add two pointers (*flashname, *id) into the
> mtd_info structure and create two sysfs nodes (flashname, id). This
> patch is modified based on the SPI-NOR flash system as we only have
> that system now. But the idea should be applied to the other flash
> driver like NAND flash.
> 
> The output of new sysfs nodes on my device are:
> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> w25q64dw
> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> ef6017

I'm not sure I like the idea of exposing this kind of info through
sysfs as it then makes part of the ABI. Did you consider exposing that
through debugfs?

> 
> Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
> ---
>  drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
>  drivers/mtd/spi-nor/spi-nor.c |  3 +++
>  include/linux/mtd/mtd.h       |  3 +++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 3ef01baef9b6..dcbe6719ad67 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
>  }
>  static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
>  
> +static ssize_t mtd_flashname_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct mtd_info *mtd = dev_get_drvdata(dev);
> +
> +	if (!mtd->flashname)
> +		return 0;
> +	return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
> +}
> +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);

MTD also deals with things that are not flashes (SRAMs, ROM, ...). How
about partname?

> +
> +static ssize_t mtd_id_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct mtd_info *mtd = dev_get_drvdata(dev);
> +
> +	if (!mtd->id)
> +		return 0;
> +	return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);

I'd recommend making mtd->id a string so that each flash type can
decide of the formatting, and maybe have a prefix that tells which kind
of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx".

> +}
> +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);

id is bit vague, how about partid.

> +
>  static ssize_t mtd_ecc_strength_show(struct device *dev,
>  				     struct device_attribute *attr, char *buf)
>  {
> @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
>  	&dev_attr_oobavail.attr,
>  	&dev_attr_numeraseregions.attr,
>  	&dev_attr_name.attr,
> +	&dev_attr_flashname.attr,
> +	&dev_attr_id.attr,
>  	&dev_attr_ecc_strength.attr,
>  	&dev_attr_ecc_step_size.attr,
>  	&dev_attr_corrected_bits.attr,
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 6e13bbd1aaa5..0e10858e532c 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  
>  	if (!mtd->name)
>  		mtd->name = dev_name(dev);
> +	mtd->flashname = info->name;
> +	mtd->id = info->id;
> +	mtd->id_size = info->id_len;
>  	mtd->priv = nor;
>  	mtd->type = MTD_NORFLASH;
>  	mtd->writesize = 1;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 677768b21a1d..0a81569fa4f6 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -210,6 +210,9 @@ struct mtd_info {
>  	uint32_t flags;
>  	uint32_t orig_flags; /* Flags as before running mtd checks */
>  	uint64_t size;	 // Total size of the MTD
> +	const char *flashname; /* The underlying flash name */
> +	const char *id; /* The ID of the flash */
> +	int id_size; /* Number of bytes of id array */
>  
>  	/* "Major" erase size for the device. Naïve users may take this
>  	 * to be the only erase size available, or may use the more detailed


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Zhuohao Lee]

Thanks Boris for the comment. Please take a look the reply at below.

On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Thu, 28 Mar 2019 12:59:10 +0800
> Zhuohao Lee <zhuohao@chromium.org> wrote:
>
> > Currently, we don't have sysfs nodes for querying the underlying flash
> > name and flash id. This information is important especially when we
> > want to know the flash detail of the defective system. In order to
> > support the query, we add two pointers (*flashname, *id) into the
> > mtd_info structure and create two sysfs nodes (flashname, id). This
> > patch is modified based on the SPI-NOR flash system as we only have
> > that system now. But the idea should be applied to the other flash
> > driver like NAND flash.
> >
> > The output of new sysfs nodes on my device are:
> > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> > w25q64dw
> > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> > ef6017
>
> I'm not sure I like the idea of exposing this kind of info through
> sysfs as it then makes part of the ABI. Did you consider exposing that
> through debugfs?

Yes, i did consider the debugfs. I think the debugfs is depended on
CONFIG_DEBUG_FS.
If removing that config, the partname and partid will be lost. So, i
proposed to use
sysfs.

>
> >
> > Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
> > ---
> >  drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
> >  drivers/mtd/spi-nor/spi-nor.c |  3 +++
> >  include/linux/mtd/mtd.h       |  3 +++
> >  3 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index 3ef01baef9b6..dcbe6719ad67 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
> >
> > +static ssize_t mtd_flashname_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf)
> > +{
> > +     struct mtd_info *mtd = dev_get_drvdata(dev);
> > +
> > +     if (!mtd->flashname)
> > +             return 0;
> > +     return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
> > +}
> > +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);
>
> MTD also deals with things that are not flashes (SRAMs, ROM, ...). How
> about partname?

Thanks, i will change the name to partname.

>
> > +
> > +static ssize_t mtd_id_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf)
> > +{
> > +     struct mtd_info *mtd = dev_get_drvdata(dev);
> > +
> > +     if (!mtd->id)
> > +             return 0;
> > +     return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);
>
> I'd recommend making mtd->id a string so that each flash type can
> decide of the formatting, and maybe have a prefix that tells which kind

ok, i will create an array to store the formatted partid.

> of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx".

We had a sysfs node, called 'type', which indicated the type of the
underlying device. We can query the 'type' to get the device type.
I think it is not necessary to add prefix. What do you think?

>
> > +}
> > +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);
>
> id is bit vague, how about partid.

Agree, i will change this.

>
> > +
> >  static ssize_t mtd_ecc_strength_show(struct device *dev,
> >                                    struct device_attribute *attr, char *buf)
> >  {
> > @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
> >       &dev_attr_oobavail.attr,
> >       &dev_attr_numeraseregions.attr,
> >       &dev_attr_name.attr,
> > +     &dev_attr_flashname.attr,
> > +     &dev_attr_id.attr,
> >       &dev_attr_ecc_strength.attr,
> >       &dev_attr_ecc_step_size.attr,
> >       &dev_attr_corrected_bits.attr,
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index 6e13bbd1aaa5..0e10858e532c 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> >
> >       if (!mtd->name)
> >               mtd->name = dev_name(dev);
> > +     mtd->flashname = info->name;
> > +     mtd->id = info->id;
> > +     mtd->id_size = info->id_len;
> >       mtd->priv = nor;
> >       mtd->type = MTD_NORFLASH;
> >       mtd->writesize = 1;
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index 677768b21a1d..0a81569fa4f6 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -210,6 +210,9 @@ struct mtd_info {
> >       uint32_t flags;
> >       uint32_t orig_flags; /* Flags as before running mtd checks */
> >       uint64_t size;   // Total size of the MTD
> > +     const char *flashname; /* The underlying flash name */
> > +     const char *id; /* The ID of the flash */
> > +     int id_size; /* Number of bytes of id array */
> >
> >       /* "Major" erase size for the device. Naïve users may take this
> >        * to be the only erase size available, or may use the more detailed
>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Boris Brezillon]

On Tue, 2 Apr 2019 15:39:54 +0800
Zhuohao Lee <zhuohao@chromium.org> wrote:

> Thanks Boris for the comment. Please take a look the reply at below.
> 
> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Thu, 28 Mar 2019 12:59:10 +0800
> > Zhuohao Lee <zhuohao@chromium.org> wrote:
> >  
> > > Currently, we don't have sysfs nodes for querying the underlying flash
> > > name and flash id. This information is important especially when we
> > > want to know the flash detail of the defective system. In order to
> > > support the query, we add two pointers (*flashname, *id) into the
> > > mtd_info structure and create two sysfs nodes (flashname, id). This
> > > patch is modified based on the SPI-NOR flash system as we only have
> > > that system now. But the idea should be applied to the other flash
> > > driver like NAND flash.
> > >
> > > The output of new sysfs nodes on my device are:
> > > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> > > w25q64dw
> > > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> > > ef6017  
> >
> > I'm not sure I like the idea of exposing this kind of info through
> > sysfs as it then makes part of the ABI. Did you consider exposing that
> > through debugfs?  
> 
> Yes, i did consider the debugfs. I think the debugfs is depended on
> CONFIG_DEBUG_FS.
> If removing that config, the partname and partid will be lost. So, i
> proposed to use
> sysfs.

Then just enable debugfs if you need this information :P.

> 
> >  
> > >
> > > Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
> > > ---
> > >  drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
> > >  drivers/mtd/spi-nor/spi-nor.c |  3 +++
> > >  include/linux/mtd/mtd.h       |  3 +++
> > >  3 files changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > > index 3ef01baef9b6..dcbe6719ad67 100644
> > > --- a/drivers/mtd/mtdcore.c
> > > +++ b/drivers/mtd/mtdcore.c
> > > @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
> > >  }
> > >  static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
> > >
> > > +static ssize_t mtd_flashname_show(struct device *dev,
> > > +             struct device_attribute *attr, char *buf)
> > > +{
> > > +     struct mtd_info *mtd = dev_get_drvdata(dev);
> > > +
> > > +     if (!mtd->flashname)
> > > +             return 0;
> > > +     return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
> > > +}
> > > +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);  
> >
> > MTD also deals with things that are not flashes (SRAMs, ROM, ...). How
> > about partname?  
> 
> Thanks, i will change the name to partname.
> 
> >  
> > > +
> > > +static ssize_t mtd_id_show(struct device *dev,
> > > +             struct device_attribute *attr, char *buf)
> > > +{
> > > +     struct mtd_info *mtd = dev_get_drvdata(dev);
> > > +
> > > +     if (!mtd->id)
> > > +             return 0;
> > > +     return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);  
> >
> > I'd recommend making mtd->id a string so that each flash type can
> > decide of the formatting, and maybe have a prefix that tells which kind  
> 
> ok, i will create an array to store the formatted partid.
> 
> > of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx".  
> 
> We had a sysfs node, called 'type', which indicated the type of the
> underlying device. We can query the 'type' to get the device type.
> I think it is not necessary to add prefix. What do you think?

No, the type is not precise enough, a NOR can be a SPI NOR or a CFI NOR
and they probably don't use the same ID-scheme. Same for NANDs (parallel
NANDs vs SPI NANDs).

> 
> >  
> > > +}
> > > +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);  
> >
> > id is bit vague, how about partid.  
> 
> Agree, i will change this.

I think you should wait for other reviews before you sending a new
version. I'm still not convinced exposing that through sysfs is a good
idea, and I'd like other MTD maintainers to give their opinion on this
aspect.

> 
> >  
> > > +
> > >  static ssize_t mtd_ecc_strength_show(struct device *dev,
> > >                                    struct device_attribute *attr, char *buf)
> > >  {
> > > @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
> > >       &dev_attr_oobavail.attr,
> > >       &dev_attr_numeraseregions.attr,
> > >       &dev_attr_name.attr,
> > > +     &dev_attr_flashname.attr,
> > > +     &dev_attr_id.attr,
> > >       &dev_attr_ecc_strength.attr,
> > >       &dev_attr_ecc_step_size.attr,
> > >       &dev_attr_corrected_bits.attr,
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > > index 6e13bbd1aaa5..0e10858e532c 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> > >
> > >       if (!mtd->name)
> > >               mtd->name = dev_name(dev);
> > > +     mtd->flashname = info->name;
> > > +     mtd->id = info->id;
> > > +     mtd->id_size = info->id_len;
> > >       mtd->priv = nor;
> > >       mtd->type = MTD_NORFLASH;
> > >       mtd->writesize = 1;
> > > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > > index 677768b21a1d..0a81569fa4f6 100644
> > > --- a/include/linux/mtd/mtd.h
> > > +++ b/include/linux/mtd/mtd.h
> > > @@ -210,6 +210,9 @@ struct mtd_info {
> > >       uint32_t flags;
> > >       uint32_t orig_flags; /* Flags as before running mtd checks */
> > >       uint64_t size;   // Total size of the MTD
> > > +     const char *flashname; /* The underlying flash name */
> > > +     const char *id; /* The ID of the flash */
> > > +     int id_size; /* Number of bytes of id array */
> > >
> > >       /* "Major" erase size for the device. Naïve users may take this
> > >        * to be the only erase size available, or may use the more detailed  
> >  


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Vignesh Raghavendra]

On 02/04/19 1:26 PM, Boris Brezillon wrote:
> On Tue, 2 Apr 2019 15:39:54 +0800
> Zhuohao Lee <zhuohao@chromium.org> wrote:
> 
>> Thanks Boris for the comment. Please take a look the reply at below.
>>
>> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
>> <boris.brezillon@collabora.com> wrote:
>>>
>>> On Thu, 28 Mar 2019 12:59:10 +0800
>>> Zhuohao Lee <zhuohao@chromium.org> wrote:
>>>  
>>>> Currently, we don't have sysfs nodes for querying the underlying flash
>>>> name and flash id. This information is important especially when we
>>>> want to know the flash detail of the defective system. In order to
>>>> support the query, we add two pointers (*flashname, *id) into the
>>>> mtd_info structure and create two sysfs nodes (flashname, id). This
>>>> patch is modified based on the SPI-NOR flash system as we only have
>>>> that system now. But the idea should be applied to the other flash
>>>> driver like NAND flash.
>>>>
>>>> The output of new sysfs nodes on my device are:
>>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
>>>> w25q64dw
>>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
>>>> ef6017  
>>>
>>> I'm not sure I like the idea of exposing this kind of info through
>>> sysfs as it then makes part of the ABI. Did you consider exposing that
>>> through debugfs?  
>>
>> Yes, i did consider the debugfs. I think the debugfs is depended on
>> CONFIG_DEBUG_FS.
>> If removing that config, the partname and partid will be lost. So, i
>> proposed to use
>> sysfs.
> 
> Then just enable debugfs if you need this information :P.
> 

Do we really need this info even in debug FS?
spi-nor.c prints following to kernel log which can be obtained by dmesg:

        dev_info(dev, "%s (%lld Kbytes)\n", info->name,
                        (long long)mtd->size >> 10);

You could probably expand that to print flash IDs as well.

Regards
Vignesh

>>
>>>  
>>>>
>>>> Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
>>>> ---
>>>>  drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
>>>>  drivers/mtd/spi-nor/spi-nor.c |  3 +++
>>>>  include/linux/mtd/mtd.h       |  3 +++
>>>>  3 files changed, 30 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>>> index 3ef01baef9b6..dcbe6719ad67 100644
>>>> --- a/drivers/mtd/mtdcore.c
>>>> +++ b/drivers/mtd/mtdcore.c
>>>> @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
>>>>  }
>>>>  static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
>>>>
>>>> +static ssize_t mtd_flashname_show(struct device *dev,
>>>> +             struct device_attribute *attr, char *buf)
>>>> +{
>>>> +     struct mtd_info *mtd = dev_get_drvdata(dev);
>>>> +
>>>> +     if (!mtd->flashname)
>>>> +             return 0;
>>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
>>>> +}
>>>> +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);  
>>>
>>> MTD also deals with things that are not flashes (SRAMs, ROM, ...). How
>>> about partname?  
>>
>> Thanks, i will change the name to partname.
>>
>>>  
>>>> +
>>>> +static ssize_t mtd_id_show(struct device *dev,
>>>> +             struct device_attribute *attr, char *buf)
>>>> +{
>>>> +     struct mtd_info *mtd = dev_get_drvdata(dev);
>>>> +
>>>> +     if (!mtd->id)
>>>> +             return 0;
>>>> +     return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);  
>>>
>>> I'd recommend making mtd->id a string so that each flash type can
>>> decide of the formatting, and maybe have a prefix that tells which kind  
>>
>> ok, i will create an array to store the formatted partid.
>>
>>> of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx".  
>>
>> We had a sysfs node, called 'type', which indicated the type of the
>> underlying device. We can query the 'type' to get the device type.
>> I think it is not necessary to add prefix. What do you think?
> 
> No, the type is not precise enough, a NOR can be a SPI NOR or a CFI NOR
> and they probably don't use the same ID-scheme. Same for NANDs (parallel
> NANDs vs SPI NANDs).
> 
>>
>>>  
>>>> +}
>>>> +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);  
>>>
>>> id is bit vague, how about partid.  
>>
>> Agree, i will change this.
> 
> I think you should wait for other reviews before you sending a new
> version. I'm still not convinced exposing that through sysfs is a good
> idea, and I'd like other MTD maintainers to give their opinion on this
> aspect.
> 
>>
>>>  
>>>> +
>>>>  static ssize_t mtd_ecc_strength_show(struct device *dev,
>>>>                                    struct device_attribute *attr, char *buf)
>>>>  {
>>>> @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
>>>>       &dev_attr_oobavail.attr,
>>>>       &dev_attr_numeraseregions.attr,
>>>>       &dev_attr_name.attr,
>>>> +     &dev_attr_flashname.attr,
>>>> +     &dev_attr_id.attr,
>>>>       &dev_attr_ecc_strength.attr,
>>>>       &dev_attr_ecc_step_size.attr,
>>>>       &dev_attr_corrected_bits.attr,
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 6e13bbd1aaa5..0e10858e532c 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>>>
>>>>       if (!mtd->name)
>>>>               mtd->name = dev_name(dev);
>>>> +     mtd->flashname = info->name;
>>>> +     mtd->id = info->id;
>>>> +     mtd->id_size = info->id_len;
>>>>       mtd->priv = nor;
>>>>       mtd->type = MTD_NORFLASH;
>>>>       mtd->writesize = 1;
>>>> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
>>>> index 677768b21a1d..0a81569fa4f6 100644
>>>> --- a/include/linux/mtd/mtd.h
>>>> +++ b/include/linux/mtd/mtd.h
>>>> @@ -210,6 +210,9 @@ struct mtd_info {
>>>>       uint32_t flags;
>>>>       uint32_t orig_flags; /* Flags as before running mtd checks */
>>>>       uint64_t size;   // Total size of the MTD
>>>> +     const char *flashname; /* The underlying flash name */
>>>> +     const char *id; /* The ID of the flash */
>>>> +     int id_size; /* Number of bytes of id array */
>>>>
>>>>       /* "Major" erase size for the device. Naïve users may take this
>>>>        * to be the only erase size available, or may use the more detailed  
>>>  
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

-- 
Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Zhuohao Lee]

On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
>
>
> On 02/04/19 1:26 PM, Boris Brezillon wrote:
> > On Tue, 2 Apr 2019 15:39:54 +0800
> > Zhuohao Lee <zhuohao@chromium.org> wrote:
> >
> >> Thanks Boris for the comment. Please take a look the reply at below.
> >>
> >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
> >> <boris.brezillon@collabora.com> wrote:
> >>>
> >>> On Thu, 28 Mar 2019 12:59:10 +0800
> >>> Zhuohao Lee <zhuohao@chromium.org> wrote:
> >>>
> >>>> Currently, we don't have sysfs nodes for querying the underlying flash
> >>>> name and flash id. This information is important especially when we
> >>>> want to know the flash detail of the defective system. In order to
> >>>> support the query, we add two pointers (*flashname, *id) into the
> >>>> mtd_info structure and create two sysfs nodes (flashname, id). This
> >>>> patch is modified based on the SPI-NOR flash system as we only have
> >>>> that system now. But the idea should be applied to the other flash
> >>>> driver like NAND flash.
> >>>>
> >>>> The output of new sysfs nodes on my device are:
> >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> >>>> w25q64dw
> >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> >>>> ef6017
> >>>
> >>> I'm not sure I like the idea of exposing this kind of info through
> >>> sysfs as it then makes part of the ABI. Did you consider exposing that
> >>> through debugfs?
> >>
> >> Yes, i did consider the debugfs. I think the debugfs is depended on
> >> CONFIG_DEBUG_FS.
> >> If removing that config, the partname and partid will be lost. So, i
> >> proposed to use
> >> sysfs.
> >
> > Then just enable debugfs if you need this information :P.
My original intention is adding the new sysfs nodes (i.e partname and
partid) into the common place like mtdcore.c
so that the userspace program can just read the common sysfs nodes.
So far, what i can contribute is for the spi-nor but if the other
flash drivers, like nand-flash, can also support
the partname and partid query, then, we can have common sysfs nodes to
query the underlying device info.
Compare to the debugfs, i think different drivers may have different
name for the partname and partid.
I believe this will make the userspace program more complicated to get
the partname and partid.

> >
>
> Do we really need this info even in debug FS?
> spi-nor.c prints following to kernel log which can be obtained by dmesg:
>
>         dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>                         (long long)mtd->size >> 10);
>
> You could probably expand that to print flash IDs as well.

The dmesg log is insufficient sometimes because the log may be
overwritten. Besides, the output log may be
inconsistent among the drivers. So, the userspace program needs to be
designed to parse the different log.
IMHO, if we can provide a common place for querying the partname and
partid, that will be good for the userspace program.

>
> Regards
> Vignesh
>
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Zhuohao Lee <zhuohao@chromium.org>
> >>>> ---
> >>>>  drivers/mtd/mtdcore.c         | 24 ++++++++++++++++++++++++
> >>>>  drivers/mtd/spi-nor/spi-nor.c |  3 +++
> >>>>  include/linux/mtd/mtd.h       |  3 +++
> >>>>  3 files changed, 30 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >>>> index 3ef01baef9b6..dcbe6719ad67 100644
> >>>> --- a/drivers/mtd/mtdcore.c
> >>>> +++ b/drivers/mtd/mtdcore.c
> >>>> @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev,
> >>>>  }
> >>>>  static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
> >>>>
> >>>> +static ssize_t mtd_flashname_show(struct device *dev,
> >>>> +             struct device_attribute *attr, char *buf)
> >>>> +{
> >>>> +     struct mtd_info *mtd = dev_get_drvdata(dev);
> >>>> +
> >>>> +     if (!mtd->flashname)
> >>>> +             return 0;
> >>>> +     return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname);
> >>>> +}
> >>>> +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL);
> >>>
> >>> MTD also deals with things that are not flashes (SRAMs, ROM, ...). How
> >>> about partname?
> >>
> >> Thanks, i will change the name to partname.
> >>
> >>>
> >>>> +
> >>>> +static ssize_t mtd_id_show(struct device *dev,
> >>>> +             struct device_attribute *attr, char *buf)
> >>>> +{
> >>>> +     struct mtd_info *mtd = dev_get_drvdata(dev);
> >>>> +
> >>>> +     if (!mtd->id)
> >>>> +             return 0;
> >>>> +     return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id);
> >>>
> >>> I'd recommend making mtd->id a string so that each flash type can
> >>> decide of the formatting, and maybe have a prefix that tells which kind
> >>
> >> ok, i will create an array to store the formatted partid.
> >>
> >>> of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx".
> >>
> >> We had a sysfs node, called 'type', which indicated the type of the
> >> underlying device. We can query the 'type' to get the device type.
> >> I think it is not necessary to add prefix. What do you think?
> >
> > No, the type is not precise enough, a NOR can be a SPI NOR or a CFI NOR
> > and they probably don't use the same ID-scheme. Same for NANDs (parallel
> > NANDs vs SPI NANDs).

ok, got it.

> >
> >>
> >>>
> >>>> +}
> >>>> +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL);
> >>>
> >>> id is bit vague, how about partid.
> >>
> >> Agree, i will change this.
> >
> > I think you should wait for other reviews before you sending a new
> > version. I'm still not convinced exposing that through sysfs is a good
> > idea, and I'd like other MTD maintainers to give their opinion on this
> > aspect.
> >
> >>
> >>>
> >>>> +
> >>>>  static ssize_t mtd_ecc_strength_show(struct device *dev,
> >>>>                                    struct device_attribute *attr, char *buf)
> >>>>  {
> >>>> @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = {
> >>>>       &dev_attr_oobavail.attr,
> >>>>       &dev_attr_numeraseregions.attr,
> >>>>       &dev_attr_name.attr,
> >>>> +     &dev_attr_flashname.attr,
> >>>> +     &dev_attr_id.attr,
> >>>>       &dev_attr_ecc_strength.attr,
> >>>>       &dev_attr_ecc_step_size.attr,
> >>>>       &dev_attr_corrected_bits.attr,
> >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >>>> index 6e13bbd1aaa5..0e10858e532c 100644
> >>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>> @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> >>>>
> >>>>       if (!mtd->name)
> >>>>               mtd->name = dev_name(dev);
> >>>> +     mtd->flashname = info->name;
> >>>> +     mtd->id = info->id;
> >>>> +     mtd->id_size = info->id_len;
> >>>>       mtd->priv = nor;
> >>>>       mtd->type = MTD_NORFLASH;
> >>>>       mtd->writesize = 1;
> >>>> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> >>>> index 677768b21a1d..0a81569fa4f6 100644
> >>>> --- a/include/linux/mtd/mtd.h
> >>>> +++ b/include/linux/mtd/mtd.h
> >>>> @@ -210,6 +210,9 @@ struct mtd_info {
> >>>>       uint32_t flags;
> >>>>       uint32_t orig_flags; /* Flags as before running mtd checks */
> >>>>       uint64_t size;   // Total size of the MTD
> >>>> +     const char *flashname; /* The underlying flash name */
> >>>> +     const char *id; /* The ID of the flash */
> >>>> +     int id_size; /* Number of bytes of id array */
> >>>>
> >>>>       /* "Major" erase size for the device. Naïve users may take this
> >>>>        * to be the only erase size available, or may use the more detailed
> >>>
> >
> >
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
>
> --
> Regards
> Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Boris Brezillon]

On Tue, 2 Apr 2019 19:06:37 +0800
Zhuohao Lee <zhuohao@chromium.org> wrote:

> On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> >
> >
> >
> > On 02/04/19 1:26 PM, Boris Brezillon wrote:  
> > > On Tue, 2 Apr 2019 15:39:54 +0800
> > > Zhuohao Lee <zhuohao@chromium.org> wrote:
> > >  
> > >> Thanks Boris for the comment. Please take a look the reply at below.
> > >>
> > >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
> > >> <boris.brezillon@collabora.com> wrote:  
> > >>>
> > >>> On Thu, 28 Mar 2019 12:59:10 +0800
> > >>> Zhuohao Lee <zhuohao@chromium.org> wrote:
> > >>>  
> > >>>> Currently, we don't have sysfs nodes for querying the underlying flash
> > >>>> name and flash id. This information is important especially when we
> > >>>> want to know the flash detail of the defective system. In order to
> > >>>> support the query, we add two pointers (*flashname, *id) into the
> > >>>> mtd_info structure and create two sysfs nodes (flashname, id). This
> > >>>> patch is modified based on the SPI-NOR flash system as we only have
> > >>>> that system now. But the idea should be applied to the other flash
> > >>>> driver like NAND flash.
> > >>>>
> > >>>> The output of new sysfs nodes on my device are:
> > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> > >>>> w25q64dw
> > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> > >>>> ef6017  
> > >>>
> > >>> I'm not sure I like the idea of exposing this kind of info through
> > >>> sysfs as it then makes part of the ABI. Did you consider exposing that
> > >>> through debugfs?  
> > >>
> > >> Yes, i did consider the debugfs. I think the debugfs is depended on
> > >> CONFIG_DEBUG_FS.
> > >> If removing that config, the partname and partid will be lost. So, i
> > >> proposed to use
> > >> sysfs.  
> > >
> > > Then just enable debugfs if you need this information :P.  
> My original intention is adding the new sysfs nodes (i.e partname and
> partid) into the common place like mtdcore.c
> so that the userspace program can just read the common sysfs nodes.
> So far, what i can contribute is for the spi-nor but if the other
> flash drivers, like nand-flash, can also support
> the partname and partid query, then, we can have common sysfs nodes to
> query the underlying device info.
> Compare to the debugfs, i think different drivers may have different
> name for the partname and partid.

We've recently unified how MTD related stuff are exposed through
debugfs and you can now have generic MTD fields exposed there.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Zhuohao Lee]

Boris, Thanks for your comment.
But I can't find the related stuff (or i got the wrong way). Is that
feature merged?
Could you please point out the commit? I can make the change base on it.

On Tue, Apr 2, 2019 at 8:01 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Tue, 2 Apr 2019 19:06:37 +0800
> Zhuohao Lee <zhuohao@chromium.org> wrote:
>
> > On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:
> > >
> > >
> > >
> > > On 02/04/19 1:26 PM, Boris Brezillon wrote:
> > > > On Tue, 2 Apr 2019 15:39:54 +0800
> > > > Zhuohao Lee <zhuohao@chromium.org> wrote:
> > > >
> > > >> Thanks Boris for the comment. Please take a look the reply at below.
> > > >>
> > > >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
> > > >> <boris.brezillon@collabora.com> wrote:
> > > >>>
> > > >>> On Thu, 28 Mar 2019 12:59:10 +0800
> > > >>> Zhuohao Lee <zhuohao@chromium.org> wrote:
> > > >>>
> > > >>>> Currently, we don't have sysfs nodes for querying the underlying flash
> > > >>>> name and flash id. This information is important especially when we
> > > >>>> want to know the flash detail of the defective system. In order to
> > > >>>> support the query, we add two pointers (*flashname, *id) into the
> > > >>>> mtd_info structure and create two sysfs nodes (flashname, id). This
> > > >>>> patch is modified based on the SPI-NOR flash system as we only have
> > > >>>> that system now. But the idea should be applied to the other flash
> > > >>>> driver like NAND flash.
> > > >>>>
> > > >>>> The output of new sysfs nodes on my device are:
> > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> > > >>>> w25q64dw
> > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> > > >>>> ef6017
> > > >>>
> > > >>> I'm not sure I like the idea of exposing this kind of info through
> > > >>> sysfs as it then makes part of the ABI. Did you consider exposing that
> > > >>> through debugfs?
> > > >>
> > > >> Yes, i did consider the debugfs. I think the debugfs is depended on
> > > >> CONFIG_DEBUG_FS.
> > > >> If removing that config, the partname and partid will be lost. So, i
> > > >> proposed to use
> > > >> sysfs.
> > > >
> > > > Then just enable debugfs if you need this information :P.
> > My original intention is adding the new sysfs nodes (i.e partname and
> > partid) into the common place like mtdcore.c
> > so that the userspace program can just read the common sysfs nodes.
> > So far, what i can contribute is for the spi-nor but if the other
> > flash drivers, like nand-flash, can also support
> > the partname and partid query, then, we can have common sysfs nodes to
> > query the underlying device info.
> > Compare to the debugfs, i think different drivers may have different
> > name for the partname and partid.
>
> We've recently unified how MTD related stuff are exposed through
> debugfs and you can now have generic MTD fields exposed there.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id

[Miquel Raynal]

Hi Zhuohao,

Zhuohao Lee <zhuohao@chromium.org> wrote on Tue, 2 Apr 2019 21:03:28
+0800:

> Boris, Thanks for your comment.
> But I can't find the related stuff (or i got the wrong way). Is that
> feature merged?
> Could you please point out the commit? I can make the change base on it.

I think Boris just told you that the debugfs MTD directory is suitable
to receive an information coming from different type of devices like
spi-nor, nor, nand, etc.

The feature of having the device name there does not exist yet, you
have to do it. For NAND I already wrote something like that which has
not been merged but could be adapted to export the data in debugfs
instead of sysfs, see:

Raw NAND infrastructure to retrieve the ID:
http://patchwork.ozlabs.org/patch/837901/

Adding a sysfs entry (which was already rejected at this time):
http://patchwork.ozlabs.org/patch/837903/

Thanks,
Miquèl

> 
> On Tue, Apr 2, 2019 at 8:01 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Tue, 2 Apr 2019 19:06:37 +0800
> > Zhuohao Lee <zhuohao@chromium.org> wrote:
> >  
> > > On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote:  
> > > >
> > > >
> > > >
> > > > On 02/04/19 1:26 PM, Boris Brezillon wrote:  
> > > > > On Tue, 2 Apr 2019 15:39:54 +0800
> > > > > Zhuohao Lee <zhuohao@chromium.org> wrote:
> > > > >  
> > > > >> Thanks Boris for the comment. Please take a look the reply at below.
> > > > >>
> > > > >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon
> > > > >> <boris.brezillon@collabora.com> wrote:  
> > > > >>>
> > > > >>> On Thu, 28 Mar 2019 12:59:10 +0800
> > > > >>> Zhuohao Lee <zhuohao@chromium.org> wrote:
> > > > >>>  
> > > > >>>> Currently, we don't have sysfs nodes for querying the underlying flash
> > > > >>>> name and flash id. This information is important especially when we
> > > > >>>> want to know the flash detail of the defective system. In order to
> > > > >>>> support the query, we add two pointers (*flashname, *id) into the
> > > > >>>> mtd_info structure and create two sysfs nodes (flashname, id). This
> > > > >>>> patch is modified based on the SPI-NOR flash system as we only have
> > > > >>>> that system now. But the idea should be applied to the other flash
> > > > >>>> driver like NAND flash.
> > > > >>>>
> > > > >>>> The output of new sysfs nodes on my device are:
> > > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname
> > > > >>>> w25q64dw
> > > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id
> > > > >>>> ef6017  
> > > > >>>
> > > > >>> I'm not sure I like the idea of exposing this kind of info through
> > > > >>> sysfs as it then makes part of the ABI. Did you consider exposing that
> > > > >>> through debugfs?  
> > > > >>
> > > > >> Yes, i did consider the debugfs. I think the debugfs is depended on
> > > > >> CONFIG_DEBUG_FS.
> > > > >> If removing that config, the partname and partid will be lost. So, i
> > > > >> proposed to use
> > > > >> sysfs.  
> > > > >
> > > > > Then just enable debugfs if you need this information :P.  
> > > My original intention is adding the new sysfs nodes (i.e partname and
> > > partid) into the common place like mtdcore.c
> > > so that the userspace program can just read the common sysfs nodes.
> > > So far, what i can contribute is for the spi-nor but if the other
> > > flash drivers, like nand-flash, can also support
> > > the partname and partid query, then, we can have common sysfs nodes to
> > > query the underlying device info.
> > > Compare to the debugfs, i think different drivers may have different
> > > name for the partname and partid.  
> >
> > We've recently unified how MTD related stuff are exposed through
> > debugfs and you can now have generic MTD fields exposed there.  
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/