Re: [PATCH net-next v11 2/7] phy: qcom: add the SGMII SerDes PHY driver for SCMI systems

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Asynchronous pm_runtime_put() in PHY power-off callback creates a race condition with device unbind, permanently leaving the hardware active.
- [High] Incorrect genpd performance state management causes voltage drops while active and powers on the device with insufficient voltage.
- [Low] Kconfig symbol PHY_QCOM_SGMII_ETH_SCMI unconditionally selects PM_GENERIC_DOMAINS without inheriting its dependency on PM.
--

commit dd2da8ee656d44ae9a3f555af9c5e8b85b759597
Author: Bartosz Golaszewski <[email protected]>

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/Kconfig b/drivers/phy/qualcomm/Kconfig
> index 60a0ead127fa9..bd7d3fe411d7f 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -232,3 +232,13 @@ config PHY_QCOM_SGMII_ETH
>  	help
>  	  Enable this to support the internal SerDes/SGMII PHY on various
>  	  Qualcomm chipsets.
> +
> +config PHY_QCOM_SGMII_ETH_SCMI
> +	tristate "Qualcomm DWMAC SGMII SerDes/PHY driver (firmware managed)"
> +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> +	select GENERIC_PHY
> +	select PM_GENERIC_DOMAINS

[Severity: Low]
Selecting a Kconfig symbol without satisfying its direct dependencies produces
"unmet direct dependencies detected" warnings.

Since PM_GENERIC_DOMAINS depends on PM, could this lead to silent build failures
on platforms where CONFIG_PM is disabled, for example during COMPILE_TEST?

> +	help
> +	  Enable this to support the internal SerDes/SGMII PHY on Qualcomm
> +	  chipsets where the SerDes hardware (clocks and registers) is owned
> +	  by the firmware.

[ ... ]

> 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..8ee62189556fe
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
> @@ -0,0 +1,161 @@

[ ... ]

> +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);
> +	if (ret)
> +		return ret;
> +
> +	ret = dev_pm_genpd_set_performance_state(dev, priv->perf_state);

[Severity: High]
Does raising the performance state after the device has already been resumed
risk a crash from insufficient voltage?

Since pm_runtime_resume_and_get() brings the device up with the currently
cached genpd state (which is dropped to 0 in
qcom_dwmac_sgmii_phy_scmi_power_off), the device is briefly active with a 0
performance state.

> +	if (ret) {
> +		pm_runtime_put(dev);

[Severity: High]
Does using an asynchronous put here risk leaving the device active?

If a probe or subsequent initialization fails and triggers devres cleanup,
pm_runtime_disable() will cancel any pending asynchronous suspends. Would
pm_runtime_put_sync() ensure the device actually reaches the suspended state?

> +		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]
Can manually dropping the performance state to 0 while the device is still
active (RPM_ACTIVE) cause voltage drops that lead to a bus lockup?

Also, as mentioned above, doing this overwrites the cached performance state
so the next resume initially operates at state 0.

> +	pm_runtime_put(dev);

[Severity: High]
Does this asynchronous pm_runtime_put() create a race condition with device
unbind?

If a consumer unbinds, calling phy_power_off() followed by the PHY provider's
devres cleanup (which calls pm_runtime_disable()), the pending async suspend
could be cancelled, leaving the hardware permanently in the active state.

> +
> +	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);
> +	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;

[Severity: High]
If the MAC driver changes the mode dynamically while the PHY is powered on,
does this only update the local priv->perf_state variable without actually
applying the new state to genpd?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic