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

[Tobias Jakobsen]

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.

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.