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

[Zhuohao Lee <zhuohao@xxxxxxxxxxxx>]

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@xxxxxxxxxxxxx> wrote:
>
> On Thu, 28 Mar 2019 12:59:10 +0800
> Zhuohao Lee <zhuohao@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >  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/