| Message ID | 20190506084414.89702-1-zhuohao@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | [v2] mtd: spi-nor: add debugfs nodes for querying the flash name and id | expand |
The previous discussion thread: https://patchwork.ozlabs.org/patch/1067763/ On Mon, May 6, 2019 at 4:44 PM 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 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@chromium.org> > --- > 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) > 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)) { > + return; > + } > + debugfs_create_file("flashid", S_IRUSR, root, nor, > + &flashid_dbg_fops); > + debugfs_create_file("flashname", S_IRUSR, root, nor, > + &flashname_dbg_fops); > +} > + > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>"); > 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 > + */ > +void spi_nor_debugfs_create(struct spi_nor *nor); > + > #endif > -- > 2.21.0.1020.gf2820cf01a-goog >
On Mon, May 6, 2019 at 5:44 PM 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 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@chromium.org> > --- 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@gmail.com>"); > 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 >
On Mon, May 6, 2019 at 5:07 PM Nicolas Boichat <drinkcat@chromium.org> wrote: > > On Mon, May 6, 2019 at 5:44 PM 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 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@chromium.org> > > --- > > 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@gmail.com>"); > > 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 > >
On Mon, May 6, 2019 at 8:36 PM Zhuohao Lee <zhuohao@chromium.org> wrote: > > On Mon, May 6, 2019 at 5:07 PM Nicolas Boichat <drinkcat@chromium.org> wrote: > > > > On Mon, May 6, 2019 at 5:44 PM 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 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@chromium.org> > > > --- > > > > 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? That, or maybe create a new spi_nor_device_register that does both mtd_device_register and that spi_nor_debugfs_create call? > > > > > 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"); At least IS_ENABLED(CONFIG_DEBUG_FS). I'm not sure what the second test is about. > > > > > +} > > > + > > > MODULE_LICENSE("GPL v2"); > > > MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>"); > > > 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 > > >
On Tue, May 7, 2019 at 10:11 AM Nicolas Boichat <drinkcat@chromium.org> wrote: > > On Mon, May 6, 2019 at 8:36 PM Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > On Mon, May 6, 2019 at 5:07 PM Nicolas Boichat <drinkcat@chromium.org> wrote: > > > > > > On Mon, May 6, 2019 at 5:44 PM 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 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@chromium.org> > > > > --- > > > > > > 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? > > That, or maybe create a new spi_nor_device_register that does both > mtd_device_register and that spi_nor_debugfs_create call? Thanks for suggestion. I feel that putting the mtd_device_register (high level api) inside the spi-nor (low level api) isn't perfect. This also will limit the caller to call this api to register mtd device with debugfs and lost the flexibility. I'll keep the original idea that adding spi_nor_debugfs_create() to all of the caller. > > > > > > > > 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"); > > At least IS_ENABLED(CONFIG_DEBUG_FS). I'm not sure what the second > test is about. Just checked the commit message, i think the code is needed. Will update a new patch for this. > > > > > > > > +} > > > > + > > > > MODULE_LICENSE("GPL v2"); > > > > MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>"); > > > > 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 > > > >
On Tue, 7 May 2019 23:06:32 +0800 Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > > @@ -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? > > > > That, or maybe create a new spi_nor_device_register that does both > > mtd_device_register and that spi_nor_debugfs_create call? > Thanks for suggestion. I feel that putting the mtd_device_register > (high level api) inside the spi-nor (low level api) > isn't perfect. This also will limit the caller to call this api to > register mtd device with debugfs and lost the flexibility. > I'll keep the original idea that adding spi_nor_debugfs_create() to > all of the caller. Why don't you move that to the MTD layer? If you add partname/partid fields to mtd_info you'll have everything you need to make that generic. It's then up to the upper layer to fill those fields before calling mtd_device_register(). > > > > > > > > > + return; > > > > > + } > > > > > + debugfs_create_file("flashid", S_IRUSR, root, nor, > > > > > + &flashid_dbg_fops); > > > > > + debugfs_create_file("flashname", S_IRUSR, root, nor, > > > > > + &flashname_dbg_fops); I thought we agreed on partname/partid. Any reason for switching back to flashname/flashid?
On Tue, May 7, 2019 at 11:10 PM Boris Brezillon <boris.brezillon@collabora.com> wrote: > > On Tue, 7 May 2019 23:06:32 +0800 > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > > > > > @@ -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? > > > > > > That, or maybe create a new spi_nor_device_register that does both > > > mtd_device_register and that spi_nor_debugfs_create call? > > Thanks for suggestion. I feel that putting the mtd_device_register > > (high level api) inside the spi-nor (low level api) > > isn't perfect. This also will limit the caller to call this api to > > register mtd device with debugfs and lost the flexibility. > > I'll keep the original idea that adding spi_nor_debugfs_create() to > > all of the caller. > > Why don't you move that to the MTD layer? If you add partname/partid > fields to mtd_info you'll have everything you need to make that generic. > It's then up to the upper layer to fill those fields before calling > mtd_device_register(). > Ah... i took the wrong way. I removed the partname/partid from mtd.h. So, i can't use it inside the mtd_core.c. Instead, i created spi_nor_debugfs_create() for creating debugfs. i'll submit a patch to add back the partname/partid to mtd.h > > > > > > > > > > > > + return; > > > > > > + } > > > > > > + debugfs_create_file("flashid", S_IRUSR, root, nor, > > > > > > + &flashid_dbg_fops); > > > > > > + debugfs_create_file("flashname", S_IRUSR, root, nor, > > > > > > + &flashname_dbg_fops); > > I thought we agreed on partname/partid. Any reason for switching back > to flashname/flashid? Sorry, per reply above, i took the wrong approach. So, i used old name because i put the debugfs stuff in spi-nor.c.
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) 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)) { + return; + } + debugfs_create_file("flashid", S_IRUSR, root, nor, + &flashid_dbg_fops); + debugfs_create_file("flashname", S_IRUSR, root, nor, + &flashname_dbg_fops); +} + MODULE_LICENSE("GPL v2"); MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>"); 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 + */ +void spi_nor_debugfs_create(struct spi_nor *nor); + #endif
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@chromium.org> --- 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(-)