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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFv23QmjV9RAtvZJT2=8BPKZn+LsZbLG45t7fr2Z7Nq2C2nH0A@mail.gmail.com>
Date:   Mon, 23 Oct 2023 10:34:34 +0800
From:   AceLan Kao <acelan.kao@...onical.com>
To:     Pratyush Yadav <pratyush@...nel.org>
Cc:     Tudor Ambarus <tudor.ambarus@...aro.org>,
        Michael Walle <michael@...le.cc>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd: spi-nor: Lower the priority of the software reset
 failure message

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