[PATCH v1 0/2] Hide flash name if it is not set

[PATCH v1 0/2] Hide flash name if it is not set

[Jaime Liao]

From: JaimeLiao <jaimeliao@mxic.com.tw>

hide flash name if it is not set

JaimeLiao (2):
  mtd: spi-nor:sysfs: hide flash name if it is not set
  mtd: spi-nor: hide flash name when the flash name does not exist

 Documentation/ABI/testing/sysfs-bus-spi-devices-spi-nor | 3 +++
 drivers/mtd/spi-nor/core.c                              | 2 +-
 drivers/mtd/spi-nor/sysfs.c                             | 2 ++
 3 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.25.1


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

[PATCH v1 1/2] mtd: spi-nor:sysfs: hide flash name if it is not set

[Jaime Liao]

From: JaimeLiao <jaimeliao@mxic.com.tw>

The info->name may be optional when flash ID founded in
ID table but didn't include flash name.

Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
---
 Documentation/ABI/testing/sysfs-bus-spi-devices-spi-nor | 3 +++
 drivers/mtd/spi-nor/sysfs.c                             | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-spi-devices-spi-nor b/Documentation/ABI/testing/sysfs-bus-spi-devices-spi-nor
index c800621eff95..6d7be97bf7d1 100644
--- a/Documentation/ABI/testing/sysfs-bus-spi-devices-spi-nor
+++ b/Documentation/ABI/testing/sysfs-bus-spi-devices-spi-nor
@@ -25,6 +25,9 @@ KernelVersion:	5.14
 Contact:	linux-mtd@lists.infradead.org
 Description:	(RO) Part name of the SPI NOR flash.
 
+		The attribute is not present if the jedec_id founded in
+		ID table but flash name didn't include in it.
+
 
 What:		/sys/bus/spi/devices/.../spi-nor/sfdp
 Date:		April 2021
diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
index 2dfdc555a69f..96064e4babf0 100644
--- a/drivers/mtd/spi-nor/sysfs.c
+++ b/drivers/mtd/spi-nor/sysfs.c
@@ -78,6 +78,8 @@ static umode_t spi_nor_sysfs_is_visible(struct kobject *kobj,
 
 	if (attr == &dev_attr_manufacturer.attr && !nor->manufacturer)
 		return 0;
+	if (attr == &dev_attr_partname.attr && !nor->info->name)
+		return 0;
 	if (attr == &dev_attr_jedec_id.attr && !nor->info->id && !nor->id)
 		return 0;
 
-- 
2.25.1


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

[PATCH v1 2/2] mtd: spi-nor: hide flash name when the flash name does not exist

[Jaime Liao]

From: JaimeLiao <jaimeliao@mxic.com.tw>

(null) will print when flash ID founded in ID table but
the flash name didn't include in it.

Make info->name optional in the print for showing flash name
and flash size.

Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
---
 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1c443fe568cf..73405bed2a5a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3518,7 +3518,7 @@ 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,
+	dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "",
 			(long long)mtd->size >> 10);
 
 	dev_dbg(dev,
-- 
2.25.1


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

Re: [PATCH v1 0/2] Hide flash name if it is not set

[Miquel Raynal]

Hi Jaime,

jaimeliao.tw@gmail.com wrote on Mon, 27 Nov 2023 18:07:52 +0800:

> From: JaimeLiao <jaimeliao@mxic.com.tw>
> 
> hide flash name if it is not set

Is this really needed? I'm fine exposing en empty (or "(null)") name. I
guess it shows to people that the name might be missing somewhere and
might push people to fix the name instead.

What's your use case?

Thanks,
Miquèl

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

Re: [PATCH v1 0/2] Hide flash name if it is not set

[Michael Walle]

Hi,

>> hide flash name if it is not set
> 
> Is this really needed? I'm fine exposing en empty (or "(null)") name. I
> guess it shows to people that the name might be missing somewhere and
> might push people to fix the name instead.
> 
> What's your use case?

An empty string is not a bug. I'm trying to push the part names out of
the kernel and just expose the jedec id.

That being said, the description is really missing the point. See my
previous suggestion:

  The attribute is optional. User space shouldn't rely on it to
  be present or even correct. Instead, user space should read the
  jedec_id attribute.

-michael

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

Re: [PATCH v1 0/2] Hide flash name if it is not set

[Tudor Ambarus]

On 11/27/23 11:58, Michael Walle wrote:
> Hi,
> 
>>> hide flash name if it is not set
>>
>> Is this really needed? I'm fine exposing en empty (or "(null)") name. I
>> guess it shows to people that the name might be missing somewhere and
>> might push people to fix the name instead.
>>
>> What's your use case?
> 
> An empty string is not a bug. I'm trying to push the part names out of
> the kernel and just expose the jedec id.
> 
> That being said, the description is really missing the point. See my
> previous suggestion:
> 
>  The attribute is optional. User space shouldn't rely on it to
>  be present or even correct. Instead, user space should read the
>  jedec_id attribute.
> 

+1

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

Re: [PATCH v1 2/2] mtd: spi-nor: hide flash name when the flash name does not exist

[Michael Walle]

Hi,

> (null) will print when flash ID founded in ID table but
> the flash name didn't include in it.
> 
> Make info->name optional in the print for showing flash name
> and flash size.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1c443fe568cf..73405bed2a5a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3518,7 +3518,7 @@ 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,
> +	dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "",

Maybe print the jedec id if the name is empty.

-michael

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

Re: [PATCH v1 2/2] mtd: spi-nor: hide flash name when the flash name does not exist

[Miquel Raynal]

Hi Michael,

michael@walle.cc wrote on Mon, 27 Nov 2023 13:21:34 +0100:

> Hi,
> 
> > (null) will print when flash ID founded in ID table but
> > the flash name didn't include in it.
> > 
> > Make info->name optional in the print for showing flash name
> > and flash size.
> > 
> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 1c443fe568cf..73405bed2a5a 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -3518,7 +3518,7 @@ 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,
> > +	dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "",  
> 
> Maybe print the jedec id if the name is empty.

What about always printing the jedec id if names aren't reliable
enough?

Thanks,
Miquèl

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

Re: [PATCH v1 2/2] mtd: spi-nor: hide flash name when the flash name does not exist

[Michael Walle]

Hi,

>> > (null) will print when flash ID founded in ID table but
>> > the flash name didn't include in it.
>> >
>> > Make info->name optional in the print for showing flash name
>> > and flash size.
>> >
>> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>> > ---
>> >  drivers/mtd/spi-nor/core.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> > index 1c443fe568cf..73405bed2a5a 100644
>> > --- a/drivers/mtd/spi-nor/core.c
>> > +++ b/drivers/mtd/spi-nor/core.c
>> > @@ -3518,7 +3518,7 @@ 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,
>> > +	dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "",
>> 
>> Maybe print the jedec id if the name is empty.
> 
> What about always printing the jedec id if names aren't reliable
> enough?

I'm fine with that, too. Actually I've considered that myself, but
maybe Tudor or Pratyush want to keep the output backwards compatible.

-michael

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

Re: [PATCH v1 2/2] mtd: spi-nor: hide flash name when the flash name does not exist

[Tudor Ambarus]

On 11/27/23 14:56, Michael Walle wrote:
> Hi,
> 
>>> > (null) will print when flash ID founded in ID table but
>>> > the flash name didn't include in it.
>>> >
>>> > Make info->name optional in the print for showing flash name
>>> > and flash size.
>>> >
>>> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>>> > ---
>>> >  drivers/mtd/spi-nor/core.c | 2 +-
>>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> > index 1c443fe568cf..73405bed2a5a 100644
>>> > --- a/drivers/mtd/spi-nor/core.c
>>> > +++ b/drivers/mtd/spi-nor/core.c
>>> > @@ -3518,7 +3518,7 @@ 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,
>>> > +    dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "",
>>>
>>> Maybe print the jedec id if the name is empty.
>>
>> What about always printing the jedec id if names aren't reliable
>> enough?
> 
> I'm fine with that, too. Actually I've considered that myself, but
> maybe Tudor or Pratyush want to keep the output backwards compatible.
> 

We won't remove the names for flashes that already have a name defined,
won't we? We just deprecate the name field and not use it anymore with
new flash additions. No backward compatibility problem.

BTW, that print should be lowered to dev_dbg, let's no longer pollute
the kernel log. Drivers should be quiet if all goes well.


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

Re: [PATCH v1 2/2] mtd: spi-nor: hide flash name when the flash name does not exist

[Michael Walle]

Hi,

>>>> > (null) will print when flash ID founded in ID table but
>>>> > the flash name didn't include in it.
>>>> >
>>>> > Make info->name optional in the print for showing flash name
>>>> > and flash size.
>>>> >
>>>> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>>>> > ---
>>>> >  drivers/mtd/spi-nor/core.c | 2 +-
>>>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>>> >
>>>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> > index 1c443fe568cf..73405bed2a5a 100644
>>>> > --- a/drivers/mtd/spi-nor/core.c
>>>> > +++ b/drivers/mtd/spi-nor/core.c
>>>> > @@ -3518,7 +3518,7 @@ 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,
>>>> > +    dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "",
>>>> 
>>>> Maybe print the jedec id if the name is empty.
>>> 
>>> What about always printing the jedec id if names aren't reliable
>>> enough?
>> 
>> I'm fine with that, too. Actually I've considered that myself, but
>> maybe Tudor or Pratyush want to keep the output backwards compatible.
>> 
> 
> We won't remove the names for flashes that already have a name defined,
> won't we?

No.

> We just deprecate the name field and not use it anymore with
> new flash additions. No backward compatibility problem.

I mean in the kernel console output. Someone out there might parse it.

If you switch from
   dev_info(dev, "%s (%lld Kbytes)\n", info->name, ...)
to
   dev_info(dev, ""%*phN (%lld Kbytes)\n", id_len, id, ...)

> BTW, that print should be lowered to dev_dbg, let's no longer pollute
> the kernel log. Drivers should be quiet if all goes well.

Well, ok, that will also answer my question :) So let's go
with the dev_dbg() and id only.

-michael

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

Re: [PATCH v1 2/2] mtd: spi-nor: hide flash name when the flash name does not exist

[Tudor Ambarus]

On 11/27/23 15:28, Michael Walle wrote:
> Hi,
> 
>>>>> > (null) will print when flash ID founded in ID table but
>>>>> > the flash name didn't include in it.
>>>>> >
>>>>> > Make info->name optional in the print for showing flash name
>>>>> > and flash size.
>>>>> >
>>>>> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>>>>> > ---
>>>>> >  drivers/mtd/spi-nor/core.c | 2 +-
>>>>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> >
>>>>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>> > index 1c443fe568cf..73405bed2a5a 100644
>>>>> > --- a/drivers/mtd/spi-nor/core.c
>>>>> > +++ b/drivers/mtd/spi-nor/core.c
>>>>> > @@ -3518,7 +3518,7 @@ 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,
>>>>> > +    dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "",
>>>>>
>>>>> Maybe print the jedec id if the name is empty.
>>>>
>>>> What about always printing the jedec id if names aren't reliable
>>>> enough?
>>>
>>> I'm fine with that, too. Actually I've considered that myself, but
>>> maybe Tudor or Pratyush want to keep the output backwards compatible.
>>>
>>
>> We won't remove the names for flashes that already have a name defined,
>> won't we?
> 
> No.
> 
>> We just deprecate the name field and not use it anymore with
>> new flash additions. No backward compatibility problem.
> 
> I mean in the kernel console output. Someone out there might parse it.
> 
> If you switch from
>   dev_info(dev, "%s (%lld Kbytes)\n", info->name, ...)
> to
>   dev_info(dev, ""%*phN (%lld Kbytes)\n", id_len, id, ...)
> 

no problem in changing what we print to kernel log, there's no
guarantees associated with it.

>> BTW, that print should be lowered to dev_dbg, let's no longer pollute
>> the kernel log. Drivers should be quiet if all goes well.
> 
> Well, ok, that will also answer my question :) So let's go
> with the dev_dbg() and id only.
> 

I looked again and I saw that together with the name we printed the
flash size in Kbytes. But then the next print printed the same flash
size in bytes and then in MBytes. And then we printed some mtd data that
should be obtained with the mtd ioctls. I didn't like that mess and
ended up removing all those prints. We shall now have a faster kernel
boot (got rid of the print info), and cleaner kernel log. All that debug
data shall be obtained with the mtd ioctls and the SPI NOR sysfs and
debugfs entries, so hopefully more people will start using them.

Here's the patch that I mentioned:
https://lore.kernel.org/linux-mtd/20231127165908.1734951-1-tudor.ambarus@linaro.org/T/#u

Cheers,
ta

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