Re: [PATCH v4 08/16] nvmem: microchip-otpc: 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:

Pre-existing issues:
- [High] Missing mutual exclusion to protect the multi-step hardware read sequence leads to race conditions and hardware state corruption.
- [High] Unconditional 32-bit writes can overflow unaligned destination buffers when the requested read size is not a multiple of 4.
--

--- Patch [8]: [PATCH v4 08/16] nvmem: microchip-otpc: nvmem: microchip-otpc: add tag-based packet lookup ---
commit 9730ed34d324f2aa5db4e67574aa110d572f9120
Author: Varshini Rajendran <varshini.rajendran@microchip.com>

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

This commit adds support for accessing OTP packets by their tag (FourCC)
while preserving backward compatibility with ID-based lookup. The driver
resolves offsets matching valid legacy selectors through ID lookup, falling
back to tag lookup for other valid values.

> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index df979e8549fdb..a17f82afdc37d 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
[ ... ]
> @@ -154,12 +225,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 isn't a bug introduced by this patch, but I wanted to ask about the
thread safety of mchp_otpc_read() and mchp_otpc_prepare_read(). 

If multiple kernel consumers or userspace via sysfs (like a pread on 
/sys/bus/nvmem/devices/*/nvmem) read from this device concurrently,
couldn't they clobber the shared hardware state machine registers
(MR, CR, SR, etc)?

Since the NVMEM core does not serialize reg_read callbacks unless keepouts
are defined, and this driver doesn't use regmap, does this driver need its
own mutex to protect the multi-step hardware read sequence?

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

[Severity: High]
This is also a pre-existing issue, but looking further down in
mchp_otpc_read() at the payload reading loop:

	do {
		*buf++ = readl_relaxed(otpc->base + MCHP_OTPC_DR);
		len += sizeof(*buf);
		offset++;
		payload_size--;
	} while (payload_size >= 0 && len < bytes);

Could this unconditionally write past the end of the destination buffer if a
caller invokes nvmem_device_read() with an unaligned size?

The loop always writes 4 bytes at a time into *buf. If bytes is not a
multiple of 4 (e.g. bytes = 5 or 6), it looks like the final write will
overflow the allocated buffer bounds since the loop condition (len < bytes)
is only checked after the write occurs.

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