Re: [PATCH 15/19] mtd: spi-nor: debugfs: Add locking support

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

Hello,

On 18/11/2025 at 13:46:52 +01, "Michael Walle" <mwalle@kernel.org> wrote:

> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>> The ioctl output may be counter intuitive in some cases. Asking for a
>> "locked status" over a region that is only partially locked will return
>> "unlocked" whereas in practice maybe the biggest part is actually
>> locked.
>>
>> Knowing what is the real software locking state through debugfs would be
>> very convenient for development/debugging purposes, hence this proposal
>> for adding two extra blocks at the end of the file:
>> - A "software locked sectors" array which lists every section, if it is
>> locked or not, showing both the address ranges and the sizes in numbers
>> of blocks.
>
> I know the file is called software write protection (or swp) but
> it's really a hardware write protection, isn't it?

Well, it depends on your configuration I guess? Without #WP pin I don't
know how to call that. I had in mind that software meant "using the BP
pins" and "hardware" meant "toggling #WP". But I have no strong opinion
about this wording.

>> - Some kind of mapping of the locked sectors, which pictures the entire
>> flash. It may be verbose, so perhaps we'll drop it in the end. I found
>> it very useful to really get a clearer mental model of what was
>> locked/unlocked, but the array just before is already a good source of
>> information.
>>
>> Here is an example of output, what is after the "sector map" is new.
>>
>> $ cat /sys/kernel/debug/spi-nor/spi0.0/params
>> name		(null)
>> id		ef a0 20 00 00 00
>> size		64.0 MiB
>> write size	1
>> page size	256
>> address nbytes	4
>> flags		HAS_SR_TB | 4B_OPCODES | HAS_4BAIT | HAS_LOCK | HAS_16BIT_SR | HAS_SR_TB_BIT6 | HAS_4BIT_BP | SOFT_RESET | NO_WP
>>
>> opcodes
>>  read		0xec
>>   dummy cycles	6
>>  erase		0xdc
>>  program	0x34
>>  8D extension	none
>>
>> protocols
>>  read		1S-4S-4S
>>  write		1S-1S-4S
>>  register	1S-1S-1S
>>
>> erase commands
>>  21 (4.00 KiB) [1]
>>  dc (64.0 KiB) [3]
>>  c7 (64.0 MiB)
>>
>> sector map
>>  region (in hex)   | erase mask | overlaid
>>  ------------------+------------+---------
>>  00000000-03ffffff |     [   3] | no
>>
>> software locked sectors
>
> drop "software" here.

Okay.

>
>>  region (in hex)   | status   | #blocks
>>  ------------------+----------+--------
>>  00000000-03ffffff | unlocked | 1024
>
> I really like that.

:-)

>> 64kiB-sectors locking map (x: locked, .: unlocked)
>> |.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
>>  ...........................|
>
> Maybe put it into an own file. In any case, a sane line wrapping
> would be good. And add a leading offset, ie, "0000: xxxx.....".

I was unsure about doing that, but yes that makes sense. May I call it
"locked_sectors_map"?

[...]

>> +	sr[0] = nor->bouncebuf[0];
>> +
>> +	if (!(nor->flags & SNOR_F_NO_READ_CR)) {
>> +		ret = spi_nor_read_cr(nor, nor->bouncebuf + 1);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	sr[1] = nor->bouncebuf[1];
>
> Shouldn't that go into the former if conditional? bouncebuf[1] might
> never be read.

Yes, that's correct. I don't remember why I did it this way, probably a
bug, I'll move that line.

> Also, until now, reading the "params" debug file never interacts
> with the flash, but with this patch it does. We don't do locking
> here which looks wrong. Maybe we should just cache the protection
> bits. Not sure.

I guess caching the status registers makes sense, but we'll still have a
possible race when accessing the 2 registers. Is it okay to
ignore this very unlikely case in debugfs? Otherwise I might just lock
the entire device for the time we access the cached registers.

>> +	spi_nor_get_locked_range_sr(nor, sr, &lock_start, &lock_length);
>> +	if (!lock_length || lock_length == params->size) {
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, params->size - 1,
>> +			   lock_length ? "  locked" : "unlocked", params->size / min_prot_len);
>> +	} else if (!lock_start) {
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_length - 1,
>> +			   "  locked", lock_length / min_prot_len);
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_length, params->size - 1,
>> +			   "unlocked", (params->size - lock_length) / min_prot_len);
>> +	} else {
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", 0ULL, lock_start - 1,
>> +			   "unlocked", lock_start / min_prot_len);
>> +		seq_printf(s, " %08llx-%08llx | %s | %llu\n", lock_start, params->size - 1,
>> +			   "  locked", lock_length / min_prot_len);
>> +	}
>> +
>> +	seq_printf(s, "\n%dkiB-sectors locking map (x: locked, .: unlocked)\n",
>> +		   min_prot_len / 1024);
>> +	seq_puts(s, "|");
>> +	for (i = 0; i < params->size; i += min_prot_len)
>> +		seq_printf(s, spi_nor_is_locked_sr(nor, i, min_prot_len, sr) ? "x" : ".");
>
> As mentioned above, newlines as well as a leading offset counter
> would be nice :)

Arf, I was hoping I could escape that step, but ok, fair enough :-)

Miquèl