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 <[email protected]>
---
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.
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 <[email protected]>
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 <[email protected]>
Out of curiosity, on what flash did you discover this?
-michael
On Thu, Jan 27, 2022 at 6:38 AM Michael Walle <[email protected]> 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.
Am 2022-01-27 04:31, schrieb Chen-Tsung Hsieh:
> On Thu, Jan 27, 2022 at 6:38 AM Michael Walle <[email protected]> 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/[email protected]/
On Thu, Jan 27, 2022 at 5:18 PM Michael Walle <[email protected]> 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/[email protected]/
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;
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 <[email protected]>
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 <[email protected]>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
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 <[email protected]>
>>
>> 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 <[email protected]>
>>
>> Acked-by: Pratyush Yadav <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
>
> Acked-by: Pratyush Yadav <[email protected]>
On 27/04/22 07:14AM, [email protected] 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >>
> >> Acked-by: Pratyush Yadav <[email protected]>
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
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, [email protected] 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>>
>>>> Acked-by: Pratyush Yadav <[email protected]>
>>
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
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.