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

[Zhuohao Lee <zhuohao@xxxxxxxxxxxx>]

On Tue, May 7, 2019 at 10:11 AM Nicolas Boichat <drinkcat@xxxxxxxxxxxx> wrote:
>
> 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?
Thanks for suggestion. I feel that putting the mtd_device_register
(high level api) inside the spi-nor (low level api)
isn't perfect. This also will limit the caller to call this api to
register mtd device with debugfs and lost the flexibility.
I'll keep the original idea that adding spi_nor_debugfs_create() to
all of the caller.
>
> > >
> > > >         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.
Just checked the commit message, i think the code is needed. Will
update a new patch for this.

>
> > >
> > > > +}
> > > > +
> > > >  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/