linux-kernel - Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR

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: <160ddac1-6d0a-c922-1380-09e7823a285e@microchip.com>
Date:   Tue, 19 Jul 2022 05:57:35 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>, <p.yadav@...com>, <miquel.raynal@...tlin.com>,
        <richard@....at>, <vigneshr@...com>, <quic_c_sbhanu@...cinc.com>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()

On 5/10/22 17:02, Michael Walle wrote:

Hi!

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Up until now, flashes were defined by specifying the JEDEC ID, the
> sector size and the number of sectors. This can be read by parsing the
> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
> new flashes which have SFDP should use this macro.

I like the idea, but you need to refine it a bit.
Your assumptions are true only for flashes that are compliant with SFDP revB or
later because params->page_size is initialized by querying BFPT DWORD 11. I think
it would be good to specify this in the comment section. Also, I think you introduce
a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not
selected. wanted_size will be zero, will you select an invalid erase type?
Would you please resubmit?

Thanks,
ta

> 
> Signed-off-by: Michael Walle <michael@...le.cc>
> ---
>  drivers/mtd/spi-nor/core.c | 7 +++++--
>  drivers/mtd/spi-nor/core.h | 9 +++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 402b37cdbcea..29329ed0a934 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2104,8 +2104,11 @@ static int spi_nor_select_pp(struct spi_nor *nor,
>   * spi_nor_select_uniform_erase() - select optimum uniform erase type
>   * @map:               the erase map of the SPI NOR
>   * @wanted_size:       the erase type size to search for. Contains the value of
> - *                     info->sector_size or of the "small sector" size in case
> - *                     CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined.
> + *                     info->sector_size, the "small sector" size in case
> + *                     CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
> + *                     there is no information about the sector size. The
> + *                     latter is the case if the flash parameters are parsed
> + *                     solely by SFDP.
>   *
>   * Once the optimum uniform sector erase command is found, disable all the
>   * other.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 61886868cd02..fab3038c4f4a 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -563,6 +563,15 @@ struct flash_info {
>                         .n_regions = (_n_regions),                      \
>                 },
> 
> +#define SNOR_ID3(_jedec_id)                                            \
> +               .id = {                                                 \
> +                       ((_jedec_id) >> 16) & 0xff,                     \
> +                       ((_jedec_id) >> 8) & 0xff,                      \
> +                       (_jedec_id) & 0xff,                             \
> +                       },                                              \
> +               .id_len = 3,                                            \
> +               .parse_sfdp = true,                                     \
> +
>  #define PARSE_SFDP                                                     \
>         .parse_sfdp = true,                                             \
> 
> --
> 2.30.2
>