[PATCH] mtd: spi-nor: Lower the priority of the software reset failure message

[PATCH] mtd: spi-nor: Lower the priority of the software reset failure message

[AceLan Kao]

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

Not all SPI drivers support soft reset enable and soft reset commands.
This failure is expected and not critical. Thus, we avoid reporting it
to regular users to prevent potential confusion regarding power-off issues.

Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
---
 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 1b0c6770c14e..7bca8ffcd756 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3252,7 +3252,7 @@ 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);
+		dev_info(nor->dev, "Software reset failed: %d\n", ret);
 		return;
 	}
 
@@ -3262,7 +3262,7 @@ 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);
+		dev_info(nor->dev, "Software reset failed: %d\n", ret);
 		return;
 	}
 
-- 
2.34.1

Re: [PATCH] mtd: spi-nor: Lower the priority of the software reset failure message

[Michael Walle]

Hi,

> Not all SPI drivers support soft reset enable and soft reset commands.
> This failure is expected and not critical.

This is not really expected. What driver is this? Let me guess, the 
intel
SPI driver.

Please mention this in the commit message.

> Thus, we avoid reporting it
> to regular users to prevent potential confusion regarding power-off 
> issues.
> 
> Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> ---
>  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 1b0c6770c14e..7bca8ffcd756 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3252,7 +3252,7 @@ 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);
> +		dev_info(nor->dev, "Software reset failed: %d\n", ret);

What is the value of ret here? Ideally it should be -EOPNOTSUPP and then
don't print this message at all. Otherwise leave it at dev_warn(). Also,
please add a comment here.

-michael

Re: [PATCH] mtd: spi-nor: Lower the priority of the software reset failure message

[Pratyush Yadav]

On Thu, Oct 19 2023, AceLan Kao wrote:

> From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
>
> Not all SPI drivers support soft reset enable and soft reset commands.
> This failure is expected and not critical. Thus, we avoid reporting it
> to regular users to prevent potential confusion regarding power-off issues.

No, failure to soft reset can very much be critical in certain cases.
For example, if you are operating the flash in 8D-8D-8D mode and do not
have the hard reset line connected, the bootloader (or the kernel) would
be unable to detect or operate the flash after a warm reboot.

Perhaps it makes sense to just call it when SNOR_F_BROKEN_RESET is set?
This way you do not unnecessarily call it when you do not need to, and
won't see the error message.

>
> Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> ---
>  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 1b0c6770c14e..7bca8ffcd756 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3252,7 +3252,7 @@ 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);
> +		dev_info(nor->dev, "Software reset failed: %d\n", ret);
>  		return;
>  	}
>  
> @@ -3262,7 +3262,7 @@ 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);
> +		dev_info(nor->dev, "Software reset failed: %d\n", ret);
>  		return;
>  	}

-- 
Regards,
Pratyush Yadav

Re: [PATCH] mtd: spi-nor: Lower the priority of the software reset failure message

[AceLan Kao]

Pratyush Yadav <pratyush@kernel.org> 於 2023年10月19日 週四 下午8:52寫道：
>
> On Thu, Oct 19 2023, AceLan Kao wrote:
>
> > From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
> >
> > Not all SPI drivers support soft reset enable and soft reset commands.
> > This failure is expected and not critical. Thus, we avoid reporting it
> > to regular users to prevent potential confusion regarding power-off issues.
Hi Pratyush,
>
> No, failure to soft reset can very much be critical in certain cases.
> For example, if you are operating the flash in 8D-8D-8D mode and do not
> have the hard reset line connected, the bootloader (or the kernel) would
> be unable to detect or operate the flash after a warm reboot.
>
> Perhaps it makes sense to just call it when SNOR_F_BROKEN_RESET is set?
> This way you do not unnecessarily call it when you do not need to, and
> won't see the error message.
The issue I found was on a x86 desktop, and I can find many other same
bug reports talked about the spi-nor reset failure.

The issue is from spi-intel driver that doesn't accept the reset
command and return false when calls its supports function
spi_nor_soft_reset() -> spi_mem_exec_op() ->
spi_mem_internal_supports_op() -> ctlr->mem_ops->supports_op() ->
intel_spi_supports_mem_op() return false
And  from spi_mem_exec_op(), it returns -ENOTSUPP.

So, do you think it's better that we distinguish -ENOTSUPP and other
failures in spi_nor_soft_reset() likes

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

>
> >
> > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> > ---
> >  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 1b0c6770c14e..7bca8ffcd756 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -3252,7 +3252,7 @@ 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);
> > +             dev_info(nor->dev, "Software reset failed: %d\n", ret);
> >               return;
> >       }
> >
> > @@ -3262,7 +3262,7 @@ 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);
> > +             dev_info(nor->dev, "Software reset failed: %d\n", ret);
> >               return;
> >       }
>
> --
> Regards,
> Pratyush Yadav