Re: [PATCH v2 1/2] nvmem: core: deprecate reg_write callback and add reg_write_const

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Incorrect boolean logic using '||' instead of '&&' forces all NVMEM devices to be treated as read-only and unconditionally disables hardware writes.

Pre-existing issues:
- [High] Concurrent nvmem writes race on the write-protect GPIO, which can lead to unprotected hardware states and failed writes.
--

--- Patch [1]: [PATCH v2 1/2] nvmem: core: deprecate reg_write callback and add reg_write_const ---
commit fd9b6bd1e67a30d018b7e8f0d9db60403988e05a
Author: Link Mauve <linkmauve@linkmauve.fr>

nvmem: core: deprecate reg_write callback and add reg_write_const

This patch deprecates the reg_write callback in favor of reg_write_const,
which takes a const pointer for the data buffer. It updates the nvmem core to
support either callback when checking for write capabilities.

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 9d182eda0c994..66a85d31149d7 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -62,13 +62,20 @@ static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
>  }
>  
>  static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
> -			     void *val, size_t bytes)
> +			     const void *val, size_t bytes)
>  {
>  	int ret;
>  
> +	if (nvmem->reg_write_const) {
> +		gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
> +		ret = nvmem->reg_write_const(nvmem->priv, offset, val, bytes);
> +		gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
> +		return ret;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does toggling the wp_gpio here have a race
condition?

If two threads concurrently write to the same NVMEM device, they could
execute __nvmem_reg_write() simultaneously. One thread could re-assert the
write-protect GPIO while the other thread's hardware write is still in
progress, potentially causing the write to fail or corrupt the write sequence.

>  	if (nvmem->reg_write) {
>  		gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
> -		ret = nvmem->reg_write(nvmem->priv, offset, val, bytes);
> +		ret = nvmem->reg_write(nvmem->priv, offset, (void *)val, bytes);
>  		gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
>  		return ret;
>  	}

[ ... ]

> @@ -264,7 +271,7 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>  
>  	count = round_down(count, nvmem->word_size);
>  
> -	if (!nvmem->reg_write || nvmem->read_only)
> +	if (!nvmem->reg_write || !nvmem->reg_write_const || nvmem->read_only)
>  		return -EPERM;

[Severity: High]
Does this logic inadvertently reject all writes?

Since drivers will typically provide either reg_write or reg_write_const
(but not both), this condition will always evaluate to true, failing all
sysfs writes with -EPERM. Should this be && instead of ||?

[ ... ]

> @@ -285,7 +292,7 @@ static umode_t nvmem_bin_attr_get_umode(struct nvmem_device *nvmem)
>  	if (!nvmem->read_only)
>  		mode |= 0200;
>  
> -	if (!nvmem->reg_write)
> +	if (!nvmem->reg_write || !nvmem->reg_write_const)
>  		mode &= ~0200;

[Severity: High]
Similar to the sysfs write function above, does this strip write permissions
from sysfs attributes for all devices?

Since at least one of these callbacks will typically be NULL, this condition
is always true.

[ ... ]

> @@ -975,7 +983,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  		goto err_put_device;
>  
>  	nvmem->read_only = device_property_present(config->dev, "read-only") ||
> -			   config->read_only || !nvmem->reg_write;
> +			   config->read_only || !nvmem->reg_write || !nvmem->reg_write_const;

[Severity: High]
Does this unconditionally set the device to read-only during registration?

Using || means any device that doesn't provide both callbacks will be marked
as read-only.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715195520.25410-1-linkmauve@linkmauve.fr?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic