Re: [PATCH net] net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: stmmac: dwmac-sun8i: reset the EMAC when opening, not when probing

The EMAC soft reset is moved out of sun8i_dwmac_probe() and into the
stmmac_dma_ops->reset hook, so it runs from stmmac_init_dma_engine()
instead of at probe time.  A new helper sun8i_dwmac_soft_reset() holds the
assert-and-poll sequence, and sun8i_dwmac_reset() is kept as a thin
wrapper for the mdio-mux switch callback.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 48c52eb96233..748ebabd933c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -272,8 +272,34 @@ static const struct emac_variant emac_variant_h6 = {
>  /* sun8i_dwmac_dma_reset() - reset the EMAC
>   * Called from stmmac via stmmac_dma_ops->reset
>   */
> +static int sun8i_dwmac_soft_reset(void __iomem *ioaddr)

[Severity: Low]
This isn't a bug, but the existing header block now sits above the wrong
function.  Neither statement in it holds for sun8i_dwmac_soft_reset():
the ops table binds .reset = sun8i_dwmac_dma_reset, and the new helper
has a second caller, sun8i_dwmac_reset(), used by the mdio-mux switch
callback.  sun8i_dwmac_dma_reset() is then the only dma ops callback in
this file without a header comment naming it, unlike
sun8i_dwmac_dma_init() just below.  Should the comment move down with the
function it describes?

> +{
> +	u32 v;
> +
> +	v = readl(ioaddr + EMAC_BASIC_CTL1);
> +	writel(v | 0x01, ioaddr + EMAC_BASIC_CTL1);
> +
> +	/* The timeout was previously set to 10ms, but some board (OrangePI0)
> +	 * need more if no cable plugged. 100ms seems OK
> +	 */
> +	return readl_poll_timeout(ioaddr + EMAC_BASIC_CTL1, v,
> +				  !(v & 0x01), 100, 100000);
> +}

[Severity: Medium]
Is 100 ms still enough in the new context?  The comment above the poll
was written for a probe-time reset, where the PHY had been powered and
clocking since boot.

In the new call site the PHY may have left BMCR_PDOWN only microseconds
earlier.  __stmmac_open() and stmmac_resume() both do:

	phylink_prepare_resume(priv->phylink);
	...
	ret = stmmac_hw_setup(dev);

and phylink_prepare_resume() itself says:

	/* IEEE 802.3 22.2.4.1.5 allows PHYs to stop their receive clock
	 * when PDOWN is set. ... Note that 802.3 allows PHYs 500ms before
	 * the clock meets requirements. We do not implement this delay.
	 */
	if (pl->config->mac_requires_rxc && phydev && phydev->suspended)
		phy_resume(phydev);

Only stmmac_reset_queues_param() and descriptor clearing sit between
phy_resume() and stmmac_init_dma_engine() -> sun8i_dwmac_dma_reset(), so
on a PHY that is slow to restart RXC after power-down the poll can expire
and stmmac_hw_setup() returns -ETIMEDOUT on ifup, MTU change and system
resume.

The other stmmac variants that soft-reset in this hook allow more
headroom: 200 ms in dwmac_lib.c, 1 s in dwmac4_lib.c and dwmac-imx.c, 2 s
in dwmac-loongson.c.  Would raising the budget here be reasonable?

>  static int sun8i_dwmac_dma_reset(void __iomem *ioaddr)
>  {
> +	int err;
> +
> +	/* The MAC soft reset only completes once the PHY is driving the RX
> +	 * clock. Doing it here rather than at probe means phylib has already
> +	 * attached and resumed the PHY, so the clock is running by
> +	 * construction -- including after a warm reboot that left the PHY
> +	 * powered down.
> +	 */

[Severity: Medium]
Is the stated precondition accurate for all callers of the hook?  The
commit message says:

  "stmmac calls it from stmmac_init_dma_engine(), under stmmac_hw_setup(),
   whose only two callers are __stmmac_open() and stmmac_resume() -- both
   of which run after stmmac_init_phy() has attached and resumed the PHY."

stmmac_resume() does not call stmmac_init_phy() at all, and __stmmac_open()
is also reached from stmmac_change_mtu() and the XDP re-open path without
going through stmmac_open() -> stmmac_init_phy().

What actually guarantees a running receive clock before stmmac_hw_setup()
on those paths is phylink_prepare_resume(), added by commit 06232cb44bc8e
three weeks before this patch:

	phylink_prepare_resume(priv->phylink);

	stmmac_reset_queues_param(priv);

	ret = stmmac_hw_setup(dev);

In the current tree the behaviour is fine, since stmmac sets
mac_requires_rxc unconditionally, so phylink_prepare_resume() does resume
a suspended PHY first.  But the patch carries Fixes: 9f93ac8d4085 (2017).
In a stable tree without 06232cb44bc8e, would sun8i_dwmac_dma_reset()
assert SOFT_RST with an external PHY still in BMCR_PDOWN on every resume
and every MTU change, turning a probe-time failure into a resume and
MTU-change failure?

Could the commit message name phylink_prepare_resume() as the actual
dependency, and note it explicitly for backporters?

> +	err = sun8i_dwmac_soft_reset(ioaddr);
> +	if (err)
> +		return err;
> +
>  	writel(0, ioaddr + EMAC_RX_CTL1);
>  	writel(0, ioaddr + EMAC_TX_CTL1);
>  	writel(0, ioaddr + EMAC_RX_FRM_FLT);
> @@ -740,23 +766,12 @@ static void sun8i_dwmac_flow_ctrl(struct mac_device_info *hw,
>  
>  static int sun8i_dwmac_reset(struct stmmac_priv *priv)
>  {
> -	u32 v;
> -	int err;
> -
> -	v = readl(priv->ioaddr + EMAC_BASIC_CTL1);
> -	writel(v | 0x01, priv->ioaddr + EMAC_BASIC_CTL1);
> +	int err = sun8i_dwmac_soft_reset(priv->ioaddr);

[Severity: Medium]
This isn't a problem introduced by this patch -- the mdio-mux reset call
site predates it -- but does the failure mode survive there for the
H3/V3s variants, the only ones with soc_has_internal_phy = true?

The commit message mentions the retained caller:

  "sun8i_dwmac_reset() stays for the mdio-mux switch callback, which needs
   a reset after changing the syscon and cannot use the hook."

That callback still runs at probe time.  mdio_mux_read() invokes the
switch callback before forwarding the first MDIO transfer:

drivers/net/mdio/mdio-mux.c:mdio_mux_read() {
	...
	r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
	if (r)
		goto out;
	...
}

so during sun8i_dwmac_probe() -> sun8i_dwmac_register_mdio_mux() ->
mdio_mux_init() -> of_mdiobus_register(), the callback runs with
current_child == -1, selects the external child, unpowers the internal
PHY and calls sun8i_dwmac_reset() -> sun8i_dwmac_soft_reset() before
phylib has attached or resumed the external PHY.

On a board whose external PHY came back from a warm reboot in BMCR_PDOWN
-- the scenario in the commit message -- would the poll return -ETIMEDOUT,
switch_fn fail, mdio_mux_read() abort, and the external PHY never be
discovered?

The new comment in sun8i_dwmac_dma_reset() states that phylib has already
attached and resumed the PHY, which this second caller of the same helper
does not honour.

>  
> -	/* The timeout was previously set to 10ms, but some board (OrangePI0)
> -	 * need more if no cable plugged. 100ms seems OK
> -	 */
> -	err = readl_poll_timeout(priv->ioaddr + EMAC_BASIC_CTL1, v,
> -				 !(v & 0x01), 100, 100000);
> -
> -	if (err) {
> +	if (err)
>  		dev_err(priv->device, "EMAC reset timeout\n");
> -		return err;
> -	}
> -	return 0;
> +
> +	return err;
>  }

[ ... ]
-- 
This is an AI-generated review.