Re: [PATCH 3/3] nvmem: stm32: add TAMP nvmem driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware I/O errors and permission denials during unaligned accesses are silently swallowed.
- [High] Endianness mismatch between aligned block accesses and unaligned byte accesses.
- [High] Non-atomic read-modify-write introduces race conditions and data corruption during byte-sized register updates.
- [Medium] Probe function unconditionally transforms all syscon errors into infinite probe deferrals.
- [Medium] Devres memory leaks for temporary allocation resources during device probe.
--

commit 3cf1f845a1eac3aa55c1581b866a9ae2309dca59
Author: Simeon Marijon <simeon.marijon@foss.st.com>
Subject: nvmem: stm32: add TAMP nvmem driver

This commit introduces the STM32 TAMP nvmem driver to expose backup
registers as nvmem cells under the nvram node in the device tree.

> diff --git a/drivers/nvmem/stm32-tamp-nvram.c b/drivers/nvmem/stm32-tamp-nvram.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/nvmem/stm32-tamp-nvram.c
[ ... ]
> +static bool *stm32_tamp_get_compartment_owner_mp2x(struct stm32_tamp_nvram_priv *priv)
> +{
[ ... ]
> +	compartment_owner = devm_kcalloc(dev,
> +					 NB_COMPARTMENT_STM32MP2,
> +					 sizeof(*compartment_owner),
> +					 GFP_KERNEL);
> +
> +	if (!compartment_owner)
> +		return ERR_PTR(-ENOMEM);

[Severity: Medium]
Does this code leak the compartment_owner array? This devm-allocated array
is returned to configure access rights during stm32_tamp_nvram_probe(), but 
it appears to remain in memory permanently without being freed after use.

[ ... ]
> +static int stm32_tamp_nvram_write_byte(struct stm32_tamp_nvram_priv *priv, u32 offset, u8 byte)
> +{
[ ... ]
> +	if (!stm32_tamp_nvram_rights(priv, reg_idx, false))
> +		return -EIO;
> +
> +	if (regmap_read(priv->bkpregs_regmap, offset_aligned, &read_value))
> +		return -EIO;
> +
> +	to_be_writen_value = read_value & ~(0xFFUL << byte_in_word * 8);
> +	to_be_writen_value |=  (u32)byte << (byte_in_word * 8);
> +
> +	return regmap_write(priv->bkpregs_regmap, offset_aligned, to_be_writen_value);
> +}

[Severity: High]
Can this race with other concurrent byte writes to the same 32-bit register? 
The regmap lock is dropped between regmap_read() and regmap_write(), which
could cause overlapping unaligned byte updates to clobber each other. Should
this use regmap_update_bits() instead to ensure atomicity?

[ ... ]
> +static int stm32_tamp_nvram_read(void *context, unsigned int offset, void *buf, size_t bytes)
> +{
[ ... ]
> +	while (i < total)  {
> +		reg_idx = i / sizeof(u32);
> +		if (i + sizeof(u32) <= total && IS_ALIGNED(i, sizeof(u32))) {
> +			if (!stm32_tamp_nvram_rights(priv, reg_idx, true)) {
> +				dev_dbg(dev, "Backup register %u is not allowed to be read\n",
> +					reg_idx);
> +				temp_u32 = 0;
> +			} else if (regmap_read(priv->bkpregs_regmap, i, &temp_u32)) {
> +				return -EIO;
> +			}
> +			memcpy(buf_u8, &temp_u32, sizeof(u32));

[Severity: High]
Does this memcpy() introduce an endianness mismatch on Big Endian systems?
The aligned path copies the host-native integer directly into the stream,
while the unaligned byte path below processes bytes in Little Endian order
via bit shifts.

> +			buf_u8 += sizeof(u32);
> +			i += sizeof(u32);
> +		} else {
> +			ret = stm32_tamp_nvram_read_byte(priv, i, &byte);
> +			if (ret) {
> +				dev_dbg(dev, "Backup register %u is not allowed to be read\n",
> +					reg_idx);
> +				byte = 0;
> +			}

[Severity: High]
Is it intentional to swallow hardware I/O errors here? If
stm32_tamp_nvram_read_byte() fails (e.g., returns -EIO), the error is
intercepted and masked as a valid read of 0x00, which might hide hardware
faults from the caller.

> +			*buf_u8 = byte;
> +			i++;
> +			buf_u8++;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int stm32_tamp_nvram_write(void *context, unsigned int offset, void *buf, size_t bytes)
> +{
[ ... ]
> +	while (i < total)  {
> +		reg_idx = i / sizeof(u32);
> +		if (i + sizeof(u32) <= total && IS_ALIGNED(i, sizeof(u32))) {
> +			if (stm32_tamp_nvram_rights(priv, reg_idx, false)) {
> +				memcpy(&temp_u32, buf_u8, sizeof(u32));

[Severity: High]
Similar to the read path, does this memcpy() reverse the intended byte
order on Big Endian architectures when compared to the unaligned branch?

> +				if (regmap_write(priv->bkpregs_regmap, i, temp_u32))
> +					return -EIO;
> +			} else {
> +				dev_dbg(dev, "Backup register %u is not allowed to be written",
> +					reg_idx);
> +			}
> +			buf_u8 += sizeof(u32);
> +			i += sizeof(u32);
> +		} else {
> +			ret = stm32_tamp_nvram_write_byte(priv, i, *buf_u8);
> +			if (ret)
> +				dev_dbg(dev, "Backup register %u is not allowed to be written",
> +					reg_idx);

[Severity: High]
Does this drop hardware I/O errors? If the unaligned write fails, the error
is ignored rather than propagated to the caller, which might give the false
impression that the write was successful.

> +			i++;
> +			buf_u8++;
> +		}
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int *stm32_tamp_nvram_get_backup_zones(struct stm32_tamp_nvram_priv *priv)
> +{
[ ... ]
> +	/* Get the n-1 frontiers of zone within the tamp configuration registers */
> +	for (zone_idx = 0; zone_idx < nb_zones - 1; zone_idx++) {
> +		const struct reg_field reg_field = priv->data->config_reg_fields[zone_idx];
> +		struct regmap_field *field = devm_regmap_field_alloc(dev,
> +								     tamp_regmap,
> +								     reg_field);

[Severity: Medium]
Does this code leak the regmap_field allocations? These fields are allocated
in a loop to extract register offsets during initialization, but they do not
appear to be freed (e.g., via devm_regmap_field_free) after use.

[ ... ]
> +static int stm32_tamp_nvram_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	priv->config_regmap = syscon_node_to_regmap(dev->parent->of_node);
> +	if (IS_ERR(priv->config_regmap)) {
> +		dev_dbg(dev, "Deferring till parent probed\n");
> +		return -EPROBE_DEFER;
> +	}

[Severity: Medium]
Does this unconditionally transform all syscon errors into infinite probe
deferrals? If syscon_node_to_regmap() fails with a fatal error such as
-EINVAL, returning -EPROBE_DEFER masks the true error code and could cause
boot delays or hangs. Should this use dev_err_probe() and return the
specific error code instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-upstream_nvram_reworked-v1-0-521c16175cef@foss.st.com?part=3