Re: [PATCH v2 05/41] mtd: spi-nor: convert .n_sectors to .size

[Michael Walle]

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@xxxxxxxxxx>
> > ---
> >  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