Re: [PATCH v4 09/16] nvmem: microchip-otpc: nvmem: add emulation mode and OTP access validation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The arguments for `sleep_us` and `timeout_us` in `read_poll_timeout` are swapped, causing the poll to sleep excessively and fail prematurely.
- [High] There is a mismatch between the calculated NVMEM device size and the number of bytes read per packet, leading to truncated reads and corrupted payload data in the buffer.
- [Medium] The OTP payload stream is read from the `MCHP_OTPC_DR` data register using `readl_relaxed()` in a loop, which causes unintended byte-swapping on big-endian architectures.
--

--- Patch [9]: [PATCH v4 09/16] nvmem: microchip-otpc: nvmem: add emulation mode and OTP access validation ---
commit b464c23d34790bae23b36f89796febe703de8718
Author: Varshini Rajendran <varshini.rajendran@microchip.com>

nvmem: microchip-otpc: nvmem: add emulation mode and OTP access validation

This commit adds validation of OTP memory accessibility and emulation mode
status during probe. When the boot packet is not configured, emulation mode
allows access to the other packets.

> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index a17f82afdc37d..6b0184423463e 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -343,7 +344,8 @@ static int mchp_otpc_probe(struct platform_device *pdev)
>  {
[ ... ]
>  	ret = mchp_otpc_init_packets_list(otpc, &size);
>  	if (ret)
>  		return ret;
>  
> +	if (!size) {
> +		dev_warn(otpc->dev, "Cannot access OTP memory\n");
> +		if (!emul_enable)
> +			dev_info(otpc->dev, "Boot packet not programmed and emulation mode disabled\n");
> +	}
> +
>  	mchp_nvmem_config.dev = otpc->dev;
>  	mchp_nvmem_config.add_legacy_fixed_of_cells = true;

While reviewing this patch, I noticed a few pre-existing issues in the same
file that you might want to address:

drivers/nvmem/microchip-otpc.c:mchp_otpc_read()
>        *buf++ = readl_relaxed(otpc->base + MCHP_OTPC_DR);

[Severity: Medium]
This is a pre-existing issue, but does this read sequential FIFO payload data
using a byteswapping scalar accessor rather than a stream accessor (e.g.
readsl())?

Using readl_relaxed() here might cause unintended byte-swapping on big-endian
architectures when reading the underlying raw byte stream.


drivers/nvmem/microchip-otpc.c:mchp_otpc_prepare_read()
>    return read_poll_timeout(readl_relaxed, tmp, !(tmp & MCHP_OTPC_SR_READ),
>                             10000, 2000, false, otpc->base + MCHP_OTPC_SR);

[Severity: High]
This is a pre-existing issue, but are the sleep_us and timeout_us arguments
swapped here?

The sleep_us parameter is set to 10000 (10ms) while timeout_us is set to
2000 (2ms), which would cause the timeout to expire before the first sleep
completes.


drivers/nvmem/microchip-otpc.c:mchp_otpc_init_packets_list()
>    *size += 4 * (payload_size + 1);

drivers/nvmem/microchip-otpc.c:mchp_otpc_read()
>    } while (payload_size >= 0 && len < bytes);

[Severity: High]
This is a pre-existing issue, but is there a mismatch between the calculated
NVMEM device size and the number of bytes read per packet?

In mchp_otpc_init_packets_list(), the total size is incremented by exactly
payload_size + 1 words (header + payload).

However, in mchp_otpc_read(), the loop condition payload_size >= 0 causes
it to iterate payload_size + 1 times for the payload alone, meaning a total
of payload_size + 2 words (including header) are fetched and written to the
buffer per packet.

Could this write a padding or gap word into the user buffer, shifting
subsequent packets and causing final packets to be truncated because the
len < bytes limit is reached prematurely?

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