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

[Miquel Raynal <miquel.raynal@bootlin.com>]

On 18/11/2025 at 10:17:55 +01, "Michael Walle" <mwalle@kernel.org> wrote:

> 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.

That's it. Actually I forgot to mention it was happening only with the
first sectors, not the last ones, so yes you are correct, it matches my
maths and experiments.

>> 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.

Ofc.

Thanks,
Miquèl