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

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

On Wed Nov 19, 2025 at 10:49 AM CET, Miquel Raynal wrote:
> 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.

See my previous mail and commit 18d7d01a0a0e ("mtd: spi-nor: Avoid
setting SRWD bit in SR if WP# signal not connected"). Personally, I
really don't like the "software" write protection, I mean you can
just use read-only for that partition or whatever. Locking for me is
really backed by the hardware. I.e. we use that to be really sure,
that we have a bootable bootloader and no one can break it.

>>> 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"?

I don't have a strong opinion here, but locking might be on a finer
granularity than sectors. Not with the BP scheme but IIRC I've seen
locking schemes with 4k blocks for example. So maybe just something
more general like "locked_erase_blocks_map" or just
"locked_blocks_map", up to you.  It's just debugfs ;)

> [...]
>
>>> +	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.

Oh I meant caching it in the core/swp.c (and invalidating/updating
when the bits are written) and just display it here. That way we
just keep that reading this file won't actually trigger any SPI
xfers.

>>> +	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 :-)

:)

-michael