Re: [PATCH net-next 5/8] net: mdio: realtek-rtl9300: Add c45 over c22 mitigation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mdio: realtek-rtl9300: Add c45 over c22 mitigation

This patch adds a per-port state machine that disables hardware MAC polling
around a register 13/14/13/14 c45-over-c22 MMD access sequence so the
sequence cannot be interrupted by a parallel polling read.  Polling is
disabled when the first MII_MMD_CTRL access is seen and re-enabled when
the four-step sequence completes or unexpected input is detected.

A few questions on the new state-machine corner cases below.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index a7fd075947b6..e206ee3e2b1c 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -254,6 +257,43 @@ static int otto_emdio_set_port_polling(struct otto_emdio_priv *priv, int port, b
>  				  BIT(port % 32), active);
>  }
>  
> +static int otto_emdio_mmd_prefix(struct otto_emdio_priv *priv, int port, int regnum)
> +{
> +	u8 newstate, *state = &priv->mmd_state[port];
> +	int expected, ret = 0;
> +
> +	if (!priv->init_done)
> +		return 0;
> +	/*
> +	 * Disabled polling might produce link flapping and false notification interrupts on the
> +	 * MAC layer. In this case disable c45 over c22 MMD access because chances are high that
> +	 * the register 13/14/13/14 sequence is intercepted by a parallel hardware access. As
> +	 * a workaround the PHY must provide its own mmd read/write() callbacks and redirect to
> +	 * normal c22 registers. See rtlgen_read_mmd().
> +	 */
> +	if (priv->info->link_flap)
> +		return (regnum == MII_MMD_DATA || regnum == MII_MMD_CTRL) ? -EIO : 0;
> +
> +	expected = (*state & 1) ? MII_MMD_DATA : MII_MMD_CTRL;
> +	newstate = regnum == expected ? *state + 1 : 0;
> +
> +	if (newstate == 1 || newstate < *state)
> +		ret = otto_emdio_set_port_polling(priv, port, !newstate);
> +	*state = newstate;
> +
> +	return ret;
> +}
> +
> +static int otto_emdio_mmd_postfix(struct otto_emdio_priv *priv, int port, int regnum)
> +{
> +	if (priv->mmd_state[port] != 4)
> +		return 0;
> +
> +	priv->mmd_state[port] = 0;
> +
> +	return otto_emdio_set_port_polling(priv, port, true);
> +}

[High]
Can otto_emdio_mmd_prefix() and otto_emdio_mmd_postfix() get out of sync
on a transient MDIO failure?

The prefix transitions state 0->1 on the first MII_MMD_CTRL access and
calls otto_emdio_set_port_polling(port, false).  In the c22 read/write
helpers below, if priv->info->read_c22() / write_c22() then returns an
error (otto_emdio_run_cmd() can return -ETIMEDOUT from
regmap_read_poll_timeout(), -ENXIO via cmd_fail, or any regmap_write()
error), the function returns immediately and never calls
otto_emdio_mmd_postfix().

Polling is left disabled, and *state stays non-zero.

On subsequent retries with regnums that still match the parity-based
expected value (the natural retry shape for an EEE 13/14/13/14
sequence), mmd_prefix keeps doing *state + 1 unbounded (state walks
through 5, 6, 7, ...).  mmd_postfix only re-enables polling when
mmd_state == 4 exactly:

	if (priv->mmd_state[port] != 4)
		return 0;

so once the counter overshoots 4 it stays disabled until either an
unrelated regnum forces newstate < *state in mmd_prefix, or the u8
wraps at 255->0.

The commit message says polling is re-enabled "as soon as the sequence
is finished or any other unexpected input is detected", but a transient
MDIO failure mid-sequence is neither.

Should the error paths in otto_emdio_read_c22() / otto_emdio_write_c22()
reset mmd_state and re-enable polling on a failed transfer?

> @@ -463,7 +503,15 @@ static int otto_emdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
>  		if (regnum == 31)
>  			return priv->page[port];
>  
> +		ret = otto_emdio_mmd_prefix(priv, port, regnum);
> +		if (ret)
> +			return ret;
> +
>  		ret = priv->info->read_c22(bus, port, regnum, &value);
> +		if (ret)
> +			return ret;
> +
> +		ret = otto_emdio_mmd_postfix(priv, port, regnum);
>  	}
>  
>  	return ret ? ret : value;

[Medium]
The regnum == 31 page-select path returns from inside the scoped_guard
before reaching mmd_prefix() / mmd_postfix():

		if (regnum == 31)
			return priv->page[port];

If a page-register access occurs while mmd_state is mid-sequence (1, 2,
or 3), the counter is not reset and the polling-disable bit is not
restored, even though a page write between MMD steps is exactly the
"unexpected input" the commit message describes.

Recovery only happens on the next non-13/14 access that falls through
mmd_prefix and triggers the newstate < *state branch.

Should the regnum == 31 path also reconcile mmd_state and re-enable
polling when a sequence is in progress?  The same question applies in
otto_emdio_write_c22().

[Low]
A separate observation about the read return path:

>  	return ret ? ret : value;

After a successful priv->info->read_c22() populates value, ret is
overwritten by mmd_postfix()'s return.  If
otto_emdio_set_port_polling(priv, port, true) inside mmd_postfix()
fails, the function returns that error even though the data transfer
succeeded, so the PHY layer discards correctly read data.  For
read-clear / latched status registers the hardware-side bits have
already been consumed.

Would it be preferable to treat a postfix failure as a polling-
bookkeeping issue (log and proceed) and still return the read value?

> @@ -487,7 +535,15 @@ static int otto_emdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16
>  			return 0;
>  		}
>  
> -		return priv->info->write_c22(bus, port, regnum, value);
> +		ret = otto_emdio_mmd_prefix(priv, port, regnum);
> +		if (ret)
> +			return ret;
> +
> +		ret = priv->info->write_c22(bus, port, regnum, value);
> +		if (ret)
> +			return ret;
> +
> +		return otto_emdio_mmd_postfix(priv, port, regnum);
>  	}
>  }

The same shape exists for the write path: a postfix failure here turns
a successful write into a reported error and may cause an upper-layer
retry of an already-applied write.