[RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Chen-Tsung Hsieh]

Read back Status Register 1 to ensure that the written byte match the
received value and return -EIO if read back test failed.

Without this patch, spi_nor_write_16bit_sr_and_check() only check the
second half of the 16bit. It causes errors like spi_nor_sr_unlock()
return success incorrectly when spi_nor_write_16bit_sr_and_check()
doesn't write SR successfully.

Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
Signed-off-by: Chen-Tsung Hsieh <chentsung@chromium.org>
---

 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 04ea180118e3..426bfa9a65f4 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.0.rc0.227.g00780c9af4-goog
Resend patch to move maintainers from 'Cc' to 'To' list.

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Michael Walle]

Am 2022-01-26 08:32, schrieb Chen-Tsung Hsieh:
> Read back Status Register 1 to ensure that the written byte match the
> received value and return -EIO if read back test failed.
> 
> Without this patch, spi_nor_write_16bit_sr_and_check() only check the
> second half of the 16bit. It causes errors like spi_nor_sr_unlock()
> return success incorrectly when spi_nor_write_16bit_sr_and_check()
> doesn't write SR successfully.
> 
> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on 
> lock()/unlock()")
> Signed-off-by: Chen-Tsung Hsieh <chentsung@chromium.org>

Looks good to me. spi_nor_write_16bit_cr_and_check() also checks
the SR1 and the function doc also mentions it will check it - although
it doesn't.

Reviewed-by: Michael Walle <michael@walle.cc>

Out of curiosity, on what flash did you discover this?

-michael

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Chen-Tsung Hsieh]

On Thu, Jan 27, 2022 at 6:38 AM Michael Walle <michael@walle.cc> wrote:
> Out of curiosity, on what flash did you discover this?

It's Winbond W25Q64JWZPIM
https://www.winbond.com/hq/product/code-storage-flash-memory/serial-nor-flash/?__locale=en&partNo=W25Q64JW

We are verifying the write protection on W25Q64JWZPIM and run into an
issue that spi_nor_sr_unlock() always return success even if HW & SW
write protection are both enabled.

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Michael Walle]

Am 2022-01-27 04:31, schrieb Chen-Tsung Hsieh:
> On Thu, Jan 27, 2022 at 6:38 AM Michael Walle <michael@walle.cc> wrote:
>> Out of curiosity, on what flash did you discover this?
> 
> It's Winbond W25Q64JWZPIM
> https://www.winbond.com/hq/product/code-storage-flash-memory/serial-nor-flash/?__locale=en&partNo=W25Q64JW
> 
> We are verifying the write protection on W25Q64JWZPIM and run into an
> issue that spi_nor_sr_unlock() always return success even if HW & SW
> write protection are both enabled.

Ah that ring a bell... Anyway, could you dump the SFDP data please?
See [1], you'll find the files in sysfs. I wonder why that flash is
using the 16bit write at all.

-michael

[1] 
https://lore.kernel.org/linux-mtd/4304e19f3399a0a6e856119d01ccabe0@walle.cc/

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Chen-Tsung Hsieh]

On Thu, Jan 27, 2022 at 5:18 PM Michael Walle <michael@walle.cc> wrote:
> Ah that ring a bell... Anyway, could you dump the SFDP data please?
> See [1], you'll find the files in sysfs. I wonder why that flash is
> using the 16bit write at all.
>
> [1]
> https://lore.kernel.org/linux-mtd/4304e19f3399a0a6e856119d01ccabe0@walle.cc/

Dump SFDP data:
# xxd -p /sys/class/mtd/mtd0/device/spi-nor/sfdp
53464450060101ff00060110800000ff84000102d00000ffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffe520f9ffffffff0344eb086b083b42bbfeffffffffff
0000ffff40eb0c200f5210d800003602a60082ea14c4e96376337a757a75
f7bdd55c19f75dffe930f880ffffffffffffffffffffffffffffffff0000
f0ffffffffff
# md5sum /sys/class/mtd/mtd0/device/spi-nor/sfdp
5294c4d4eb2b1c89c6fd8573d8ceaa2d  /sys/class/mtd/mtd0/device/spi-nor/sfdp
# cat /sys/class/mtd/mtd0/device/spi-nor/jedec_id
ef8017
# cat /sys/class/mtd/mtd0/device/spi-nor/partname
w25q64jwm
# cat /sys/class/mtd/mtd0/device/spi-nor/manufacturer
winbond

"bfpt.dwords[BFPT_DWORD(15)]" is 0xff5df719, and flag
SNOR_F_HAS_16BIT_SR is set here:
        case BFPT_DWORD15_QER_SR2_BIT1:
                /*
                 * JESD216 rev B or later does not specify if writing only one
                 * byte to the Status Register clears or not the Status
                 * Register 2, so let's be cautious and keep the default
                 * assumption of a 16-bit Write Status (01h) command.
                 */
                nor->flags |= SNOR_F_HAS_16BIT_SR;
                params->quad_enable = spi_nor_sr2_bit1_quad_enable;
                break;

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Pratyush Yadav]

On 26/01/22 03:32PM, Chen-Tsung Hsieh wrote:
> Read back Status Register 1 to ensure that the written byte match the
> received value and return -EIO if read back test failed.
> 
> Without this patch, spi_nor_write_16bit_sr_and_check() only check the
> second half of the 16bit. It causes errors like spi_nor_sr_unlock()
> return success incorrectly when spi_nor_write_16bit_sr_and_check()
> doesn't write SR successfully.
> 
> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
> Signed-off-by: Chen-Tsung Hsieh <chentsung@chromium.org>

