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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Tue, 2 Apr 2019 15:39:54 +0800
Zhuohao Lee <zhuohao@xxxxxxxxxxxx> 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@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.

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

> 
> >  
> > >
> > > 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?

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/