| Message ID | 20261004040657.3156402-3-hs@nabladev.com |
|---|---|
| State | New |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | mtd: Microchip 23LCV1024 SPI SRAM | expand |
Hi Heiko, On 2026-10-04T04:06:44, Heiko Schocher <hs@nabladev.com> wrote: > mtd: align a partition on the write size if the device cannot erase > > allocate_partition() aligns a partition on the erase size of its master > and checks the offset and the size against it with mtd_mod_by_eb(). A > device which cannot erase has no erase size, so that check divides by > zero, "mtd list" shows > > ### ERROR ### Please RESET the board ### > > Linux aligns the partition of such a device on the write size instead, > see wr_alignment in allocate_partition() in drivers/mtd/mtdpart.c. Do > the same here, and take the alignment for the MTDPART_OFS_NXTBLK > rounding from the same place, which divided by the erase size as well. > > Signed-off-by: Heiko Schocher <hs@nabladev.com> > > drivers/mtd/mtdpart.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > @@ -563,8 +563,12 @@ static struct mtd_info *allocate_partition(struct mtd_info *master, > + u32 wr_alignment = (master->flags & MTD_NO_ERASE) ? master->writesize : > + master->erasesize; Patch 1 decides that a device cannot erase with !mtd->erasesize || !mtd->_erase, but this patch checks MTD_NO_ERASE, and the two don't always agree. A driver that leaves erasesize at 0 without setting MTD_NO_ERASE still divides by zero here, as does an MTD_NO_ERASE driver that leaves writesize at 0. Since the point is to stop 'mtd list' crashing, I suggest skipping the alignment checks, or adding a WARN_ON(), when wr_alignment is 0, rather than relying on every driver setting the flags correctly. What do you think? > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > @@ -731,19 +737,30 @@ static struct mtd_info *allocate_partition(struct mtd_info *master, > + tmp = slave->offset; > + remainder = do_div(tmp, wr_alignment); > + if ((slave->flags & MTD_WRITEABLE) && remainder) { > /* Doesn't start on a boundary of major erase size */ This comment is now out of date. In any case: Reviewed-by: Simon Glass <sjg@chromium.org> Regards, Simon
Hello Simon, On 09.10.26 09:02, Simon Glass wrote: > Hi Heiko, > > On 2026-10-04T04:06:44, Heiko Schocher <hs@nabladev.com> wrote: >> mtd: align a partition on the write size if the device cannot erase >> >> allocate_partition() aligns a partition on the erase size of its master >> and checks the offset and the size against it with mtd_mod_by_eb(). A >> device which cannot erase has no erase size, so that check divides by >> zero, "mtd list" shows >> >> ### ERROR ### Please RESET the board ### >> >> Linux aligns the partition of such a device on the write size instead, >> see wr_alignment in allocate_partition() in drivers/mtd/mtdpart.c. Do >> the same here, and take the alignment for the MTDPART_OFS_NXTBLK >> rounding from the same place, which divided by the erase size as well. > >> >> Signed-off-by: Heiko Schocher <hs@nabladev.com> >> >> drivers/mtd/mtdpart.c | 35 ++++++++++++++++++++++++++--------- >> 1 file changed, 26 insertions(+), 9 deletions(-) > >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> @@ -563,8 +563,12 @@ static struct mtd_info *allocate_partition(struct mtd_info *master, >> + u32 wr_alignment = (master->flags & MTD_NO_ERASE) ? master->writesize : >> + master->erasesize; > > Patch 1 decides that a device cannot erase with !mtd->erasesize || > !mtd->_erase, but this patch checks MTD_NO_ERASE, and the two don't > always agree. A driver that leaves erasesize at 0 without setting > MTD_NO_ERASE still divides by zero here, as does an MTD_NO_ERASE > driver that leaves writesize at 0. Since the point is to stop 'mtd > list' crashing, I suggest skipping the alignment checks, or adding a > WARN_ON(), when wr_alignment is 0, rather than relying on every driver > setting the flags correctly. What do you think? Makes sense, I think I remove the alignment checks and add a WARN_ON() > >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c >> @@ -731,19 +737,30 @@ static struct mtd_info *allocate_partition(struct mtd_info *master, >> + tmp = slave->offset; >> + remainder = do_div(tmp, wr_alignment); >> + if ((slave->flags & MTD_WRITEABLE) && remainder) { >> /* Doesn't start on a boundary of major erase size */ > > This comment is now out of date. Will fix this in v3. > > In any case: > > Reviewed-by: Simon Glass <sjg@chromium.org> Thanks! bye, HEiko > > Regards, > Simon >
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index a3003cf44b0..eed0add71ff 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -563,8 +563,12 @@ static struct mtd_info *allocate_partition(struct mtd_info *master, const struct mtd_partition *part, int partno, uint64_t cur_offset) { + u32 wr_alignment = (master->flags & MTD_NO_ERASE) ? master->writesize : + master->erasesize; struct mtd_info *slave; + u32 remainder; char *name; + u64 tmp; /* allocate the partition structure */ slave = kzalloc(sizeof(*slave), GFP_KERNEL); @@ -665,9 +669,11 @@ static struct mtd_info *allocate_partition(struct mtd_info *master, slave->offset = cur_offset; if (slave->offset == MTDPART_OFS_NXTBLK) { slave->offset = cur_offset; - if (mtd_mod_by_eb(cur_offset, master) != 0) { - /* Round up to next erasesize */ - slave->offset = (mtd_div_by_eb(cur_offset, master) + 1) * master->erasesize; + tmp = cur_offset; + remainder = do_div(tmp, wr_alignment); + if (remainder) { + /* Round up to the next alignment boundary */ + slave->offset += wr_alignment - remainder; debug("Moving partition %d: " "0x%012llx -> 0x%012llx\n", partno, (unsigned long long)cur_offset, (unsigned long long)slave->offset); @@ -731,19 +737,30 @@ static struct mtd_info *allocate_partition(struct mtd_info *master, slave->erasesize = master->erasesize; } - if ((slave->flags & MTD_WRITEABLE) && - mtd_mod_by_eb(slave->offset, slave)) { + /* + * The erase size of the partition may differ from the one of the + * master if the master exposes several erase regions. Align to the + * one the partition ended up with. + */ + if (!(slave->flags & MTD_NO_ERASE)) + wr_alignment = slave->erasesize; + + tmp = slave->offset; + remainder = do_div(tmp, wr_alignment); + if ((slave->flags & MTD_WRITEABLE) && remainder) { /* Doesn't start on a boundary of major erase size */ /* FIXME: Let it be writable if it is on a boundary of * _minor_ erase size though */ slave->flags &= ~MTD_WRITEABLE; - printk(KERN_WARNING"mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n", + printk(KERN_WARNING"mtd: partition \"%s\" doesn't start on an erase/write block boundary -- force read-only\n", part->name); } - if ((slave->flags & MTD_WRITEABLE) && - mtd_mod_by_eb(slave->size, slave)) { + + tmp = slave->size; + remainder = do_div(tmp, wr_alignment); + if ((slave->flags & MTD_WRITEABLE) && remainder) { slave->flags &= ~MTD_WRITEABLE; - printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n", + printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase/write block -- force read-only\n", part->name); }
allocate_partition() aligns a partition on the erase size of its master and checks the offset and the size against it with mtd_mod_by_eb(). A device which cannot erase has no erase size, so that check divides by zero, "mtd list" shows ### ERROR ### Please RESET the board ### Linux aligns the partition of such a device on the write size instead, see wr_alignment in allocate_partition() in drivers/mtd/mtdpart.c. Do the same here, and take the alignment for the MTDPART_OFS_NXTBLK rounding from the same place, which divided by the erase size as well. Signed-off-by: Heiko Schocher <hs@nabladev.com> --- checkpatch reports two warnings and four checks on the two printk(KERN_WARNING"mtd: partition ... force read-only\n") lines. This patch only changes s|erase block|erase/write block in the string of the print, so ignoring the warnings. Changes in v2: - New patch mtd: align a partition on the write size if the device cannot erase Testing on the board dropped for "mtd list" command: ### ERROR ### Please RESET the board ### This is because division by erase_size, which the SPI RAM driver sets to 0. Added for this case also sandbox test. drivers/mtd/mtdpart.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)