I don't know much about this bit of code but this patch looks fine to me 
from the surface. Would be nice to hear from Tudor about this too since 
he added the function.

Acked-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Tudor.Ambarus]

On 1/31/22 19:19, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 26/01/22 03:32PM, Chen-Tsung Hsieh wrote:
>> Read back Status Register 1 to ensure that the written byte match the
>> received value and return -EIO if read back test failed.
>>
>> Without this patch, spi_nor_write_16bit_sr_and_check() only check the
>> second half of the 16bit. It causes errors like spi_nor_sr_unlock()
>> return success incorrectly when spi_nor_write_16bit_sr_and_check()
>> doesn't write SR successfully.
>>
>> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
>> Signed-off-by: Chen-Tsung Hsieh <chentsung@chromium.org>
> 
> I don't know much about this bit of code but this patch looks fine to me
> from the surface. Would be nice to hear from Tudor about this too since
> he added the function.

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Acked-by: Pratyush Yadav <p.yadav@ti.com>

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Tudor.Ambarus]

On 4/27/22 10:11, Tudor Ambarus wrote:
> On 1/31/22 19:19, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 26/01/22 03:32PM, Chen-Tsung Hsieh wrote:
>>> Read back Status Register 1 to ensure that the written byte match the
>>> received value and return -EIO if read back test failed.
>>>
>>> Without this patch, spi_nor_write_16bit_sr_and_check() only check the
>>> second half of the 16bit. It causes errors like spi_nor_sr_unlock()
>>> return success incorrectly when spi_nor_write_16bit_sr_and_check()
>>> doesn't write SR successfully.
>>>

cc to stable please

>>> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
>>> Signed-off-by: Chen-Tsung Hsieh <chentsung@chromium.org>
>>
>> I don't know much about this bit of code but this patch looks fine to me
>> from the surface. Would be nice to hear from Tudor about this too since
>> he added the function.
> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Acked-by: Pratyush Yadav <p.yadav@ti.com>

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Pratyush Yadav]

On 27/04/22 07:14AM, Tudor.Ambarus@microchip.com wrote:
> On 4/27/22 10:11, Tudor Ambarus wrote:
> > On 1/31/22 19:19, Pratyush Yadav wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>
> >> On 26/01/22 03:32PM, Chen-Tsung Hsieh wrote:
> >>> Read back Status Register 1 to ensure that the written byte match the
> >>> received value and return -EIO if read back test failed.
> >>>
> >>> Without this patch, spi_nor_write_16bit_sr_and_check() only check the
> >>> second half of the 16bit. It causes errors like spi_nor_sr_unlock()
> >>> return success incorrectly when spi_nor_write_16bit_sr_and_check()
> >>> doesn't write SR successfully.
> >>>
> 
> cc to stable please

Since this has the Fixes tag, once the patch hits the MTD tree stable 
should pick it up right?

> 
> >>> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
> >>> Signed-off-by: Chen-Tsung Hsieh <chentsung@chromium.org>
> >>
> >> I don't know much about this bit of code but this patch looks fine to me
> >> from the surface. Would be nice to hear from Tudor about this too since
> >> he added the function.
> > 
> > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >>
> >> Acked-by: Pratyush Yadav <p.yadav@ti.com>
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Pratyush Yadav]

On Wed, 26 Jan 2022 15:32:26 +0800, Chen-Tsung Hsieh wrote:
> Read back Status Register 1 to ensure that the written byte match the
> received value and return -EIO if read back test failed.
> 
> Without this patch, spi_nor_write_16bit_sr_and_check() only check the
> second half of the 16bit. It causes errors like spi_nor_sr_unlock()
> return success incorrectly when spi_nor_write_16bit_sr_and_check()
> doesn't write SR successfully.
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next, thanks!
[1/1] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()
      https://git.kernel.org/mtd/c/70dd83d737

--
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [RESEND PATCH] mtd: spi-nor: core: Check written SR value in spi_nor_write_16bit_sr_and_check()

[Tudor.Ambarus]

On 4/27/22 11:59, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/04/22 07:14AM, Tudor.Ambarus@microchip.com wrote:
>> On 4/27/22 10:11, Tudor Ambarus wrote:
>>> On 1/31/22 19:19, Pratyush Yadav wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 26/01/22 03:32PM, Chen-Tsung Hsieh wrote:
>>>>> Read back Status Register 1 to ensure that the written byte match the
>>>>> received value and return -EIO if read back test failed.
>>>>>
>>>>> Without this patch, spi_nor_write_16bit_sr_and_check() only check the
>>>>> second half of the 16bit. It causes errors like spi_nor_sr_unlock()
>>>>> return success incorrectly when spi_nor_write_16bit_sr_and_check()
>>>>> doesn't write SR successfully.
>>>>>
>>
>> cc to stable please
> 
> Since this has the Fixes tag, once the patch hits the MTD tree stable
> should pick it up right?

yes, but indirectly. The recommended way to submit to stable is to cc stable.
See: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

> 
>>
>>>>> Fixes: 39d1e3340c73 ("mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()")
>>>>> Signed-off-by: Chen-Tsung Hsieh <chentsung@chromium.org>
>>>>
>>>> I don't know much about this bit of code but this patch looks fine to me
>>>> from the surface. Would be nice to hear from Tudor about this too since
>>>> he added the function.
>>>
>>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>>
>>>> Acked-by: Pratyush Yadav <p.yadav@ti.com>
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.