| Message ID | 20231127165908.1734951-1-tudor.ambarus@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: stop printing superfluous debug info | expand |
Hi Tudor, tudor.ambarus@linaro.org wrote on Mon, 27 Nov 2023 16:59:08 +0000: > The mtd data can be obtain with the mtd ioctls and the SPI NOR > flash name can be determined interrogating the sysfs entries. > Stop polluting the kernel log. Actually I like these prints when developing/fixing stuff, it's a clear indication of what's been discovered that is available even if for some reason my rootfs is not available (which is common when the rootfs is on a spi-nor). So I would not trash all these lines personally... I believe the dev_info can be lowered if you prefer, but dev_dbg is already meant for debugging purposes and will anyway be discarded by default. > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/mtd/spi-nor/core.c | 19 ------------------- > 1 file changed, 19 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 25a64c65717d..6de76fd009d1 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3517,25 +3517,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > /* No mtd_info fields should be used up to this point. */ > spi_nor_set_mtd_info(nor); > > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, > - (long long)mtd->size >> 10); > - > - dev_dbg(dev, > - "mtd .name = %s, .size = 0x%llx (%lldMiB), " > - ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", > - mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), > - mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); > - > - if (mtd->numeraseregions) > - for (i = 0; i < mtd->numeraseregions; i++) > - dev_dbg(dev, > - "mtd.eraseregions[%d] = { .offset = 0x%llx, " > - ".erasesize = 0x%.8x (%uKiB), " > - ".numblocks = %d }\n", > - i, (long long)mtd->eraseregions[i].offset, > - mtd->eraseregions[i].erasesize, > - mtd->eraseregions[i].erasesize / 1024, > - mtd->eraseregions[i].numblocks); > return 0; > } > EXPORT_SYMBOL_GPL(spi_nor_scan); Thanks, Miquèl
On 11/27/23 17:15, Miquel Raynal wrote: > Hi Tudor, > Hi! > tudor.ambarus@linaro.org wrote on Mon, 27 Nov 2023 16:59:08 +0000: > >> The mtd data can be obtain with the mtd ioctls and the SPI NOR >> flash name can be determined interrogating the sysfs entries. >> Stop polluting the kernel log. > > Actually I like these prints when developing/fixing stuff, it's a clear > indication of what's been discovered that is available even if for some > reason my rootfs is not available (which is common when the rootfs is > on a spi-nor). > > So I would not trash all these lines personally... I believe the What do you find useful about these prints? Maybe you mean that you'd like some indicator that the SPI NOR probed successfully? How abut adding a debug message at the end of mtd_device_register(), or otherwise after mtd_device_register() is called? > dev_info can be lowered if you prefer, but dev_dbg is already meant for > debugging purposes and will anyway be discarded by default. Yes, if we're going to keep these sort of prints I think we should definitely lower them from info to dbg. It will speed the boot time and stop polluting the kernel log. Cheers, ta
Hi, > The mtd data can be obtain with the mtd ioctls and the SPI NOR > flash name can be determined interrogating the sysfs entries. > Stop polluting the kernel log. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > --- > drivers/mtd/spi-nor/core.c | 19 ------------------- > 1 file changed, 19 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 25a64c65717d..6de76fd009d1 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3517,25 +3517,6 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, > /* No mtd_info fields should be used up to this point. */ > spi_nor_set_mtd_info(nor); > > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, > - (long long)mtd->size >> 10); I'd lower this to dev_dbg() and print the jedec id. It might come in handy for a quick glance during bootup if debug is enabled. > - > - dev_dbg(dev, > - "mtd .name = %s, .size = 0x%llx (%lldMiB), " > - ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", > - mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), > - mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); > - > - if (mtd->numeraseregions) > - for (i = 0; i < mtd->numeraseregions; i++) > - dev_dbg(dev, > - "mtd.eraseregions[%d] = { .offset = 0x%llx, " > - ".erasesize = 0x%.8x (%uKiB), " > - ".numblocks = %d }\n", > - i, (long long)mtd->eraseregions[i].offset, > - mtd->eraseregions[i].erasesize, > - mtd->eraseregions[i].erasesize / 1024, > - mtd->eraseregions[i].numblocks); > return 0; Part of this is already available through the spi-nor debugfs, although not the actual mtd properties. These I think, should go into the mtdcore itself if really needed. Either through dev_dbg() or debugfs. -michael
Hello, michael@walle.cc wrote on Tue, 28 Nov 2023 09:47:28 +0100: > Hi, > > > The mtd data can be obtain with the mtd ioctls and the SPI NOR > > flash name can be determined interrogating the sysfs entries. > > Stop polluting the kernel log. > > > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > > > --- > > drivers/mtd/spi-nor/core.c | 19 ------------------- > > 1 file changed, 19 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 25a64c65717d..6de76fd009d1 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -3517,25 +3517,6 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, > > /* No mtd_info fields should be used up to this point. */ > > spi_nor_set_mtd_info(nor); > > > > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, > > - (long long)mtd->size >> 10); > > I'd lower this to dev_dbg() and print the jedec id. It might come in > handy for a quick glance during bootup if debug is enabled. Ack. Although, your boot time will almost be unaffected if you don't print the info messages to the console. What takes the most time is not writing to the kernel buffer, it's to display the lines on a serial console, and dev_info() are by default discarded, you need to select a lower log level manually, and if you do that it means you're not looking for quick boot times but rather more for additional information. > > - dev_dbg(dev, > > - "mtd .name = %s, .size = 0x%llx (%lldMiB), " > > - ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", > > - mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), > > - mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); > > - > > - if (mtd->numeraseregions) > > - for (i = 0; i < mtd->numeraseregions; i++) > > - dev_dbg(dev, > > - "mtd.eraseregions[%d] = { .offset = 0x%llx, " > > - ".erasesize = 0x%.8x (%uKiB), " > > - ".numblocks = %d }\n", > > - i, (long long)mtd->eraseregions[i].offset, > > - mtd->eraseregions[i].erasesize, > > - mtd->eraseregions[i].erasesize / 1024, > > - mtd->eraseregions[i].numblocks); > > return 0; > > Part of this is already available through the spi-nor debugfs, although not > the actual mtd properties. These I think, should go into the mtdcore > itself if really needed. Either through dev_dbg() or debugfs. Maybe we don't need this at all, as long as one message remains about the JEDEC ID, but keep in mind that spi-nors are commonly storing the rootfs and if your spi-nor does not boot you don't have a userspace yet and all the debugfs entries are purely useless. Thanks, Miquèl
Hi, >> > The mtd data can be obtain with the mtd ioctls and the SPI NOR >> > flash name can be determined interrogating the sysfs entries. >> > Stop polluting the kernel log. >> > >> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> > >> > --- >> > drivers/mtd/spi-nor/core.c | 19 ------------------- >> > 1 file changed, 19 deletions(-) >> > >> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> > index 25a64c65717d..6de76fd009d1 100644 >> > --- a/drivers/mtd/spi-nor/core.c >> > +++ b/drivers/mtd/spi-nor/core.c >> > @@ -3517,25 +3517,6 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, >> > /* No mtd_info fields should be used up to this point. */ >> > spi_nor_set_mtd_info(nor); >> > >> > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, >> > - (long long)mtd->size >> 10); >> >> I'd lower this to dev_dbg() and print the jedec id. It might come in >> handy for a quick glance during bootup if debug is enabled. > > Ack. Although, your boot time will almost be unaffected if you don't > print the info messages to the console. What takes the most time is not > writing to the kernel buffer, it's to display the lines on a serial > console, and dev_info() are by default discarded, you need to select a > lower log level manually, and if you do that it means you're not > looking for quick boot times but rather more for additional > information. Also with recent (or planned, dunno the current state) printk won't wait anymore for the slowest console. I really don't have a strong opinion, either dev_info(jedec_id) or dev_dbg(jedec_id). Whatever Tudor prefers. >> > - dev_dbg(dev, >> > - "mtd .name = %s, .size = 0x%llx (%lldMiB), " >> > - ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", >> > - mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), >> > - mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); >> > - >> > - if (mtd->numeraseregions) >> > - for (i = 0; i < mtd->numeraseregions; i++) >> > - dev_dbg(dev, >> > - "mtd.eraseregions[%d] = { .offset = 0x%llx, " >> > - ".erasesize = 0x%.8x (%uKiB), " >> > - ".numblocks = %d }\n", >> > - i, (long long)mtd->eraseregions[i].offset, >> > - mtd->eraseregions[i].erasesize, >> > - mtd->eraseregions[i].erasesize / 1024, >> > - mtd->eraseregions[i].numblocks); >> > return 0; >> >> Part of this is already available through the spi-nor debugfs, >> although not >> the actual mtd properties. These I think, should go into the mtdcore >> itself if really needed. Either through dev_dbg() or debugfs. > > Maybe we don't need this at all, as long as one message remains about > the JEDEC ID, but keep in mind that spi-nors are commonly storing the > rootfs and if your spi-nor does not boot you don't have a userspace yet > and all the debugfs entries are purely useless. Good point. Just curious, do you know any boards which has the rootfs writable on the spi-nor flash? -michael
Hi > > Hi, > > >> > The mtd data can be obtain with the mtd ioctls and the SPI NOR > >> > flash name can be determined interrogating the sysfs entries. > >> > Stop polluting the kernel log. > >> > > >> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >> > > >> > --- > >> > drivers/mtd/spi-nor/core.c | 19 ------------------- > >> > 1 file changed, 19 deletions(-) > >> > > >> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > >> > index 25a64c65717d..6de76fd009d1 100644 > >> > --- a/drivers/mtd/spi-nor/core.c > >> > +++ b/drivers/mtd/spi-nor/core.c > >> > @@ -3517,25 +3517,6 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, > >> > /* No mtd_info fields should be used up to this point. */ > >> > spi_nor_set_mtd_info(nor); > >> > > >> > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, > >> > - (long long)mtd->size >> 10); > >> > >> I'd lower this to dev_dbg() and print the jedec id. It might come in > >> handy for a quick glance during bootup if debug is enabled. > > > > Ack. Although, your boot time will almost be unaffected if you don't > > print the info messages to the console. What takes the most time is not > > writing to the kernel buffer, it's to display the lines on a serial > > console, and dev_info() are by default discarded, you need to select a > > lower log level manually, and if you do that it means you're not > > looking for quick boot times but rather more for additional > > information. > > Also with recent (or planned, dunno the current state) printk won't wait > anymore for the slowest console. I really don't have a strong opinion, > either dev_info(jedec_id) or dev_dbg(jedec_id). Whatever Tudor prefers. > > >> > - dev_dbg(dev, > >> > - "mtd .name = %s, .size = 0x%llx (%lldMiB), " > >> > - ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", > >> > - mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), > >> > - mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); > >> > - > >> > - if (mtd->numeraseregions) > >> > - for (i = 0; i < mtd->numeraseregions; i++) > >> > - dev_dbg(dev, > >> > - "mtd.eraseregions[%d] = { .offset = 0x%llx, " > >> > - ".erasesize = 0x%.8x (%uKiB), " > >> > - ".numblocks = %d }\n", > >> > - i, (long long)mtd->eraseregions[i].offset, > >> > - mtd->eraseregions[i].erasesize, > >> > - mtd->eraseregions[i].erasesize / 1024, > >> > - mtd->eraseregions[i].numblocks); > >> > return 0; > >> > >> Part of this is already available through the spi-nor debugfs, > >> although not > >> the actual mtd properties. These I think, should go into the mtdcore > >> itself if really needed. Either through dev_dbg() or debugfs. > > > > Maybe we don't need this at all, as long as one message remains about > > the JEDEC ID, but keep in mind that spi-nors are commonly storing the > > rootfs and if your spi-nor does not boot you don't have a userspace yet > > and all the debugfs entries are purely useless. > > Good point. > > Just curious, do you know any boards which has the rootfs writable on > the spi-nor flash? I am also interested. > > -michael > Thanks Jaime
cut >>> Maybe we don't need this at all, as long as one message remains about >>> the JEDEC ID, but keep in mind that spi-nors are commonly storing the >>> rootfs and if your spi-nor does not boot you don't have a userspace yet >>> and all the debugfs entries are purely useless. >> >> Good point. >> >> Just curious, do you know any boards which has the rootfs writable on >> the spi-nor flash? > I am also interested. > Having the rootfs stored on SPI NOR is a poor design decision as you're better of with a NAND, which is cheaper and faster on writes. I tried in the past a ubifs on top of a large (64 and 128MB) SPI NOR flash. But they were plug-able flashes, not something that is always tied to the board. Microchip's sama7g5ek comes with a 128MB macronix SPI NOR flash populated. But there are other vendors that provide large SPI NORs if really needed. Cheers, ta
On 11/28/23 09:39, Tudor Ambarus wrote: > > cut > >>>> Maybe we don't need this at all, as long as one message remains about >>>> the JEDEC ID, but keep in mind that spi-nors are commonly storing the >>>> rootfs and if your spi-nor does not boot you don't have a userspace yet >>>> and all the debugfs entries are purely useless. >>> >>> Good point. >>> >>> Just curious, do you know any boards which has the rootfs writable on >>> the spi-nor flash? >> I am also interested. >> > > Having the rootfs stored on SPI NOR is a poor design decision as you're > better of with a NAND, which is cheaper and faster on writes. I tried in > the past a ubifs on top of a large (64 and 128MB) SPI NOR flash. But > they were plug-able flashes, not something that is always tied to the > board. Microchip's sama7g5ek comes with a 128MB macronix SPI NOR flash > populated. But there are other vendors that provide large SPI NORs if > really needed. > pressed sent too soon :). What I wanted to say is that it's not uncommon for vendors to populate large SPI NOR flashes, there are others as well, thus Miquel's concerns are valid. There may be people out there having rootfs on top of SPI NORs. Cheers, ta
Hello, tudor.ambarus@linaro.org wrote on Tue, 28 Nov 2023 09:47:22 +0000: > On 11/28/23 09:39, Tudor Ambarus wrote: > > > > cut > > > >>>> Maybe we don't need this at all, as long as one message remains about > >>>> the JEDEC ID, but keep in mind that spi-nors are commonly storing the > >>>> rootfs and if your spi-nor does not boot you don't have a userspace yet > >>>> and all the debugfs entries are purely useless. > >>> > >>> Good point. > >>> > >>> Just curious, do you know any boards which has the rootfs writable on > >>> the spi-nor flash? > >> I am also interested. > >> > > > > Having the rootfs stored on SPI NOR is a poor design decision as you're > > better of with a NAND, which is cheaper and faster on writes. I tried in > > the past a ubifs on top of a large (64 and 128MB) SPI NOR flash. But > > they were plug-able flashes, not something that is always tied to the > > board. Microchip's sama7g5ek comes with a 128MB macronix SPI NOR flash > > populated. But there are other vendors that provide large SPI NORs if > > really needed. > > > > pressed sent too soon :). What I wanted to say is that it's not uncommon > for vendors to populate large SPI NOR flashes, there are others as well, > thus Miquel's concerns are valid. There may be people out there having > rootfs on top of SPI NORs. I don't think it is a too bad approach to store a rootfs on a spi-nor, as long as your "regularly written" data files are somewhere else (otherwise it's fine, but slow). It's actually nice to keep your boot files and rootfs in a "safe" place whereas your app is stored somewhere else, so users can mess around: they will not break the system. Thanks, Miquèl
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 25a64c65717d..6de76fd009d1 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3517,25 +3517,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, /* No mtd_info fields should be used up to this point. */ spi_nor_set_mtd_info(nor); - dev_info(dev, "%s (%lld Kbytes)\n", info->name, - (long long)mtd->size >> 10); - - dev_dbg(dev, - "mtd .name = %s, .size = 0x%llx (%lldMiB), " - ".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n", - mtd->name, (long long)mtd->size, (long long)(mtd->size >> 20), - mtd->erasesize, mtd->erasesize / 1024, mtd->numeraseregions); - - if (mtd->numeraseregions) - for (i = 0; i < mtd->numeraseregions; i++) - dev_dbg(dev, - "mtd.eraseregions[%d] = { .offset = 0x%llx, " - ".erasesize = 0x%.8x (%uKiB), " - ".numblocks = %d }\n", - i, (long long)mtd->eraseregions[i].offset, - mtd->eraseregions[i].erasesize, - mtd->eraseregions[i].erasesize / 1024, - mtd->eraseregions[i].numblocks); return 0; } EXPORT_SYMBOL_GPL(spi_nor_scan);
The mtd data can be obtain with the mtd ioctls and the SPI NOR flash name can be determined interrogating the sysfs entries. Stop polluting the kernel log. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/mtd/spi-nor/core.c | 19 ------------------- 1 file changed, 19 deletions(-)