[PATCH net-next] net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

[PATCH net-next] net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

[David Yang]

A register access over MDIO is 4 frames: 2 address writes followed by
2 data reads or writes. The SMI slave tracks the phase in a state
machine, so an access interrupted between frames, e.g. on an MDIO bus
error, leaves the machine in an unknown state, and every following
access fails or acts on a stale address until the machine is realigned.

The driver so far assumed every access succeeds, and relied on manual
resetting when the state machine wedged. However, without a dedicated
reset line, the reset itself goes through the same SMI interface,
causing the chicken-and-egg problem.

Realign the machine before the next access does its own work. Also, the
initial value is unsynced, so any stale state is not inherited.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/chip.c | 106 +++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 33e5fc127244..8b6c9739b684 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -94,10 +94,98 @@ struct yt921x_reg_mdio {
 	 * Keep this because we need switchid to form MII regaddrs anyway.
 	 */
 	unsigned char switchid;
+
+	bool synced;
 };
 
 /* TODO: SPI/I2C */
 
+/* An SMI register access is 4 MDIO frames. An interrupted access leaves the
+ * machine in an unknown state, and every following access fails or acts on a
+ * stale address.
+ *
+ * The driver used to assume every access succeeds, and relied on manual
+ * resetting when the state machine wedged. However, without a dedicated reset
+ * line, the reset itself goes through the same SMI interface, causing the
+ * chicken-and-egg problem. Furthermore, our driver verifies chip id before
+ * firing the reset, so that can never be triggered reliably.
+ *
+ * (A/D: address/data, H/L: hi/lo byte, r/w: read/write)
+ *
+ * State Input Next Comment
+ * ----- ----- ---- -------
+ * AH    aw    AL
+ * AH    dr    AH   `0xdead` sentinel
+ * AL    aw    DH
+ * AL    dr    AL   `0xdead` sentinel
+ * DHr   aw    AH   abort, frame discarded
+ * DHr   dr    DLr
+ * DLr   aw    AH   abort, frame discarded
+ * DLr   dr    AH   the access is done
+ * DHw   aw    AH   abort, frame discarded
+ * DHw   dw    DLw
+ * DLw   aw    AL   abort, frame consumed as next access's addr-hi
+ * DLw   dr    ?    `0xdead` sentinel; stay or advance undetermined
+ * DLw   dw    AH   the access commits
+ *
+ * The table lists the whitelisted frames only; any other REGAD/op combination
+ * is not characterized and is never emitted here.
+ *
+ * The resync sequence is dr, dr, aw, aw, dr, dr, plus one more dr as a sanity
+ * check: the machine must be back at AH, which reads the `0xdead` sentinel.
+ */
+static int yt921x_reg_mdio_resync(struct yt921x_reg_mdio *mdio)
+{
+	struct mii_bus *bus = mdio->bus;
+	int addr = mdio->addr;
+	u32 reg_addr;
+	u32 reg_data;
+	int res;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	reg_addr = YT921X_SMI_SWITCHID(mdio->switchid) | YT921X_SMI_ADDR |
+		   YT921X_SMI_READ;
+	reg_data = YT921X_SMI_SWITCHID(mdio->switchid) | YT921X_SMI_DATA |
+		   YT921X_SMI_READ;
+
+	/* dr */
+	res = __mdiobus_read(bus, addr, reg_data);
+	if (res < 0)
+		goto end;
+	/* dr */
+	res = __mdiobus_read(bus, addr, reg_data);
+	if (res < 0)
+		goto end;
+	/* aw */
+	res = __mdiobus_write(bus, addr, reg_addr, 0);
+	if (res)
+		goto end;
+	/* aw */
+	res = __mdiobus_write(bus, addr, reg_addr, 0);
+	if (res)
+		goto end;
+	/* dr */
+	res = __mdiobus_read(bus, addr, reg_data);
+	if (res < 0)
+		goto end;
+	/* dr */
+	res = __mdiobus_read(bus, addr, reg_data);
+	if (res < 0)
+		goto end;
+	/* AH --dr--> AH */
+	res = __mdiobus_read(bus, addr, reg_data);
+	if (res < 0)
+		goto end;
+	res = res != 0xdead ? -EIO : 0;
+
+end:
+	mutex_unlock(&bus->mdio_lock);
+
+	mdio->synced = !res;
+	return res;
+}
+
 static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
 {
 	struct yt921x_reg_mdio *mdio = context;
@@ -108,6 +196,12 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
 	u32 val;
 	int res;
 
+	if (!mdio->synced) {
+		res = yt921x_reg_mdio_resync(mdio);
+		if (res)
+			return res;
+	}
+
 	/* Hold the mdio bus lock to avoid (un)locking for 4 times */
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
@@ -136,6 +230,9 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
 
 end:
 	mutex_unlock(&bus->mdio_lock);
+
+	if (res)
+		mdio->synced = false;
 	return res;
 }
 
