| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4cbc9c03-7b47-48e9-8a91-f08c44284579@linaro.org>
Date: Wed, 6 Sep 2023 07:12:12 +0100
From: Tudor Ambarus <tudor.ambarus@...aro.org>
To: Michael Walle <mwalle@...nel.org>,
Pratyush Yadav <pratyush@...nel.org>,
Miquel Raynal <miquel.raynal@...tlin.com>,
Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>
Cc: linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org
Subject: Re: [PATCH v2 13/41] mtd: spi-nor: move the .id and .id_len into an
own structure
On 8/22/23 08:09, Michael Walle wrote:
> Create a new structure to hold a flash ID and its length. The goal is to
> have a new macro SNOR_ID() which can have a flexible id length. This way
> we can get rid of all the individual INFOx() macros.
>
> Signed-off-by: Michael Walle <mwalle@...nel.org>
> ---
> drivers/mtd/spi-nor/core.c | 6 +++---
> drivers/mtd/spi-nor/core.h | 33 ++++++++++++++++++++++++---------
> drivers/mtd/spi-nor/micron-st.c | 4 ++--
> drivers/mtd/spi-nor/spansion.c | 4 ++--
> drivers/mtd/spi-nor/sysfs.c | 6 +++---
> drivers/mtd/spi-nor/winbond.c | 1 -
> 6 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4ba1778eda4b..80c340c7863a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2028,8 +2028,8 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
> for (j = 0; j < manufacturers[i]->nparts; j++) {
> part = &manufacturers[i]->parts[j];
> - if (part->id_len &&
> - !memcmp(part->id, id, part->id_len)) {
> + if (part->id &&
> + !memcmp(part->id->bytes, id, part->id->len)) {
> nor->manufacturer = manufacturers[i];
> return part;
> }
> @@ -3370,7 +3370,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> * If caller has specified name of flash model that can normally be
> * detected using JEDEC, let's verify it.
> */
> - if (name && info->id_len) {
> + if (name && info->id) {
> const struct flash_info *jinfo;
>
> jinfo = spi_nor_detect(nor);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index c42b65623da7..81535f31907f 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -446,12 +446,22 @@ struct spi_nor_fixups {
> int (*late_init)(struct spi_nor *nor);
> };
>
> +/**
> + * struct spi_nor_id - SPI NOR flash ID.
> + *
> + * @bytes: the flash's ID bytes. The first three bytes are the JEDIC ID.
typo, JEDIC. But are there 3 bytes of ID specified by JEDEC? I remeber
there's only the MFR ID specified, the rest are flash specific, which
are not mentioned by JEDEC.
> + * @len: the number of bytes of ID.
> + */
> +struct spi_nor_id {
> + const u8 *bytes;
> + u8 len;
> +};
> +
> /**
> * struct flash_info - SPI NOR flash_info entry.
> + * @id: pointer to struct spi_nor_id or NULL, which means "no ID" (mostly
> + * older chips).
> * @name: the name of the flash.
> - * @id: the flash's ID bytes. The first three bytes are the
> - * JEDIC ID. JEDEC ID zero means "no ID" (mostly older chips).
> - * @id_len: the number of bytes of ID.
> * @size: the size of the flash in bytes.
> * @sector_size: (optional) the size listed here is what works with
> * SPINOR_OP_SE, which isn't necessarily called a "sector" by
> @@ -510,8 +520,7 @@ struct spi_nor_fixups {
> */
> struct flash_info {
> char *name;
> - u8 id[SPI_NOR_MAX_ID_LEN];
> - u8 id_len;
> + const struct spi_nor_id *id;
> size_t size;
> unsigned sector_size;
> u16 page_size;
> @@ -554,12 +563,18 @@ struct flash_info {
> #define SPI_NOR_ID_3ITEMS(_id) ((_id) >> 16) & 0xff, SPI_NOR_ID_2ITEMS(_id)
>
> #define SPI_NOR_ID(_jedec_id, _ext_id) \
> - .id = { SPI_NOR_ID_3ITEMS(_jedec_id), SPI_NOR_ID_2ITEMS(_ext_id) }, \
> - .id_len = !(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))
> + .id = &(const struct spi_nor_id){ \
> + .bytes = (const u8[]){ SPI_NOR_ID_3ITEMS(_jedec_id), \
> + SPI_NOR_ID_2ITEMS(_ext_id) }, \
> + .len = !(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0)), \
> + }
>
> #define SPI_NOR_ID6(_jedec_id, _ext_id) \
> - .id = { SPI_NOR_ID_3ITEMS(_jedec_id), SPI_NOR_ID_3ITEMS(_ext_id) }, \
> - .id_len = 6
> + .id = &(const struct spi_nor_id){ \
> + .bytes = (const u8[]){ SPI_NOR_ID_3ITEMS(_jedec_id), \
> + SPI_NOR_ID_3ITEMS(_ext_id) }, \
> + .len = 6, \
> + }
>
> #define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks) \
> .size = (_sector_size) * (_n_sectors), \
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index 5406a3af2ce0..229c951efcce 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -78,7 +78,7 @@ static int micron_st_nor_octal_dtr_en(struct spi_nor *nor)
> return ret;
> }
>
> - if (memcmp(buf, nor->info->id, nor->info->id_len))
> + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len))
> return -EINVAL;
>
> return 0;
> @@ -114,7 +114,7 @@ static int micron_st_nor_octal_dtr_dis(struct spi_nor *nor)
> return ret;
> }
>
> - if (memcmp(buf, nor->info->id, nor->info->id_len))
> + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len))
> return -EINVAL;
>
> return 0;
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index e6468569f178..d7012ab3de2c 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -228,7 +228,7 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
> return ret;
> }
>
> - if (memcmp(buf, nor->info->id, nor->info->id_len))
> + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len))
> return -EINVAL;
>
> return 0;
> @@ -272,7 +272,7 @@ static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
> return ret;
> }
>
> - if (memcmp(buf, nor->info->id, nor->info->id_len))
> + if (memcmp(buf, nor->info->id->bytes, nor->info->id->len))
> return -EINVAL;
>
> return 0;
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> index c09bb832b3b9..2dfdc555a69f 100644
> --- a/drivers/mtd/spi-nor/sysfs.c
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -35,8 +35,8 @@ static ssize_t jedec_id_show(struct device *dev,
> struct spi_device *spi = to_spi_device(dev);
> struct spi_mem *spimem = spi_get_drvdata(spi);
> struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> - const u8 *id = nor->info->id_len ? nor->info->id : nor->id;
> - u8 id_len = nor->info->id_len ?: SPI_NOR_MAX_ID_LEN;
> + const u8 *id = nor->info->id ? nor->info->id->bytes : nor->id;
> + u8 id_len = nor->info->id ? nor->info->id->len : SPI_NOR_MAX_ID_LEN;
>
> return sysfs_emit(buf, "%*phN\n", id_len, id);
> }
> @@ -78,7 +78,7 @@ static umode_t spi_nor_sysfs_is_visible(struct kobject *kobj,
>
> if (attr == &dev_attr_manufacturer.attr && !nor->manufacturer)
> return 0;
> - if (attr == &dev_attr_jedec_id.attr && !nor->info->id_len && !nor->id)
> + if (attr == &dev_attr_jedec_id.attr && !nor->info->id && !nor->id)
> return 0;
>
> return 0444;
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index c21fed842762..7873cc394f07 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -121,7 +121,6 @@ static const struct flash_info winbond_nor_parts[] = {
> { "w25q80bl", INFO(0xef4014, 0, 64 * 1024, 16)
> NO_SFDP_FLAGS(SECT_4K) },
> { "w25q128", INFO(0xef4018, 0, 0, 0)
> - PARSE_SFDP
this is a leftover that should be squashed in a previous commit.
I'm fine with the overal idea, but please fix the comment about the 3
bytes of jedec id.
> FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> { "w25q256", INFO(0xef4019, 0, 64 * 1024, 512)
> NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>
Powered by blists - more mailing lists