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'?
>
> 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.
> 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.
With the above:
Reviewed-by: Simon Glass <[email protected]>
Regards,
Simon
