linux-kernel - Re: [PATCH v3 1/3] mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode methods

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63b485a1e48d3116f8f49cc427049f81@walle.cc>
Date:   Thu, 14 Apr 2022 11:03:21 +0200
From:   Michael Walle <michael@...le.cc>
To:     Tudor Ambarus <tudor.ambarus@...rochip.com>
Cc:     p.yadav@...com, miquel.raynal@...tlin.com, richard@....at,
        vigneshr@...com, linux-mtd@...ts.infradead.org,
        linux-kernel@...r.kernel.org, nicolas.ferre@...rochip.com
Subject: Re: [PATCH v3 1/3] mtd: spi-nor: Parse BFPT to determine the 4-Byte
 Address Mode methods

> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index a5211543d30d..2e40eba3744d 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -401,6 +401,93 @@ static void
> spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>  	}
>  }
> 
> +/**
> + * spansion_set_4byte_addr_mode() - Set 4-byte address mode for 
> Spansion
> + * flashes.
> + * @nor:	pointer to 'struct spi_nor'.
> + * @enable:	true to enter the 4-byte address mode, false to exit the 
> 4-byte
> + *		address mode.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)

Mh, so now some callback functions are in the core like the quad enable
methods and some are in sfdp.c. I think there should be only the parsing
in sfdp.c and all the callback methods should be in core.c; as they are
potentially used by the vendor modules.

> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		break;
>  	}
> 
> +	switch (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK) 
> {

I was wondering why this is encoded as single bits and not as an
enumeration. To me it looks like it is intended to support
different methods at the same time. Thus I think there might be
multiple bits set in each entry and exit mask at once. The spec
lists all the entries as x_xxx1, x_xx1x, ..

> +	case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
.. then this will only match if exactly these two bits are set.


> +		params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;

Shouldn't this be getting a neutral name? The quad_enable methods
were renamed once, too.

-michael