Re: [PATCH v3 05/23] mtd: spi-nor: Create the concept of fixup table with match function

[Michael Walle]

Hi,

On Thu Aug 27, 2026 at 6:43 PM CEST, Miquel Raynal wrote:
> I took me a lot of time to go through all your requests, in the end I
> will not implement most of your feedback because either I do not
> understand what is the gain, or because the result is not better at all
> from my point of view. I try to explain that in my feedback
> below. Nevertheless, I truly appreciate the deep review you made. I
> think I mostly agree with the comments in the other messages.
>
> On 14/08/2026 at 11:40:21 +02, "Michael Walle" <mwalle@xxxxxxxxxx> wrote:
>
>> On Thu Aug 13, 2026 at 5:19 PM CEST, Miquel Raynal wrote:
>>> Manufacturer ID tables increase and fixup() hooks proliferate. Having
>>> one possible structure per chip was fine until the fixups started being
>>> more and more common and needed, to some extend due to ID reuses. Mixing
>>> fixups and chips becomes hard and requires extra helpers to sort which
>>> ones are needed for a given chip, which every time this happens requires
>>> a lot of rework.
>>
>> Thanks, this patch looks really promising!
>>
>>> Replace the two-level fixup association (a manufacturer wide hook and a
>>> per flash_info hook) with a per-manufacturer list of fixups that can be
>>> looked up by flash ID and/or match function.
>>>
>>> The match logic works as follows:
>>> - If there is an ID, it must match
>>> - If there is a match function, it must match (cumulative)
>>> - If there is no identifier (no ID nor any match function), it's a
>>>   catch-all entry typically used for flagging manufacturer fixups.

I think this one is the crux. See more below.

>> This mimics, how it's currently handled. But since the fixups are
>> now treated as a separate list, i.e. it has it's own SNOR_ID, we
>> shall move away from this manufacturer thingy too.
>
> Why shall we? See below, I don't get the point. I drafted something it
> took me two full days to work on that, to eventually realize I did not
> see *any* benefit.

Then let me ask the other way around, what is the benefit of having
the manufacturer? 

There is now one central place for the fixups. Good. But now there
is that catch-all thing, which is the replacement for the former
manufacturer fixup if I read your code correctly.

BUT. We don't have a concept of a 'manufacturer'. We don't match the
manufacturer id. The manufacturer is just set as an additional
information if there is already  matching entry for a specific
flash, so we already know the exact part. Before the generic driver,
this might have made sense. The intention was to have a common place
for the default flags for all *known* flashes of a manufacturer
(which probably also was the result of the core rewrite). But now,
we might have flashes we do not know, that is, aren't in the
database. At least I would assume, that the manufacturer fixups are
still applied though. There were already a few different times I
proposed to have fixups for SNOR_ID(vendor_id).

I mean, even you fell into the trap that you'll have to have that
catch-all/fallback entry in the flash db (can't find the mail
anymore though, so please correct me if I'm wrong).

Also, having the fixups separate from the flash db, we can use the
generic flash driver and still have some fixups for given flashes.
If - for example - one would be adding locking support later,
nothing but the new flashdb entry has to be added as the fixups are
still applied.

>> IMHO that really
>> clutters the code.
>
> Clearly an opinion I do not share. Maybe I am missing something.
>
>> As a fun fact, spi_nor_match_id() will have the
>> unexpected side effect of setting nor->manufacturer.
>
> Not sure I get why this is unexpected?

The signature of the function is:
static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
						 const u8 *id)

Clearly it will return a pointer to the flash_info entry for a given
id. But as a side effect it also sets nor->manufacturer. What if I'd
call that twice with different ids? Maybe it's just me, but it looks
like a pure function but isn't. I probably got burnt by the generic
nor driver which hit that in the first place.

