Re: [PATCH net-next v13 03/10] phy: qcom: add the SGMII SerDes PHY driver for SCMI systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Improper GenPD performance state management causes hardware to resume with insufficient voltage.
- [Medium] Explicit PM runtime management in PHY power callbacks is redundant and double-increments usage counters.
- [Low] The local `perf_state` cache is updated before the hardware call, causing state mismatch on failure.
--

--- Patch [3]: [PATCH net-next v13 03/10] phy: qcom: add the SGMII SerDes PHY driver for SCMI systems ---
commit 15d41616bc0d5eacc339e6ee3b574f4d5ac6e400
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

phy: qcom: add the SGMII SerDes PHY driver for SCMI systems
    
Implement support for the firmware-managed SGMII/SerDes PHY present on
Qualcomm platforms. Do this as a separate driver from the HLOS-managed
variant as they don't share almost any code.

> diff --git a/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
> new file mode 100644
> index 0000000000000..aa7ce9393b285
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
> @@ -0,0 +1,165 @@

[ ... ]

> +static int qcom_dwmac_sgmii_phy_scmi_power_on(struct phy *phy)
> +{
> +	struct qcom_dwmac_sgmii_phy_scmi *priv = phy_get_drvdata(phy);
> +	struct device *dev = phy->dev.parent;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);

[Severity: Medium]
Is this explicit pm_runtime_resume_and_get() necessary? 

The PHY core automatically handles runtime PM for the PHY device, and because
devm_phy_create() is used, the PHY core invokes phy_pm_runtime_get_sync()
which fully resumes the parent device before qcom_dwmac_sgmii_phy_scmi_power_on()
executes. Does this explicit call just double-increment the usage counter?

> +	if (ret)
> +		return ret;
> +
> +	ret = dev_pm_genpd_set_performance_state(dev, priv->perf_state);

[Severity: High]
Will this sequence cause the hardware to resume with insufficient voltage?

Because the PHY core already resumed the device prior to this callback, the 
domain is already powered on by GenPD. Since qcom_dwmac_sgmii_phy_scmi_power_off()
manually zeroes the performance state, GenPD will cache a 0 voltage request
and power on the domain with 0 state.

This dev_pm_genpd_set_performance_state() then raises the voltage only after
the hardware has already been resumed. GenPD natively supports dropping the
performance state of runtime-suspended devices. Could we unconditionally set
the performance state in probe() and set_mode() instead of manually toggling
it in the power callbacks?

> +	if (ret) {
> +		pm_runtime_put(dev);
> +		return ret;
> +	}
> +
> +	usleep_range(5000, 10000);
> +
> +	return 0;
> +}
> +
> +static int qcom_dwmac_sgmii_phy_scmi_power_off(struct phy *phy)
> +{
> +	struct device *dev = phy->dev.parent;
> +
> +	dev_pm_genpd_set_performance_state(dev, 0);

[Severity: High]
Does manually zeroing the performance state here interfere with GenPD's
built-in state caching, forcing the next resume to happen at 0 voltage?

> +	pm_runtime_put(dev);

[Severity: Medium]
Is this pm_runtime_put() redundant since the PHY core will drop the runtime PM
reference after this callback completes?

> +
> +	return 0;
> +}

[ ... ]

> +static int qcom_dwmac_sgmii_phy_scmi_set_mode(struct phy *phy, enum phy_mode mode,
> +					      int submode)
> +{
> +	struct qcom_dwmac_sgmii_phy_scmi *priv = phy_get_drvdata(phy);
> +	struct device *dev = phy->dev.parent;
> +	int ret;
> +
> +	ret = qcom_dwmac_sgmii_phy_scmi_validate(phy, mode, submode, NULL);
> +	if (ret)
> +		return ret;
> +
> +	priv->perf_state = (submode == PHY_INTERFACE_MODE_2500BASEX) ?
> +			   SPEED_2500 : SPEED_1000;
> +
> +	if (phy->power_count == 0)
> +		return 0;

[Severity: High]
Is it necessary to explicitly skip the GenPD call when the device is suspended?

GenPD automatically handles caching performance states for suspended devices.
Bypassing the framework here prevents GenPD from knowing the new state before
the next resume.

> +
> +	return dev_pm_genpd_set_performance_state(dev, priv->perf_state);

[Severity: Low]
If dev_pm_genpd_set_performance_state() fails and returns an error,
priv->perf_state has already been updated.

Will this cause a state mismatch where the software caches the new mode but
the hardware remains in the old mode, causing a subsequent power_on() to
apply the wrong state?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-sa8255p-emac-v13-0-119f8699ef8e@oss.qualcomm.com?part=3