* [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id @ 2019-03-28 4:59 Zhuohao Lee 2019-04-01 8:43 ` Zhuohao Lee 2019-04-01 9:27 ` Boris Brezillon 0 siblings, 2 replies; 10+ messages in thread From: Zhuohao Lee @ 2019-03-28 4:59 UTC (permalink / raw) To: linux-mtd Cc: drinkcat, zhuohao, bbrezillon, richard, briannorris, marek.vasut, computersforpeace, dwmw2 Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the mtd_info structure and create two sysfs nodes (flashname, id). This patch is modified based on the SPI-NOR flash system as we only have that system now. But the idea should be applied to the other flash driver like NAND flash. The output of new sysfs nodes on my device are: cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname w25q64dw cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id ef6017 Signed-off-by: Zhuohao Lee <zhuohao@chromium.org> --- drivers/mtd/mtdcore.c | 24 ++++++++++++++++++++++++ drivers/mtd/spi-nor/spi-nor.c | 3 +++ include/linux/mtd/mtd.h | 3 +++ 3 files changed, 30 insertions(+) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 3ef01baef9b6..dcbe6719ad67 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev, } static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL); +static ssize_t mtd_flashname_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct mtd_info *mtd = dev_get_drvdata(dev); + + if (!mtd->flashname) + return 0; + return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname); +} +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL); + +static ssize_t mtd_id_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct mtd_info *mtd = dev_get_drvdata(dev); + + if (!mtd->id) + return 0; + return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id); +} +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL); + static ssize_t mtd_ecc_strength_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = { &dev_attr_oobavail.attr, &dev_attr_numeraseregions.attr, &dev_attr_name.attr, + &dev_attr_flashname.attr, + &dev_attr_id.attr, &dev_attr_ecc_strength.attr, &dev_attr_ecc_step_size.attr, &dev_attr_corrected_bits.attr, diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 6e13bbd1aaa5..0e10858e532c 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (!mtd->name) mtd->name = dev_name(dev); + mtd->flashname = info->name; + mtd->id = info->id; + mtd->id_size = info->id_len; 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..0a81569fa4f6 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -210,6 +210,9 @@ struct mtd_info { uint32_t flags; uint32_t orig_flags; /* Flags as before running mtd checks */ uint64_t size; // Total size of the MTD + const char *flashname; /* The underlying flash name */ + const char *id; /* The ID of the flash */ + int id_size; /* Number of bytes of id array */ /* "Major" erase size for the device. Naïve users may take this * to be the only erase size available, or may use the more detailed -- 2.21.0.392.gf8f6787159e-goog ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-03-28 4:59 [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id Zhuohao Lee @ 2019-04-01 8:43 ` Zhuohao Lee 2019-04-01 9:27 ` Boris Brezillon 1 sibling, 0 replies; 10+ messages in thread From: Zhuohao Lee @ 2019-04-01 8:43 UTC (permalink / raw) To: linux-mtd Cc: Nicolas Boichat, bbrezillon, richard, Brian Norris, Marek Vašut, Brian Norris, David Woodhouse Dear reviewers, May i have your professional feedback for this patch? Could we land this patch? Do you have any concern? Please let me know what you think. Thank you. On Thu, Mar 28, 2019 at 12:59 PM Zhuohao Lee <zhuohao@chromium.org> wrote: > > Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > mtd_info structure and create two sysfs nodes (flashname, id). This > patch is modified based on the SPI-NOR flash system as we only have > that system now. But the idea should be applied to the other flash > driver like NAND flash. > > The output of new sysfs nodes on my device are: > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > w25q64dw > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > ef6017 > > Signed-off-by: Zhuohao Lee <zhuohao@chromium.org> > --- > drivers/mtd/mtdcore.c | 24 ++++++++++++++++++++++++ > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > include/linux/mtd/mtd.h | 3 +++ > 3 files changed, 30 insertions(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 3ef01baef9b6..dcbe6719ad67 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev, > } > static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL); > > +static ssize_t mtd_flashname_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct mtd_info *mtd = dev_get_drvdata(dev); > + > + if (!mtd->flashname) > + return 0; > + return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname); > +} > +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL); > + > +static ssize_t mtd_id_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct mtd_info *mtd = dev_get_drvdata(dev); > + > + if (!mtd->id) > + return 0; > + return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id); > +} > +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL); > + > static ssize_t mtd_ecc_strength_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = { > &dev_attr_oobavail.attr, > &dev_attr_numeraseregions.attr, > &dev_attr_name.attr, > + &dev_attr_flashname.attr, > + &dev_attr_id.attr, > &dev_attr_ecc_strength.attr, > &dev_attr_ecc_step_size.attr, > &dev_attr_corrected_bits.attr, > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 6e13bbd1aaa5..0e10858e532c 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > if (!mtd->name) > mtd->name = dev_name(dev); > + mtd->flashname = info->name; > + mtd->id = info->id; > + mtd->id_size = info->id_len; > 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..0a81569fa4f6 100644 > --- a/include/linux/mtd/mtd.h > +++ b/include/linux/mtd/mtd.h > @@ -210,6 +210,9 @@ struct mtd_info { > uint32_t flags; > uint32_t orig_flags; /* Flags as before running mtd checks */ > uint64_t size; // Total size of the MTD > + const char *flashname; /* The underlying flash name */ > + const char *id; /* The ID of the flash */ > + int id_size; /* Number of bytes of id array */ > > /* "Major" erase size for the device. Naïve users may take this > * to be the only erase size available, or may use the more detailed > -- > 2.21.0.392.gf8f6787159e-goog > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-03-28 4:59 [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id Zhuohao Lee 2019-04-01 8:43 ` Zhuohao Lee @ 2019-04-01 9:27 ` Boris Brezillon 2019-04-02 7:39 ` Zhuohao Lee 1 sibling, 1 reply; 10+ messages in thread From: Boris Brezillon @ 2019-04-01 9:27 UTC (permalink / raw) To: Zhuohao Lee Cc: drinkcat, bbrezillon, richard, briannorris, marek.vasut, linux-mtd, computersforpeace, dwmw2 On Thu, 28 Mar 2019 12:59:10 +0800 Zhuohao Lee <zhuohao@chromium.org> wrote: > Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > mtd_info structure and create two sysfs nodes (flashname, id). This > patch is modified based on the SPI-NOR flash system as we only have > that system now. But the idea should be applied to the other flash > driver like NAND flash. > > The output of new sysfs nodes on my device are: > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > w25q64dw > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > ef6017 I'm not sure I like the idea of exposing this kind of info through sysfs as it then makes part of the ABI. Did you consider exposing that through debugfs? > > Signed-off-by: Zhuohao Lee <zhuohao@chromium.org> > --- > drivers/mtd/mtdcore.c | 24 ++++++++++++++++++++++++ > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > include/linux/mtd/mtd.h | 3 +++ > 3 files changed, 30 insertions(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 3ef01baef9b6..dcbe6719ad67 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev, > } > static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL); > > +static ssize_t mtd_flashname_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct mtd_info *mtd = dev_get_drvdata(dev); > + > + if (!mtd->flashname) > + return 0; > + return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname); > +} > +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL); MTD also deals with things that are not flashes (SRAMs, ROM, ...). How about partname? > + > +static ssize_t mtd_id_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct mtd_info *mtd = dev_get_drvdata(dev); > + > + if (!mtd->id) > + return 0; > + return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id); I'd recommend making mtd->id a string so that each flash type can decide of the formatting, and maybe have a prefix that tells which kind of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx". > +} > +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL); id is bit vague, how about partid. > + > static ssize_t mtd_ecc_strength_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = { > &dev_attr_oobavail.attr, > &dev_attr_numeraseregions.attr, > &dev_attr_name.attr, > + &dev_attr_flashname.attr, > + &dev_attr_id.attr, > &dev_attr_ecc_strength.attr, > &dev_attr_ecc_step_size.attr, > &dev_attr_corrected_bits.attr, > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 6e13bbd1aaa5..0e10858e532c 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > if (!mtd->name) > mtd->name = dev_name(dev); > + mtd->flashname = info->name; > + mtd->id = info->id; > + mtd->id_size = info->id_len; > 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..0a81569fa4f6 100644 > --- a/include/linux/mtd/mtd.h > +++ b/include/linux/mtd/mtd.h > @@ -210,6 +210,9 @@ struct mtd_info { > uint32_t flags; > uint32_t orig_flags; /* Flags as before running mtd checks */ > uint64_t size; // Total size of the MTD > + const char *flashname; /* The underlying flash name */ > + const char *id; /* The ID of the flash */ > + int id_size; /* Number of bytes of id array */ > > /* "Major" erase size for the device. Naïve users may take this > * to be the only erase size available, or may use the more detailed ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-04-01 9:27 ` Boris Brezillon @ 2019-04-02 7:39 ` Zhuohao Lee 2019-04-02 7:56 ` Boris Brezillon 0 siblings, 1 reply; 10+ messages in thread From: Zhuohao Lee @ 2019-04-02 7:39 UTC (permalink / raw) To: Boris Brezillon Cc: Nicolas Boichat, bbrezillon, richard, Brian Norris, Marek Vašut, linux-mtd, Brian Norris, David Woodhouse Thanks Boris for the comment. Please take a look the reply at below. On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon <boris.brezillon@collabora.com> wrote: > > On Thu, 28 Mar 2019 12:59:10 +0800 > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > > mtd_info structure and create two sysfs nodes (flashname, id). This > > patch is modified based on the SPI-NOR flash system as we only have > > that system now. But the idea should be applied to the other flash > > driver like NAND flash. > > > > The output of new sysfs nodes on my device are: > > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > > w25q64dw > > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > > ef6017 > > I'm not sure I like the idea of exposing this kind of info through > sysfs as it then makes part of the ABI. Did you consider exposing that > through debugfs? Yes, i did consider the debugfs. I think the debugfs is depended on CONFIG_DEBUG_FS. If removing that config, the partname and partid will be lost. So, i proposed to use sysfs. > > > > > Signed-off-by: Zhuohao Lee <zhuohao@chromium.org> > > --- > > drivers/mtd/mtdcore.c | 24 ++++++++++++++++++++++++ > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > > include/linux/mtd/mtd.h | 3 +++ > > 3 files changed, 30 insertions(+) > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > index 3ef01baef9b6..dcbe6719ad67 100644 > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev, > > } > > static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL); > > > > +static ssize_t mtd_flashname_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct mtd_info *mtd = dev_get_drvdata(dev); > > + > > + if (!mtd->flashname) > > + return 0; > > + return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname); > > +} > > +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL); > > MTD also deals with things that are not flashes (SRAMs, ROM, ...). How > about partname? Thanks, i will change the name to partname. > > > + > > +static ssize_t mtd_id_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct mtd_info *mtd = dev_get_drvdata(dev); > > + > > + if (!mtd->id) > > + return 0; > > + return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id); > > I'd recommend making mtd->id a string so that each flash type can > decide of the formatting, and maybe have a prefix that tells which kind ok, i will create an array to store the formatted partid. > of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx". We had a sysfs node, called 'type', which indicated the type of the underlying device. We can query the 'type' to get the device type. I think it is not necessary to add prefix. What do you think? > > > +} > > +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL); > > id is bit vague, how about partid. Agree, i will change this. > > > + > > static ssize_t mtd_ecc_strength_show(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = { > > &dev_attr_oobavail.attr, > > &dev_attr_numeraseregions.attr, > > &dev_attr_name.attr, > > + &dev_attr_flashname.attr, > > + &dev_attr_id.attr, > > &dev_attr_ecc_strength.attr, > > &dev_attr_ecc_step_size.attr, > > &dev_attr_corrected_bits.attr, > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 6e13bbd1aaa5..0e10858e532c 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > > > if (!mtd->name) > > mtd->name = dev_name(dev); > > + mtd->flashname = info->name; > > + mtd->id = info->id; > > + mtd->id_size = info->id_len; > > 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..0a81569fa4f6 100644 > > --- a/include/linux/mtd/mtd.h > > +++ b/include/linux/mtd/mtd.h > > @@ -210,6 +210,9 @@ struct mtd_info { > > uint32_t flags; > > uint32_t orig_flags; /* Flags as before running mtd checks */ > > uint64_t size; // Total size of the MTD > > + const char *flashname; /* The underlying flash name */ > > + const char *id; /* The ID of the flash */ > > + int id_size; /* Number of bytes of id array */ > > > > /* "Major" erase size for the device. Naïve users may take this > > * to be the only erase size available, or may use the more detailed > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-04-02 7:39 ` Zhuohao Lee @ 2019-04-02 7:56 ` Boris Brezillon 2019-04-02 8:27 ` Vignesh Raghavendra 0 siblings, 1 reply; 10+ messages in thread From: Boris Brezillon @ 2019-04-02 7:56 UTC (permalink / raw) To: Zhuohao Lee Cc: Nicolas Boichat, bbrezillon, richard, Brian Norris, Marek Vašut, linux-mtd, Brian Norris, David Woodhouse On Tue, 2 Apr 2019 15:39:54 +0800 Zhuohao Lee <zhuohao@chromium.org> wrote: > Thanks Boris for the comment. Please take a look the reply at below. > > On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon > <boris.brezillon@collabora.com> wrote: > > > > On Thu, 28 Mar 2019 12:59:10 +0800 > > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > > Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > > > mtd_info structure and create two sysfs nodes (flashname, id). This > > > patch is modified based on the SPI-NOR flash system as we only have > > > that system now. But the idea should be applied to the other flash > > > driver like NAND flash. > > > > > > The output of new sysfs nodes on my device are: > > > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > > > w25q64dw > > > cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > > > ef6017 > > > > I'm not sure I like the idea of exposing this kind of info through > > sysfs as it then makes part of the ABI. Did you consider exposing that > > through debugfs? > > Yes, i did consider the debugfs. I think the debugfs is depended on > CONFIG_DEBUG_FS. > If removing that config, the partname and partid will be lost. So, i > proposed to use > sysfs. Then just enable debugfs if you need this information :P. > > > > > > > > > Signed-off-by: Zhuohao Lee <zhuohao@chromium.org> > > > --- > > > drivers/mtd/mtdcore.c | 24 ++++++++++++++++++++++++ > > > drivers/mtd/spi-nor/spi-nor.c | 3 +++ > > > include/linux/mtd/mtd.h | 3 +++ > > > 3 files changed, 30 insertions(+) > > > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > > index 3ef01baef9b6..dcbe6719ad67 100644 > > > --- a/drivers/mtd/mtdcore.c > > > +++ b/drivers/mtd/mtdcore.c > > > @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev, > > > } > > > static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL); > > > > > > +static ssize_t mtd_flashname_show(struct device *dev, > > > + struct device_attribute *attr, char *buf) > > > +{ > > > + struct mtd_info *mtd = dev_get_drvdata(dev); > > > + > > > + if (!mtd->flashname) > > > + return 0; > > > + return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname); > > > +} > > > +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL); > > > > MTD also deals with things that are not flashes (SRAMs, ROM, ...). How > > about partname? > > Thanks, i will change the name to partname. > > > > > > + > > > +static ssize_t mtd_id_show(struct device *dev, > > > + struct device_attribute *attr, char *buf) > > > +{ > > > + struct mtd_info *mtd = dev_get_drvdata(dev); > > > + > > > + if (!mtd->id) > > > + return 0; > > > + return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id); > > > > I'd recommend making mtd->id a string so that each flash type can > > decide of the formatting, and maybe have a prefix that tells which kind > > ok, i will create an array to store the formatted partid. > > > of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx". > > We had a sysfs node, called 'type', which indicated the type of the > underlying device. We can query the 'type' to get the device type. > I think it is not necessary to add prefix. What do you think? No, the type is not precise enough, a NOR can be a SPI NOR or a CFI NOR and they probably don't use the same ID-scheme. Same for NANDs (parallel NANDs vs SPI NANDs). > > > > > > +} > > > +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL); > > > > id is bit vague, how about partid. > > Agree, i will change this. I think you should wait for other reviews before you sending a new version. I'm still not convinced exposing that through sysfs is a good idea, and I'd like other MTD maintainers to give their opinion on this aspect. > > > > > > + > > > static ssize_t mtd_ecc_strength_show(struct device *dev, > > > struct device_attribute *attr, char *buf) > > > { > > > @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = { > > > &dev_attr_oobavail.attr, > > > &dev_attr_numeraseregions.attr, > > > &dev_attr_name.attr, > > > + &dev_attr_flashname.attr, > > > + &dev_attr_id.attr, > > > &dev_attr_ecc_strength.attr, > > > &dev_attr_ecc_step_size.attr, > > > &dev_attr_corrected_bits.attr, > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > > index 6e13bbd1aaa5..0e10858e532c 100644 > > > --- a/drivers/mtd/spi-nor/spi-nor.c > > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > > @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > > > > > if (!mtd->name) > > > mtd->name = dev_name(dev); > > > + mtd->flashname = info->name; > > > + mtd->id = info->id; > > > + mtd->id_size = info->id_len; > > > 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..0a81569fa4f6 100644 > > > --- a/include/linux/mtd/mtd.h > > > +++ b/include/linux/mtd/mtd.h > > > @@ -210,6 +210,9 @@ struct mtd_info { > > > uint32_t flags; > > > uint32_t orig_flags; /* Flags as before running mtd checks */ > > > uint64_t size; // Total size of the MTD > > > + const char *flashname; /* The underlying flash name */ > > > + const char *id; /* The ID of the flash */ > > > + int id_size; /* Number of bytes of id array */ > > > > > > /* "Major" erase size for the device. Naïve users may take this > > > * to be the only erase size available, or may use the more detailed > > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-04-02 7:56 ` Boris Brezillon @ 2019-04-02 8:27 ` Vignesh Raghavendra 2019-04-02 11:06 ` Zhuohao Lee 0 siblings, 1 reply; 10+ messages in thread From: Vignesh Raghavendra @ 2019-04-02 8:27 UTC (permalink / raw) To: Boris Brezillon, Zhuohao Lee Cc: Nicolas Boichat, bbrezillon, richard, Brian Norris, Marek Vašut, linux-mtd, Brian Norris, David Woodhouse On 02/04/19 1:26 PM, Boris Brezillon wrote: > On Tue, 2 Apr 2019 15:39:54 +0800 > Zhuohao Lee <zhuohao@chromium.org> wrote: > >> Thanks Boris for the comment. Please take a look the reply at below. >> >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon >> <boris.brezillon@collabora.com> wrote: >>> >>> On Thu, 28 Mar 2019 12:59:10 +0800 >>> Zhuohao Lee <zhuohao@chromium.org> wrote: >>> >>>> Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the >>>> mtd_info structure and create two sysfs nodes (flashname, id). This >>>> patch is modified based on the SPI-NOR flash system as we only have >>>> that system now. But the idea should be applied to the other flash >>>> driver like NAND flash. >>>> >>>> The output of new sysfs nodes on my device are: >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname >>>> w25q64dw >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id >>>> ef6017 >>> >>> I'm not sure I like the idea of exposing this kind of info through >>> sysfs as it then makes part of the ABI. Did you consider exposing that >>> through debugfs? >> >> Yes, i did consider the debugfs. I think the debugfs is depended on >> CONFIG_DEBUG_FS. >> If removing that config, the partname and partid will be lost. So, i >> proposed to use >> sysfs. > > Then just enable debugfs if you need this information :P. > Do we really need this info even in debug FS? spi-nor.c prints following to kernel log which can be obtained by dmesg: dev_info(dev, "%s (%lld Kbytes)\n", info->name, (long long)mtd->size >> 10); You could probably expand that to print flash IDs as well. Regards Vignesh >> >>> >>>> >>>> Signed-off-by: Zhuohao Lee <zhuohao@chromium.org> >>>> --- >>>> drivers/mtd/mtdcore.c | 24 ++++++++++++++++++++++++ >>>> drivers/mtd/spi-nor/spi-nor.c | 3 +++ >>>> include/linux/mtd/mtd.h | 3 +++ >>>> 3 files changed, 30 insertions(+) >>>> >>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >>>> index 3ef01baef9b6..dcbe6719ad67 100644 >>>> --- a/drivers/mtd/mtdcore.c >>>> +++ b/drivers/mtd/mtdcore.c >>>> @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev, >>>> } >>>> static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL); >>>> >>>> +static ssize_t mtd_flashname_show(struct device *dev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + struct mtd_info *mtd = dev_get_drvdata(dev); >>>> + >>>> + if (!mtd->flashname) >>>> + return 0; >>>> + return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname); >>>> +} >>>> +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL); >>> >>> MTD also deals with things that are not flashes (SRAMs, ROM, ...). How >>> about partname? >> >> Thanks, i will change the name to partname. >> >>> >>>> + >>>> +static ssize_t mtd_id_show(struct device *dev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + struct mtd_info *mtd = dev_get_drvdata(dev); >>>> + >>>> + if (!mtd->id) >>>> + return 0; >>>> + return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id); >>> >>> I'd recommend making mtd->id a string so that each flash type can >>> decide of the formatting, and maybe have a prefix that tells which kind >> >> ok, i will create an array to store the formatted partid. >> >>> of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx". >> >> We had a sysfs node, called 'type', which indicated the type of the >> underlying device. We can query the 'type' to get the device type. >> I think it is not necessary to add prefix. What do you think? > > No, the type is not precise enough, a NOR can be a SPI NOR or a CFI NOR > and they probably don't use the same ID-scheme. Same for NANDs (parallel > NANDs vs SPI NANDs). > >> >>> >>>> +} >>>> +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL); >>> >>> id is bit vague, how about partid. >> >> Agree, i will change this. > > I think you should wait for other reviews before you sending a new > version. I'm still not convinced exposing that through sysfs is a good > idea, and I'd like other MTD maintainers to give their opinion on this > aspect. > >> >>> >>>> + >>>> static ssize_t mtd_ecc_strength_show(struct device *dev, >>>> struct device_attribute *attr, char *buf) >>>> { >>>> @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = { >>>> &dev_attr_oobavail.attr, >>>> &dev_attr_numeraseregions.attr, >>>> &dev_attr_name.attr, >>>> + &dev_attr_flashname.attr, >>>> + &dev_attr_id.attr, >>>> &dev_attr_ecc_strength.attr, >>>> &dev_attr_ecc_step_size.attr, >>>> &dev_attr_corrected_bits.attr, >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>>> index 6e13bbd1aaa5..0e10858e532c 100644 >>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>> @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >>>> >>>> if (!mtd->name) >>>> mtd->name = dev_name(dev); >>>> + mtd->flashname = info->name; >>>> + mtd->id = info->id; >>>> + mtd->id_size = info->id_len; >>>> 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..0a81569fa4f6 100644 >>>> --- a/include/linux/mtd/mtd.h >>>> +++ b/include/linux/mtd/mtd.h >>>> @@ -210,6 +210,9 @@ struct mtd_info { >>>> uint32_t flags; >>>> uint32_t orig_flags; /* Flags as before running mtd checks */ >>>> uint64_t size; // Total size of the MTD >>>> + const char *flashname; /* The underlying flash name */ >>>> + const char *id; /* The ID of the flash */ >>>> + int id_size; /* Number of bytes of id array */ >>>> >>>> /* "Major" erase size for the device. Naïve users may take this >>>> * to be the only erase size available, or may use the more detailed >>> > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > -- Regards Vignesh ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-04-02 8:27 ` Vignesh Raghavendra @ 2019-04-02 11:06 ` Zhuohao Lee 2019-04-02 12:01 ` Boris Brezillon 0 siblings, 1 reply; 10+ messages in thread From: Zhuohao Lee @ 2019-04-02 11:06 UTC (permalink / raw) To: Vignesh Raghavendra Cc: Nicolas Boichat, bbrezillon, richard, Brian Norris, Marek Vašut, Boris Brezillon, linux-mtd, Brian Norris, David Woodhouse On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote: > > > > On 02/04/19 1:26 PM, Boris Brezillon wrote: > > On Tue, 2 Apr 2019 15:39:54 +0800 > > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > >> Thanks Boris for the comment. Please take a look the reply at below. > >> > >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon > >> <boris.brezillon@collabora.com> wrote: > >>> > >>> On Thu, 28 Mar 2019 12:59:10 +0800 > >>> Zhuohao Lee <zhuohao@chromium.org> wrote: > >>> > >>>> Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > >>>> mtd_info structure and create two sysfs nodes (flashname, id). This > >>>> patch is modified based on the SPI-NOR flash system as we only have > >>>> that system now. But the idea should be applied to the other flash > >>>> driver like NAND flash. > >>>> > >>>> The output of new sysfs nodes on my device are: > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > >>>> w25q64dw > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > >>>> ef6017 > >>> > >>> I'm not sure I like the idea of exposing this kind of info through > >>> sysfs as it then makes part of the ABI. Did you consider exposing that > >>> through debugfs? > >> > >> Yes, i did consider the debugfs. I think the debugfs is depended on > >> CONFIG_DEBUG_FS. > >> If removing that config, the partname and partid will be lost. So, i > >> proposed to use > >> sysfs. > > > > Then just enable debugfs if you need this information :P. My original intention is adding the new sysfs nodes (i.e partname and partid) into the common place like mtdcore.c so that the userspace program can just read the common sysfs nodes. So far, what i can contribute is for the spi-nor but if the other flash drivers, like nand-flash, can also support the partname and partid query, then, we can have common sysfs nodes to query the underlying device info. Compare to the debugfs, i think different drivers may have different name for the partname and partid. I believe this will make the userspace program more complicated to get the partname and partid. > > > > Do we really need this info even in debug FS? > spi-nor.c prints following to kernel log which can be obtained by dmesg: > > dev_info(dev, "%s (%lld Kbytes)\n", info->name, > (long long)mtd->size >> 10); > > You could probably expand that to print flash IDs as well. The dmesg log is insufficient sometimes because the log may be overwritten. Besides, the output log may be inconsistent among the drivers. So, the userspace program needs to be designed to parse the different log. IMHO, if we can provide a common place for querying the partname and partid, that will be good for the userspace program. > > Regards > Vignesh > > >> > >>> > >>>> > >>>> Signed-off-by: Zhuohao Lee <zhuohao@chromium.org> > >>>> --- > >>>> drivers/mtd/mtdcore.c | 24 ++++++++++++++++++++++++ > >>>> drivers/mtd/spi-nor/spi-nor.c | 3 +++ > >>>> include/linux/mtd/mtd.h | 3 +++ > >>>> 3 files changed, 30 insertions(+) > >>>> > >>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > >>>> index 3ef01baef9b6..dcbe6719ad67 100644 > >>>> --- a/drivers/mtd/mtdcore.c > >>>> +++ b/drivers/mtd/mtdcore.c > >>>> @@ -241,6 +241,28 @@ static ssize_t mtd_name_show(struct device *dev, > >>>> } > >>>> static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL); > >>>> > >>>> +static ssize_t mtd_flashname_show(struct device *dev, > >>>> + struct device_attribute *attr, char *buf) > >>>> +{ > >>>> + struct mtd_info *mtd = dev_get_drvdata(dev); > >>>> + > >>>> + if (!mtd->flashname) > >>>> + return 0; > >>>> + return snprintf(buf, PAGE_SIZE, "%s\n", mtd->flashname); > >>>> +} > >>>> +static DEVICE_ATTR(flashname, S_IRUGO, mtd_flashname_show, NULL); > >>> > >>> MTD also deals with things that are not flashes (SRAMs, ROM, ...). How > >>> about partname? > >> > >> Thanks, i will change the name to partname. > >> > >>> > >>>> + > >>>> +static ssize_t mtd_id_show(struct device *dev, > >>>> + struct device_attribute *attr, char *buf) > >>>> +{ > >>>> + struct mtd_info *mtd = dev_get_drvdata(dev); > >>>> + > >>>> + if (!mtd->id) > >>>> + return 0; > >>>> + return snprintf(buf, PAGE_SIZE, "%*phN\n", mtd->id_size, mtd->id); > >>> > >>> I'd recommend making mtd->id a string so that each flash type can > >>> decide of the formatting, and maybe have a prefix that tells which kind > >> > >> ok, i will create an array to store the formatted partid. > >> > >>> of ID this is: "spi-nor:xxxxx", "nand:xxxx", "spi-nand:xxxx". > >> > >> We had a sysfs node, called 'type', which indicated the type of the > >> underlying device. We can query the 'type' to get the device type. > >> I think it is not necessary to add prefix. What do you think? > > > > No, the type is not precise enough, a NOR can be a SPI NOR or a CFI NOR > > and they probably don't use the same ID-scheme. Same for NANDs (parallel > > NANDs vs SPI NANDs). ok, got it. > > > >> > >>> > >>>> +} > >>>> +static DEVICE_ATTR(id, S_IRUGO, mtd_id_show, NULL); > >>> > >>> id is bit vague, how about partid. > >> > >> Agree, i will change this. > > > > I think you should wait for other reviews before you sending a new > > version. I'm still not convinced exposing that through sysfs is a good > > idea, and I'd like other MTD maintainers to give their opinion on this > > aspect. > > > >> > >>> > >>>> + > >>>> static ssize_t mtd_ecc_strength_show(struct device *dev, > >>>> struct device_attribute *attr, char *buf) > >>>> { > >>>> @@ -340,6 +362,8 @@ static struct attribute *mtd_attrs[] = { > >>>> &dev_attr_oobavail.attr, > >>>> &dev_attr_numeraseregions.attr, > >>>> &dev_attr_name.attr, > >>>> + &dev_attr_flashname.attr, > >>>> + &dev_attr_id.attr, > >>>> &dev_attr_ecc_strength.attr, > >>>> &dev_attr_ecc_step_size.attr, > >>>> &dev_attr_corrected_bits.attr, > >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > >>>> index 6e13bbd1aaa5..0e10858e532c 100644 > >>>> --- a/drivers/mtd/spi-nor/spi-nor.c > >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c > >>>> @@ -4027,6 +4027,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > >>>> > >>>> if (!mtd->name) > >>>> mtd->name = dev_name(dev); > >>>> + mtd->flashname = info->name; > >>>> + mtd->id = info->id; > >>>> + mtd->id_size = info->id_len; > >>>> 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..0a81569fa4f6 100644 > >>>> --- a/include/linux/mtd/mtd.h > >>>> +++ b/include/linux/mtd/mtd.h > >>>> @@ -210,6 +210,9 @@ struct mtd_info { > >>>> uint32_t flags; > >>>> uint32_t orig_flags; /* Flags as before running mtd checks */ > >>>> uint64_t size; // Total size of the MTD > >>>> + const char *flashname; /* The underlying flash name */ > >>>> + const char *id; /* The ID of the flash */ > >>>> + int id_size; /* Number of bytes of id array */ > >>>> > >>>> /* "Major" erase size for the device. Naïve users may take this > >>>> * to be the only erase size available, or may use the more detailed > >>> > > > > > > ______________________________________________________ > > Linux MTD discussion mailing list > > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > > > -- > Regards > Vignesh ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-04-02 11:06 ` Zhuohao Lee @ 2019-04-02 12:01 ` Boris Brezillon 2019-04-02 13:03 ` Zhuohao Lee 0 siblings, 1 reply; 10+ messages in thread From: Boris Brezillon @ 2019-04-02 12:01 UTC (permalink / raw) To: Zhuohao Lee Cc: Nicolas Boichat, Vignesh Raghavendra, bbrezillon, richard, Brian Norris, Marek Vašut, linux-mtd, Brian Norris, David Woodhouse On Tue, 2 Apr 2019 19:06:37 +0800 Zhuohao Lee <zhuohao@chromium.org> wrote: > On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote: > > > > > > > > On 02/04/19 1:26 PM, Boris Brezillon wrote: > > > On Tue, 2 Apr 2019 15:39:54 +0800 > > > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > > >> Thanks Boris for the comment. Please take a look the reply at below. > > >> > > >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon > > >> <boris.brezillon@collabora.com> wrote: > > >>> > > >>> On Thu, 28 Mar 2019 12:59:10 +0800 > > >>> Zhuohao Lee <zhuohao@chromium.org> wrote: > > >>> > > >>>> Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > > >>>> mtd_info structure and create two sysfs nodes (flashname, id). This > > >>>> patch is modified based on the SPI-NOR flash system as we only have > > >>>> that system now. But the idea should be applied to the other flash > > >>>> driver like NAND flash. > > >>>> > > >>>> The output of new sysfs nodes on my device are: > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > > >>>> w25q64dw > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > > >>>> ef6017 > > >>> > > >>> I'm not sure I like the idea of exposing this kind of info through > > >>> sysfs as it then makes part of the ABI. Did you consider exposing that > > >>> through debugfs? > > >> > > >> Yes, i did consider the debugfs. I think the debugfs is depended on > > >> CONFIG_DEBUG_FS. > > >> If removing that config, the partname and partid will be lost. So, i > > >> proposed to use > > >> sysfs. > > > > > > Then just enable debugfs if you need this information :P. > My original intention is adding the new sysfs nodes (i.e partname and > partid) into the common place like mtdcore.c > so that the userspace program can just read the common sysfs nodes. > So far, what i can contribute is for the spi-nor but if the other > flash drivers, like nand-flash, can also support > the partname and partid query, then, we can have common sysfs nodes to > query the underlying device info. > Compare to the debugfs, i think different drivers may have different > name for the partname and partid. We've recently unified how MTD related stuff are exposed through debugfs and you can now have generic MTD fields exposed there. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-04-02 12:01 ` Boris Brezillon @ 2019-04-02 13:03 ` Zhuohao Lee 2019-04-03 8:31 ` Miquel Raynal 0 siblings, 1 reply; 10+ messages in thread From: Zhuohao Lee @ 2019-04-02 13:03 UTC (permalink / raw) To: Boris Brezillon Cc: Nicolas Boichat, Vignesh Raghavendra, bbrezillon, richard, Brian Norris, Marek Vašut, linux-mtd, Brian Norris, David Woodhouse Boris, Thanks for your comment. But I can't find the related stuff (or i got the wrong way). Is that feature merged? Could you please point out the commit? I can make the change base on it. On Tue, Apr 2, 2019 at 8:01 PM Boris Brezillon <boris.brezillon@collabora.com> wrote: > > On Tue, 2 Apr 2019 19:06:37 +0800 > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote: > > > > > > > > > > > > On 02/04/19 1:26 PM, Boris Brezillon wrote: > > > > On Tue, 2 Apr 2019 15:39:54 +0800 > > > > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > > > > >> Thanks Boris for the comment. Please take a look the reply at below. > > > >> > > > >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon > > > >> <boris.brezillon@collabora.com> wrote: > > > >>> > > > >>> On Thu, 28 Mar 2019 12:59:10 +0800 > > > >>> Zhuohao Lee <zhuohao@chromium.org> wrote: > > > >>> > > > >>>> Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > > > >>>> mtd_info structure and create two sysfs nodes (flashname, id). This > > > >>>> patch is modified based on the SPI-NOR flash system as we only have > > > >>>> that system now. But the idea should be applied to the other flash > > > >>>> driver like NAND flash. > > > >>>> > > > >>>> The output of new sysfs nodes on my device are: > > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > > > >>>> w25q64dw > > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > > > >>>> ef6017 > > > >>> > > > >>> I'm not sure I like the idea of exposing this kind of info through > > > >>> sysfs as it then makes part of the ABI. Did you consider exposing that > > > >>> through debugfs? > > > >> > > > >> Yes, i did consider the debugfs. I think the debugfs is depended on > > > >> CONFIG_DEBUG_FS. > > > >> If removing that config, the partname and partid will be lost. So, i > > > >> proposed to use > > > >> sysfs. > > > > > > > > Then just enable debugfs if you need this information :P. > > My original intention is adding the new sysfs nodes (i.e partname and > > partid) into the common place like mtdcore.c > > so that the userspace program can just read the common sysfs nodes. > > So far, what i can contribute is for the spi-nor but if the other > > flash drivers, like nand-flash, can also support > > the partname and partid query, then, we can have common sysfs nodes to > > query the underlying device info. > > Compare to the debugfs, i think different drivers may have different > > name for the partname and partid. > > We've recently unified how MTD related stuff are exposed through > debugfs and you can now have generic MTD fields exposed there. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id 2019-04-02 13:03 ` Zhuohao Lee @ 2019-04-03 8:31 ` Miquel Raynal 0 siblings, 0 replies; 10+ messages in thread From: Miquel Raynal @ 2019-04-03 8:31 UTC (permalink / raw) To: Zhuohao Lee Cc: Nicolas Boichat, Vignesh Raghavendra, bbrezillon, richard, Brian Norris, Marek Vašut, Boris Brezillon, linux-mtd, Brian Norris, David Woodhouse Hi Zhuohao, Zhuohao Lee <zhuohao@chromium.org> wrote on Tue, 2 Apr 2019 21:03:28 +0800: > Boris, Thanks for your comment. > But I can't find the related stuff (or i got the wrong way). Is that > feature merged? > Could you please point out the commit? I can make the change base on it. I think Boris just told you that the debugfs MTD directory is suitable to receive an information coming from different type of devices like spi-nor, nor, nand, etc. The feature of having the device name there does not exist yet, you have to do it. For NAND I already wrote something like that which has not been merged but could be adapted to export the data in debugfs instead of sysfs, see: Raw NAND infrastructure to retrieve the ID: http://patchwork.ozlabs.org/patch/837901/ Adding a sysfs entry (which was already rejected at this time): http://patchwork.ozlabs.org/patch/837903/ Thanks, Miquèl > > On Tue, Apr 2, 2019 at 8:01 PM Boris Brezillon > <boris.brezillon@collabora.com> wrote: > > > > On Tue, 2 Apr 2019 19:06:37 +0800 > > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > > On Tue, Apr 2, 2019 at 4:26 PM Vignesh Raghavendra <vigneshr@ti.com> wrote: > > > > > > > > > > > > > > > > On 02/04/19 1:26 PM, Boris Brezillon wrote: > > > > > On Tue, 2 Apr 2019 15:39:54 +0800 > > > > > Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > > > > > > >> Thanks Boris for the comment. Please take a look the reply at below. > > > > >> > > > > >> On Mon, Apr 1, 2019 at 5:27 PM Boris Brezillon > > > > >> <boris.brezillon@collabora.com> wrote: > > > > >>> > > > > >>> On Thu, 28 Mar 2019 12:59:10 +0800 > > > > >>> Zhuohao Lee <zhuohao@chromium.org> wrote: > > > > >>> > > > > >>>> Currently, we don't have sysfs 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 two pointers (*flashname, *id) into the > > > > >>>> mtd_info structure and create two sysfs nodes (flashname, id). This > > > > >>>> patch is modified based on the SPI-NOR flash system as we only have > > > > >>>> that system now. But the idea should be applied to the other flash > > > > >>>> driver like NAND flash. > > > > >>>> > > > > >>>> The output of new sysfs nodes on my device are: > > > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/flashname > > > > >>>> w25q64dw > > > > >>>> cat /sys/devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/id > > > > >>>> ef6017 > > > > >>> > > > > >>> I'm not sure I like the idea of exposing this kind of info through > > > > >>> sysfs as it then makes part of the ABI. Did you consider exposing that > > > > >>> through debugfs? > > > > >> > > > > >> Yes, i did consider the debugfs. I think the debugfs is depended on > > > > >> CONFIG_DEBUG_FS. > > > > >> If removing that config, the partname and partid will be lost. So, i > > > > >> proposed to use > > > > >> sysfs. > > > > > > > > > > Then just enable debugfs if you need this information :P. > > > My original intention is adding the new sysfs nodes (i.e partname and > > > partid) into the common place like mtdcore.c > > > so that the userspace program can just read the common sysfs nodes. > > > So far, what i can contribute is for the spi-nor but if the other > > > flash drivers, like nand-flash, can also support > > > the partname and partid query, then, we can have common sysfs nodes to > > > query the underlying device info. > > > Compare to the debugfs, i think different drivers may have different > > > name for the partname and partid. > > > > We've recently unified how MTD related stuff are exposed through > > debugfs and you can now have generic MTD fields exposed there. > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-03 8:31 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-28 4:59 [PATCH v1] mtd: core: add sysfs nodes for querying the flash name and id Zhuohao Lee 2019-04-01 8:43 ` Zhuohao Lee 2019-04-01 9:27 ` Boris Brezillon 2019-04-02 7:39 ` Zhuohao Lee 2019-04-02 7:56 ` Boris Brezillon 2019-04-02 8:27 ` Vignesh Raghavendra 2019-04-02 11:06 ` Zhuohao Lee 2019-04-02 12:01 ` Boris Brezillon 2019-04-02 13:03 ` Zhuohao Lee 2019-04-03 8:31 ` Miquel Raynal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).