Re: [PATCH v2 2/3] mtd: cfi_cmdset_0002: implement point() for simple linear maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: cfi_cmdset_0002: FL_POINT deadlocks and hangs XIP erases via FL_XIP_WHILE_ERASING
--

commit 5f3b3543d5ee6dad5e8171d86ea7a290abab737e
Author: Orgad Shaneh <orgads@gmail.com>

mtd: cfi_cmdset_0002: implement point() for simple linear maps

This commit adds a point() and unpoint() pair for cfi_cmdset_0002, modeled
on cfi_intelext_point(), allowing linearly mapped chips to be scanned in
place. It handles FL_POINT operations and ensures that a point does not
suspend an erase, but waits for it to finish instead.

> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 517db2f2707f4..b0330839a2133 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c

[ ... ]

> @@ -906,8 +926,12 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
>  		return 0;
>  
>  	case FL_ERASING:
> +		/*
> +		 * Not for a point: the erase would stay suspended for as
> +		 * long as it is held, which for cramfs is the whole mount.
> +		 */
>  		if (!cfip || !(cfip->EraseSuspend & (0x1|0x2)) ||
> -		    !(mode == FL_READY || mode == FL_POINT ||
> +		    !(mode == FL_READY ||
>  		    (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))))
>  			goto sleep;

[Severity: High]
This patch correctly prevents FL_POINT from suspending an erase here, but
does this also need to be applied to FL_XIP_WHILE_ERASING?

If mtd_point() is called during FL_XIP_WHILE_ERASING, get_chip() still
allows mode == FL_POINT:

drivers/mtd/chips/cfi_cmdset_0002.c:get_chip() {
    ...
	case FL_XIP_WHILE_ERASING:
		if (mode != FL_READY && mode != FL_POINT &&
		    (!cfip || !(cfip->EraseSuspend&2)))
			goto sleep;
		chip->oldstate = chip->state;
		chip->state = FL_READY;
		return 0;
    ...
}

This allows do_point_onechip() to successfully change chip->state to FL_POINT
while leaving chip->oldstate as FL_XIP_WHILE_ERASING.

[ ... ]

> @@ -960,8 +984,13 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
>  		return -EIO;
>  
>  	case FL_POINT:
> -		/* Only if there's no operation suspended... */
> -		if (mode == FL_READY && chip->oldstate == FL_READY)
> +		/*
> +		 * Only if there's no operation suspended: the chip is in
> +		 * array mode, so reads, further points and the reboot
> +		 * reset (which only re-enters array mode) can go ahead.
> +		 */
> +		if ((mode == FL_READY || mode == FL_POINT ||
> +		     mode == FL_SHUTDOWN) && chip->oldstate == FL_READY)
>  			return 0;
>  		fallthrough;
>  	default:

[Severity: High]
If chip->oldstate is FL_XIP_WHILE_ERASING because it wasn't filtered out
above, will subsequent operations deadlock here?

Because oldstate is not FL_READY, normal read operations (FL_READY) would
fall through to the sleep block indefinitely until the point is released
(which for cramfs is the mount lifetime).

Could this also cause the XIP background erase loop to spin indefinitely
waiting for chip->state to return to FL_XIP_WHILE_ERASING?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010180840.2152492-1-orgads@gmail.com?part=2