Re: [PATCH net-next v10 6/6] net: stmmac: qcom-ethqos: add support for sa8255p

[Simon Horman <horms@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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: qcom-ethqos: add support for sa8255p

Extend the driver to support a new model - sa8255p. Unlike the
previously supported variants, this one's power management is done in
the firmware using SCMI. This is modeled in linux using power domains so
add support for them.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index f32ff0d9ce51..09ce80b446cb 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> @@ -710,6 +785,45 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  	return ret;
>  }
>
> +static int ethqos_mac_finish_serdes_pd(struct net_device *ndev, void *priv,
> +				       unsigned int mode,
> +				       phy_interface_t interface)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +	struct device *dev = ethqos->pd.pd_list->pd_devs[ETHQOS_PD_SERDES];
> +	int ret = 0;
> +
> +	qcom_ethqos_set_sgmii_loopback(ethqos, false);
> +
> +	if (interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX)
> +		ret = dev_pm_opp_set_level(dev, ethqos->pd.serdes_level);
> +
> +	return ret;
> +}
> +
> +static int qcom_ethqos_pd_serdes_powerup(struct net_device *ndev, void *priv)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +	struct device *dev = ethqos->pd.pd_list->pd_devs[ETHQOS_PD_SERDES];
> +	int ret;
> +
> +	ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
> +	if (ret < 0)
> +		return ret;
> +
> +	return dev_pm_opp_set_level(dev, ethqos->pd.serdes_level);
> +}

Does this leak the PM runtime reference if dev_pm_opp_set_level() fails?

qcom_ethqos_pd_serdes_powerup() calls qcom_ethqos_domain_on() which
acquires a PM runtime reference via pm_runtime_resume_and_get():

qcom_ethqos_pd_serdes_powerup()
  qcom_ethqos_domain_on()
    pm_runtime_resume_and_get(dev)

If the subsequent dev_pm_opp_set_level() fails, the function returns the
error without calling qcom_ethqos_domain_off() to release the reference.

The caller stmmac_legacy_serdes_power_up() in stmmac_main.c does not set
legacy_serdes_is_powered=true on error, so stmmac_legacy_serdes_power_down()
in the cleanup path skips the powerdown call.

In the existing non-PD code path, qcom_ethqos_serdes_powerup() correctly
handles this case by calling phy_exit() when phy_power_on() fails:

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:qcom_ethqos_serdes_powerup() {
	ret = phy_init(ethqos->pm.serdes_phy);
	if (ret)
		return ret;

	ret = phy_power_on(ethqos->pm.serdes_phy);
	if (ret) {
		phy_exit(ethqos->pm.serdes_phy);
		return ret;
	}
	...
}

Should qcom_ethqos_pd_serdes_powerup() call qcom_ethqos_domain_off()
before returning the error from dev_pm_opp_set_level()?

> +
> +static void qcom_ethqos_pd_serdes_powerdown(struct net_device *ndev, void *priv)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +	struct device *dev = ethqos->pd.pd_list->pd_devs[ETHQOS_PD_SERDES];
> +
> +	dev_pm_opp_set_level(dev, 0);
> +	qcom_ethqos_domain_off(ethqos, ETHQOS_PD_SERDES);
> +}

[ ... ]