Re: [PATCH net-next v15 5/9] net: stmmac: qcom-ethqos: set serdes mode before powerup

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In qcom_ethqos_serdes_powerup()
  (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c), the patch…

--

net: stmmac: qcom-ethqos: set serdes mode before powerup

This patch adds a call to phy_set_mode_ext() before phy_power_on() in
qcom_ethqos_serdes_powerup(). On SCMI systems the PHY driver needs it to
pick the right performance level, which sets the link speed. It prepares
for adding support for the firmware-managed EMAC on Qualcomm sa8255p.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index ac7d6d3e205a1..47b70b5e706f2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -601,10 +601,19 @@ static int qcom_ethqos_serdes_powerup(struct net_device *ndev, void *priv)
>  	if (ret)
>  		return ret;
>  
> +	ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
> +			       ethqos->phy_mode);
> +	if (ret)
> +		goto err_out;

[Severity: Medium]
What happens here if ethqos->phy_mode is one of the RGMII variants?

qcom_ethqos_probe() accepts RGMII, RGMII_ID, RGMII_RXID and RGMII_TXID.
It installs serdes_powerup whenever a "serdes" PHY is found, whatever the
phy_mode is:

qcom_ethqos_probe() {
    ...
	if (ethqos->serdes_phy) {
		plat_dat->serdes_powerup = qcom_ethqos_serdes_powerup;
		plat_dat->serdes_powerdown  = qcom_ethqos_serdes_powerdown;
	}
    ...
}

The qcom,ethqos binding allows this combination too. It has "phys: true"
and "phy-names: const: serdes", with no condition on phy-mode.

Both SerDes PHY drivers reject any submode other than SGMII, 1000BASEX
and 2500BASEX. In phy-qcom-sgmii-eth.c, qcom_dwmac_sgmii_phy_validate()
is called from qcom_dwmac_sgmii_phy_set_mode():

	if (submode == PHY_INTERFACE_MODE_SGMII ||
	    submode == PHY_INTERFACE_MODE_1000BASEX ||
	    submode == PHY_INTERFACE_MODE_2500BASEX)
		return 0;

	return -EINVAL;

The new qcom_dwmac_sgmii_phy_scmi_validate() in
phy-qcom-sgmii-eth-scmi.c does the same.

Take a DT that sets phy-mode = "rgmii*" and also references an enabled
serdes PHY. With this patch, powerup would fail like this:

stmmac_open()
  stmmac_legacy_serdes_power_up()
    qcom_ethqos_serdes_powerup()
      phy_set_mode_ext()
        qcom_dwmac_sgmii_phy_set_mode()
          qcom_dwmac_sgmii_phy_validate() returns -EINVAL
      goto err_out -> phy_exit()

"SerDes powerup failed" is logged, and both stmmac_open() and
stmmac_resume() fail. Before this patch the same DT powered the SerDes
on without an error.

The other caller of this API in the same file already limits the call to
SerDes modes:

ethqos_mac_finish_serdes() {
    ...
	if (interface == PHY_INTERFACE_MODE_SGMII ||
	    interface == PHY_INTERFACE_MODE_2500BASEX)
		ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
				       interface);
    ...
}

Later patches in the series don't change serdes_powerup. The patch that
updates phy_mode to the resolved interface in mac_finish() only touches
phy_mode inside mac_finish. mac_finish is only installed for SGMII and
2500BASEX, so an RGMII phy_mode would still reach this call at the end
of the series.

I checked the in-tree DTs. The boards that reference a serdes PHY
(lemans.dtsi and monaco.dtsi based) all use sgmii or 2500base-x. The
RGMII boards have no serdes PHY. So "harmless for existing users" holds
for in-tree users, and this only affects DTs the binding allows but that
aren't in the tree.

Should the call be guarded the same way as in ethqos_mac_finish_serdes()
(SGMII, 1000BASEX and 2500BASEX only)? Another option would be to
install serdes_powerup only for SerDes phy-modes in qcom_ethqos_probe(),
or to restrict the binding.

> +
>  	ret = phy_power_on(ethqos->serdes_phy);
>  	if (ret)
> -		phy_exit(ethqos->serdes_phy);
> +		goto err_out;
>  
> +	return 0;
> +
> +err_out:
> +	phy_exit(ethqos->serdes_phy);
>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-qcom-sa8255p-emac-v15-0-e82f33720737%40oss.qualcomm.com