linux-kernel - Re: [PATCH v2 05/41] mtd: spi-nor: convert .n

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: <d7985b397e19f66c0692369e20a4fba6@kernel.org>
Date:   Fri, 01 Sep 2023 13:00:45 +0200
From:   Michael Walle <mwalle@...nel.org>
To:     Tudor Ambarus <tudor.ambarus@...aro.org>
Cc:     Pratyush Yadav <pratyush@...nel.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org
Subject: Re: [PATCH v2 05/41] mtd: spi-nor: convert .n_sectors to .size

Am 2023-08-24 10:25, schrieb Tudor Ambarus:
> On 8/22/23 08:09, Michael Walle wrote:
>> .n_sectors is rarely used. In fact it is only used in swp.c and to
>> calculate the flash size in the core. The use in swp.c might be
>> converted to use the (largest) flash erase size. For now, we just
>> locally calculate the sector size.
>> 
>> Simplify the flash_info database and set the size of the flash 
>> directly.
>> This also let us use the SZ_x macros.
>> 
>> Signed-off-by: Michael Walle <mwalle@...nel.org>
>> ---
>>  drivers/mtd/spi-nor/core.c   | 2 +-
>>  drivers/mtd/spi-nor/core.h   | 8 ++++----
>>  drivers/mtd/spi-nor/swp.c    | 9 +++++----
>>  drivers/mtd/spi-nor/xilinx.c | 4 ++--
>>  4 files changed, 12 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 286155002cdc..f4cc2eafcc5e 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2999,7 +2999,7 @@ static void spi_nor_init_default_params(struct 
>> spi_nor *nor)
>> 
>>  	/* Set SPI NOR sizes. */
>>  	params->writesize = 1;
>> -	params->size = (u64)info->sector_size * info->n_sectors;
>> +	params->size = info->size;
> 
> would be good to check the sanity of info->size to not be null and to 
> be
> divisible by sector_size.

Have a look at the later patches, info->size can be zero, indicating
that we need to parse SFDP for this flash.

I could validate that the size is a multiple of sector_size, but
that one might also be zero. Of course you could solve this by
additional logic. But I treated that one as a misconfiguration
of the flash_info entry. It's nothing which can happen during
runtime. Anyway, I have no strong opinion on the "is multiple
of sector_size" check (for now, maybe there's something I didn't
thought of now).

> 
>>  	params->bank_size = params->size;
>>  	params->page_size = info->page_size;
>> 
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index dfc20a3296fb..12c35409493b 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -443,9 +443,9 @@ struct spi_nor_fixups {
>>   * @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:    the size listed here is what works with 
>> SPINOR_OP_SE, which
>>   *                  isn't necessarily called a "sector" by the 
>> vendor.
>> - * @n_sectors:      the number of sectors.
>>   * @n_banks:        the number of banks.
>>   * @page_size:      the flash's page size.
>>   * @addr_nbytes:    number of address bytes to send.
>> @@ -505,8 +505,8 @@ struct flash_info {
>>  	char *name;
>>  	u8 id[SPI_NOR_MAX_ID_LEN];
>>  	u8 id_len;
>> +	size_t size;
>>  	unsigned sector_size;
>> -	u16 n_sectors;
>>  	u16 page_size;
>>  	u8 n_banks;
>>  	u8 addr_nbytes;
>> @@ -556,8 +556,8 @@ struct flash_info {
>>  	.id_len = 6
>> 
>>  #define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks)		\
>> +	.size = (_sector_size) * (_n_sectors),				\
>>  	.sector_size = (_sector_size),					\
>> -	.n_sectors = (_n_sectors),					\
>>  	.page_size = 256,						\
>>  	.n_banks = (_n_banks)
>> 
>> @@ -575,8 +575,8 @@ struct flash_info {
>>  	SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 1),
>> 
>>  #define CAT25_INFO(_sector_size, _n_sectors, _page_size, 
>> _addr_nbytes)	\
>> +		.size = (_sector_size) * (_n_sectors),			\
>>  		.sector_size = (_sector_size),				\
>> -		.n_sectors = (_n_sectors),				\
>>  		.page_size = (_page_size),				\
>>  		.n_banks = 1,						\
>>  		.addr_nbytes = (_addr_nbytes),				\
>> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
>> index 5ab9d5324860..40bf52867095 100644
>> --- a/drivers/mtd/spi-nor/swp.c
>> +++ b/drivers/mtd/spi-nor/swp.c
>> @@ -34,17 +34,18 @@ static u8 spi_nor_get_sr_tb_mask(struct spi_nor 
>> *nor)
>>  static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
>>  {
>>  	unsigned int bp_slots, bp_slots_needed;
>> +	unsigned int sector_size = nor->info->sector_size;
>> +	u64 n_sectors = div_u64(nor->params->size, sector_size);
> 
> if params(info)->size is zero here, we get into trouble.

Please note that this is not the info->size, params->size cannot
be zero. If info->size was zero, we have to parse SFDP which hopefully
will give us a sane size.

And regarding the sector_size, which could be zero here. I want to
replace that logic, so we don't rely on the flash_info sector size.
 From a later patch:

         /*
          * sector_size will eventually be replaced with the max erase 
size of
          * the flash. For now, we need to have that ugly default.
          */

-michael