Re: [PATCH v3 06/13] nvmem: microchip-otpc: add tag-based packet lookup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use of `readl_relaxed()` for sequential FIFO stream reads and ASCII tag matching causes data corruption and tag lookup failures on big-endian systems.

Pre-existing issues:
- [High] Concurrent NVMEM reads lack serialization, leading to race conditions on shared OTPC hardware control registers.
--

commit 3ff90071d02f74b840b6a4d51b5716387dc75e88
Author: Varshini Rajendran <varshini.rajendran@microchip.com>

nvmem: microchip-otpc: add tag-based packet lookup

This commit adds support for accessing OTP packets by their 4-byte ASCII
tag while preserving backward compatibility with the existing ID-based
lookup. It caches packet metadata including the tag during probe and
changes the NVMEM memory stride to 1 to support tag-based offsets.

> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index df979e8549fdb..bf8589048e17c 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c

[ ... ]

> @@ -154,12 +233,11 @@ static int mchp_otpc_read(void *priv, unsigned int off, void *val,
>  	int ret, payload_size;
>  
>  	/*

[Severity: High]
This is a pre-existing issue, but does mchp_otpc_read() need serialization
to protect against concurrent access?

The NVMEM core does not serialize reg_read callbacks. If two threads
concurrently access the NVMEM device (e.g., via sysfs and a kernel
consumer), they will interleave calls to mchp_otpc_prepare_read().

Because mchp_otpc_prepare_read() mutates the shared MCHP_OTPC_MR and
MCHP_OTPC_CR registers without holding any locks, could this lead to
hardware register corruption and invalid payload data being returned?

> -	 * We reach this point with off being multiple of stride = 4 to
> -	 * be able to cross the subsystem. Inside the driver we use continuous
> -	 * unsigned integer numbers for packet id, thus divide off by 4
> -	 * before passing it to mchp_otpc_id_to_packet().
> +	 * From this point the offset has to be translated into the actual
> +	 * packet. For this we traverse the table of contents stored in a list
> +	 * "packet" based on the access type - packet id or tag.
>  	 */
> -	packet = mchp_otpc_id_to_packet(otpc, off / 4);
> +	packet = mchp_otpc_resolve_packet(otpc, off);

[ ... ]

> @@ -190,6 +268,29 @@ static int mchp_otpc_read(void *priv, unsigned int off, void *val,
>  	return 0;
>  }
>  
> +/**
> + * mchp_otpc_read_packet_tag() - read tag from packet payload
> + * @otpc: OTPC private data
> + * @offset: packet offset in OTP memory
> + * @val: pointer to store the tag value
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +static int mchp_otpc_read_packet_tag(struct mchp_otpc *otpc, unsigned int offset,
> +				     unsigned int *val)
> +{
> +	int ret;
> +
> +	ret = mchp_otpc_prepare_read(otpc, offset);
> +	if (ret)
> +		return ret;
> +
> +	writel_relaxed(0, otpc->base + MCHP_OTPC_AR);
> +	*val = readl_relaxed(otpc->base + MCHP_OTPC_DR);

[Severity: High]
Since readl_relaxed() performs a CPU-dependent byteswap, will this corrupt
the 4-byte ASCII tag on big-endian architectures?

If the tag bytes are swapped, the tag lookup will fail against the
constants provided by the device tree.

This is also a pre-existing issue in the payload stream read loop inside
mchp_otpc_read(). In that loop, readl_relaxed() is used to pull sequential
FIFO stream data, which corrupts payload byte ordering on big-endian
systems.

Should these accessors be changed to use stream accessors to prevent
unwanted byteswapping?

> +
> +	return 0;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630093603.38663-1-varshini.rajendran@microchip.com?part=6