[PATCH] mtd: spi-nor: core: Fix 16bit write sr_and_check status check

[PATCH] mtd: spi-nor: core: Fix 16bit write sr_and_check status check

[Nathan Rossi]

From: Nathan Rossi <nathan.rossi@digi.com>

The spi_nor_write_16bit_sr_and_check function description describes that
the function compares the value of the status and config registers after
the write. However the function does not implement the status register
compare only the config register check.

This causes the function to differ in behaviour to the equivalent
spi_nor_write_sr1_and_check for non-16bit writes to the status register.
This is important as other functions rely on the return code of
spi_nor_write_sr_and_check. For example spi_nor_sr_unlock returns the
result directly, which is returned to userspace such that failing to
unlock the spi-nor device was resulting in a return code of 0 instead of
the expected non-zero indicating the failure.

Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
---
 drivers/mtd/spi-nor/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 04ea180118..d75d4f8a45 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1007,6 +1007,15 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
 	if (ret)
 		return ret;
 
+	ret = spi_nor_read_sr(nor, sr_cr);
+	if (ret)
+		return ret;
+
+	if (sr1 != sr_cr[0]) {
+		dev_dbg(nor->dev, "SR: read back test failed\n");
+		return -EIO;
+	}
+
 	if (nor->flags & SNOR_F_NO_READ_CR)
 		return 0;
 
---
2.35.1

Re: [PATCH] mtd: spi-nor: core: Fix 16bit write sr_and_check status check

[Pratyush Yadav]

Hi Nathan,

On 24/03/22 07:00AM, Nathan Rossi wrote:
> From: Nathan Rossi <nathan.rossi@digi.com>
> 
> The spi_nor_write_16bit_sr_and_check function description describes that
> the function compares the value of the status and config registers after
> the write. However the function does not implement the status register
> compare only the config register check.
> 
> This causes the function to differ in behaviour to the equivalent
> spi_nor_write_sr1_and_check for non-16bit writes to the status register.
> This is important as other functions rely on the return code of
> spi_nor_write_sr_and_check. For example spi_nor_sr_unlock returns the
> result directly, which is returned to userspace such that failing to
> unlock the spi-nor device was resulting in a return code of 0 instead of
> the expected non-zero indicating the failure.
> 
> Signed-off-by: Nathan Rossi <nathan.rossi@digi.com>
> ---
>  drivers/mtd/spi-nor/core.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 04ea180118..d75d4f8a45 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1007,6 +1007,15 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>  	if (ret)
>  		return ret;
>  
> +	ret = spi_nor_read_sr(nor, sr_cr);
> +	if (ret)
> +		return ret;
> +
> +	if (sr1 != sr_cr[0]) {
> +		dev_dbg(nor->dev, "SR: read back test failed\n");
> +		return -EIO;
> +	}
> +
>  	if (nor->flags & SNOR_F_NO_READ_CR)
>  		return 0;

This patch seems exactly like [0]. I will pick that one if it applies on 
the current tree since it already has one Reviewed-by.

[0] https://lore.kernel.org/linux-mtd/20220126073227.3401275-1-chentsung@chromium.org/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.