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

[Michael Walle <michael@walle.cc>]

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.
(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?

-michael

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