[v3] mtd: spi-nor: add debugfs nodes for querying the flash name and id
diff mbox series

Message ID 20190509071005.198887-1-zhuohao@chromium.org
State Changes Requested
Delegated to: Ambarus Tudor
Headers show
Series
  • [v3] mtd: spi-nor: add debugfs nodes for querying the flash name and id
Related show

Commit Message

Zhuohao Lee May 9, 2019, 7:10 a.m. UTC
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@chromium.org>
---
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(+)

Comments

Zhuohao Lee May 23, 2019, 9:38 p.m. UTC | #1
Hi all, May i have your comment for this patch? Thanks.

On Thu, May 9, 2019 at 12:10 AM Zhuohao Lee <zhuohao@chromium.org> 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@chromium.org>
> ---
> 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
>
Boris Brezillon May 24, 2019, 6:15 a.m. UTC | #2
On Thu,  9 May 2019 15:10:05 +0800
Zhuohao Lee <zhuohao@chromium.org> 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@chromium.org>
> ---
> 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 +++

Please split this patch in 2: one adding the generic bits to mtdcore
and another one initializing ->partname/partid for the spi-nor case.

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

How about mtd_debugfs_populate() instead of _create(). Can you also use
a consistent prefix. Looks like sometimes it's dbgfs and others
debugfs.

> +{
> +	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;

Please return the real error code and move each test next to the
create_file() call.

> +	}
> +
> +	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];

I think we can afford an dynamic allocation here and use
devm_kasprintf(). Plus, flash_info is supposed to be const all the
time, so this field does not belong here (should be placed in
struct spi_nor).

>  
>  	/* 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)

How about renaming this function spi_nor_debugfs_init() and moving the
part->{partname,name} assignment there.

> +{
> +	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;

Move those fields in mtd_debug_info.

>  };
>  
>  int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,

Patch
diff mbox series

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,