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

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

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

Look at nandsim_debugfs_create for an example (also, we probably want
to check for CONFIG_DEBUG_FS before calling these.

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