| Message ID | 20260831-spi-nor-platform-lock-v2-1-6cc75b909241@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Report platform enforced SPI flash write protection | expand |
Hi, On Mon Aug 31, 2026 at 2:05 PM CEST, Tobias Jakobsen via B4 Relay wrote: > From: Tobias Jakobsen <tjakobsen84@protonmail.com> > > Some flashes are write protected by the platform they are attached to > rather than by their own block protection bits. An Intel PCH SPI > controller programmed with protected range registers is one example: it > refuses writes to a range regardless of what the chip's status register > says, while the chip's block protection bits are typically left clear. > > MEMISLOCKED therefore either fails with -EOPNOTSUPP, or, once the chip > gains SPI_NOR_HAS_LOCK, reports a range as unlocked while writes to it > are in fact being refused. > > Let the platform supply an optional is_locked() callback in struct > flash_platform_data, alongside the partitions it can already supply, and > prefer it over the chip's own block protection bits. This is independent > of SPI_NOR_HAS_LOCK, so an answer is also given for chips that have no > block protection support of their own. > > lock() and unlock() return -EOPNOTSUPP, as platform enforced protection > is not expected to be changed at runtime. > > Platforms that do not supply the callback are unaffected. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=221927 > Assisted-by: LLM > Signed-off-by: Tobias Jakobsen <tjakobsen84@protonmail.com> > --- > drivers/mtd/spi-nor/core.c | 52 ++++++++++++++++++++++++++++++++++++++++++++-- > include/linux/spi/flash.h | 12 +++++++++++ > 2 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index ccf4396cd..a5c37eea4 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3001,6 +3001,50 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor) > nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE; > } > > +static int spi_nor_platform_lock(struct spi_nor *nor, loff_t ofs, u64 len) > +{ > + return -EOPNOTSUPP; > +} > + > +static int spi_nor_platform_unlock(struct spi_nor *nor, loff_t ofs, u64 len) > +{ > + return -EOPNOTSUPP; > +} > + > +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? > + > +/** > + * spi_nor_init_platform_locking_ops() - Use the platform supplied write > + * protection query, if there is one. > + * @nor: pointer to a 'struct spi_nor' > + * > + * Some flashes are write protected by the platform they are attached to rather > + * than by their own block protection bits, for example by an Intel PCH SPI > + * controller programmed with protected range registers. In that case the chip's > + * block protection bits are typically left clear and say nothing about what is > + * actually enforced, so prefer the platform supplied query when available. > + */ > +static void spi_nor_init_platform_locking_ops(struct spi_nor *nor) > +{ > + struct flash_platform_data *data = dev_get_platdata(nor->dev); > + > + if (!data || !data->is_locked || !nor->spimem) > + return; > + > + nor->params->locking_ops = &spi_nor_platform_locking_ops; > +} > + > /** > * spi_nor_late_init_params() - Late initialization of default flash parameters. > * @nor: pointer to a 'struct spi_nor' > @@ -3040,9 +3084,13 @@ static int spi_nor_late_init_params(struct spi_nor *nor) > spi_nor_init_fixup_flags(nor); > > /* > - * 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? -michael > + if (!nor->params->locking_ops) > + spi_nor_init_platform_locking_ops(nor); > + > if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops) > spi_nor_init_default_locking_ops(nor); > > diff --git a/include/linux/spi/flash.h b/include/linux/spi/flash.h > index 2401a0887..f415e2c0b 100644 > --- a/include/linux/spi/flash.h > +++ b/include/linux/spi/flash.h > @@ -2,7 +2,10 @@ > #ifndef LINUX_SPI_FLASH_H > #define LINUX_SPI_FLASH_H > > +#include <linux/types.h> > + > struct mtd_partition; > +struct spi_device; > > /** > * struct flash_platform_data: board-specific flash data > @@ -11,6 +14,13 @@ struct mtd_partition; > * @nr_parts: number of mtd_partitions for static partitioning > * @type: optional flash device type (e.g. m25p80 vs m25p64), for use > * with chips that can't be queried for JEDEC or other IDs > + * @is_locked: optional callback to query write protection enforced by the > + * platform rather than by the flash chip itself, for example a SPI > + * controller that gates writes to a range of the flash. Returns 1 if > + * the whole range is protected, 0 if it is not, or a negative errno. > + * When supplied it takes precedence over the chip's own block > + * protection bits, which do not necessarily reflect what is actually > + * being enforced. > * > * Board init code (in arch/.../mach-xxx/board-yyy.c files) can > * provide information about SPI flash parts (such as DataFlash) to > @@ -26,6 +36,8 @@ struct flash_platform_data { > > char *type; > > + int (*is_locked)(struct spi_device *spi, loff_t ofs, u64 len); > + > /* we'll likely add more ... use JEDEC IDs, etc */ > }; >
Hello! On Monday, 31 August 2026 at 14:37, Michael Walle <mwalle@kernel.org> 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.
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@kernel.org> 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.
On Tuesday, 1 September 2026 at 10:43, Michael Walle <mwalle@kernel.org> wrote: > 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. Already disclosed LLM was used in translating and research/execution. But point taken. I will bow out. I never intended to write a patch but was asked. My intention was to merely report a bug.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index ccf4396cd..a5c37eea4 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3001,6 +3001,50 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor) nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE; } +static int spi_nor_platform_lock(struct spi_nor *nor, loff_t ofs, u64 len) +{ + return -EOPNOTSUPP; +} + +static int spi_nor_platform_unlock(struct spi_nor *nor, loff_t ofs, u64 len) +{ + return -EOPNOTSUPP; +} + +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, +}; + +/** + * spi_nor_init_platform_locking_ops() - Use the platform supplied write + * protection query, if there is one. + * @nor: pointer to a 'struct spi_nor' + * + * Some flashes are write protected by the platform they are attached to rather + * than by their own block protection bits, for example by an Intel PCH SPI + * controller programmed with protected range registers. In that case the chip's + * block protection bits are typically left clear and say nothing about what is + * actually enforced, so prefer the platform supplied query when available. + */ +static void spi_nor_init_platform_locking_ops(struct spi_nor *nor) +{ + struct flash_platform_data *data = dev_get_platdata(nor->dev); + + if (!data || !data->is_locked || !nor->spimem) + return; + + nor->params->locking_ops = &spi_nor_platform_locking_ops; +} + /** * spi_nor_late_init_params() - Late initialization of default flash parameters. * @nor: pointer to a 'struct spi_nor' @@ -3040,9 +3084,13 @@ static int spi_nor_late_init_params(struct spi_nor *nor) spi_nor_init_fixup_flags(nor); /* - * 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. */ + if (!nor->params->locking_ops) + spi_nor_init_platform_locking_ops(nor); + if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops) spi_nor_init_default_locking_ops(nor); diff --git a/include/linux/spi/flash.h b/include/linux/spi/flash.h index 2401a0887..f415e2c0b 100644 --- a/include/linux/spi/flash.h +++ b/include/linux/spi/flash.h @@ -2,7 +2,10 @@ #ifndef LINUX_SPI_FLASH_H #define LINUX_SPI_FLASH_H +#include <linux/types.h> + struct mtd_partition; +struct spi_device; /** * struct flash_platform_data: board-specific flash data @@ -11,6 +14,13 @@ struct mtd_partition; * @nr_parts: number of mtd_partitions for static partitioning * @type: optional flash device type (e.g. m25p80 vs m25p64), for use * with chips that can't be queried for JEDEC or other IDs + * @is_locked: optional callback to query write protection enforced by the + * platform rather than by the flash chip itself, for example a SPI + * controller that gates writes to a range of the flash. Returns 1 if + * the whole range is protected, 0 if it is not, or a negative errno. + * When supplied it takes precedence over the chip's own block + * protection bits, which do not necessarily reflect what is actually + * being enforced. * * Board init code (in arch/.../mach-xxx/board-yyy.c files) can * provide information about SPI flash parts (such as DataFlash) to @@ -26,6 +36,8 @@ struct flash_platform_data { char *type; + int (*is_locked)(struct spi_device *spi, loff_t ofs, u64 len); + /* we'll likely add more ... use JEDEC IDs, etc */ };