Re: [PATCH net-next v14 06/10] net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()

[Jakub Kicinski]

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: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()

This makes ethqos_mac_finish_serdes() store the interface that phylink
resolved into ethqos->phy_mode, so that a later serdes_powerup() call
programs the SerDes for the interface currently in use rather than the one
read from the DT phy-mode property at probe.

A few questions below.

[Severity: Low]
The commit message says:

  "The next serdes_powerup() call, on a subsequent link cycle, then
  reinitialises the SerDes to the wrong mode."

That describes a concrete hardware failure, but there is no Fixes: tag and
no mention of affected platforms or kernels.

Looking at the baseline, qcom_ethqos_serdes_powerup() does not read
ethqos->phy_mode at all:

  static int qcom_ethqos_serdes_powerup(struct net_device *ndev, void *priv)
  {
        struct qcom_ethqos *ethqos = priv;
        int ret;

        ret = phy_init(ethqos->serdes_phy);
        if (ret)
                return ret;

        ret = phy_power_on(ethqos->serdes_phy);
  ...

The phy_set_mode_ext(..., ethqos->phy_mode) call that consumes the stale
value is added by the preceding patch in this series, "net: stmmac:
qcom-ethqos: set serdes mode before powerup" (7bf7d5daca55). So the
condition being repaired here does not exist upstream, and a Fixes: tag
would have to point at an unmerged in-series commit.

Would it make more sense to fold this hunk into that patch, or reorder the
two, and reword the message so it does not read as a fix for a shipping
regression? As posted it is hard for a backporter to tell whether this is a
stable candidate or preparation work.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 47b70b5e706f2..fd7414a50eb4e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -634,6 +634,12 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  
>  	qcom_ethqos_set_sgmii_loopback(ethqos, false);
>  
> +	/* The interface mode is only known once phylink has resolved it and
> +	 * may change at runtime, so keep it in sync for later
> +	 * serdes_powerup()/serdes_powerdown() calls.
> +	 */

[Severity: Low]
This isn't a bug, but does qcom_ethqos_serdes_powerdown() really depend on
this field?

  static void qcom_ethqos_serdes_powerdown(struct net_device *ndev, void *priv)
  {
        struct qcom_ethqos *ethqos = priv;

        phy_power_off(ethqos->serdes_phy);
        phy_exit(ethqos->serdes_phy);
  }

It takes no interface argument and never reads ethqos->phy_mode. The two
readers of the field are qcom_ethqos_serdes_powerup() and
qcom_ethqos_set_sgmii_loopback(); the latter is the one whose behaviour
this assignment changes, and it is not mentioned in the comment.

> +	ethqos->phy_mode = interface;

[Severity: High]
Can this break qcom_ethqos_set_sgmii_loopback() on sa8775p?

That helper gates on phy_mode being exactly the DT interface:

  static void
  qcom_ethqos_set_sgmii_loopback(struct qcom_ethqos *ethqos, bool enable)
  {
        if (!ethqos->needs_sgmii_loopback ||
            ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX)
                return;
  ...

and it is called from ethqos_clks_config() with the clocks just enabled:

  ethqos_clks_config()
        /* Enable functional clock to prevent DMA reset to timeout due
         * to lacking PHY clock after the hardware block has been power
         * cycled. ...
         */
        qcom_ethqos_set_sgmii_loopback(ethqos, true);
        ethqos_set_func_clk_en(ethqos);

ethqos_clks_config() runs at probe and again on every resume through
stmmac_bus_clks_config() -> plat_dat->clks_config(), i.e. before phylink
has resolved anything for the new session, so it reads whatever the
previous session left behind.

emac_v4_0_0_data sets .needs_sgmii_loopback = true, so on a 2500base-x
board that last linked at 1G SGMII:

  ethqos_mac_finish_serdes()  -> ethqos->phy_mode = PHY_INTERFACE_MODE_SGMII
  suspend / ifdown
  resume -> ethqos_clks_config(true)
         -> qcom_ethqos_set_sgmii_loopback(ethqos, true)   /* returns early */
         -> DMA soft reset with no RX clock

Does dwmac4_dma_reset() then time out and fail open()/resume?

The next patch in this series, "net: stmmac: qcom-ethqos: fix SGMII
loopback not set on resume after speed change", describes the same symptom
("any subsequent resume with a 1G SGMII link skips the loopback setup and
the DMA reset hangs") and widens the guard to also accept SGMII. Could the
guard change be squashed into this patch, or this patch ordered after it,
so that a bisect landing here does not hang resume?

Separately, even with the widened guard the workaround is keyed on
{2500BASEX, SGMII} only, while phylink hands mac_finish() whatever the PHY
reported:

  drivers/net/phy/phylink.c:phylink_resolve()
        ...
        link_state.interface = pl->phy_state.interface;

Would keeping the DT interface in its own field for the wrapper workaround,
and using the resolved interface only for the SerDes mode, avoid coupling
the two?

>  	if (interface == PHY_INTERFACE_MODE_SGMII ||
>  	    interface == PHY_INTERFACE_MODE_2500BASEX)
>  		ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,

[ ... ]