Re: [PATCH v5 1/2] mtd: mtdcore: add debugfs nodes for querying the flash name and id

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Mon,  3 Jun 2019 11:14:50 +0800
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_populate() 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().
> 
> Signed-off-by: Zhuohao Lee <zhuohao@xxxxxxxxxxxx>
> ---
> Changes in V5:
> - Move debugfs_create_dir() to mtd_debugfs_populate()
> - Previous discussion: https://patchwork.ozlabs.org/patch/1107810/
> Changes in V4:
> - Separate the change to two patches. The first patch is adding the general
>   handling for the partname and partid in the mtdcore.c. The second patch
>   is enabling the two debugfs nodes for spi-nor.
> - Previous discussion: https://patchwork.ozlabs.org/patch/1097377/
> 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   | 88 ++++++++++++++++++++++++++++++++++++-----
>  include/linux/mtd/mtd.h |  4 ++
>  2 files changed, 83 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 3ef01baef9b6..b1911c21eb1a 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -357,6 +357,84 @@ 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->dbg.partid)
> +		return 0;
> +
> +	seq_printf(s, "%s\n", mtd->dbg.partid);
> +
> +	return 0;
> +}
> +
> +static int mtd_partid_debugfs_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, mtd_partid_show, inode->i_private);
> +}
> +
> +static const struct file_operations mtd_partid_debug_fops = {
> +	.open           = mtd_partid_debugfs_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->dbg.partname)
> +		return 0;
> +
> +	seq_printf(s, "%s\n", mtd->dbg.partname);
> +
> +	return 0;
> +}
> +
> +static int mtd_partname_debugfs_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, mtd_partname_show, inode->i_private);
> +}
> +
> +static const struct file_operations mtd_partname_debug_fops = {
> +	.open           = mtd_partname_debugfs_open,
> +	.read           = seq_read,
> +	.llseek         = seq_lseek,
> +	.release        = single_release,
> +};
> +
> +static struct dentry *dfs_dir_mtd;
> +
> +static void mtd_debugfs_populate(struct mtd_info *mtd)
> +{
> +	struct device *dev = &mtd->dev;
> +	struct dentry *root, *dent;
> +
> +	if (IS_ERR_OR_NULL(dfs_dir_mtd))
> +		return;
> +
> +	root = mtd->dbg.dfs_dir = debugfs_create_dir(dev_name(dev),
> +						     dfs_dir_mtd);
> +	if (IS_ERR_OR_NULL(root)) {
> +		pr_debug("mtd device %s won't show data in debugfs\n",
> +			 dev_name(dev));
> +		return;
> +	}
> +
> +	dent = debugfs_create_file("partid", S_IRUSR, root, mtd,
> +				   &mtd_partid_debug_fops);
> +	if (IS_ERR_OR_NULL(dent)) {
> +		pr_err("cannot create debugfs entry for partid\n");
> +		return;
> +	}
> +	dent = debugfs_create_file("partname", S_IRUSR, root, mtd,
> +				   &mtd_partname_debug_fops);
> +	if (IS_ERR_OR_NULL(dent))
> +		pr_err("cannot create debugfs entry for partname\n");

I wonder if it's not better to create those files only when
mtd->dbg.part{name,id} != NULL. This way you could get rid of the
if (!mtd->dbg.part{name,id}) test in the show funcs, and most
importantly, the user would know when this fields are not present
without having to read the file and test for an empty string.

> +}
> +
>  #ifndef CONFIG_MMU
>  unsigned mtd_mmap_capabilities(struct mtd_info *mtd)
>  {
> @@ -534,8 +612,6 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>  	return 0;
>  }
>  
> -static struct dentry *dfs_dir_mtd;
> -
>  /**
>   *	add_mtd_device - register an MTD device
>   *	@mtd: pointer to new MTD device info structure
> @@ -621,13 +697,7 @@ int add_mtd_device(struct mtd_info *mtd)
>  	if (error)
>  		goto fail_nvmem_add;
>  
> -	if (!IS_ERR_OR_NULL(dfs_dir_mtd)) {
> -		mtd->dbg.dfs_dir = debugfs_create_dir(dev_name(&mtd->dev), dfs_dir_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));
> -		}
> -	}
> +	mtd_debugfs_populate(mtd);
>  
>  	device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
>  		      "mtd%dro", i);
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 677768b21a1d..c20f53c77899 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -203,6 +203,10 @@ struct module;	/* only needed for owner field in mtd_info */
>   */
>  struct mtd_debug_info {
>  	struct dentry *dfs_dir;
> +
> +	/* debugfs stuff starts here */

This comment is no longer valid: the whole struct contains debugfs
stuff :P.

> +	const char *partname;
> +	const char *partid;
>  };
>  
>  struct mtd_info {


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