linux-kernel - Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

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: <mafs04j9ng1ay.fsf@kernel.org>
Date: Thu, 20 Jun 2024 16:02:13 +0200
From: Pratyush Yadav <pratyush@...nel.org>
To: "Michael Walle" <mwalle@...nel.org>
Cc: "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>,  "Linus Walleij" <linus.walleij@...aro.org>,
  <linux-mtd@...ts.infradead.org>,  <linux-kernel@...r.kernel.org>,
  "e9hack" <e9hack@...il.com>
Subject: Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

On Tue, Jun 18 2024, Michael Walle wrote:

> Hi Tudor,
>
> On Tue Jun 18, 2024 at 12:33 PM CEST, Tudor Ambarus wrote:
>> On 6/10/24 8:48 AM, Michael Walle wrote:
>> > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128")
>>
>> That commit did:
>> -       { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
>> -               NO_SFDP_FLAGS(SECT_4K) },
>> +       { "w25q128", INFO(0xef4018, 0, 0, 0)
>> +               PARSE_SFDP
>> +               FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>>
[...]
>> > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
>> > index ca67bf2c46c3..6b6dec6f8faf 100644
>> > --- a/drivers/mtd/spi-nor/winbond.c
>> > +++ b/drivers/mtd/spi-nor/winbond.c
>> > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = {
>> >  	}, {
>> >  		.id = SNOR_ID(0xef, 0x40, 0x18),
>> >  		.name = "w25q128",
>> > +		.size = SZ_16M,
>> >  		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB,
>> > +		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>>
>> and here you add dual and quad to trigger SFDP parsing I guess. All fine
>> if the old flash supports dual and quad read. But please update the
>> commit message describing the intention. With that ACK. Would be good to
>> have this merged soon.
>
> Right. It's not because it will trigger the SFDP parsing, but
> because that what was tested by Esben. We're lucky that this will
> trigger the SFDP parsing ;) I'll explain that in more detail and add
> a Link: to the bug report mail.

Should we treat this flash similar to the Macronix ones Esben sent out
patches for [0]? It seems that there are some old parts without SFDP
support and new ones with SFDP support. From your comment in [1]:

> This is an entry matching various flash families from Winbond, see my
> reply in v1. I'm not sure we should remove these as we could break the
> older ones, which might or might not have SFDP tables. We don't know.

Since the entry matches multiple families, do _all_ of them support dual
and quad read? If not, attempting to enable dual or quad reads on them
can cause problems.

Also, for parts that _do_ have SFDP available, won't it be better to use
the information in SFDP instead of our hard-coded ones anyway? Using
SPI_NOR_TRY_SFDP here would let us do that.

[0] https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/
[1] https://lore.kernel.org/linux-mtd/0525440a652854a2a575256cd07d3559@walle.cc/

-- 
Regards,
Pratyush Yadav