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

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

[Polak, Leszek]

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.

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)

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>
---
 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);
 }
 
 static int macb_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum)
-- 
2.43.0

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

[Théo Lebrun]

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?

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

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

[Théo Lebrun]

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

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

[Théo Lebrun]

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

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

[Andrew Lunn]

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

Maybe.

The FEC driver used to use interrupts for its MDIO bus. But interrupts
add a lot of overhead. I swapped the FEC to polling and got a
significant speed improvement.

A lot depends on what you have actually doing with the bus. I was
using it in combination with Marvell PHYs, which can give the raw
cable test values, which is a big data set. So the speed up was
welcome. But if all the bus is used for is once per second polls for
link change, a slower bus probably does not matter.

       Andrew