Re: [PATCH v2 1/2] mtd: spi-nor: allow the platform to supply write protection state

[Michael Walle]

On Tue Sep 1, 2026 at 10:32 AM CEST, Tobias Jakobsen wrote:
>
> Hello!
>
> On Monday, 31 August 2026 at 14:37, Michael Walle <mwalle@xxxxxxxxxx> wrote:
>> > +static int spi_nor_platform_is_locked(struct spi_nor *nor, loff_t ofs, u64 len)
>> > +{
>> > +	struct flash_platform_data *data = dev_get_platdata(nor->dev);
>> > +
>> > +	return data->is_locked(nor->spimem->spi, ofs, len);
>> > +}
>> > +
>> > +static const struct spi_nor_locking_ops spi_nor_platform_locking_ops = {
>> > +	.lock = spi_nor_platform_lock,
>> > +	.unlock = spi_nor_platform_unlock,
>> > +	.is_locked = spi_nor_platform_is_locked,
>> > +};
>> 
>> If we can't unlock the flash, what's it's use then? Can't we just
>> clear the HAS_LOCK if there is an intel-spi driver?
>
> Clearing HAS_LOCK would replace a wrong answer with no answer
> (-EOPNOTSUPP). That is an improvement, but it discards information the
> kernel already has, since spi-intel reads the protected range registers
> at probe anyway for the MTD_WRITEABLE masking in
> intel_spi_fill_partition(). Userspace is then left parsing the Intel
> specific sysfs attributes to find out, which is the platform specific
> special casing this series is trying to remove the need for.
>
> If I understood correctly MEMISLOCKED is a query rather than a control.
> "check if chip is locked" with no qualifier restricting it to the
> chip's own block protection bits. On PCH protected machines the answer it
> gives is wrong: the region is protected and it reports otherwise.

This feels like I'm talking with an AI agent. Honestly, this is
rather discouraging.

So my short answer: I don't want to clutter the code just for some
weird behavior and my point stands: whats the use, if it's not
possible to unprotect that region. The intel-spi controller is
rather restrictive anyway.

-michael


> spi-nor cannot tell which controller it sits behind,
> so suppressing HAS_LOCK needs the same channel through
> flash_platform_data; only the payload changes.
>
> The reason it is a callback rather than a flag is that the answer is per
> range. On the machine I tested, PR0 covers 0x860000-0xffffff of a 16M
> chip:
>
>   query 0x860000 + 0x7a0000   -> locked
>   query 0x0      + 0x1000000  -> not locked
>   query 0x0      + 0x10000    -> not locked
>
> A boolean would have to claim the whole device is locked, which is wrong
> for everything below 0x860000.
>
> That said, if you would rather have the simpler suppression, I am happy
> to do that instead.
>
>> >  	/*
>> > -	 * NOR protection support. When locking_ops are not provided, we pick
>> > -	 * the default ones.
>> > +	 * NOR protection support. Platform enforced protection is preferred
>> > +	 * over the chip's own, as the chip is not necessarily aware of it.
>> > +	 * When locking_ops are not provided, we pick the default ones.
>> >  	 */
>> 
>> This doesn't work, does it? What if a flash already provide locking
>> ops?
>
> You are right, it does not. The vendor late_init hooks run before this
> (core.c 3063 and 3072), so atmel.c and sst.c have already set
> params->locking_ops by then and the platform query is skipped. A chip
> with its own locking ops behind an Intel PCH would still report the
> chip's answer.
>
> I will fix that in v3 by having the platform ops take precedence
> unconditionally rather than only filling in when nothing else has.
> Let me know how you'd like to proceed regarding HAS_LOCK clearing.
>
> Regards,
> Tobias.