Re: [PATCH v3 06/13] nvmem: microchip-otpc: add tag-based packet lookup
From: sashiko-bot@kernel.org
Date: 2026-06-30 10:00:41
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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