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

macb_mdio_wait_for_idle() serves two purposes: checking that the bus is
free before a management frame is issued, and waiting for that frame to
complete. In the second case it samples NSR.IDLE with no delay --
readx_poll_timeout() expands to read_poll_timeout() with
sleep_before_read = false, so the first NSR read is issued immediately
after the MAN write.

The immediate sample dates back to the phylib conversion. Before it, the
driver did not look at IDLE at all: it enabled the MFD interrupt and did
wait_for_completion(&bp->mdio_complete), so the frame was known to be
finished before MAN was read back. That was replaced by

	/* wait for end of transfer */
	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
		cpu_relax();

placed directly after the MAN write, and the later readx_poll_timeout()
conversion preserved the eager first read.

On a Versal board (GEM1, Marvell 88Q1111 PHY) NSR.IDLE is still set at
that point, and only clears shortly afterwards. The documentation
presents NSR.IDLE purely as a completion indication and says nothing
about this, but the measured behaviour is consistent with IDLE following
the MDIO state machine, which cannot start driving the frame before the
next MDC edge. Neither macb_mdc_clk_div() nor gem_mdc_clk_div() selects
an MDC above 2.5 MHz, so one MDC period is at least 400 ns, far longer
than a back-to-back register access.

The poll therefore matches the IDLE left over from the previous frame and
returns before the frame just issued has started. The caller goes on to
read a MAN DATA field still holding the previous result, so the read
returns the data of the previously accessed register. Clause 45 accesses
are hit twice per read, once for the address frame and once for the data
frame.

Reading each MMD register twice showed the first read returning the value
of the register accessed just before it:

  31.8002 -> 0x0149, then 0x002b   (0x0149 is the value of 31.8001)
  31.8004 -> 0x0b21, then 0x1401   (0x0b21 is the value of 31.8003)
  31.8011 -> 0x400e, then 0x6000   (0x400e is the value of 31.8010)
  31.8010 -> 0x6000, then 0x400e   (0x6000 is the value of 31.8011)

Retrying does not recover latch-high, clear-on-read registers, because
the first read does reach the PHY and clears the latch.

Sleep before the first NSR sample on the completion path, so the frame is
guaranteed to be underway when IDLE is read. 20 us covers MDC down to
50 kHz and is small against a frame, which takes over 25 us at 2.5 MHz
MDC. The bus-free check is left untouched: IDLE is legitimately set
there and must be reported without delay.

Fixes: 6c36a7074436 ("macb: Use generic PHY layer")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Polak, Leszek <redacted>
---
Changes in v2:
- Add the "net" tree to the subject prefix; target net rather than
  net-next.
- Add Fixes: and Cc: stable@vger.kernel.org trailers.
- Describe in the commit message where the eager IDLE sample came from:
  the driver waited on the MFD interrupt until 6c36a7074436.
- Keep the poll interval at 1 us. The delay is now a single fsleep()
  ahead of the poll instead of a raised sleep_us, so the polling
  behaviour after the first sample is unchanged:
    v1: usleep(20) -> readl(NSR) -> usleep(20) -> readl(NSR) -> ...
    v2: fsleep(20) -> readl(NSR) -> usleep(1)  -> readl(NSR) -> ...
- Apply the delay only on the completion path, via a separate helper, so
  the bus-free pre-check still returns immediately.
- Drop the start_delay_us variable.
- Reduce the comments to two lines.
- Reword the commit message: the IDLE behaviour is now presented as
  measured on this controller, with the state-machine explanation given
  as a hypothesis, rather than asserted as documented. The v1 wording
  also had "one MDC period is up to ~400 ns" backwards: 400 ns is the
  minimum period, at the maximum MDC rate, so the delay has to be sized
  for the slowest MDC, not the fastest.
- Link to v1:
  https://lore.kernel.org/netdev/970df4d9-6199-4148-840a-ed1897ff27eb

Notes, not for the commit message:

- Théo, on your debug patch (the second version, with
  macb_mdio_wait_for_idle_busy()): results are quoted in the reply to
  your mail rather than here. The reproducer that produced the register
  dump above is there too.

- On "wouldn't others have discovered it before": I went through every
  MDIO bus implementation under drivers/net that polls for completion,
  ~22 of them. Sixteen poll a BUSY/START/FIRE bit that the driver itself
  sets in the command write, so the bit necessarily reads back as set and
  the window cannot exist. Three poll a latched done event that is
  consumed per transfer (fec, adin1110, mdio-mux-bcm-iproc). Only macb
  and xilinx_axienet_mdio poll a level ready/idle bit that software never
  sets, which is the only shape where sampling too early can succeed on
  stale state. That is not evidence the bug is real, but it is why almost
  nobody else could have hit it.

