[PATCH] mtd: spi-nor: stop printing superfluous debug info

[PATCH] mtd: spi-nor: stop printing superfluous debug info

[Tudor Ambarus]

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);
-
-	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);
-- 
2.43.0.rc1.413.gea7ed67945-goog


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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Miquel Raynal]

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

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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Tudor Ambarus]

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

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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Michael Walle]

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

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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Miquel Raynal]

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

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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Michael Walle]

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


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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[liao jaime]

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

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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Tudor Ambarus]

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

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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Tudor Ambarus]

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

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

Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info

[Miquel Raynal]

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

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