Re: [PATCH] mtd: spi-nor: Fix divide by zero for spi-nor-generic flashes

[Miquel Raynal]

Hello,

hdegoede@xxxxxxxxxx wrote on Mon, 22 May 2023 11:34:55 +0200:

> Hi,
> 
> On 5/22/23 11:22, Tudor Ambarus wrote:
> > 
> > 
> > On 5/22/23 09:29, Miquel Raynal wrote:  
> >> Hi Tudor,  
> > 
> > Hi, Miquel,
> >   
> >>
> >> tudor.ambarus@xxxxxxxxxx wrote on Thu, 18 May 2023 08:54:40 +0000:
> >>  
> >>> We failed to initialize n_banks for spi-nor-generic flashes, which
> >>> caused a devide by zero when computing the bank_size.
> >>>
> >>> By default we consider that all chips have a single bank. Initialize
> >>> the default number of banks for spi-nor-generic flashes. Even if the
> >>> bug is fixed with this simple initialization, check the n_banks value
> >>> before dividing so that we make sure this kind of bug won't occur again
> >>> if some other struct instance is created uninitialized.
> >>>
> >>> Suggested-by: Todd Brandt <todd.e.brandt@xxxxxxxxxxxxxxx>
> >>> Reported-by: Todd Brandt <todd.e.brandt@xxxxxxxxxxxxxxx>
> >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217448
> >>> Fixes: 9d6c5d64f028 ("mtd: spi-nor: Introduce the concept of bank")
> >>> Link: https://lore.kernel.org/all/20230516225108.29194-1-todd.e.brandt@xxxxxxxxx/
> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> >>> ---
> >>>  drivers/mtd/spi-nor/core.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >>> index 0bb0ad14a2fc..5f29fac8669a 100644
> >>> --- a/drivers/mtd/spi-nor/core.c
> >>> +++ b/drivers/mtd/spi-nor/core.c
> >>> @@ -2018,6 +2018,7 @@ static const struct spi_nor_manufacturer *manufacturers[] = {
> >>>  
> >>>  static const struct flash_info spi_nor_generic_flash = {
> >>>  	.name = "spi-nor-generic",
> >>> +	.n_banks = 1,  
> >>
> >> I definitely missed that structure.
> >>  
> >>>  	/*
> >>>  	 * JESD216 rev A doesn't specify the page size, therefore we need a
> >>>  	 * sane default.
> >>> @@ -2921,7 +2922,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);
> >>> +	if (nor->info->n_banks > 1)
> >>> +		params->bank_size = div64_u64(params->size, nor->info->n_banks);  
> >>
> >> I'm fine with the check as it is written because it also look like an
> >> optimization, but bank_size should never be 0 otherwise it's a real bug  
> > 
> > bank_size was introduced just for chips featuring several banks, but we
> > made this field mandatory for all flashes, regardless of their type. I
> > find this restriction unnecessary, because we can differentiate the RWW
> > flashes by checking the SNOR_F_RWW flag. So the alternative to this
> > patch is to remove the n_banks restriction and set it just for the RWW
> > flashes. I think I prefer this, but keep in mind that I never read a RWW
> > flash's datasheet, not publicly available, so the decision is in your
> > court. Happy to make a patch.  
> 
> Since this is causing a serious regression causing people's laptops to hang
> on suspend I believe it would be best to queue up the simplest fix for
> this (which seems to be this patch as is) for 6.4 asap.
> 
> Any more involved changes to fix this in a better way can then be added
> on top for the 6.5 cycle.

Absolutely. I actually am totally fine with the current state, I
believe expecting all flashes to have one bank is sane, that's why I
took this path in the first place. But this can be changed later
anyway.

Thanks,
Miquèl