Messages in this thread | | | Date | Mon, 22 May 2023 11:50:56 +0200 | From | Miquel Raynal <> | Subject | Re: [PATCH] mtd: spi-nor: Fix divide by zero for spi-nor-generic flashes |
| |
Hello,
hdegoede@redhat.com 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@linaro.org 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@linux.intel.com> > >>> Reported-by: Todd Brandt <todd.e.brandt@linux.intel.com> > >>> 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@intel.com/ > >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > >>> --- > >>> 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
| |