Re: [PATCH v2] mtd: spi-nor: add debugfs nodes for querying the flash name and id

[Nicolas Boichat <drinkcat@xxxxxxxxxxxx>]

On Mon, May 6, 2019 at 8:36 PM Zhuohao Lee <zhuohao@xxxxxxxxxxxx> wrote:
>
> On Mon, May 6, 2019 at 5:07 PM Nicolas Boichat <drinkcat@xxxxxxxxxxxx> wrote:
> >
> > On Mon, May 6, 2019 at 5:44 PM Zhuohao Lee <zhuohao@xxxxxxxxxxxx> wrote:
> > >
> > > Currently, we don't have vfs 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 a function spi_nor_debugfs_create()
> > > to create the debugfs node (ie. flashname and flashid)
> > > This patch is modified based on the SPI-NOR flash system as we
> > > only have the SPI-NOR system now. But the idea should be applied to
> > > the other flash driver like NAND flash.
> > >
> > > The output of new debugfs nodes on my device are:
> > > cat /sys/kernel/debug/mtd/mtd0/flashid
> > > ef6017
> > > cat /sys/kernel/debug/mtd/mtd0/flashname
> > > w25q64dw
> > >
> > > Signed-off-by: Zhuohao Lee <zhuohao@xxxxxxxxxxxx>
> > > ---
> >
> > For next time, please include notes/changelog here (after ---), e.g.
> > things like link to previous discussion thread(s), changes since vX,
> > etc.
> >
> > >  drivers/mtd/devices/m25p80.c  |  5 ++-
> > >  drivers/mtd/spi-nor/spi-nor.c | 62 +++++++++++++++++++++++++++++++++++
> > >  include/linux/mtd/spi-nor.h   |  6 ++++
> > >  3 files changed, 72 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > > index c4a1d04b8c80..be11e7d96646 100644
> > > --- a/drivers/mtd/devices/m25p80.c
> > > +++ b/drivers/mtd/devices/m25p80.c
> > > @@ -231,8 +231,11 @@ static int m25p_probe(struct spi_mem *spimem)
> >
> > Can we add this to function that is generic to all spi-nor devices,
> > instead of making this specific to m25p?
> I can't find a better way to insert the spi_nor_debugfs_create()
> inside spi_nor.c.
> Another way is adding spi_nor_debugfs_create() to all of the caller.
> What do you think? Any other suggestion?

That, or maybe create a new spi_nor_device_register that does both
mtd_device_register and that spi_nor_debugfs_create call?

> >
> > >         if (ret)
> > >                 return ret;
> > >
> > > -       return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> > > +       ret = mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> > >                                    data ? data->nr_parts : 0);
> > > +       if (!ret)
> > > +               spi_nor_debugfs_create(nor);
> > > +       return ret;
> > >  }
> > >
> > >
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > > index 6e13bbd1aaa5..004b6adf5866 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -21,6 +21,8 @@
> > >  #include <linux/of_platform.h>
> > >  #include <linux/spi/flash.h>
> > >  #include <linux/mtd/spi-nor.h>
> > > +#include <linux/debugfs.h>
> > > +#include <linux/seq_file.h>
> > >
> > >  /* Define max times to check status register before we give up. */
> > >
> > > @@ -4161,6 +4163,66 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> > >  }
> > >  EXPORT_SYMBOL_GPL(spi_nor_scan);
> > >
> > > +static int flashid_dbg_show(struct seq_file *s, void *p)
> > > +{
> > > +       struct spi_nor *nor = (struct spi_nor *)s->private;
> > > +       const struct flash_info *info = nor->info;
> > > +
> > > +       if (!info->id_len)
> > > +               return 0;
> > > +       seq_printf(s, "%*phN\n", info->id_len, info->id);
> > > +       return 0;
> > > +}
> > > +
> > > +static int flashid_debugfs_open(struct inode *inode, struct file *file)
> > > +{
> > > +       return single_open(file, flashid_dbg_show, inode->i_private);
> > > +}
> > > +
> > > +static const struct file_operations flashid_dbg_fops = {
> > > +       .open           = flashid_debugfs_open,
> > > +       .read           = seq_read,
> > > +       .llseek         = seq_lseek,
> > > +       .release        = single_release,
> > > +};
> > > +
> > > +static int flashname_dbg_show(struct seq_file *s, void *p)
> > > +{
> > > +       struct spi_nor *nor = (struct spi_nor *)s->private;
> > > +       const struct flash_info *info = nor->info;
> > > +
> > > +       if (!info->name)
> > > +               return 0;
> > > +       seq_printf(s, "%s\n", info->name);
> > > +       return 0;
> > > +}
> > > +
> > > +static int flashname_debugfs_open(struct inode *inode, struct file *file)
> > > +{
> > > +       return single_open(file, flashname_dbg_show, inode->i_private);
> > > +}
> > > +
> > > +static const struct file_operations flashname_dbg_fops = {
> > > +       .open           = flashname_debugfs_open,
> > > +       .read           = seq_read,
> > > +       .llseek         = seq_lseek,
> > > +       .release        = single_release,
> > > +};
> > > +
> > > +void spi_nor_debugfs_create(struct spi_nor *nor)
> > > +{
> > > +       struct mtd_info *mtd = &nor->mtd;
> > > +       struct dentry *root = mtd->dbg.dfs_dir;
> > > +
> > > +       if (IS_ERR_OR_NULL(root) || IS_ERR_OR_NULL(nor)) {
> >
> > The second check looks useless. Or, to be precise, the kernel should
> > already have crashed above. I'd just drop the check.
> Ah.. i restructured the code and forgot to change this. I'll remove
> this on the next patch.
> >
> > > +               return;
> > > +       }
> > > +       debugfs_create_file("flashid", S_IRUSR, root, nor,
> > > +                       &flashid_dbg_fops);
> > > +       debugfs_create_file("flashname", S_IRUSR, root, nor,
> > > +                       &flashname_dbg_fops);
> >
> > Should we do something with the return values?
> ok, i will add it on the next patch.
> >
> > Look at nandsim_debugfs_create for an example (also, we probably want
> > to check for CONFIG_DEBUG_FS before calling these.
> Do you mean adding the change like this?
>                 if (IS_ENABLED(CONFIG_DEBUG_FS) &&
>                     !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
>                         NS_WARN("CONFIG_MTD_PARTITIONED_MASTER must be
> enabled to expose debugfs stuff\n");

At least IS_ENABLED(CONFIG_DEBUG_FS). I'm not sure what the second
test is about.

> >
> > > +}
> > > +
> > >  MODULE_LICENSE("GPL v2");
> > >  MODULE_AUTHOR("Huang Shijie <shijie8@xxxxxxxxx>");
> > >  MODULE_AUTHOR("Mike Lavender");
> > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > > index fa2d89e38e40..eadb5230c6d0 100644
> > > --- a/include/linux/mtd/spi-nor.h
> > > +++ b/include/linux/mtd/spi-nor.h
> > > @@ -530,4 +530,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> > >   */
> > >  void spi_nor_restore(struct spi_nor *nor);
> > >
> > > +/**
> > > + * spi_nor_debugfs_create() - create debug fs
> > > + * @mtd:       the spi_nor structure
> >
> > @nor
> >
> > > + */
> > > +void spi_nor_debugfs_create(struct spi_nor *nor);
> > > +
> > >  #endif
> > > --
> > > 2.21.0.1020.gf2820cf01a-goog
> > >

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