Re: [PATCH v2 1/3] mtd: spi-nor: fix the lock left held by spi_nor_rww_start_exclusive()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing spi_nor_unprep() on error path when wait_event_killable() fails in spi_nor_prep_and_lock() and its variants.
--

commit c817801772cd87c9d5fe101e67f14bf0d06c97df
Author: Itai Handler <itai.handler@gmail.com>
Subject: mtd: spi-nor: fix the lock left held by spi_nor_rww_start_exclusive()

This commit changes spi_nor_rww_start_exclusive() to use scope-based
mutex cleanup helpers instead of manually locking without unlocking.
This fixes a deadlock and allows operations to proceed when the flash
is busy by ensuring the mutex is released upon returning.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ccf4396cdcd04..8bc117b46e028 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1310,7 +1310,7 @@ static bool spi_nor_rww_start_exclusive(struct spi_nor *nor)
>  {
>  	struct spi_nor_rww *rww = &nor->rww;
>  
> -	mutex_lock(&nor->lock);
> +	guard(mutex)(&nor->lock);
>  
>  	if (rww->ongoing_io || rww->ongoing_rd || rww->ongoing_pe)
>  		return false;

[Severity: High]
This is a pre-existing issue, but while reviewing the wait condition here,
I noticed a missing cleanup path in spi_nor_prep_and_lock() and its program,
erase, and read variants.

Looking at spi_nor_prep_and_lock() in drivers/mtd/spi-nor/core.c, it starts
by calling spi_nor_prep() which acquires resources like a hardware mutex and
clock. When parallel locking is enabled, it calls wait_event_killable():

drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock() {
    ...
	ret = spi_nor_prep(nor);
	if (ret)
		return ret;

	if (!spi_nor_use_parallel_locking(nor))
		mutex_lock(&nor->lock);
	else
		ret = wait_event_killable(nor->rww.wait,
					  spi_nor_rww_start_exclusive(nor));

	return ret;
}

If wait_event_killable() is interrupted by a fatal signal, it returns
-ERESTARTSYS. Does this directly return the error and skip spi_nor_unprep()?

Since the controller's .prepare hook acquires hardware resources, would
returning directly here cause a permanent deadlock on the next access and a
resource leak?

Userspace triggering I/O on an RWW flash can be signaled while waiting for
ongoing I/O to finish, so this error path seems reachable. 

Does this same pattern appear in spi_nor_prep_and_lock_pe() using
spi_nor_rww_start_pe() and in spi_nor_prep_and_lock_rd() using
spi_nor_rww_start_rd()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914081149.1916589-1-itai.handler@gmail.com?part=1

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