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

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

On Wed Aug 5, 2026 at 4:52 PM CEST, Théo Lebrun wrote:
> 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.

One more thing to add, sorry for the stuttering.

A proper solution would use the IRQ as detection source rather than
sleep-polling. See BIT 0, called MFD in driver.

>>
>> ---
>>
>> 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