[PATCH 0/4] mtd: spi-nor: mark the flash name as obsolete

[PATCH 0/4] mtd: spi-nor: mark the flash name as obsolete

[Tudor Ambarus]

The flash name is unreliable as we saw flash ID collisions. Mark the
flash name as obsolete and print the manufacturer and device ID where
name was printed.

JaimeLiao (1):
  mtd: spi-nor: sysfs: hide the flash name if not set

Tudor Ambarus (3):
  mtd: spi-nor: print flash ID instead of name
  mtd: spi-nor: mark the flash name as obsolete
  mtd: spi-nor: drop superfluous debug prints

 .../ABI/testing/sysfs-bus-spi-devices-spi-nor |  3 +++
 drivers/mtd/spi-nor/core.c                    | 22 +++----------------
 drivers/mtd/spi-nor/core.h                    |  2 +-
 drivers/mtd/spi-nor/sysfs.c                   |  2 ++
 4 files changed, 9 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/4] mtd: spi-nor: print flash ID instead of name

[Tudor Ambarus]

We saw flash ID collisions which make the flash name unreliable. Print
the manufacgturer and device ID instead of the flash name.

Lower the print to dev_dbg to stop polluting the kernel log.

Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/mtd/spi-nor/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 503fed90c2fa..ca5bd93d1f17 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3558,8 +3558,8 @@ 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, "Manufacturer and device ID: %*phN\n",
+		SPI_NOR_MAX_ID_LEN, nor->id);
 
 	dev_dbg(dev,
 		"mtd .name = %s, .size = 0x%llx (%lldMiB), "
-- 
2.34.1

Re: [PATCH 1/4] mtd: spi-nor: print flash ID instead of name

[Michael Walle]

Am 2023-12-15 09:21, schrieb Tudor Ambarus:
> We saw flash ID collisions which make the flash name unreliable. Print
> the manufacgturer and device ID instead of the flash name.
> 
> Lower the print to dev_dbg to stop polluting the kernel log.
> 
> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Reviewed-by: Michael Walle <michael@walle.cc>

Re: [PATCH 1/4] mtd: spi-nor: print flash ID instead of name

[Pratyush Yadav]

On Fri, Dec 15 2023, Tudor Ambarus wrote:

> We saw flash ID collisions which make the flash name unreliable. Print
> the manufacgturer and device ID instead of the flash name.

Typo. s/manufacgturer/manufacturer/

>
> Lower the print to dev_dbg to stop polluting the kernel log.

FWIW, I find these prints to be somewhat useful. they serve to
"announce" that the kernel probed a device successfully. This can be
somewhat helpful when reading logs from customers trying to figure out
why something doesn't work.

I don't have any strong opinions of course since it is not such a big
deal but I wanted to point out that these prints _are_ somewhat useful.

Acked-by: Pratyush Yadav <pratyush@kernel.org>

> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

-- 
Regards,
Pratyush Yadav

[PATCH 2/4] mtd: spi-nor: mark the flash name as obsolete

[Tudor Ambarus]

The flash name is unreliable as we saw flash ID collisions. Mark the
name as obsolete.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/mtd/spi-nor/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 29ed67725b18..d36c0e072954 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -465,7 +465,7 @@ struct spi_nor_id {
  * struct flash_info - SPI NOR flash_info entry.
  * @id:   pointer to struct spi_nor_id or NULL, which means "no ID" (mostly
  *        older chips).
- * @name: the name of the flash.
+ * @name: (obsolete) the name of the flash. Do not set it for new additions.
  * @size:           the size of the flash in bytes.
  * @sector_size:    (optional) the size listed here is what works with
  *                  SPINOR_OP_SE, which isn't necessarily called a "sector" by
-- 
2.34.1

Re: [PATCH 2/4] mtd: spi-nor: mark the flash name as obsolete

[Michael Walle]

Am 2023-12-15 09:21, schrieb Tudor Ambarus:
> The flash name is unreliable as we saw flash ID collisions. Mark the
> name as obsolete.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Reviewed-by: Michael Walle <michael@walle.cc>

Re: [PATCH 2/4] mtd: spi-nor: mark the flash name as obsolete

[Pratyush Yadav]

On Fri, Dec 15 2023, Tudor Ambarus wrote:

> The flash name is unreliable as we saw flash ID collisions. Mark the
> name as obsolete.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

-- 
Regards,
Pratyush Yadav

[PATCH 3/4] mtd: spi-nor: sysfs: hide the flash name if not set

[Tudor Ambarus]

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

The flash name is not reliable as we saw flash ID collisions.
Hide the flash name if not set.

Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
[ta: update commit subject and description]
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 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.34.1

Re: [PATCH 3/4] mtd: spi-nor: sysfs: hide the flash name if not set

[Michael Walle]

Am 2023-12-15 09:21, schrieb Tudor Ambarus:
> From: JaimeLiao <jaimeliao@mxic.com.tw>
> 
> The flash name is not reliable as we saw flash ID collisions.
> Hide the flash name if not set.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> [ta: update commit subject and description]
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Great!

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

Re: [PATCH 3/4] mtd: spi-nor: sysfs: hide the flash name if not set

[Pratyush Yadav]

On Fri, Dec 15 2023, Tudor Ambarus wrote:

> From: JaimeLiao <jaimeliao@mxic.com.tw>
>
> The flash name is not reliable as we saw flash ID collisions.
> Hide the flash name if not set.
>
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> [ta: update commit subject and description]
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  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.

Nitpick: strange sentence construction makes it a bit confusing.
Perhaps:

    The attribute is not present if the JEDEC ID table for the flash
    does not include its name.

> +
>  
>  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;

Do you want to show the name for the "spi-nor-generic" flash? If not
then perhaps drop its name as well?

>  	if (attr == &dev_attr_jedec_id.attr && !nor->info->id && !nor->id)
>  		return 0;

-- 
Regards,
Pratyush Yadav

[PATCH 4/4] mtd: spi-nor: drop superfluous debug prints

[Tudor Ambarus]

The mtd data hall be obtained with the mtd ioctls or with new debugs
entries if one cares. Drop the debug prints.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/mtd/spi-nor/core.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ca5bd93d1f17..a708c3448809 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3561,22 +3561,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	dev_dbg(dev, "Manufacturer and device ID: %*phN\n",
 		SPI_NOR_MAX_ID_LEN, nor->id);
 
-	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.34.1

Re: [PATCH 4/4] mtd: spi-nor: drop superfluous debug prints

[Tudor Ambarus]

I missed to drop some unused variables. Will drop them if everything
else is fine.

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index a708c3448809..92c992eb73d5 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3492,9 +3492,7 @@ int spi_nor_scan(struct spi_nor *nor, const char
*name,
 {
        const struct flash_info *info;
        struct device *dev = nor->dev;
-       struct mtd_info *mtd = &nor->mtd;
        int ret;
-       int i;

        ret = spi_nor_check(nor);
        if (ret)

Re: [PATCH 4/4] mtd: spi-nor: drop superfluous debug prints

[Pratyush Yadav]

On Fri, Dec 15 2023, Tudor Ambarus wrote:

> I missed to drop some unused variables. Will drop them if everything
> else is fine.
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index a708c3448809..92c992eb73d5 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3492,9 +3492,7 @@ int spi_nor_scan(struct spi_nor *nor, const char
> *name,
>  {
>         const struct flash_info *info;
>         struct device *dev = nor->dev;
> -       struct mtd_info *mtd = &nor->mtd;
>         int ret;
> -       int i;
>
>         ret = spi_nor_check(nor);
>         if (ret)


With these, 

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

-- 
Regards,
Pratyush Yadav

Re: [PATCH 4/4] mtd: spi-nor: drop superfluous debug prints

[Tudor Ambarus]

On 1/29/24 13:26, Pratyush Yadav wrote:
> On Fri, Dec 15 2023, Tudor Ambarus wrote:
> 
>> I missed to drop some unused variables. Will drop them if everything
>> else is fine.
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index a708c3448809..92c992eb73d5 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3492,9 +3492,7 @@ int spi_nor_scan(struct spi_nor *nor, const char
>> *name,
>>  {
>>         const struct flash_info *info;
>>         struct device *dev = nor->dev;
>> -       struct mtd_info *mtd = &nor->mtd;
>>         int ret;
>> -       int i;
>>
>>         ret = spi_nor_check(nor);
>>         if (ret)
> 
> 
> With these, 
> 
> Reviewed-by: Pratyush Yadav <pratyush@kernel.org>
> 
Hi, Pratyush,

All in these series were already applied, as I specified in a reply.
Please check patchwork for patches that are not yet handled.

Cheers,
ta

Re: [PATCH 4/4] mtd: spi-nor: drop superfluous debug prints

[Pratyush Yadav]

On Mon, Jan 29 2024, Tudor Ambarus wrote:

> On 1/29/24 13:26, Pratyush Yadav wrote:
>> On Fri, Dec 15 2023, Tudor Ambarus wrote:
>> 
>>> I missed to drop some unused variables. Will drop them if everything
>>> else is fine.
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index a708c3448809..92c992eb73d5 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -3492,9 +3492,7 @@ int spi_nor_scan(struct spi_nor *nor, const char
>>> *name,
>>>  {
>>>         const struct flash_info *info;
>>>         struct device *dev = nor->dev;
>>> -       struct mtd_info *mtd = &nor->mtd;
>>>         int ret;
>>> -       int i;
>>>
>>>         ret = spi_nor_check(nor);
>>>         if (ret)
>> 
>> 
>> With these, 
>> 
>> Reviewed-by: Pratyush Yadav <pratyush@kernel.org>
>> 
> Hi, Pratyush,
>
> All in these series were already applied, as I specified in a reply.
> Please check patchwork for patches that are not yet handled.

Ah sorry, I didn't see that. I was browsing through my email backlog and
these patches just caught my eye.

Will look at the newer ones.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 4/4] mtd: spi-nor: drop superfluous debug prints

[Michael Walle]

Am 2023-12-15 09:21, schrieb Tudor Ambarus:
> The mtd data hall be obtained with the mtd ioctls or with new debugs

shall, debugfs

> entries if one cares. Drop the debug prints.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>

With the above and the unused variables fixed:

Reviewed-by: Michael Walle <michael@walle.cc>

Miquel raised a valid point that if the rootfs is on this very
flash and if there is an error, you cannot use debugfs or the
ioctls. But I doubt that the info about the erase regions will
help much in that case.

-michael

Re: [PATCH 0/4] mtd: spi-nor: mark the flash name as obsolete

[Tudor Ambarus]

On Fri, 15 Dec 2023 10:21:34 +0200, Tudor Ambarus wrote:
> The flash name is unreliable as we saw flash ID collisions. Mark the
> flash name as obsolete and print the manufacturer and device ID where
> name was printed.
> 
> JaimeLiao (1):
>   mtd: spi-nor: sysfs: hide the flash name if not set
> 
> [...]

Dropped unused variables, fixed typos and
applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
spi-nor/next branch. Thanks!

[1/4] mtd: spi-nor: print flash ID instead of name
      https://git.kernel.org/mtd/c/9fcb0999345e
[2/4] mtd: spi-nor: mark the flash name as obsolete
      https://git.kernel.org/mtd/c/15eb8303bb42
[3/4] mtd: spi-nor: sysfs: hide the flash name if not set
      https://git.kernel.org/mtd/c/62f92e62c1ef
[4/4] mtd: spi-nor: drop superfluous debug prints
      https://git.kernel.org/mtd/c/7bf018ea5cb6

Cheers,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>