Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231019064547.348446-1-acelan.kao@canonical.com> <mafs07cnin59q.fsf@kernel.org>
In-Reply-To: <mafs07cnin59q.fsf@kernel.org>
From:   AceLan Kao <acelan.kao@canonical.com>
Date:   Mon, 23 Oct 2023 10:34:34 +0800
Message-ID: <CAFv23QmjV9RAtvZJT2=8BPKZn+LsZbLG45t7fr2Z7Nq2C2nH0A@mail.gmail.com>
Subject: Re: [PATCH] mtd: spi-nor: Lower the priority of the software reset
 failure message
To:     Pratyush Yadav <pratyush@kernel.org>
Cc:     Tudor Ambarus <tudor.ambarus@linaro.org>,
        Michael Walle <michael@walle.cc>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Pratyush Yadav <pratyush@kernel.org> =E6=96=BC 2023=E5=B9=B410=E6=9C=8819=
=E6=97=A5 =E9=80=B1=E5=9B=9B =E4=B8=8B=E5=8D=888:52=E5=AF=AB=E9=81=93=EF=BC=
=9A
>
> 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 iss=
ues.
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 =3D spi_mem_exec_op(nor->spimem, &op);
        if (ret) {
-               dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+               if (ret =3D=3D -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", r=
et);
                return;
        }

@@ -3262,7 +3265,10 @@ static void spi_nor_soft_reset(struct spi_nor *nor)

        ret =3D spi_mem_exec_op(nor->spimem, &op);
        if (ret) {
-               dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+               if (ret =3D=3D -ENOTSUPP)
+                       dev_info(nor->dev, "Software reset command
doesn't support: %d\n", ret);
+               else
+                       dev_warn(nor->dev, "Software reset failed: %d\n", r=
et);
                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 *no=
r)
> >
> >       ret =3D 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 *no=
r)
> >
> >       ret =3D 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