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

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

[-- Attachment #1: Type: text/plain, Size: 5969 bytes --]

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 */
>  };
>  


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]