Hello Simon,
On 09.10.26 09:00, Simon Glass wrote:
Hi Heiko,
On 2026-10-04T04:06:44, Heiko Schocher <[email protected]> wrote:
mtd: refuse an erase on a device that cannot erase
mtd_erase() calls mtd->_erase() without checking that the driver has
one, so a device which does not provide one will end in accessing
NULL pointer.
MTD_NO_ERASE flag exists, but nobody reads it yet.
This sentence is confusing. spi-nor-core.c already sets MTD_NO_ERASE
for SPI_NOR_NO_ERASE parts, which still have an _erase and a non-zero
erasesize. This patch doesn't read the flag either, so it would be
clearer to drop the sentence or to say why the check uses erasesize
and _erase rather than the flag. Also, how about 'jumps to a NULL
pointer' instead of 'will end in accessing NULL pointer'?
I drop the sentence which is wrong. Also reword to "jumps to a NULL
pointer"
Linux guards mtd_erase() with
if (!mtd->erasesize || !master->_erase)
return -ENOTSUPP;
so do the same in U-Boot, but with EOPNOTSUPP as the error code,
as checkpatch asks for ("ENOTSUPP is not a SUSV4 error code, prefer
EOPNOTSUPP").
Signed-off-by: Heiko Schocher <[email protected]>
drivers/mtd/mtdcore.c | 3 +++
drivers/mtd/mtdpart.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
@@ -930,6 +930,9 @@ EXPORT_SYMBOL_GPL(__put_mtd_device);
int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
{
+ if (!mtd->erasesize || !mtd->_erase)
+ return -EOPNOTSUPP;
+
From what I can see, but 'mtd erase' never gets this far.
do_mtd_erase() first calls mtd_is_aligned_with_block_size(), which
does do_div(size, mtd->erasesize), then mtd_div_by_eb(). With
erasesize == 0 both divide by zero.
do_mtd_io() also calls mtd_is_aligned_with_block_size(mtd, off) for
every page, so I think 'mtd read' and 'mtd write' hit it too. The test
in patch 5 calls mtd_read() and mtd_write() directly, so it doesn't
catch this.
Please can you make mtd_is_aligned_with_block_size() cope with
erasesize == 0 (e.g. treating it as 'no blocks'), and have
do_mtd_erase() bail out early with a message for such a device? A
small test that runs 'mtd read', 'mtd write' and 'mtd erase' against
the SRAM would cover it.
Of course! I think I start with a test/cmd/mtd.c
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
@@ -654,7 +654,8 @@ static struct mtd_info *allocate_partition(struct mtd_info
*master,
- slave->_erase = part_erase;
+ if (master->_erase)
+ slave->_erase = part_erase;
This is fine, but please mention it in the commit message: a partition
only gets part_erase() when its master has an _erase, so part_erase()
can't call through a NULL master->_erase either.
Will add this.
With the above:
Reviewed-by: Simon Glass <[email protected]>
Will add this too, thanks for your review!
bye,
Heiko
Regards,
Simon
--
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: [email protected]
Geschäftsführer : Stefano Babic
