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

[<Tudor.Ambarus@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@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@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.

> (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.
Can I implement your suggestion and add Suggestion-by tags, or do you want
to handle it yourself?

Cheers,
ta
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel