Re: [PATCH v6 1/2] mtd: spi-nor: Delay the initialization of bank_size

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Tudor,

tudor.ambarus@linaro.org wrote on Mon, 3 Apr 2023 09:23:24 +0100:

> On 3/31/23 20:46, Miquel Raynal wrote:
> > Bank size is derived from the chip's size, which in
> > spi_nor_init_default_params() can still be zero if the size is not
> > hardcoded in a table (eg. sfdp parsing). We need to delay the
> > initialization of this field in the "late" helpers.
> > 
> > Based on the fact that RWW compatible devices will use the bank_size to
> > derive in which bank an access is made, we absolutely want to avoid
> > divisions by 0. Right now there is no such opportunity between
> > spi_nor_init_default_params() and spi_nor_late_init_params() but it is
> > probably safer to enforce a default value (assuming 1 bank) as early as
> > possible, just in case some day code using this variable is moved
> > earlier than expected.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > I made the choice to keep a default value, to be honest this is not
> > actually needed today, so feel free to drop the line if you don't like
> > it, both choices seem reasonable to me.
> > ---
> >  drivers/mtd/spi-nor/core.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 9e6a0730cdb8..f436395ca489 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2882,6 +2882,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
> >  	 */
> >  	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
> >  		spi_nor_init_default_locking_ops(nor);
> > +
> > +	nor->params->bank_size = div64_u64(nor->params->size, nor->info->n_banks);
> >  }
> >  
> >  /**
> > @@ -2948,7 +2950,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->bank_size = div64_u64(params->size, info->n_banks);
> > +	params->bank_size = params->size;  
> 
> I'll get rid of this line entirely as you overwrite it anyway in
> late_init and it's not used in between.

Yes, as said above, I'm fine with both approaches. Shall I resend or
will you edit when applying?

> 
> >  	params->page_size = info->page_size;
> >  
> >  	if (!(info->flags & SPI_NOR_NO_FR)) {  


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/