Re: [PATCH v3] mtd: spi-nor: use guard() in spi_nor_rww_start_exclusive

[Tudor Ambarus <tudor.ambarus@linaro.org>]

Hi, Runyu, Miquel,

On 8/11/26 1:11 PM, Tudor Ambarus wrote:
> 
> 
> On 8/11/26 11:31 AM, Runyu Xiao wrote:
>> Commit 03e7bb864d9a ("mtd: spi-nor: use scope-based mutex cleanup
>> helpers") converted the RWW start helpers to scoped mutex cleanup, but
>> left spi_nor_rww_start_exclusive() using a plain mutex_lock().
>>
>> spi_nor_rww_start_exclusive() is used as a wait_event_killable()
>> condition. When it returns false on the busy path, the raw mutex_lock()
>> leaves nor->lock held and blocks the operation that must clear the RWW
>> state in spi_nor_rww_end_exclusive().
>>
>> Use guard(mutex) here as well so this helper matches the other RWW
>> start helpers and releases nor->lock on the false return path.
>>
>> Fixes: 03e7bb864d9a ("mtd: spi-nor: use scope-based mutex cleanup helpers")
> 
> The fixes tag is wrong. The blamed commit did not change the functionality
> of the code, while yours does.
> 
> You shall instead blame the commit that failed to release the lock in the
> first place. But then if you want to have this backported to stable kernels
> you may want to add 03e7bb864d9a ("mtd: spi-nor: use scope-based mutex
> cleanup helpers") as a prerequisite patch, as it includes <linux/cleanup.h>.
> 

Runyu, please also update the commit subject, the point of the patch is
that it fixes a mutex leak, thus I suggest to reword it with something like
"mtd: spi-nor: core: Fix mutex leak in spi_nor_rww_start_exclusive()"

>> Cc: stable@vger.kernel.org
>> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
>> ---
>> Changes in v3:
>> - Reframe the changelog around the scoped-helper conversion that missed
>>   spi_nor_rww_start_exclusive().
>>
>> Changes in v2:
>> - Replace the analysis-style explanation with a short maintainer-facing
>>   description of the busy-path lock hold.
>> - Keep the fix focused on using guard(mutex) so the mutex is released on
>>   both paths.
>>
>> drivers/mtd/spi-nor/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index ccf4396cdcd0..8bc117b46e02 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);
>>  

Miquel,

Have you seen sashiko's review at 
https://sashiko.dev/#/patchset/20260811083111.403453-1-runyu.xiao%40seu.edu.cn?

Both concerns look valid to me. Do you think you can allocate time to fix them?

Cheers,
ta

>>  	if (rww->ongoing_io || rww->ongoing_rd || rww->ongoing_pe)
>>  		return false;
>