Re: [PATCH v2 05/12] nvmem: microchip-otpc: add tag-based packet lookup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing serialization in hardware register accesses during NVMEM read leads to data corruption on concurrent access.
--

commit 9261dabb0a9ba8aa6fc617051559e1385e26b1cc
Author: Varshini Rajendran <varshini.rajendran@microchip.com>

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

Add support for accessing OTP packets by their 4-byte ASCII tag while
preserving backward compatibility with the existing ID-based lookup. The
driver resolves offsets matching valid legacy selectors through ID
lookup, falling back to tag lookup for other values.

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

[ ... ]

> @@ -154,12 +232,11 @@ static int mchp_otpc_read(void *priv, unsigned int off, void *val,
>  	int ret, payload_size;
>  
>  	/*
> -	 * 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);

[Severity: High]
This is a pre-existing issue, but does mchp_otpc_read() need serialization to
prevent concurrent hardware register accesses?

Since the NVMEM core does not serialize reg_read callbacks, multiple threads
reading the NVMEM device simultaneously could interleave hardware register
manipulations.

If mchp_otpc_read() loops over mchp_otpc_prepare_read() concurrently:

Thread A:
    writel_relaxed(tmp, otpc->base + MCHP_OTPC_MR); // Sets address

Thread B:
    writel_relaxed(tmp, otpc->base + MCHP_OTPC_MR); // Overwrites address

Could Thread B overwrite the address Thread A just wrote before Thread A
sets MCHP_OTPC_CR_READ? Both threads would then poll the same status register
and consume from the same data registers, which could lead to corrupted
payloads or invalid hardware state.

>  	if (!packet)
>  		return -EINVAL;
>  	offset = packet->offset;

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