@@ -148,6 +245,12 @@ static int yt921x_reg_mdio_write(void *context, u32 reg, u32 val)
 	u32 reg_data;
 	int res;
 
+	if (!mdio->synced) {
+		res = yt921x_reg_mdio_resync(mdio);
+		if (res)
+			return res;
+	}
+
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
 	reg_addr = YT921X_SMI_SWITCHID(mdio->switchid) | YT921X_SMI_ADDR |
@@ -172,6 +275,9 @@ static int yt921x_reg_mdio_write(void *context, u32 reg, u32 val)
 
 end:
 	mutex_unlock(&bus->mdio_lock);
+
+	if (res)
+		mdio->synced = false;
 	return res;
 }
 
-- 
2.53.0

Re: [PATCH net-next] net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

[Andrew Lunn]

>  static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
>  {
>  	struct yt921x_reg_mdio *mdio = context;
> @@ -108,6 +196,12 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
>  	u32 val;
>  	int res;
>  
> +	if (!mdio->synced) {
> +		res = yt921x_reg_mdio_resync(mdio);
> +		if (res)
> +			return res;
> +	}
> +
>  	/* Hold the mdio bus lock to avoid (un)locking for 4 times */
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
>  
> @@ -136,6 +230,9 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
>  
>  end:
>  	mutex_unlock(&bus->mdio_lock);
> +
> +	if (res)
> +		mdio->synced = false;

Why defer the recovery to the next transaction? Why not do it now. And
if the recovery fails, you can report the transaction which actually
killed it, which might be useful for debugging.

       Andrew

Re: [PATCH net-next] net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

[David Yang]

On Sat, Nov 14, 2026 at 11:40 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> >  static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
> >  {
> >       struct yt921x_reg_mdio *mdio = context;
> > @@ -108,6 +196,12 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
> >       u32 val;
> >       int res;
> >
> > +     if (!mdio->synced) {
> > +             res = yt921x_reg_mdio_resync(mdio);
> > +             if (res)
> > +                     return res;
> > +     }
> > +
> >       /* Hold the mdio bus lock to avoid (un)locking for 4 times */
> >       mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> >
> > @@ -136,6 +230,9 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
> >
> >  end:
> >       mutex_unlock(&bus->mdio_lock);
> > +
> > +     if (res)
> > +             mdio->synced = false;
>
> Why defer the recovery to the next transaction? Why not do it now. And
> if the recovery fails, you can report the transaction which actually
> killed it, which might be useful for debugging.
>
>        Andrew

If the recovery fails, you have to, anyway, try to resync in the next
transaction, so recovery here does not help other than adding more
code. And we did not and do not report failure here, instead we let
the caller do it.

Re: [PATCH net-next] net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

[Andrew Lunn]

> > > @@ -136,6 +230,9 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
> > >
> > >  end:
> > >       mutex_unlock(&bus->mdio_lock);
> > > +
> > > +     if (res)
> > > +             mdio->synced = false;
> >
> > Why defer the recovery to the next transaction? Why not do it now. And
> > if the recovery fails, you can report the transaction which actually
> > killed it, which might be useful for debugging.
> >
> >        Andrew
> 
> If the recovery fails, you have to, anyway, try to resync in the next
> transaction, so recovery here does not help other than adding more
> code.

I don't know why it adds more code. You don't need mdio->synced. You
just call the recovery function. If it fails, it fails, and you assume
the switch is dead, but report the error and keep going. The next
transaction is also likely to fail, probably causing another sync
attempt, which might work, but by then, you have to assume the switch
is wrongly configured, even if the transaction works.

	Andrew

Re: [PATCH net-next] net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

[Andrew Lunn]

> Realign the machine before the next access does its own work. Also, the
> initial value is unsynced, so any stale state is not inherited.

Seems like you should call the sync function in yt921x_chip_reset
before doing anything else.

       Andrew

Re: [PATCH net-next] net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] yt921x_reg_mdio_resync() does not reliably realign the SMI slave
  after an interrupted write, even by the patch's own state table.
- [Medium] yt921x_mdio_probe() allocates mdio with devm_kzalloc(), so
  synced starts out false.
- [Low] The commit message presents this as a fix for a user-visible
  defect: one interrupted MDIO access leaves the SMI slave misaligned,…

--

net: dsa: motorcomm: Resync SMI slave after interrupted MDIO access

This patch adds a synced flag to struct yt921x_reg_mdio. It also adds
yt921x_reg_mdio_resync(), which sends a fixed sequence of MDIO frames to
realign the SMI slave state machine before the next register access, and
clears the flag whenever an access fails. The flag starts out false, so
the first access after probe always runs the resync.

> The driver so far assumed every access succeeds, and relied on manual
> resetting when the state machine wedged.

[Severity: Low]
This reads as a fix for a user-visible problem, but there's no Fixes:
tag. The access helpers that assumed every access succeeds appear to come
from:

186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

Should a Fixes: tag pointing at that commit be added, so stable trees
that carry the driver can pick up this recovery?

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 33e5fc127244d..8b6c9739b6841 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -94,10 +94,98 @@ struct yt921x_reg_mdio {

[ ... ]

> + * State Input Next Comment
> + * ----- ----- ---- -------
> + * AH    aw    AL
> + * AH    dr    AH   `0xdead` sentinel
> + * AL    aw    DH
> + * AL    dr    AL   `0xdead` sentinel
> + * DHr   aw    AH   abort, frame discarded
> + * DHr   dr    DLr
> + * DLr   aw    AH   abort, frame discarded
> + * DLr   dr    AH   the access is done
> + * DHw   aw    AH   abort, frame discarded
> + * DHw   dw    DLw
> + * DLw   aw    AL   abort, frame consumed as next access's addr-hi
> + * DLw   dr    ?    `0xdead` sentinel; stay or advance undetermined
> + * DLw   dw    AH   the access commits
> + *
> + * The table lists the whitelisted frames only; any other REGAD/op combination
> + * is not characterized and is never emitted here.
> + *
> + * The resync sequence is dr, dr, aw, aw, dr, dr, plus one more dr as a sanity
> + * check: the machine must be back at AH, which reads the `0xdead` sentinel.
> + */
> +static int yt921x_reg_mdio_resync(struct yt921x_reg_mdio *mdio)
> +{

[ ... ]

> +	/* dr */
> +	res = __mdiobus_read(bus, addr, reg_data);
> +	if (res < 0)
> +		goto end;
> +	/* dr */
> +	res = __mdiobus_read(bus, addr, reg_data);
> +	if (res < 0)
> +		goto end;
> +	/* aw */
> +	res = __mdiobus_write(bus, addr, reg_addr, 0);
> +	if (res)
> +		goto end;
> +	/* aw */
> +	res = __mdiobus_write(bus, addr, reg_addr, 0);
> +	if (res)
> +		goto end;
> +	/* dr */
> +	res = __mdiobus_read(bus, addr, reg_data);
> +	if (res < 0)
> +		goto end;
> +	/* dr */
> +	res = __mdiobus_read(bus, addr, reg_data);
> +	if (res < 0)
> +		goto end;
> +	/* AH --dr--> AH */
> +	res = __mdiobus_read(bus, addr, reg_data);
> +	if (res < 0)
> +		goto end;
> +	res = res != 0xdead ? -EIO : 0;
> +
> +end:
> +	mutex_unlock(&bus->mdio_lock);
> +
> +	mdio->synced = !res;
> +	return res;
> +}

[Severity: High]
Does this sequence realign the slave when it starts in DHw?

The table has no row for DHw + dr, and the comment says uncharacterized
combinations are never emitted here. Even so, yt921x_reg_mdio_resync()
always starts with two dr frames, whatever state the slave is in.

DHw can be reached from yt921x_reg_mdio_write() in two ways:

- The aw(lo) frame is latched by the slave, but the controller still
  reports an error.
- aw(lo) succeeds, and the first data write fails before the slave
  latches it:

yt921x_reg_mdio_write() {
    ...
	res = __mdiobus_write(bus, addr, reg_data, (u16)(val >> 16));
	if (res)
		goto end;
    ...
}

Either way synced becomes false, and the next access runs the resync
starting from DHw.

Suppose DHw + dr is a no-op, the same as dr in AH and AL. Then the
sequence would go:

DHw -dr-> DHw -dr-> DHw -aw-> AH (abort) -aw-> AL -dr-> AL -dr-> AL

The final dr in AL then returns 0xdead, per the "AL dr AL" row.

Can the res != 0xdead check tell AH apart from AL? By the table, dr
returns 0xdead in AH, AL and DLw. So the check would pass, and
mdio->synced would be set to true while the slave is still at AL.

What happens on the next access after that? A read would go:

AL -aw-> DHr -aw-> AH (abort) -dr-> 0xdead -dr-> 0xdead

yt921x_reg_mdio_read() would then return success with *valp = 0xdeaddead.
yt921x_reg_update_bits() in smi.c could write (0xdeaddead & ~mask) | val
back to a real register, because by then the slave is at AH and that
write commits.

A write would go AL -aw-> DHw -aw-> AH (abort), followed by dw, dw in
AH. The table doesn't cover that, so the write would be lost while still
returning success.

synced is only cleared when __mdiobus_read() or __mdiobus_write() returns
an error, so neither case would be detected.

Would a sequence built only from whitelisted transitions work better? One
example is aw, dr, dr, aw, aw, dr, dr. The leading aw aborts from DHr, DLr
and DHw, and DLw + aw is a documented abort.

Ending with a full read of a register whose known value is not 0xdead
would also let the check tell AH from AL.

> @@ -108,6 +196,12 @@ static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
>  	u32 val;
>  	int res;
>  
> +	if (!mdio->synced) {
> +		res = yt921x_reg_mdio_resync(mdio);
> +		if (res)
> +			return res;
> +	}
> +

[Severity: Medium]
yt921x_mdio_probe() allocates mdio with devm_kzalloc(), so synced starts
out false. The first register access at probe therefore goes through the
resync:

yt921x_dsa_setup()
  yt921x_chip_reset()
    yt921x_chip_detect()
      yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid)
        yt921x_reg_mdio_read()
          yt921x_reg_mdio_resync()

A supported part might return something other than exactly 0xdead for a
data read in AH. In that case yt921x_reg_mdio_resync() returns -EIO
without logging anything. yt921x_chip_detect() passes the error straight
up, so its "Unexpected chipid" and "Unsupported chipid" messages can no
longer be reached.

Every later access would then retry the full 7-frame resync, and each
attempt would fail silently with -EIO.

Is the 0xdead sentinel known to hold on all of YT9213, YT9214, YT9215
and YT9218? The driver header mentions only YT9215+SGMII as tested, and
the commit message doesn't mention the sentinel.

Could the resync print something when the sentinel doesn't match, so
this new failure mode can be diagnosed?

Also, each resync does a real read access of register 0x0 (aw 0, aw 0,
dr, dr). The commit message doesn't mention that either.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111163954.1132921-1-mmyangfl%40gmail.com