| Message ID | 20210727045222.905056-6-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
On 27/07/21 07:51AM, Tudor Ambarus wrote: > 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> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Am 2021-07-27 06:51, 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 | 1 + > drivers/mtd/spi-nor/core.h | 1 + > drivers/mtd/spi-nor/manuf-id-collisions.c | 22 ++++++++++++++++++++++ > 4 files changed, 25 insertions(+) > 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 1a278d33b02d..d30c8f350dc1 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1829,6 +1829,7 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor > *nor) > } > > static const struct spi_nor_manufacturer *manufacturers[] = { > + &spi_nor_manuf_id_collisions, Intentionally at the beginning of all the flashes? So these might take precedence over "normal" ones? Shouldn't it be the other way around? > &spi_nor_atmel, > &spi_nor_catalyst, > &spi_nor_eon, > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 55fceb0ec894..e9896cd60695 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -476,6 +476,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..bf7dba34f018 > --- /dev/null > +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c > @@ -0,0 +1,22 @@ > +// 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 const struct flash_info id_collision_parts[] = { > + /* Boya */ > + { "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP | > + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, > +}; > + > +const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = { > + .name = "manufacturer ID collisions", mhh, so we loose the manufacturer name. Doh. Can we do better? -michael > + .parts = id_collision_parts, > + .nparts = ARRAY_SIZE(id_collision_parts), > +};
On 8/24/21 1:47 AM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-07-27 06:51, 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 | 1 + >> drivers/mtd/spi-nor/core.h | 1 + >> drivers/mtd/spi-nor/manuf-id-collisions.c | 22 ++++++++++++++++++++++ >> 4 files changed, 25 insertions(+) >> 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 1a278d33b02d..d30c8f350dc1 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -1829,6 +1829,7 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor >> *nor) >> } >> >> static const struct spi_nor_manufacturer *manufacturers[] = { >> + &spi_nor_manuf_id_collisions, > > Intentionally at the beginning of all the flashes? So these yes > might take precedence over "normal" ones? Shouldn't it be yes > the other way around? It doesn't really matter, either way we should correctly identify the flash. I thought of putting the collisions driver first for better test coverage. In case we miss a collision between a (future) entry from the manufacturer collisions driver and one from a dedicated manufacturer driver, to hit the flash entry from the collisions driver, which will report a wrong name for the flash in the dedicated manufacturer driver. People will report that something's wrong and we can fix the things sooner. > >> &spi_nor_atmel, >> &spi_nor_catalyst, >> &spi_nor_eon, >> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >> index 55fceb0ec894..e9896cd60695 100644 >> --- a/drivers/mtd/spi-nor/core.h >> +++ b/drivers/mtd/spi-nor/core.h >> @@ -476,6 +476,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..bf7dba34f018 >> --- /dev/null >> +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c >> @@ -0,0 +1,22 @@ >> +// 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 const struct flash_info id_collision_parts[] = { >> + /* Boya */ >> + { "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP | >> + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, >> +}; >> + >> +const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = { >> + .name = "manufacturer ID collisions", > > mhh, so we loose the manufacturer name. Doh. Can we do better? yes, we can introduce a const char *manufacturer_name in struct spi_nor and update it with the correct manufacturer name. Will do. cheers, ta > > -michael > >> + .parts = id_collision_parts, >> + .nparts = ARRAY_SIZE(id_collision_parts), >> +};
Am 2021-07-27 06:51, 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. Btw. how will this work in practice? Let's say we have a flash of a "bad" manufacturer B which is using the manufacturer id of another "good" manufacturer G. Now flashes of B are added to the kernel. Some kernel versions later G releases a flash of which uses the exact same id. How can we now add support for this flash now? If we can differentiate at runtime, fine. But what if not? Will then the legitimate owner of the ID will need a new compatible string? That's the only way I see how this can work. -michael
On 10/24/21 8:44 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-07-27 06:51, 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. > > Btw. how will this work in practice? Let's say we have a flash of a > "bad" manufacturer B which is using the manufacturer id of another > "good" manufacturer G. Now flashes of B are added to the kernel. > > Some kernel versions later G releases a flash of which uses the exact > same id. How can we now add support for this flash now? If we can > differentiate at runtime, fine. But what if not? Will then > the legitimate owner of the ID will need a new compatible string? > Yes, in order to not break support for the first flash added. Cheers, ta
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 1a278d33b02d..d30c8f350dc1 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1829,6 +1829,7 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor) } static const struct spi_nor_manufacturer *manufacturers[] = { + &spi_nor_manuf_id_collisions, &spi_nor_atmel, &spi_nor_catalyst, &spi_nor_eon, diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 55fceb0ec894..e9896cd60695 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -476,6 +476,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..bf7dba34f018 --- /dev/null +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c @@ -0,0 +1,22 @@ +// 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 const struct flash_info id_collision_parts[] = { + /* Boya */ + { "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP | + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, +}; + +const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = { + .name = "manufacturer ID collisions", + .parts = id_collision_parts, + .nparts = ARRAY_SIZE(id_collision_parts), +};
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 | 1 + drivers/mtd/spi-nor/core.h | 1 + drivers/mtd/spi-nor/manuf-id-collisions.c | 22 ++++++++++++++++++++++ 4 files changed, 25 insertions(+) create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c