Re: [PATCH net-next v14 07/10] net: stmmac: qcom-ethqos: fix SGMII loopback not set on resume after speed change

[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: stmmac: qcom-ethqos: fix SGMII loopback not set on resume after speed change

This extends the guard in qcom_ethqos_set_sgmii_loopback() so that the MAC
wrapper TX-to-RX loopback is also armed when ethqos->phy_mode holds
PHY_INTERFACE_MODE_SGMII, not only PHY_INTERFACE_MODE_2500BASEX. Without
the loopback, dwmac4_dma_reset() polls an RX clock that never toggles and
times out.

No Link: tags were present in the commit header.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index fd7414a50eb4e..e067ceee01621 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -190,7 +190,8 @@ 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)
> +	    (ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX &&
> +	     ethqos->phy_mode != PHY_INTERFACE_MODE_SGMII))
>  		return;

[Severity: Medium]
Should this hunk be squashed into the preceding patch of the series, or at
least carry a Fixes: tag naming it?

The commit message says "mac_finish() now updates phy_mode to the
runtime-resolved interface", which is the change made one patch earlier in
this same series:

  f40e72d57bcb38 ("net: stmmac: qcom-ethqos: update phy_mode to the
  resolved interface in mac_finish()")

git blame at this commit shows the assignment in ethqos_mac_finish_serdes():

	ethqos->phy_mode = interface;

came from f40e72d57bcb38. Before that, ethqos->phy_mode was written only
once, in qcom_ethqos_probe() from the DT phy-mode property, so the
2500BASEX-only guard could not go stale at runtime and the DMA reset
timeout described here did not exist.

Does that leave f40e72d57bcb38 broken on its own, e.g. as a bisect point or
a stable backport? On a needs_sgmii_loopback platform (emac_v4_0_0_data)
with DT phy-mode "2500base-x" whose link resolves to 1G SGMII:

  ethqos_mac_finish_serdes()      -> ethqos->phy_mode = PHY_INTERFACE_MODE_SGMII
  suspend / ifdown
  resume / ifup
  ethqos_clks_config(ethqos, true)
    qcom_ethqos_set_sgmii_loopback(ethqos, true)
      if (... ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX)
              return;                 /* loopback never enabled */
  dwmac4_dma_reset()                  /* polls a non-toggling clock, times out */

Also, the subject line reads as a standalone fix for a pre-existing
problem, which hides the dependency on the previous patch. Would either
squashing the guard change into f40e72d57bcb38, or adding

  Fixes: f40e72d57bcb38 ("net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()")

make the ordering requirement explicit?

One more note on the commit message wording: the second paragraph reads
"qcom_ethqos_set_sgmii_loopback() gates this on phy_mode being 2500BASEX.
mac_finish() now updates phy_mode to the runtime-resolved interface, any
subsequent resume with a 1G SGMII link skips the loopback setup" — the
sentence joining is missing a connector such as "so that" or "and since",
which makes the causal relationship hard to follow.