linux-kernel - Re: [PATCH v3] 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: <6B9232E3-7159-4D97-9B40-EE77E2F6EA5B@walle.cc>
Date:   Wed, 25 Oct 2023 06:25:48 +0200
From:   Michael Walle <michael@...le.cc>
To:     AceLan Kao <acelan.kao@...onical.com>,
        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>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] mtd: spi-nor: Improve reporting for software reset failures

>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
>v3. replace ENOTSUPP with EOPNOTSUPP and check the first command only
>
>Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@...onical.com>
>---
> drivers/mtd/spi-nor/core.c | 5 ++++-
> drivers/spi/spi-mem.c      | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>index 1b0c6770c14e..42e52af76289 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 == -EOPNOTSUPP)
>+			dev_info(nor->dev, "Software reset enable command doesn't support: %d\n", ret);

As mentioned in the previous version, this doesn't add any useful information. Please just drop it. That is, just guard the current dev_warn() with "if ret! = - EOPNOTSUPP".

>+		else
>+			dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> 		return;
> 	}
> 
>diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>index edd7430d4c05..93b77ac0b798 100644
>--- a/drivers/spi/spi-mem.c
>+++ b/drivers/spi/spi-mem.c
>@@ -323,7 +323,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> 		return ret;
> 
> 	if (!spi_mem_internal_supports_op(mem, op))
>-		return -ENOTSUPP;
>+		return -EOPNOTSUPP;

This should be an own patch and you'll have to fix current users of it. For example, there is at least one in spi-nor core. Probably more. 

-michael

> 
> 	if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) {
> 		ret = spi_mem_access_start(mem);