Re: [PATCH v5 09/17] nvmem: microchip-otpc: nvmem: microchip-otpc: add tag-based packet lookup

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

New issues:
- [Medium] TAG-based packet reads fail to bound the requested bytes to the actual packet length, leaking adjacent OTP packets.

Pre-existing issues:
- [High] Concurrent OTP reads cause a hardware race condition on OTP registers without locking.
- [High] Using `readl_relaxed()` for a FIFO data stream causes data corruption on big-endian systems.
- [High] Global static struct `mchp_nvmem_config` is dynamically modified in probe, causing a data race if multiple devices are probed.
- [High] Mismatch between logical size tracking and physical offset causes out-of-bounds hardware reads during probe.
- [High] Incorrect arguments passed to read_poll_timeout(), with sleep_us significantly larger than timeout_us.
--

--- Patch [9]: [PATCH v5 09/17] nvmem: microchip-otpc: nvmem: microchip-otpc: add tag-based packet lookup ---
commit e6e6046319784f7ce1cf86169392921f7f8bb397
Author: Varshini Rajendran <[email protected]>

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

This commit adds support for accessing OTP packets by their FourCC tag,
preserving backward compatibility with ID-based lookup. The driver supports
both legacy ID-based offsets and TAG-based offsets by reading and caching
packet metadata during probe. The nvmem stride is also set to 1 to enable
tag-based access.

> 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
[ ... ]
> @@ -140,8 +190,29 @@ static int mchp_otpc_prepare_read(struct mchp_otpc *otpc,
>   * offset returned by hardware.
>   *
>   * For this, the read function will return the first requested bytes in the
> - * packet. The user will have to be aware of the memory footprint before doing
> - * the read request.

[Severity: High]
This is a pre-existing issue, but are the arguments passed to
read_poll_timeout() in mchp_otpc_prepare_read() correct?

	return read_poll_timeout(readl_relaxed, tmp, !(tmp & MCHP_OTPC_SR_READ),
				 10000, 2000, false, otpc->base + MCHP_OTPC_SR);

The sleep_us parameter is set to 10000 (10ms) while timeout_us is set to 2000
(2ms). Since the sleep time is 5x larger than the total timeout, won't this
instantly return a timeout error on the first sleep cycle if the hardware isn't
immediately ready?

> + * packet.
[ ... ]
> @@ -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);
>  	if (!packet)
>  		return -EINVAL;
>  	offset = packet->offset;

[Severity: High]
This is a pre-existing issue, but can concurrent OTP reads cause a hardware race
condition?

Looking at mchp_otpc_read(), there is an outer loop that accesses the hardware:

	while (len < bytes) {
		ret = mchp_otpc_prepare_read(otpc, offset);

When multiple consumers call nvmem_device_read() concurrently,
mchp_otpc_prepare_read() modifies global hardware registers without locking:

	writel_relaxed(tmp, otpc->base + MCHP_OTPC_MR);
	...
	writel_relaxed(tmp, otpc->base + MCHP_OTPC_CR);

Could this clobber the hardware state machine and lead to unpredictable
behavior?


[Severity: High]
This is a pre-existing issue, but does using readl_relaxed() for the FIFO data
stream cause data corruption on big-endian systems?

When reading the payload content in mchp_otpc_read():

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

The readl_relaxed() macro performs an implicit CPU-to-little-endian byte swap
on big-endian architectures. Should a stream accessor be used instead to
preserve the byte order between the device and memory?


[Severity: Medium]
Does TAG-based packet reading fail to bound the requested bytes to the actual
packet length, leading to a leak of adjacent OTP packets?

If a client requests a read size larger than the actual packet payload, the
inner loop in mchp_otpc_read() exits when the payload size is exhausted:

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

However, if len is still less than bytes, the outer while (len < bytes) loop
will continue and issue new hardware reads for the next sequential packet.
Could this undermine the packet-based access abstraction by returning adjacent
packet data?

[ ... ]
> @@ -215,6 +300,20 @@ static int mchp_otpc_init_packets_list(struct mchp_otpc *otpc, u32 *size)
>  
>  		packet->id = id++;
>  		packet->offset = word_pos;
> +		type = FIELD_GET(MCHP_OTPC_HR_PACKET, word);
> +
> +		if (type == MCHP_OTPC_HR_PACKET_REGULAR) {
> +			ret = mchp_otpc_read_packet_tag(otpc, packet->offset,
> +							&packet->tag);
> +			if (ret)
> +				return ret;
> +
> +			if (!mchp_otpc_is_valid_fourcc(packet->tag))
> +				packet->tag = 0;
> +		} else {
> +			packet->tag = 0;
> +		}
> +

[Severity: High]
This is a pre-existing issue, but does a mismatch between the logical size
tracking and the physical offset cause out-of-bounds hardware reads during
probe?

In mchp_otpc_init_packets_list(), the loop increments the size and word_pos:

		/* Count size by adding header and paload sizes. */
		*size += 4 * (payload_size + 1);
		/* Next word: this packet (header, payload) position + 1. */
		word_pos += payload_size + 2;

The size pointer increments by 4 * (payload_size + 1) bytes, while word_pos
increments by payload_size + 2 words (which is 4 * (payload_size + 2) bytes).
Because word_pos grows faster than size, could word_pos exceed the hardware
limits before size reaches MCHP_OTPC_SIZE, causing mchp_otpc_prepare_read() to
poll invalid hardware offsets?

[ ... ]
> @@ -236,7 +335,7 @@ static struct nvmem_config mchp_nvmem_config = {
>  	.type = NVMEM_TYPE_OTP,
>  	.read_only = true,
>  	.word_size = 4,
> -	.stride = 4,
> +	.stride = 1,
>  	.reg_read = mchp_otpc_read,
>  };

[Severity: High]
This isn't a bug introduced by this patch, but does modifying the global static
mchp_nvmem_config struct during probe cause a data race?

In mchp_otpc_probe(), the shared global struct is dynamically modified:

	mchp_nvmem_config.dev = otpc->dev;
	mchp_nvmem_config.add_legacy_fixed_of_cells = true;
	mchp_nvmem_config.size = size;
	mchp_nvmem_config.priv = otpc;

If multiple instances of the mchp-otpc driver are probed concurrently, could
this result in invalid private data pointers being registered with the nvmem
core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9