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

[Zhuohao Lee <zhuohao@xxxxxxxxxxxx>]

On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@xxxxxx> wrote:
>
>
>
> On 02/04/19 1:26 PM, Boris Brezillon wrote:
> > 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.
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@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).

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/