linux-kernel - Re: [PATCH v3 00/15] mtd: spi-nor: macronix: workaround for device id re-use

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: <877cayhclj.fsf@geanix.com>
Date: Thu, 26 Sep 2024 13:35:20 +0200
From: Esben Haabendal <esben@...nix.com>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: Pratyush Yadav <pratyush@...nel.org>,  Michael Walle
 <mwalle@...nel.org>,  Miquel Raynal <miquel.raynal@...tlin.com>,  Richard
 Weinberger <richard@....at>,  Vignesh Raghavendra <vigneshr@...com>,
  Nicolas Ferre <nicolas.ferre@...rochip.com>,  Alexandre Belloni
 <alexandre.belloni@...tlin.com>,  Claudiu Beznea
 <claudiu.beznea@...on.dev>,  linux-mtd@...ts.infradead.org,
  linux-kernel@...r.kernel.org,  Rasmus Villemoes
 <rasmus.villemoes@...vas.dk>,  linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 00/15] mtd: spi-nor: macronix: workaround for device
 id re-use

Tudor Ambarus <tudor.ambarus@...aro.org> writes:

> Hiya, Esben,
>
> On 7/11/24 2:00 PM, Esben Haabendal wrote:
>> Following up to various discussions, this series have now been
>> modified so that it gets rid of the old deprecated approach
>> for detecting when to do optional SFDP parsing.
>> 
>> Before these changes, spi-nor flashes were handled in 4 different
>> ways:
>> 
>
> I'm adding a bit of extra context on each point you made. All your
> points contain static init, you missed spi_nor_init_default_params()

I might be even more confused now :) Let me try to understand...

> There's a 0/ case where we have indeed just SFDP initialized flashes,
> and that is the generic flash driver.
>
> 0/ SFDP only, generic flash driver

Ok. So this is when spi_nor_detect() falls back to
spi_nor_generic_flash, right?

>> (1) SFDP only [size==0]
>
> 1/ static + SFDP

So we have found a matching flash_info, and it has size==0. Right?

> spi_nor_init_default_params(nor);

I guess we can assume that flash_info in this case will always have
something set (other than .name and .size=0)?

> spi_nor_parse_sfdp();

>> (2a) static config only [size!=0 && no_sfdp_flags & SPI_NOR_SKIP_SFDP]
>
> 2a/ is
> spi_nor_init_default_params(nor);
> spi_nor_no_sfdp_init_params(nor);

Got it.

>> (2b) static config only
>>        [size!=0 &&
>>         !(no_sfdp_flags & (SPI_NOR_SKIP_SFDP | 
>>            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]
>
> 2b/ is
> spi_nor_init_default_params(nor);
> spi_nor_init_params_deprecated(nor); //where parse SFDP is not called
> 	spi_nor_no_sfdp_init_params(nor);
> 	spi_nor_manufacturer_init_params(nor);>

Got it.

>> (3) SFDP with fallback to static config
>>        [size!=0 &&
>>         !(no_sfdp_flags & SPI_NOR_SKIP_SFDP) &&
>>         (no_sfdp_flags & SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]
>> 
>
> 3/ is 2b/ with parse SFDP called and rollback mechanism
> spi_nor_init_default_params(nor);
> spi_nor_init_params_deprecated(nor);
> 	spi_nor_no_sfdp_init_params(nor);
> 	spi_nor_manufacturer_init_params(nor);
> 	spi_nor_sfdp_init_params_deprecated(nor);
>
> All cases from above are followed by a call to spi_nor_late_init_params().

Got it.

Should I move your annotations to the cover letter? If I find it
helpful, I think it might also be helpful to somebody else :)

>> Cases (2a) and (2b) have been handled slightly different, with
>> manufacturer and flash_info ->default_init() hooks being called in
>> case (2b) but not in case (2a).
>
> default_init() was a mistake and we shall remove it and replace it with
> late_init(). The challenge is to do that without affecting backwards
> compatibility. But let's move this aside for the moment
>> 
>> With this series, that is changed to this simpler approach instead:
>> 
>> (1) SFDP only [size==0]
>> 
>> (2) static config only
>>       [size!=0 && !(no_sfdp_flags & SPI_NOR_TRY_SFDP)]
>> 
>> (3) SFDP with fallback to static config
>>       [size!=0 && (no_sfdp_flags & SPI_NOR_TRY_SFDP)]
>> 
>> Existing struct flash_info entries are modified, so that all those
>> that was case (2a) or (2b) are now case (2), and those that were (1)
>> and (3) are still (1) and (3).
>
> We indeed want 2a/ and 2b/ to be squashed, ideally by removing the
> default_init() hook.

But you think we should not remove default_init() hook as part of this
series?

> And if we really want SFDP-only init, we shall not call
> spi_nor_init_default_params() in this case.

So move spi_nor_init_default_params() into the if (spi_nor_needs_sfdp())
else section?
I have pushed a fixup commit doing this to my branch. Should I amend
patch 1/15 with it?

/Esben