Re: [PATCH net-next 6/9] net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] Unbounded log spam: the new dev_warn("legacy RGMII phy-mode
  detected; consider upgrading to a newer DTB") is placed in the body of…
- [Medium] The warning fires for a PHY mode the commit message never
  mentions.

--

net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes

This adds a dev_warn() in ethqos_fix_mac_speed_rgmii() for the RGMII
configuration paths that are not rgmii-id, to encourage migration to
DTBs using "rgmii-id" with PHY-provided delays.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 67738c1598c50..2ca2f03de3017 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -554,6 +554,8 @@ static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
>  		return;
>  	}
>  
> +	dev_warn(dev, "legacy RGMII phy-mode detected; consider upgrading to a newer DTB\n");

[Severity: Medium]
Can this print repeat without bound?  ethqos_fix_mac_speed_rgmii() is
installed as plat_dat->fix_mac_speed in qcom_ethqos_probe():

  ethqos->phy_mode = plat_dat->phy_interface;
  switch (ethqos->phy_mode) {
  case PHY_INTERFACE_MODE_RGMII:
  case PHY_INTERFACE_MODE_RGMII_ID:
  case PHY_INTERFACE_MODE_RGMII_RXID:
  case PHY_INTERFACE_MODE_RGMII_TXID:
          plat_dat->fix_mac_speed = ethqos_fix_mac_speed_rgmii;

and stmmac_mac_link_up() calls it on every resolved link:

  if (priv->plat->fix_mac_speed)
          priv->plat->fix_mac_speed(priv->plat->bsp_priv, interface,
                                    speed, mode);

The only early return before the new print is the
phy_mode == PHY_INTERFACE_MODE_RGMII_ID branch, so for the other RGMII
modes every link-up, cable re-plug, link flap, autoneg speed change,
ifdown/ifup and resume emits another identical warning.  The condition
being reported is the static DT phy-mode, which is already known at probe
time and does not change afterwards.

Would dev_warn_once(), or a single check in qcom_ethqos_probe() where the
phy_mode switch already exists, be preferable here?  dev_warn_ratelimited()
would at least bound the output.

[Severity: Medium]
The commit message says:

  "However, existing DTBs use "rgmii" and "rgmii-txid" and must remain
   supported for backwards compatibility."

  "Warn when either of these legacy PHY modes is used [...]"

but the code warns for anything reaching ethqos_fix_mac_speed_rgmii()
that is not PHY_INTERFACE_MODE_RGMII_ID, which also covers
PHY_INTERFACE_MODE_RGMII_RXID, dispatched to this callback by the switch
in qcom_ethqos_probe().

Documentation/devicetree/bindings/net/ethernet-controller.yaml lists
rgmii-rxid as the correct description when the TX delay is provided by the
PCB:

      # TX delay is provided by the PCB. See below
      - rgmii-rxid

So a board that legitimately describes itself as rgmii-rxid is now told to
move to "rgmii-id", which would misdescribe the hardware.

Should the condition be narrowed to the two modes named in the commit
message, or should the commit message explain why rgmii-rxid is deprecated
too?

>  	/* Initialize the DLL first */
>  
>  	/* Set DLL_RST */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com