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

[<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?
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/