Re: [PATCH 02/19] mtd: spi-nor: swp: Improve locking user experience

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]

On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
> In the case of a single block being locked, if the user want to fully
> unlock the device it has two possibilities:
> - either it asks to unlock the entire device, and this works;
> - or it asks to unlock just the blocks that are currently locked, which
> fails.
>
> It fails because the conditions "can_be_top" and "can_be_bottom" are
> true. Indeed, in this case, we unlock everything, to the TB bit does not
> matter. However in the current implementation, use_top would be true (as
> this is the favourite option) and lock_len, which in practice should be
> reduced down to 0, is set to "nor->params->size - (ofs + len)" which is
> a positive number. This is wrong.

This only happens if you try to unlock the first sector, correct? If
my maths are correct, trying it on the last sector, lock_len should
be 0, i.e in that case "ofs + len == size".

If it's the first sector (or sectors), lock_len will end up with
"size - N * 64k", which is clearly wrong.

> An easy way is to simply add an extra condition. In the unlock() path,
> if we can achieve the results from both sides, it means we unlock
> everything and lock_len must simply be 0.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> For me, this result was clearly unexpected, but I am not sure this
> qualifies as a fix.

That's definetly a bug, esp. because it will lock an entire
unrelated region. And it seems to go back all the to commit
3dd8012a8eeb "mtd: spi-nor: add TB (Top/Bottom) protect support").

> ---
>  drivers/mtd/spi-nor/swp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
> index 9b07f83aeac76dce2109f90dfa1534c9bd93330d..9bc5a356444665ad8824e9e12d679fd551b3e67d 100644
> --- a/drivers/mtd/spi-nor/swp.c
> +++ b/drivers/mtd/spi-nor/swp.c
> @@ -281,7 +281,9 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
>  	use_top = can_be_top;
>  
>  	/* lock_len: length of region that should remain locked */
> -	if (use_top)
> +	if (can_be_top && can_be_bottom)
> +		lock_len = 0;

Could you please add a comment stating that if both are true, it
means that both adjacent regions are unlocked and thus the entire
flash will be unlocked.

-michael

> +	else if (use_top)
>  		lock_len = nor->params->size - (ofs + len);
>  	else
>  		lock_len = ofs;


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]