>> So here's how I'd do it: 
>> (1) remove spi_nor_manufacturer.
>> (2) have a list of initcalls in the core to call into the individual
>>     vendor modules (that could later be replaced by something more
>>     sophiticated)
>> (3) that initcall will then call spi_nor_register_parts(const struct
>>     flash_info *parts, int nparts)
>> (4) (optionally) calls spi_nor_register_fixups(const struct
>>     spi_nor_fixup *fixups, int nfixups)
>
> I've been trying hard to follow your approach, but really, I don't get
> the point. Initcalls are not possible here, or it would prevent
> CONIFG_MTD_SPI_NOR=m so I went for an alternative approach with an init
> registration which I believe matches your expectations. This is not
> an actual problem.
>
> But registering parts and fixups independently, maybe, but again, why?
> What is the intended benefit? We now need to go through 96 (spi-nor
> wide) fixups. But what do we get in exchange?

I don't get that. You already have a (new) winbond_fixups[] for
example. What is the difference between having that set in
spi_nor_manufacturer vs having that in a

winbond_init() {
	spi_nor_register_flash_devices(&winbond_nor_parts);
	spi_nor_register_fixups(&winbond_fixups);
}

And probably setting the manufacturer name in winbond_nor_fixups().

I guess that all falls apart because of the empty match entry. That
could be replaced with the SNOR_ID(winbond_id), though. Then we have
*real* per manufacturer fixups.

