Re: [PATCH v2 2/2] nvmem: rockchip-efuse: add write support with optional VQPS supply gating

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Lack of preemption and IRQ protection around precisely timed STROBE pulse during eFuse programming.
- [Critical] Missing driver-level locking allows concurrent reads and writes to corrupt global controller registers and destroy eFuses.
- [Medium] VQPS regulator is enabled without explicitly configuring its strictly required programming voltage.

Pre-existing issues:
- [High] Read address bits are not cleared before ORing the next address, permanently corrupting multi-word read addresses.
- [High] CPU-endian register reads are copied via memcpy, causing byte-order reversal on Big Endian systems.
--

--- Patch [2]: [PATCH v2 2/2] nvmem: rockchip-efuse: add write support with optional VQPS supply gating ---
commit 9787a852435c97fd8c38650a5baf58286ca592d1
Author: Hrushiraj Gandhi <hrushirajg23@gmail.com>

nvmem: rockchip-efuse: add write support with optional VQPS supply gating

This commit adds write support for the RK3399 eFuse using the A_PGM
programming mode. It retrieves an optional VQPS regulator that is enabled
only during write operations to fulfill hardware requirements. If the
regulator is absent, the device is left read-only to prevent unsafe writes.

> diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c
> index 013e67136f3b9..3bde09236c74c 100644
> --- a/drivers/nvmem/rockchip-efuse.c
> +++ b/drivers/nvmem/rockchip-efuse.c
[ ... ]
> @@ -177,6 +193,7 @@ static int rockchip_rk3399_efuse_read(void *context, unsigned int offset,
>  	writel(RK3399_LOAD | RK3399_PGENB | RK3399_STROBSFTSEL | RK3399_RSB,
>  	       efuse->base + REG_EFUSE_CTRL);
>  	udelay(1);
> +
>  	while (addr_len--) {
>  		writel(readl(efuse->base + REG_EFUSE_CTRL) | RK3399_STROBE |
>  		       ((addr_start++ & RK3399_A_MASK) << RK3399_A_SHIFT),
>  		       efuse->base + REG_EFUSE_CTRL);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for multi-word
reads to use corrupted addresses here? 

The loop bitwise-ORs the new read address into REG_EFUSE_CTRL but never
clears the previous address bits. On subsequent iterations, the old address
bits remain set, which causes the new read address to be a bitwise OR of all
prior addresses in the sequence.

>  		udelay(1);
>  		out_value = readl(efuse->base + REG_EFUSE_DOUT);
>  		writel(readl(efuse->base + REG_EFUSE_CTRL) & (~RK3399_STROBE),
>  		       efuse->base + REG_EFUSE_CTRL);
>  		udelay(1);
>  
>  		memcpy(&buf[i], &out_value, RK3399_NBYTES);

[Severity: High]
This is a pre-existing issue, but does copying the register value this way
cause byte-order reversal on big-endian systems? 

readl() returns the 32-bit value in the CPU's native endianness, and using
memcpy() to serialize it will store the most significant byte at the lowest
address on big-endian systems. Should this use put_unaligned_le32() or
similar to guarantee consistent byte layout across architectures?

[ ... ]
> +static int rockchip_rk3399_efuse_write(void *context, unsigned int offset,
> +				       void *val, size_t bytes)
> +{
> +	struct rockchip_efuse_chip *efuse = context;
> +	u8 *buf = val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(efuse->clk);
> +	if (ret < 0) {
> +		dev_err(efuse->dev, "failed to prepare/enable efuse clk\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(efuse->vqps);

[Severity: Medium]
Could enabling the VQPS regulator without enforcing a specific voltage cause
hardware issues? 

The commit message mentions a 1.8V to 1.98V requirement, but
regulator_enable() does not enforce this. If the device tree defines this as
a shared or variable regulator currently set to a different voltage, could it
fail to program the OTP or cause overvoltage damage? Should this use
regulator_set_voltage() to guarantee the required programming voltage?

> +	if (ret < 0) {
> +		dev_err(efuse->dev, "failed to enable vqps regulator\n");
> +		clk_disable_unprepare(efuse->clk);
> +		return ret;
> +	}
> +
> +	while (bytes--) {
> +		u8 byte = *buf++;
> +		int bit;
> +
> +		for (bit = 0; bit < 8; bit++) {
> +			u32 addr, ctrl;
> +
> +			if (!(byte & BIT(bit)))
> +				continue;
> +
> +			addr = offset * 8 + bit;
> +
> +			/*
> +			 * A_PGM mode (TRM table 23-3):
> +			 * CSB=L, PGENB=L, PS=H, PD=L, LOAD=L, RSB=L,
> +			 * STROBSFTSEL=H (software controls STROBE)
> +			 * STROBE is asserted separately below.
> +			 */
> +			ctrl = RK3399_STROBSFTSEL | RK3399_PS |
> +			       ((addr & RK3399_A_MASK) << RK3399_A_SHIFT);
> +
> +			writel(ctrl, efuse->base + REG_EFUSE_CTRL);

[Severity: Critical]
Can concurrent reads and writes corrupt the global controller registers? 

The NVMEM core does not serialize device-level reads and writes. Both
rockchip_rk3399_efuse_write() and rockchip_rk3399_efuse_read() modify the
global REG_EFUSE_CTRL register.

If a concurrent read occurs from sysfs during a write operation, couldn't it
abruptly overwrite the A_PGM state, clear RK3399_PS, or assert standby mode
while the programming STROBE is active? Furthermore, reading while the vqps
regulator is enabled violates the hardware requirement that VQPS must be 0V
during reads. Should a driver-level mutex be used to protect the register
state and regulator transitions?

> +			udelay(1);
> +
> +			/* Assert STROBE to program the selected bit */
> +			writel(ctrl | RK3399_STROBE, efuse->base + REG_EFUSE_CTRL);
> +			udelay(RK3399_PGM_TIME_US);

[Severity: Critical]
Can this tight programming loop cause issues if it gets preempted? 

The RK3399 TRM specifies a precise programming STROBE pulse. This code uses
a udelay() loop that spins in process context without disabling interrupts or
preemption.

If a timer interrupt fires or the thread is preempted during this udelay(),
would the STROBE line remain asserted for the duration of the interruption,
potentially permanently degrading the OTP memory cells? Should
local_irq_save() and local_irq_restore() or similar atomic context guarantees
be used around the strobe?

> +
> +			/* Deassert STROBE */
> +			writel(ctrl, efuse->base + REG_EFUSE_CTRL);
> +			udelay(1);
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729085856.226001-1-hrushirajg23@gmail.com?part=2