Re: [PATCH] net: macb: fix stale data returned by MDIO reads

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Théo,

On Wed Aug 5, 2026 at 4:45 PM CEST, Théo Lebrun wrote:
> 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 <LPolak@arri.de>
>
> 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
>
>> ---
>>  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?
>
> [...]

Oops this test triggers on the pre-check macb_mdio_wait_for_idle() calls.
Instead this:



diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d58430fe9c41..98e0cafb0de1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -308,6 +308,20 @@ static int macb_mdio_wait_for_idle(struct macb *bp)
              1, MACB_MDIO_TIMEOUT);
 }

+static int macb_mdio_wait_for_idle_busy(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 macb_mdio_wait_for_idle(bp);
+}
+
 static int macb_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum)
 {
   struct macb *bp = bus->priv;
@@ -327,7 +341,7 @@ static int macb_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum)
               | MACB_BF(REGA, regnum)
               | MACB_BF(CODE, MACB_MAN_C22_CODE)));

-  status = macb_mdio_wait_for_idle(bp);
+  status = macb_mdio_wait_for_idle_busy(bp);
   if (status < 0)
      goto mdio_read_exit;

@@ -362,7 +376,7 @@ static int macb_mdio_read_c45(struct mii_bus *bus, int mii_id, int devad,
               | MACB_BF(DATA, regnum & 0xFFFF)
               | MACB_BF(CODE, MACB_MAN_C45_CODE)));

-  status = macb_mdio_wait_for_idle(bp);
+  status = macb_mdio_wait_for_idle_busy(bp);
   if (status < 0)
      goto mdio_read_exit;

@@ -372,7 +386,7 @@ static int macb_mdio_read_c45(struct mii_bus *bus, int mii_id, int devad,
               | MACB_BF(REGA, devad & 0x1F)
               | MACB_BF(CODE, MACB_MAN_C45_CODE)));

-  status = macb_mdio_wait_for_idle(bp);
+  status = macb_mdio_wait_for_idle_busy(bp);
   if (status < 0)
      goto mdio_read_exit;

@@ -405,7 +419,7 @@ static int macb_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
               | MACB_BF(CODE, MACB_MAN_C22_CODE)
               | MACB_BF(DATA, value)));

-  status = macb_mdio_wait_for_idle(bp);
+  status = macb_mdio_wait_for_idle_busy(bp);
   if (status < 0)
      goto mdio_write_exit;

@@ -439,7 +453,7 @@ static int macb_mdio_write_c45(struct mii_bus *bus, int mii_id,
               | MACB_BF(DATA, regnum & 0xFFFF)
               | MACB_BF(CODE, MACB_MAN_C45_CODE)));

-  status = macb_mdio_wait_for_idle(bp);
+  status = macb_mdio_wait_for_idle_busy(bp);
   if (status < 0)
      goto mdio_write_exit;

@@ -450,7 +464,7 @@ static int macb_mdio_write_c45(struct mii_bus *bus, int mii_id,
               | MACB_BF(CODE, MACB_MAN_C45_CODE)
               | MACB_BF(DATA, value)));

-  status = macb_mdio_wait_for_idle(bp);
+  status = macb_mdio_wait_for_idle_busy(bp);
   if (status < 0)
      goto mdio_write_exit;




Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com