That is basically what commit afe1ea1344bb ("mtd: spi-nor: add
support for Macronix Octal flash") made.

> From my point of view:
> extra churn and boilerplate, nothing particularly better than before. I
> must admit, I do not understand your aversion for the manufacturer
> structure. So while I like the idea of a flash-info independent fixup
> list (which I implemented in v3), I do not understand the need for this
> extra step.

See above.

> Since I do not get the point of this request, I prefer to not implement
> it, because I will anyway do it the wrong way. If someone wants to make
> a proposal, I will have a look, but at this point I need to back
> off.

I can do that myself, but I'd like to avoid any code churn. So I
don't know what you've already tried to come up with, but if there
is something which somewhat goes in the right direction of moving the
registration to a function inside of the vendor modules. That would
really be appreciated.

> There are already ~70 patches pending which I hope will be applied
> ASAP after -rc1 gets tagged and we agree on the remaining points (there
> are other comments from you and Takahiro which I need to address).

Looks like I need to apply the patches to the tree, too. Honestly,
I've never done that, so I first need to actually see how it's done
and if I even have access to the tree.

>> Also both could be a macro, then we can get rid of the size
>> argument.
>>
>> The current manufacturer fixups gets registered with a
>> "SNOR_ID(vendor)". 
>>
>> That leaves us with the manufacturer name, which should be set by
>> that vendor catch all fixup.
>>
>> Btw, I'm fine with deprecating the name as well as the manufacturer
>> sysfs entry, but it shouldn't be as in "it just goes away".
>
> I have no strong wish to get rid of those, especially the manufacturer
> name which is not particularly problematic today.
>
> [...]
>
>>> +	for (i = 0; i < nor->manufacturer->nfixups; i++) {
>>> +		if (fixups[i].fixups->post_bfpt &&
>>> +		    spi_nor_fixup_match(nor, &fixups[i])) {
>>> +			ret = fixups[i].fixups->post_bfpt(nor, bfpt_header, bfpt);
>>> +			if (ret)
>>> +				return ret;
>>> +		}
>>
>> This is repeated several times, can we have something like
>>
>> spi_nor_apply_fixups(struct spi_nor *nor, int (*fixup_fn), void *fixup_args);
>>
>> int spi_nor_post_bfpt_fixup(struct spi_nor *nor, const struct spi_nor_fixup *fixup, void *_args)
>> {
>> 	struct post_bfpt_fixup_args *args = _args;
>>
>> 	if (!fixup->post_bfpt)
>> 		return 0;
>>
>> 	return fixup->post_bfpt(nor, args->bfpt_header, args->bfpt);
>> }
>>
>> which then will be called with
>> spi_nor_apply_fixups(nor, spi_nor_post_bfpt_fixup, args);
>
> I drafted it, but arguments are different, argument numbers are
> different, return values are different, we need stubs for each
> fixup... Nothing is identical, we just save a couple of simple checks
> and a for loop. On the other hand, code just grows, complexity is
> higher, readability is definitely lower. Sorry, but again, I do not see
> the gain with this attempt, it just darkens what the core does. It would
> definitely work with another -more advanced- language than C, though.
>
>>> +	for (i = 0; i < nor->manufacturer->nfixups; i++) {
>>> +		if (fixups[i].fixups->post_bfpt &&
>>> +		    spi_nor_fixup_match(nor, &fixups[i])) {
>>> +			ret = fixups[i].fixups->post_bfpt(nor, bfpt_header, bfpt);
>>> +			if (ret)
>>> +				return ret;
>>>  	}
>>>  
>
> [...]
>
>>> --- a/drivers/mtd/spi-nor/gigadevice.c
>>> +++ b/drivers/mtd/spi-nor/gigadevice.c
>>> @@ -64,7 +64,6 @@ static const struct flash_info gigadevice_nor_parts[] = {
>>>  		.id = SNOR_ID(0xc8, 0x40, 0x19),
>>>  		.name = "gd25q256",
>>>  		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6,
>>> -		.fixups = &gd25q256_fixups,
>>>  		.fixup_flags = SPI_NOR_4B_OPCODES,
>>
>> The fixup_flags should also go into the fixups list.
>
> Yes, we can use a similar logic here. Actually if at some point we want
> to get rid of those flags, we could transform the flags into proper
> fixup functions, but I'm not going to do that now.

The flags actually serve the purpose of *not* having a huge amount
of different fixup functions. So these should be kept for now. As an
alternative, with the new list it might be possible to have multiple
fixups for the same flash part. In any case, the fixup_flags have to
be removed from the flash_info struct.

>
> [...]
>
>>> +/* PM25LV parts have no JEDEC ID and are likely matched by name */
>>> +static bool issi_pm25lv_match(const struct spi_nor *nor)
>>> +{
>>> +	const char *name = nor->info ? nor->info->name : NULL;
>>> +
>>> +	return name && !strncmp(name, "pm25lv", 6);
>>
>> Won't just strcmp("pm25lv", name) do it? Both arguments are
>> guaranteed to be NUL terminated. The above would also match
>> "pm25lvextrabytes", no, and we get rid of that redundant size
>> argument.
>
> No it won't, because that would no longer be a match, you would compare
> pm25lv against pm25lv010 or pm25lv512 -> no match if you don't limit to
> the first 6 bytes. Since names are part of the sysfs ABI, we cannot just
> modify the names for that. We also need two different entries (two
> different sizes) since we do not even match with an ID.

Ah I see, you actually *want* that extra bytes, in that case, please
use strstarts().


> [...]
>
>>> @@ -233,6 +230,17 @@ static int mt25qu512a_post_bfpt_fixup(struct spi_nor *nor,
>>>  	return 0;
>>>  }
>>>  
>>> +/*
>>> + * n25q00a parts share the first same 3 ID bytes with mt25qu01g.
>>> + * In order to not mix the fixups, further filter out using the part name.
>>
>> How would that work? The part name is also set by the duplicated ID,
>> so the first one wins, no?
>
> - First part (mt25qu01g) is checked
>   -> ID match
> - There is a match function where we compare the name with n25q00a
>   -> no match, not applying the fixup
>
> - Second part (n25q00a) is checked
>   -> ID match
> - There is a match function where we compare the name with n25q00a
>   -> name match
>
> The fixup is only applied for n25q00a.

Ah, I now get what you want to try to achieve. Only the first three
bytes (I missed that in the function doc) are shared. So no actual ID
collision.

> For the other chip we can just give more ID bytes for the match.

Giving there are more IDs for the n25q00a.

> So I believe there is no problem here?

Yes, no problem. But use strncmp() here, because this actually
matches just one particular flash.

FWIW I don't think this is sustainable, though. It's basically a
workaround for the different matching in spi_nor_match_id() (first
entry wins) and the fixups ID matching (any matching entry is
applied). Though, I can't think of a more generic solution for now.

-michael

Attachment: signature.asc
Description: PGP signature