[PATCH v2] mtd: spi-nor: Improve reporting for software reset failures

[PATCH v2] mtd: spi-nor: Improve reporting for software reset failures

[AceLan Kao]

From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>

When the software reset command isn't supported, we now report it as an
informational message(dev_info) instead of a warning(dev_warn). 
This adjustment helps avoid unnecessary alarm and confusion regarding
software reset capabilities.

v2. only lower the priority for the not supported failure

Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
---
 drivers/mtd/spi-nor/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1b0c6770c14e..76920dbc568b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3252,7 +3252,10 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
 
 	ret = spi_mem_exec_op(nor->spimem, &op);
 	if (ret) {
-		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+		if (ret == -ENOTSUPP)
+			dev_info(nor->dev, "Software reset enable command doesn't support: %d\n", ret);
+		else
+			dev_warn(nor->dev, "Software reset failed: %d\n", ret);
 		return;
 	}
 
@@ -3262,7 +3265,10 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
 
 	ret = spi_mem_exec_op(nor->spimem, &op);
 	if (ret) {
-		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+		if (ret == -ENOTSUPP)
+			dev_info(nor->dev, "Software reset command doesn't support: %d\n", ret);
+		else
+			dev_warn(nor->dev, "Software reset failed: %d\n", ret);
 		return;
 	}
 
-- 
2.34.1

Re: [PATCH v2] mtd: spi-nor: Improve reporting for software reset failures

[Michael Walle]

[+Mika]


> From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
> 
> When the software reset command isn't supported, we now report it as an
> informational message(dev_info) instead of a warning(dev_warn).
> This adjustment helps avoid unnecessary alarm and confusion regarding
> software reset capabilities.
> 
> v2. only lower the priority for the not supported failure
> 
> Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> ---
>  drivers/mtd/spi-nor/core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e..76920dbc568b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3252,7 +3252,10 @@ static void spi_nor_soft_reset(struct spi_nor 
> *nor)
> 
>  	ret = spi_mem_exec_op(nor->spimem, &op);
>  	if (ret) {
> -		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> +		if (ret == -ENOTSUPP)

It bothers me that we use ENOTSUPP here. We should really use 
EOPNOTSUPP.
The core uses EOPNOTSUPP everywhere except for the intel things.

Please have a look at changing that to EOPNOTSUPP. See also:
https://lore.kernel.org/linux-mtd/85f9c462-c155-dc17-dc97-3254acfa55d2@microchip.com/


> +			dev_info(nor->dev, "Software reset enable command doesn't support: 
> %d\n", ret);

I'm not sure this is helpful. It's only the intel SPI controller which
does magic things (instead of just issuing our commands). Mika, do you
know wether your controller will do a reset on it's own? I presume so,
because AFAIR you have some kind of high level controller which also 
does
SFDP parsing and read opcode handling on their own.

> +		else
> +			dev_warn(nor->dev, "Software reset failed: %d\n", ret);
>  		return;
>  	}
> 
> @@ -3262,7 +3265,10 @@ static void spi_nor_soft_reset(struct spi_nor 
> *nor)
> 
>  	ret = spi_mem_exec_op(nor->spimem, &op);
>  	if (ret) {
> -		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> +		if (ret == -ENOTSUPP)
> +			dev_info(nor->dev, "Software reset command doesn't support: %d\n", 
> ret);

I'd leave that as is, because how are the chances that the first one is
supported and the second command, isn't?
When working with the intel controller, we'll return early after the
first spi_mem_exec_op().

-michael

> +		else
> +			dev_warn(nor->dev, "Software reset failed: %d\n", ret);
>  		return;
>  	}

Re: [PATCH v2] mtd: spi-nor: Improve reporting for software reset failures

[Mika Westerberg]

Hi,

On Tue, Oct 24, 2023 at 10:59:50AM +0200, Michael Walle wrote:
> [+Mika]
> 
> 
> > From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
> > 
> > When the software reset command isn't supported, we now report it as an
> > informational message(dev_info) instead of a warning(dev_warn).
> > This adjustment helps avoid unnecessary alarm and confusion regarding
> > software reset capabilities.
> > 
> > v2. only lower the priority for the not supported failure
> > 
> > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> > ---
> >  drivers/mtd/spi-nor/core.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 1b0c6770c14e..76920dbc568b 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -3252,7 +3252,10 @@ static void spi_nor_soft_reset(struct spi_nor
> > *nor)
> > 
> >  	ret = spi_mem_exec_op(nor->spimem, &op);
> >  	if (ret) {
> > -		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> > +		if (ret == -ENOTSUPP)
> 
> It bothers me that we use ENOTSUPP here. We should really use EOPNOTSUPP.
> The core uses EOPNOTSUPP everywhere except for the intel things.
> 
> Please have a look at changing that to EOPNOTSUPP. See also:
> https://lore.kernel.org/linux-mtd/85f9c462-c155-dc17-dc97-3254acfa55d2@microchip.com/

Makes sense.

> > +			dev_info(nor->dev, "Software reset enable command doesn't support:
> > %d\n", ret);
> 
> I'm not sure this is helpful. It's only the intel SPI controller which
> does magic things (instead of just issuing our commands). Mika, do you
> know wether your controller will do a reset on it's own? I presume so,
> because AFAIR you have some kind of high level controller which also does
> SFDP parsing and read opcode handling on their own.

Yes, that's right.