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

[Michael Walle <michael@walle.cc>]

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.

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
(i.e. device tree compatible or similar). But these should
go into a seperate list then.

>  	&spi_nor_atmel,
>  	&spi_nor_catalyst,
>  	&spi_nor_eon,
> @@ -3037,6 +3038,8 @@ int spi_nor_scan(struct spi_nor *nor, const char 
> *name,
> 
>  	if (!nor->name)
>  		nor->name = info->name;
> +	if (!nor->manufacturer_name)
> +		nor->manufacturer_name = nor->manufacturer->name;
> 
>  	dev_info(dev, "%s (%lld Kbytes)\n", nor->name,
>  			(long long)mtd->size >> 10);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index b7fd760e3b47..f727e632c0ee 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -500,6 +500,7 @@ struct sfdp {
>  };
> 
>  /* Manufacturer drivers. */
> +extern const struct spi_nor_manufacturer spi_nor_manuf_id_collisions;
>  extern const struct spi_nor_manufacturer spi_nor_atmel;
>  extern const struct spi_nor_manufacturer spi_nor_catalyst;
>  extern const struct spi_nor_manufacturer spi_nor_eon;
> diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c
> b/drivers/mtd/spi-nor/manuf-id-collisions.c
> new file mode 100644
> index 000000000000..75c5ad6480ee
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Used to handle collisions between manufacturers, where 
> manufacturers are
> + * ignorant enough to not implement the ID continuation scheme 
> described in the
> + * JEP106 JEDEC standard.
> + */
> +
> +#include <linux/mtd/spi-nor.h>
> +#include "core.h"
> +
> +static void boya_nor_late_init(struct spi_nor *nor)
> +{
> +	nor->manufacturer_name = "boya";
> +}
> +
> +static const struct spi_nor_fixups boya_nor_fixups = {
> +	.late_init = boya_nor_late_init,
> +};
> +
> +static const struct flash_info id_collision_parts[] = {
> +	/* Boya */
> +	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256)
> +		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> +		NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SECT_4K | SPI_NOR_DUAL_READ |
> +			      SPI_NOR_QUAD_READ)
> +		.fixups = &boya_nor_fixups },

No PARSE_SFDP nor SKIP_SFDP?

> +};
> +
> +const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = {
> +	.parts = id_collision_parts,
> +	.nparts = ARRAY_SIZE(id_collision_parts),
> +};
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> index 017119768f32..fa0cf1a96797 100644
> --- a/drivers/mtd/spi-nor/sysfs.c
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -14,7 +14,7 @@ static ssize_t manufacturer_show(struct device *dev,
>  	struct spi_mem *spimem = spi_get_drvdata(spi);
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> 
> -	return sysfs_emit(buf, "%s\n", nor->manufacturer->name);
> +	return sysfs_emit(buf, "%s\n", nor->manufacturer_name);
>  }
>  static DEVICE_ATTR_RO(manufacturer);
> 
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 449496b57acb..3087589d01ac 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -351,7 +351,10 @@ struct spi_nor_flash_parameter;
>   * @bouncebuf:		bounce buffer used when the buffer passed by the MTD
>   *                      layer is not DMA-able
>   * @bouncebuf_size:	size of the bounce buffer
> - * @name:		used to point to correct name in case of ID collisions.
> + * @name:		used to point to correct flash name in case of ID
> + *                      collisions.
> + * @manufacturer_name:	used to point to correct manufacturer name in 
> case of
> + *                      ID collisions.
>   * @info:		SPI NOR part JEDEC MFR ID and other info
>   * @manufacturer:	SPI NOR manufacturer
>   * @addr_width:		number of address bytes
> @@ -382,6 +385,7 @@ struct spi_nor {
>  	u8			*bouncebuf;
>  	size_t			bouncebuf_size;
>  	const char *name;
> +	const char *manufacturer_name;
>  	const struct flash_info	*info;
>  	const struct spi_nor_manufacturer *manufacturer;
>  	u8			addr_width;

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/