[PATCH v4 5/6] mtd: spi-nor: Introduce Manufacturer ID collisions driver

[Michael Walle]

Am 2022-03-04 08:07, schrieb Tudor.Ambarus at microchip.com:
> On 3/3/22 23:38, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-03-03 17:12, schrieb Tudor.Ambarus at microchip.com:
>>> On 3/2/22 00:19, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Am 2022-02-28 14:45, schrieb Tudor Ambarus:
>>>>> Some manufacturers completely ignore the manufacturer's
>>>>> identification
>>>>> code
>>>>> standard (JEP106) and do not define the manufacturer ID 
>>>>> continuation
>>>>> scheme. This will result in manufacturer ID collisions.
>>>>> 
>>>>> An an example, JEP106BA requires Boya that it's manufacturer ID to 
>>>>> be
>>>>> preceded by 8 continuation codes. Boya's identification code must 
>>>>> be:
>>>>> 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya
>>>>> ignores
>>>>> the
>>>>> continuation scheme and its ID collides with the manufacturer 
>>>>> defined
>>>>> in
>>>>> bank one: Convex Computer.
>>>>> 
>>>>> Introduce the manuf-id-collisions driver in order to address ID
>>>>> collisions
>>>>> between manufacturers. flash_info entries will be added in a first
>>>>> come,
>>>>> first served manner. Differentiation between flashes will be done 
>>>>> at
>>>>> runtime if possible. Where runtime differentiation is not possible,
>>>>> new
>>>>> compatibles will be introduced, but this will be done as a last
>>>>> resort.
>>>>> Every new flash addition that define the SFDP tables, should dump 
>>>>> its
>>>>> SFDP
>>>>> tables in the patch's comment section below the --- line, so that 
>>>>> we
>>>>> can
>>>>> reference it in case of collisions.
>>>>> 
>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/Makefile              |  1 +
>>>>>  drivers/mtd/spi-nor/core.c                |  3 +++
>>>>>  drivers/mtd/spi-nor/core.h                |  1 +
>>>>>  drivers/mtd/spi-nor/manuf-id-collisions.c | 32
>>>>> +++++++++++++++++++++++
>>>>>  drivers/mtd/spi-nor/sysfs.c               |  2 +-
>>>>>  include/linux/mtd/spi-nor.h               |  6 ++++-
>>>>>  6 files changed, 43 insertions(+), 2 deletions(-)
>>>>>  create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c
>>>>> 
>>>>> diff --git a/drivers/mtd/spi-nor/Makefile
>>>>> b/drivers/mtd/spi-nor/Makefile
>>>>> index 6b904e439372..48763d10daad 100644
>>>>> --- a/drivers/mtd/spi-nor/Makefile
>>>>> +++ b/drivers/mtd/spi-nor/Makefile
>>>>> @@ -1,6 +1,7 @@
>>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>> 
>>>>>  spi-nor-objs                 := core.o sfdp.o swp.o otp.o sysfs.o
>>>>> +spi-nor-objs                 += manuf-id-collisions.o
>>>>>  spi-nor-objs                 += atmel.o
>>>>>  spi-nor-objs                 += catalyst.o
>>>>>  spi-nor-objs                 += eon.o
>>>>> diff --git a/drivers/mtd/spi-nor/core.c 
>>>>> b/drivers/mtd/spi-nor/core.c
>>>>> index aef00151c116..80d6ce41122a 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>> @@ -1610,6 +1610,7 @@ int spi_nor_sr2_bit7_quad_enable(struct 
>>>>> spi_nor
>>>>> *nor)
>>>>>  }
>>>>> 
>>>>>  static const struct spi_nor_manufacturer *manufacturers[] = {
>>>>> +     &spi_nor_manuf_id_collisions,
>>>> 
>>>> I'm still not convinced it should be the first entry here. We will
>>>> put other vendors at a disadvantage who play fair. I doubt we will
>>>> always checking any new IDs for duplications - or some might slip
>>>> through. Putting it as the last entry will make sure, legitimate
>>>> users will always come first.
>>>> 
>>>> Esp. because xmc reuses vendor id whose flashes we also support
>>>> making a collision very likely. Unlike boya who reuses "convex
>>>> computers" where we will probably never see an SPI flash from.
>>> 
>>> Yes, being the first was intentional. The rationale is that if 
>>> someone
>>> adds a micron and sees an XMC name it's clear that it's a collision,
>>> so we get the chance to fix it from the first day. Better test
>>> coverage,
>>> easier to identify the collisions, thus easier work for maintainers.
>>> But at the same time ID collisions for new flash additions can be
>>> identified by a simple grep, so I will not insist here given that it 
>>> is
>>> the second time you mention putting the collisions driver the last in
>>> the
>>> array.
>>> 
>>>> 
>>>> That being said. I'd also suggest to only allow flashes with
>>>> SFDP here, so we have at least some clue to differentiate
>>>> between flashes. If there will ever be a flash without SFDP
>>>> and which is using a non-legitimate vendor id, then we'll
>>>> need to either deny support for it or specify it by a name
>>> 
>>> we can't deny support for this reason, we'll be forced to use dt to 
>>> get
>>> the flash name.
>>> 
>>>> (i.e. device tree compatible or similar). But these should
>>>> go into a seperate list then.
>>>> 
>>> How you will differentiate between two flashes of different
>>> manufacturers that
>>> collide, one that supports SFDP and one that doesn't? You'll have to
>>> have a
>>> single flash entry in one of the drivers, where will you put it?
>> 
>> Hm, I see. But it doesn't end there. Imagine one would need
>> function from a different (vendor) module. So we have to export it
>> again which formerly was just private to this module.
>> 
>> All of this makes me wonder if we can't just add one device id
>> multiple times in our lists in different vendor modules. To
>> distinguish between entries with the same id, we provide another
>> callback:
>>   bool is_match(nor, sfdp, ..)
>> 
>> That would solve the following:
>> (1) we can have the proper flags per flash instead of having
>>     to change them in fixups later
>> (2) vendor functions can be left private in the corresponding
>>     module, because all entries will be held in a per vendor list
>> (3) we can provide some sanity checks (enabled by a Kconfig)
>>     to walk the list and watch for invalid duplicate entries.
>>     See more below.
> 
> we already have to go through the entire list of flash info entries
> to check for collisions, we can just add a dev_info or dev_warn for
> invalid duplicate entries. No need for a kconfig.

It's some kind of selftest. I'm not just talking about the current
flash being probed. But always iterate over any flashes and look
duplicates, so the naive implementation would be O(N^2). Which is a
runtime overhead and only really needed by ourselves when we add
a new flash. It doesn't bring any good otherwise, so it should be
off.

>> (4) sane fallback. I.e. if there is a duplicate in the future,
>>     we just have to add a new entry with a is_match(). If it
>>     doesn't match, we just continue and will finally fall back
>>     to the original entry.
>> (5) if necessary, compatible strings matches should be easy to
>>     add. Think of something like:
>>      bool is_match(nor, sfdp) {
>>         return of_device_is_compatible(nor, "macronix,mx..");
>>      }
>> 
>> is_match() is optional, but if given, both the flash id has to
>> match as well as is_match() has to return true.
>> 
>> I.e. one sanity could be: walk the list and see if there are
>> two entries with the same id, but both without an is_match()
>> function. This would mean an invalid duplicate entry.
>> 
>> What do you think?
> 
> I find the idea good and would like to give it a try. The downside that
> I see is that we'll always have to go through the entire list of
> flash_info entries to determine the collisions and to handle them all, 
> so a
> little delay in finding the flash, but I think we can live with this.

If the flash is the last one in the list you have the same delay ;)

> Can I implement your suggestion and add Suggestion-by tags, or do you 
> want
> to handle it yourself?

Sure, go ahead!

-michael