Re: spi-nor: excessive locking in spi_nor_erase()

[Pratyush Yadav <pratyush@kernel.org>]

Hi,

On Mon, Apr 15 2024, Joakim Tjernlund wrote:

> static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
> {
> ...
> Lock here
> 	ret = spi_nor_lock_and_prep(nor);
> 	if (ret)
> 		return ret;
> ....
> Then we have:
> 	} else if (spi_nor_has_uniform_erase(nor)) {
> 		while (len) {
> 			ret = spi_nor_write_enable(nor);
> 			if (ret)
> 				goto erase_err;
>
> 			ret = spi_nor_erase_sector(nor, addr);
> 			if (ret)
> 				goto erase_err;
>
> 			ret = spi_nor_wait_till_ready(nor);
> 			if (ret)
> 				goto erase_err;
>
> 			addr += mtd->erasesize;
> 			len -= mtd->erasesize;
> 		}
>
> 	/* erase multiple sectors */
> 	} else {
> 		ret = spi_nor_erase_multi_sectors(nor, addr, len);
> 		if (ret)
> 			goto erase_err;
> 	}
> ....
> erase_err:
> unlock here
> 	spi_nor_unlock_and_unprep(nor);
>
> 	return ret;
> }

Firstly, seems like you are looking at an older version of the driver.
spi_nor_erase() looks a bit different now, though works pretty much the
same in practice. See [0].

>
> So erase locks the flash for the whole erase op which can include many sectors and I don't see
> any Erase Suspend handling in spi-nor like the cfi_cmdset_0001/cfi_cmdset_0002 has.
>
> Locking can be a challenge but this seems over the top, anyone working/looking into improving this?

I think you should lead with what the problem you see with this and what
you think can be improved. Why do you think the locking is "over the
top"? What should be improved?

Most flashes can't do any operations (except poll status register) when
a program or erase operation is in progress. So the locking here makes
sense to me. We do not want to let other tasks attempt any other
operations until the erase is done. The only exception is some
multi-bank flashes that can do erase on one bank and reads on other
banks. Miquel's series [1] takes care of those (though I do not see any
flashes using that feature yet).

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mtd/spi-nor/core.c#n1789
[1] https://lore.kernel.org/linux-mtd/20230328154105.448540-1-miquel.raynal@bootlin.com/

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/