Re: [PATCH] net: macb: fix stale data returned by MDIO reads
From: Théo Lebrun <theo.lebrun@bootlin.com>
Date: 2026-08-05 14:45:51
Also in:
lkml
Subsystem:
atmel macb ethernet driver, networking drivers, the rest · Maintainers:
Théo Lebrun, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
Hello Leszek, Your subject is missing the [PATCH net] prefix. On Wed Aug 5, 2026 at 2:01 PM CEST, Polak, Leszek wrote:
macb_mdio_wait_for_idle() samples NSR.IDLE with no delay after MAN has been written. readx_poll_timeout() expands to read_poll_timeout() with sleep_before_read = false, so the very first NSR read is issued immediately after the register write. The controller does not deassert NSR.IDLE until its MDIO state machine actually begins the frame, which takes up to one MDC period. Neither macb_mdc_clk_div() nor gem_mdc_clk_div() ever selects an MDC above 2.5 MHz, so one period is up to ~400 ns - on the order of a hundred pclk cycles, and far longer than a back-to-back register access. The poll therefore observes IDLE still set from the *previous* operation and returns success before the operation just issued has started.
You (or rather the LLM) say that with confidence. How do you know about this MDIO state machine implying some delay inbetween the MAN writel (which triggers a MDIO management frame) and the time at which NSR.IDLE starts becoming 0? The doc doesn't mention anything of the sort. It explicitely mentions NSR.IDLE as being there to signal for completion. Also it sounds like a massive bug, wouldn't others have discovered it before? Just checked and nothing out of the ordinary on EyeQ (first NSR is always 0x2 after writel to MAN). Patch at the end, which should trigger error logs on your system from what I understood. Please report back on that.
The caller then reads a MAN DATA field that still holds the result of whatever completed before, so every read returns the data of the previously accessed register. For clause 45 the race occurs three times per access: the address operation is still in flight when the read operation is written, and the final MACB_BFEXT(DATA, macb_readl(bp, MAN)) picks up a stale value. Observed on a Versal board with a Marvell 88Q1111 on GEM1, where reading each MMD register twice showed the first read returning the preceding register's value: 31.8002: 0x0149 then 0x002b (0x0149 is 31.8001) 31.8004: 0x0b21 then 0x1401 (0x0b21 is 31.8003) 31.8011: 0x400e then 0x6000 (0x400e is 31.8010) 31.8010: 0x6000 then 0x400e (0x6000 is 31.8011)
I don't understand those logs? Can you provide the code that generated them and more context? You probably faced some terrible bugs if all MDIO reads returned the value from the previous read.
The lag persists across callers and across driver entry points, so it is a property of the controller path rather than of any PHY driver. It silently corrupts every MMD read. Latch-high, clear-on-read registers are worse than corrupted: their content cannot be recovered by retrying, because the first read still reaches the PHY and clears the latch. Sleep before the first NSR sample so that an operation issued immediately beforehand is guaranteed to be underway. The pre-operation bus-free check pays the same delay, which is immaterial - a single MDIO frame takes over 25 us at 2.5 MHz MDC, so this is well under the cost of the transfer itself. Assisted-by: Claude:claude-opus-5 Signed-off-by: Polak, Leszek <redacted>
If this bug gets confirmed it needs (1) to go into net and (2) have `Cc: stable...` and `Fixes: ...` trailers. Docs on (1) are Documentation/process/maintainer-netdev.rst https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html Docs on (2) are sources are in: Documentation/process/stable-kernel-rules.rst https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
quoted hunk ↗ jump to hunk
--- drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c@@ -340,10 +340,26 @@ static void macb_get_hwaddr(struct macb *bp) static int macb_mdio_wait_for_idle(struct macb *bp) { + /* + * NSR.IDLE is not deasserted until the MDIO state machine begins the + * frame, which takes up to one MDC period after MAN is written. MDC is + * never clocked above 2.5 MHz, so one period is at most ~400 ns; allow + * an order of magnitude of margin. read_poll_timeout() sleeps at least + * (sleep_us >> 2) + 1 us, so the guaranteed minimum here is 6 us. + */ + const unsigned int start_delay_us = 20; u32 val; - return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE), - 1, MACB_MDIO_TIMEOUT); + /* + * Do not sample NSR.IDLE immediately. When this is called straight + * after a MAN write, IDLE is still set from the previous operation, + * so the poll would return at once and the caller would go on to read + * a MAN DATA field that still holds the previous transaction's + * result. Sleeping first guarantees the new operation has started and + * IDLE has gone low before it is sampled. + */ + return read_poll_timeout(MACB_READ_NSR, val, val & MACB_BIT(IDLE), + start_delay_us, MACB_MDIO_TIMEOUT, true, bp); }
Your code is not just adding a 20µs start delay, it changes all delays from 1µs to 20µs. I don't think we want that. Before: readl(NSR) -> usleep(1) -> readl(NSR) -> usleep(1) -> ... After: usleep(20) -> readl(NSR) -> usleep(20) -> readl(NSR) -> ... Your introduced variable start_delay_us is not needed. There are way too many comment lines. Your LLM hasn't read enough kernel code! Rare comments, mostly terse. Commit messages can be relied upon. --- pr_err() should trigger on your system from what I understood. Can you confirm?
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d58430fe9c41..1eaee3e1f735 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c@@ -304,6 +304,12 @@ static int macb_mdio_wait_for_idle(struct macb *bp) { u32 val; + val = macb_readl(bp, NSR); + if (val & MACB_BIT(IDLE)) { + pr_err("PHY IDLE after writel(MAN): NSR %#x MAN %#x\n", val, macb_readl(bp, MAN)); + fsleep(20); + } + return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE), 1, MACB_MDIO_TIMEOUT); }
Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com