- On the documentation not mentioning it: drivers/net/mdio/mdio-aspeed.c
  carries an explicit workaround whose comment says the controller may
  return stale data when a read follows immediately after a write, fixed
  with a dummy read before the poll. So this class of behaviour does get
  acknowledged in-tree even when the datasheet is silent. Different
  mechanism from what I am claiming here -- register read pipeline rather
  than MDC clock domain -- so it is precedent for the category, not proof
  of my case.

- Counter-evidence, in the interest of not only citing what helps me:
  xilinx_axienet_mdio.c has the same shape as macb, pre-check ready,
  write MCR with INITIATE, poll ready again with no delay, and nobody
  reports corruption there. Théo's EyeQ measurement points the same way.
  If IDLE deasserts in the pclk domain rather than the MDC domain, my
  explanation is simply wrong and the Versal behaviour needs a different
  one.

- On MFD, this may satisfy both of you. Andrew, the design you put in FEC
  is not interrupt-driven, but it does not poll a level bit either: it
  polls FEC_IEVENT & FEC_ENET_MII, the same latched event the interrupt
  used, clears it after each transfer, and fec_enet_collect_events() has
  "int_events &= ~FEC_ENET_MII;" with the comment "Don't clear MDIO
  events, we poll for those". Polling an event bit with the interrupt
  left masked gets the correctness without the interrupt overhead. macb
  has that bit -- ISR bit 0, MFD, "Management frame sent" -- and it is
  entirely unused today, two defines in macb.h and no reference in
  macb_main.c.

  The blocker is that it does not port cleanly. macb_interrupt() opens
  with an unconditional queue_readl(queue, ISR), and macb_queue_isr_clear()
  only writes ISR when MACB_CAPS_ISR_CLEAR_ON_WRITE is set, i.e. ISR is
  read-to-clear on the parts without that cap. FEC can preserve one bit
  because IEVENT is write-one-to-clear; on read-to-clear macb parts there
  is no equivalent, so any RX/TX interrupt landing between the MAN write
  and the poll would eat MFD and the poll would run to timeout. Solvable
  on clear-on-write parts, not obviously on the others. I did not want to
  put that in this patch, but it looks like the right direction if
  someone wants it.

- On bus throughput: smaller than I first thought. fsleep() in mainline
  is usleep_range(usecs, usecs + (usecs >> 2)), so fsleep(20) is
  usleep_range(20, 25), not (20, 40). A clause 22 frame is 64 bits, so
  25.6 us at 2.5 MHz MDC. Even at the upper bound the sleep finishes
  before the frame does, and slower MDC hides it completely, so the added
  latency is around zero rather than the ~14 us I would have guessed.

- Worth noting for the v1 comparison: read_poll_timeout() with
  sleep_before_read sleeps usleep_range((sleep_us >> 2) + 1, sleep_us),
  so v1 guaranteed only 6 us before the first sample despite asking for
  20. The explicit fsleep() is the only one of the two that actually
  delivers the delay it claims.

 drivers/net/ethernet/cadence/macb_main.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 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

@@ -308,6 +308,16 @@ static int macb_mdio_wait_for_idle(struct macb *bp)
 				  1, MACB_MDIO_TIMEOUT);
 }
 
+/* IDLE clears only once the frame has started, up to one MDC period after
+ * MAN is written.
+ */
+static int macb_mdio_wait_for_frame_done(struct macb *bp)
+{
+	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 +337,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_frame_done(bp);
 	if (status < 0)
 		goto mdio_read_exit;
 

@@ -362,7 +372,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_frame_done(bp);
 	if (status < 0)
 		goto mdio_read_exit;
 

@@ -372,7 +382,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_frame_done(bp);
 	if (status < 0)
 		goto mdio_read_exit;
 

@@ -405,7 +415,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_frame_done(bp);
 	if (status < 0)
 		goto mdio_write_exit;
 

@@ -439,7 +449,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_frame_done(bp);
 	if (status < 0)
 		goto mdio_write_exit;
 

@@ -450,7 +460,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_frame_done(bp);
 	if (status < 0)
 		goto mdio_write_exit;
 

-- 
2.43.0