linux-kernel - Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

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: <871q413x72.fsf@geanix.com>
Date: Wed, 10 Jul 2024 20:42:57 +0200
From: Esben Haabendal <esben@...nix.com>
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>,  <linux-mtd@...ts.infradead.org>,
  <linux-kernel@...r.kernel.org>,  "Rasmus Villemoes"
 <rasmus.villemoes@...vas.dk>
Subject: Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional
 SFDP

"Michael Walle" <mwalle@...nel.org> writes:

> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>> On 6/3/24 14:09, Esben Haabendal wrote:
>> > A dedicated flag for triggering call to
>> > spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>> > and parse, with fallback to legacy flash parameters, without having dual,
>> > quad or octal parameters set in the legacy flash parameters.
>> > 
>> > With this, spi-nor flash parts without SFDP that is replaced with a
>> > different flash NOR flash part that does have SFDP, but shares the same
>> > manufacturer and device ID is easily handled.
>> > 
>> > Signed-off-by: Esben Haabendal <esben@...nix.com>
>> > ---
>> >  drivers/mtd/spi-nor/core.c | 3 ++-
>> >  drivers/mtd/spi-nor/core.h | 1 +
>> >  2 files changed, 3 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> > index 3e1f1913536b..1c4d66fc993b 100644
>> > --- a/drivers/mtd/spi-nor/core.c
>> > +++ b/drivers/mtd/spi-nor/core.c
>> > @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>> >  
>> >  	spi_nor_manufacturer_init_params(nor);
>> >  
>> > -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>> > +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>
>> I don't like that we update deprecated methods. The solution though is
>> elegant.
>
> I actually had the same concern. But currently there is no
> non-deprecated way to handle this case, right?
>
> Right now we have the following cases:
>  (1) pure SFDP parsing
>  (2) non-SFDP flashes with static configuration only
>  (3) legacy implementation, where the magic flags decide whether we
>      use SFDP

Actually, in the code we have two variants of 2.

(2a) non-SFDP flashes with SPI_NOR_SKIP_SFDP set
(2b) non-SFDP flashes without SPI_NOR_SKIP_SFDP and with none of the
     DUAL/QUAD/OCTAL read bits set

These almost handled the same way. But
spi_nor_manufacturer_init_params() is only called for 2b, and not for
2a.

Is this desired behavior, or something that we want to align?

> Which case is eventually used depends on the ID of the flash -
> assuming there will only be IDs which either fall into (1) *or* (2).
> That assumption is clearly wrong :)
>
> I'd propose a new case in spi_nor_init_params()
>  (4) try SFDP with a fallback to the static flags from the
>      flash_info db.

/Esben