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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <4c11d06931f9b4e29c0e8feafc18e763@walle.cc>
Date:   Tue, 24 Oct 2023 10:59:50 +0200
From:   Michael Walle <michael@...le.cc>
To:     AceLan Kao <acelan.kao@...onical.com>
Cc:     Tudor Ambarus <tudor.ambarus@...aro.org>,
        Pratyush Yadav <pratyush@...nel.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        mika.westerberg@...ux.intel.com
Subject: Re: [PATCH v2] mtd: spi-nor: Improve reporting for software reset
 failures

[+Mika]


> From: "Chia-Lin Kao (AceLan)" <acelan.kao@...onical.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@...onical.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;
>  	}