linux-kernel - Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

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: <50de19f7-2021-433e-b8f8-d928ed7d5d57@linaro.org>
Date: Thu, 20 Mar 2025 15:45:44 +0000
From: Tudor Ambarus <tudor.ambarus@...aro.org>
To: Rob Herring <robh@...nel.org>
Cc: Takahiro Kuwano <tkuw584924@...il.com>,
 Pratyush Yadav <pratyush@...nel.org>, Michael Walle <mwalle@...nel.org>,
 Miquel Raynal <miquel.raynal@...tlin.com>,
 Richard Weinberger <richard@....at>, Vignesh Raghavendra <vigneshr@...com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-mtd@...ts.infradead.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Bacem Daassi <Bacem.Daassi@...ineon.com>,
 Takahiro Kuwano <Takahiro.Kuwano@...ineon.com>,
 Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 2/3] mtd: spi-nor: use rdid-dummy-ncycles DT property

On 3/20/25 2:06 PM, Rob Herring wrote:
> On Thu, Mar 20, 2025 at 2:44 AM Tudor Ambarus <tudor.ambarus@...aro.org> wrote:
>>
>> Hi, Rob,
>>
>> On 3/19/25 11:30 PM, Rob Herring wrote:
>>> On Wed, Mar 19, 2025 at 06:47:44PM +0900, Takahiro Kuwano wrote:
>>>> There are infineon flashes [1] that require 8 dummy cycles for the
>>>> 1-1-1 Read ID command. Since the command is not covered by JESD216
>>>> or any other standard, get the number of dummy cycles from DT and use
>>>> them to correctly identify the flash.
>>>
>>> If Read ID fails, then couldn't you just retry with dummy cycles? Or
>>
>> I think Read ID won't fail when the op requires 8 dummy cycles, it
>> probably just reads garbage on the first 8 cycles, so we risk to wrongly
>> match other flash IDs.
>>
>>> would unconditionally adding dummy cycles adversely affect other chips?
>>
>> Adding 8 dummy cycles to chips that don't need it, would mean ignoring
>> the first byte of the flash ID, thus we again risk to wrongly match
>> against other flash IDs.
>>
>>>
>>> Otherwise, add a specific compatible to imply this requirement. Adding
>>> quirk properties doesn't scale.
>>
>> Do you mean a flash name compatible, like "cyrs17b512,spi-nor"?
> 
> Yes, but that's not the format of compatible strings.
> 
>> The
>> problem that I see with that is that we no longer bind against the
>> generic jedec,spi-nor compatible, so people need to update their DT in
>> case they use/plug-in a different flash on their board.
> 
> This chip is clearly *not* compatible with a generic chip.

I think it is compatible. The chip defines the SFDP (serial flash
discoverable parameters) tables. At probe time we parse those tables and
initialize the flash based on them.

We don't even care about the chip ID, if all the flash parameters can be
discovered via SFDP. Unfortunately these tables do not describe all the
flash capabilities (block protection being one). Or worse, manufacturers
mangle these tables.

So vendors need to identify chips to either fix those tables via some
quirks after the parsing is done, or to specify support that's not
covered by those tables.

For basic ops, flashes that get the SFDP tables right, don't even need a
flash entry defined, we don't care about their ID, we just initialize
the flash solely based on SFDP.

In this particular case, this flash needs identification to fix some
wrong SFDP field, it corrects just the mode cycles for the FAST READ
command. All the other commands seem fine according to patch 3/3.

> 
> You have the same problem with a property. Users have to add or remove

True. It's the same problem. Even if we specify the dummy cycles via a
property, the next plugged-in flash will use those. We can of course
fallback to the SFDP only init if the ID doesn't match any flash entry,
but the problem is the same.

> the property if the flash changes. Anyone thinking they can use this
> chip as a compatible 2nd source is SOL.
> 

I think the property vs compatible decision resumes at whether we
consider that the dummy cycles requirement for Read ID is/will be
generic or not.

I noticed that with higher frequencies or protocol modes (e.g, octal
DTR), flashes tend to require more dummy cycles. I think with time,
we'll have more flashes with such requirement. Takahiro can jump in and
tell if it's already the case with IFX.

Thus instead of having lots of new compatibles for this, I lean towards
having this property. I'm still open for the compatible idea, I just
wanted to explain better where we are.

Thanks,
ta