linux-kernel - Re: [PATCH] mtd: spi-nor: use 16-bit WRR command when QE is set on spansion flashes

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 for Android: free password hash cracker in your pocket

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

Message-ID: <02babf5a-2a50-848c-27d9-9f810078cbcf@microchip.com>
Date:   Wed, 19 Jun 2019 15:47:13 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <geert@...ux-m68k.org>
CC:     <marek.vasut+renesas@...il.com>, <marek.vasut@...il.com>,
        <vigneshr@...com>, <jonas@...rbonn.se>, <dwmw2@...radead.org>,
        <computersforpeace@...il.com>, <miquel.raynal@...tlin.com>,
        <richard@....at>, <linux-mtd@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: use 16-bit WRR command when QE is set on
 spansion flashes

Hi, Geert,

On 06/11/2019 11:35 AM, Geert Uytterhoeven wrote:
> Hi Tudor,
> 
> On Mon, Jun 10, 2019 at 8:24 AM <Tudor.Ambarus@...rochip.com> wrote:
>> From: Tudor Ambarus <tudor.ambarus@...rochip.com>
>>
>> SPI memory devices from different manufacturers have widely
>> different configurations for Status, Control and Configuration
>> registers. JEDEC 216C defines a new map for these common register
>> bits and their functions, and describes how the individual bits may
>> be accessed for a specific device. For the JEDEC 216B compliant
>> flashes, we can partially deduce Status and Configuration registers
>> functions by inspecting the 16th DWORD of BFPT. Older flashes that
>> don't declare the SFDP tables (SPANSION FL512SAIFG1 311QQ063 A ©11
>> SPANSION) let the software decide how to interact with these registers.
>>
>> The commit dcb4b22eeaf4 ("spi-nor: s25fl512s supports region locking")
>> uncovered a probe error for s25fl512s, when the QUAD bit CR[1] was set
>> in the bootloader. When this bit is set, only the Write Register
>> WRR command format with 16 data bits may be used, WRR with 8 bits
>> is not recognized and hence the error when trying to clear the block
>> protection bits.
>>
>> Fix the above by using 16-bits WRR command when Quad bit is set.
>>
>> Backward compatibility should be fine. The newly introduced
>> spi_nor_spansion_clear_sr_bp() is tightly coupled with the
>> spansion_quad_enable() function. Both assume that the Write Register
>> with 16 bits, together with the Read Configuration Register (35h)
>> instructions are supported.
>>
>> Reported-by: Geert Uytterhoeven <geert@...ux-m68k.org>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@...rochip.com>
>> ---
>> Geert, Jonas,
>>
>> This patch is compile-tested only. I don't have the flash, I need your
>> help for testing this.
> 
> Thanks, this revives access to the s25fl512s on Koelsch.
> 
> Fixes: dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region locking")

I didn't add the Fixes tag because this commit helped us discover a case that
has not been taken into consideration before. It didn't introduce a bug, but
rather revealed one. However, it's not the time to walk over this thin line, so
I'll add it, thanks!

> Tested-by: Geert Uytterhoeven <geert+renesas@...der.be>Hi Tudor,
> 
> Two questions below...
> 
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> 
>> +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor)
>> +{
> 
> [...]
> 
>> +        * When the configuration register QUAD bit CR[1] is 1, only
>> +        * the WRR command format with 16 data bits may be used.
> 
> s/WRR/WRSR/?

S25FL512S named it "Write Registers" command and chose the "WRR" acronym.
JESD216D names it "Write Register" command and doesn't suggest an acronym. I'll
s/"WRR"/"Write Register command", to use the JESD216D naming and avoid confusion.

I also forgot to describe int (*clear_sr_bp), v2 will follow. Will keep the R-b
and T-b tags since I'll just update comments.

ta