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

[Zhuohao Lee <zhuohao@xxxxxxxxxxxx>]

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?
>
> >         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");
>
> > +}
> > +
> >  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/