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

[Zhuohao Lee <zhuohao@xxxxxxxxxxxx>]

Hi all, May i have your comment for this patch? Thanks.

On Thu, May 9, 2019 at 12:10 AM 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 mtd_debugfs_create() to create two debugfs nodes
> (ie. partname and partid). The upper driver can assign the pointer to
> partname and partid before calling mtd_device_register().
> 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/partid
> spi-nor:ef6017
> cat /sys/kernel/debug/mtd/mtd0/partname
> w25q64dw
>
> Signed-off-by: Zhuohao Lee <zhuohao@xxxxxxxxxxxx>
> ---
> Changes in v3:
> - Add partname and partid to mtd.h and create debugfs inside mtdcore.c
> - Previous discussion: https://patchwork.ozlabs.org/patch/1095731/
> Changes in v2:
> - Change to use debugfs to output flash name and id
> - Previous discussion: https://patchwork.ozlabs.org/patch/1067763/
> ---
>  drivers/mtd/mtdcore.c         | 68 +++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/spi-nor.c | 19 ++++++++++
>  include/linux/mtd/mtd.h       |  4 +++
>  3 files changed, 91 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 3ef01baef9b6..f68f055bfd47 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -357,6 +357,71 @@ static const struct device_type mtd_devtype = {
>         .release        = mtd_release,
>  };
>
> +static int mtd_partid_show(struct seq_file *s, void *p)
> +{
> +       struct mtd_info *mtd = s->private;
> +
> +       if (!mtd->partid)
> +               return 0;
> +
> +       seq_printf(s, "%s\n", mtd->partid);
> +
> +       return 0;
> +}
> +
> +static int mtd_partid_dbgfs_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, mtd_partid_show, inode->i_private);
> +}
> +
> +static const struct file_operations mtd_partid_dbg_fops = {
> +       .open           = mtd_partid_dbgfs_open,
> +       .read           = seq_read,
> +       .llseek         = seq_lseek,
> +       .release        = single_release,
> +};
> +
> +static int mtd_partname_show(struct seq_file *s, void *p)
> +{
> +       struct mtd_info *mtd = s->private;
> +
> +       if (!mtd->partname)
> +               return 0;
> +
> +       seq_printf(s, "%s\n", mtd->partname);
> +
> +       return 0;
> +}
> +
> +static int mtd_partname_dbgfs_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, mtd_partname_show, inode->i_private);
> +}
> +
> +static const struct file_operations mtd_partname_dbg_fops = {
> +       .open           = mtd_partname_dbgfs_open,
> +       .read           = seq_read,
> +       .llseek         = seq_lseek,
> +       .release        = single_release,
> +};
> +
> +static int mtd_debugfs_create(struct mtd_info *mtd)
> +{
> +       struct dentry *root = mtd->dbg.dfs_dir;
> +       struct dentry *dent_id, *dent_name;
> +
> +       dent_id = debugfs_create_file("partid", S_IRUSR, root, mtd,
> +                                     &mtd_partid_dbg_fops);
> +       dent_name = debugfs_create_file("partname", S_IRUSR, root, mtd,
> +                                       &mtd_partname_dbg_fops);
> +       if (IS_ERR_OR_NULL(dent_id) || IS_ERR_OR_NULL(dent_name)) {
> +               pr_err("cannot create debugfs entry\n");
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
>  #ifndef CONFIG_MMU
>  unsigned mtd_mmap_capabilities(struct mtd_info *mtd)
>  {
> @@ -626,6 +691,9 @@ int add_mtd_device(struct mtd_info *mtd)
>                 if (IS_ERR_OR_NULL(mtd->dbg.dfs_dir)) {
>                         pr_debug("mtd device %s won't show data in debugfs\n",
>                                  dev_name(&mtd->dev));
> +               } else if (mtd_debugfs_create(mtd)) {
> +                       pr_debug("mtd device %s can't create debugfs\n",
> +                                dev_name(&mtd->dev));
>                 }
>         }
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 6e13bbd1aaa5..9f157dff0f2c 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -38,6 +38,7 @@
>
>  #define SPI_NOR_MAX_ID_LEN     6
>  #define SPI_NOR_MAX_ADDR_WIDTH 4
> +#define SPI_NOR_MAX_ID_STRING  (SPI_NOR_MAX_ID_LEN + 9)
>
>  struct spi_nor_read_command {
>         u8                      num_mode_clocks;
> @@ -240,6 +241,12 @@ struct flash_info {
>          */
>         u8              id[SPI_NOR_MAX_ID_LEN];
>         u8              id_len;
> +       /*
> +        * This string stores the output format of the id
> +        * The format looks like this: spi-nor:xxxxxx\0
> +        * The max length of the string is 8 + SPI_NOR_MAX_ID_LEN + 1
> +        */
> +       char            id_string[SPI_NOR_MAX_ID_STRING];
>
>         /* The size listed here is what works with SPINOR_OP_SE, which isn't
>          * necessarily called a "sector" by the vendor.
> @@ -3935,6 +3942,15 @@ static void spi_nor_resume(struct mtd_info *mtd)
>                 dev_err(dev, "resume() failed\n");
>  }
>
> +static void spi_nor_format_id_string(const struct flash_info *info)
> +{
> +       char *id_string = (char *)info->id_string;
> +
> +       if (!snprintf(id_string, SPI_NOR_MAX_ID_STRING,
> +                     "spi-nor:%*phN", info->id_len, info->id))
> +               memset(id_string, 0, SPI_NOR_MAX_ID_STRING);
> +}
> +
>  void spi_nor_restore(struct spi_nor *nor)
>  {
>         /* restore the addressing mode */
> @@ -4009,6 +4025,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>         }
>
>         nor->info = info;
> +       spi_nor_format_id_string(info);
>
>         mutex_init(&nor->lock);
>
> @@ -4027,6 +4044,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>
>         if (!mtd->name)
>                 mtd->name = dev_name(dev);
> +       mtd->partname = info->name;
> +       mtd->partid = info->id_string;
>         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..f7b193167680 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -344,6 +344,10 @@ struct mtd_info {
>         int usecount;
>         struct mtd_debug_info dbg;
>         struct nvmem_device *nvmem;
> +
> +       /* debugfs stuff starts here */
> +       const char *partname;
> +       const char *partid;
>  };
>
>  int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,
> --
> 2.21.0.1020.gf2820cf01a-goog
>

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