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

[AceLan Kao]

Pratyush Yadav <pratyush@xxxxxxxxxx> 於 2023年10月19日 週四 下午8:52寫道：
>
> On Thu, Oct 19 2023, AceLan Kao wrote:
>
> > From: "Chia-Lin Kao (AceLan)" <acelan.kao@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > ---
> >  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