Re: [PATCH v1 1/7] nvmem: Add 'protect' operation to core framework

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, May 30, 2025 at 01:41:00PM +0200, Oleksij Rempel wrote:
> Introduce a generic "protect" operation to the NVMEM framework.
> This allows NVMEM providers to expose hardware-specific protection or
> locking mechanisms through the character device interface. Existing
> read/write operations do not cover this type of state-altering
> protection.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/nvmem/core.c           | 48 ++++++++++++++++++++++++++++++++++
>  drivers/nvmem/partition.c      |  7 +++++
>  include/linux/nvmem-provider.h |  5 ++++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 38dfb2cf2d1f..ac54a56f3c9f 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -30,6 +30,8 @@ struct nvmem_device {
>  					     const void *val, size_t val_size);
>  	int			(*reg_read)(void *ctx, unsigned int reg,
>  					    void *val, size_t val_size);
> +	int			(*reg_protect)(void *ctx, unsigned int reg,
> +					       size_t bytes, int prot);
>  };
>  
>  struct nvmem_cell {
> @@ -93,9 +95,54 @@ static ssize_t nvmem_cdev_write(struct cdev *cdev, const void *buf, size_t count
>  	return retlen;
>  }
>  
> +static int nvmem_cdev_protect(struct cdev *cdev, size_t count, loff_t offset,
> +			      int prot)
> +{
> +	struct nvmem_device *nvmem;
> +	int nvmem_prot;
> +
> +	if (cdev->master)
> +		nvmem = container_of(cdev->master, struct nvmem_device, cdev);
> +	else
> +		nvmem = container_of(cdev, struct nvmem_device, cdev);

Please rebase on https://lore.kernel.org/20250602-cdev-part-fixes-v1-0-814d9aa195ed@pengutronix.de

This quirk is buggy.

> +
> +	dev_dbg(cdev->dev, "protect ofs: 0x%08llx count: 0x%08zx prot: %d\n",
> +		offset, count, prot);
> +
> +	if (!nvmem->reg_protect) {
> +		dev_warn(cdev->dev, "NVMEM device %s does not support protect operation\n",
> +			 nvmem->name);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!count)
> +		return 0;
> +
> +	if (offset >= nvmem->size || count > nvmem->size - offset) {

Can be simplified to:

	if (offset + count > nvmem->size)


> +		dev_err(cdev->dev, "protect range out of bounds (ofs: 0x%08llx, count 0x%08zx, size 0x%08zx)\n",
> +			offset, count, nvmem->size);
> +		return -EINVAL;
> +	}
> +
> +	switch (prot) {
> +	case 0:
> +		nvmem_prot = NVMEM_PROTECT_ENABLE_WRITE;
> +		break;
> +	case 1:
> +		nvmem_prot = NVMEM_PROTECT_DISABLE_WRITE;
> +		break;
> +	default:
> +		dev_err(cdev->dev, "unsuported protection type %d\n", prot);
> +		return -EINVAL;
> +	}

I would appreciate introducing defines for the prot parameter treewide
(or converting it to boolean), but introducing it for nvmem only doesn't
improve the current state.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |