Re: [PATCH net-next v14 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] The driver's devres teardown sequence cancels an asynchronous suspend of the platform device, leaving the device permanently active.
- [High] Missing cleanup of the genpd performance state on driver unbind leaves a persistent power vote on the PM domain.
- [Low] The driver forcefully selects the user-visible PM subsystem from a leaf hardware driver, overriding user configuration.
--

commit dae3dafc17dff2635ed1048ccc9e18ab67fdf8a3
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Subject: phy: qcom: add the SGMII SerDes PHY driver for SCMI systems

This commit adds a new firmware-managed variant of the Qualcomm DWMAC SGMII 
SerDes/PHY driver. It introduces a separate driver from the HLOS-managed 
variant since they share little code.

> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index d910a5d1a1ac3..677662f642e42 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
[ ... ]
> +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
> +	select PM_GENERIC_DOMAINS

[Severity: Low]
Is it intended to forcefully select the user-visible PM subsystem from a leaf 
hardware driver? 

Leaf drivers typically use "depends on" for user-visible features like PM 
to avoid overriding user configurations.

> +	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..ae961975dcd87
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c
[ ... ]
> +static int qcom_dwmac_sgmii_phy_scmi_set_mode(struct phy *phy, enum phy_mode mode,
> +					      int submode)
> +{
> +	struct device *dev = phy->dev.parent;
> +	unsigned int perf_state;
> +	int ret;
> +
> +	ret = qcom_dwmac_sgmii_phy_scmi_validate(phy, mode, submode, NULL);
> +	if (ret)
> +		return ret;
> +
> +	perf_state = (submode == PHY_INTERFACE_MODE_2500BASEX) ?
> +		     SPEED_2500 : SPEED_1000;
> +
> +	return dev_pm_genpd_set_performance_state(dev, perf_state);

[Severity: High]
If the genpd performance state is dynamically set to a value greater than 0
here, where does it get reset to 0? 

Without a teardown path (such as a .remove callback or a dedicated devres 
action) to explicitly reset the performance state, the PM domain hardware 
might remain forced at a high performance state permanently after driver 
unbind, wasting power. The genpd core does not automatically drop dynamically 
set performance states on unbind.

> +}
[ ... ]
> +static void qcom_dwmac_sgmii_phy_scmi_runtime_disable(void *data)
> +{
> +	struct device *dev = data;
> +
> +	pm_runtime_disable(dev);

[Severity: High]
Will this devres teardown sequence cancel the asynchronous suspend of the 
platform device? 

In the devres unwinding sequence, phy_destroy() unregisters the PHY child, 
which causes the PM core to queue an asynchronous pm_request_idle() for the 
parent platform device. 

If qcom_dwmac_sgmii_phy_scmi_runtime_disable() executes pm_runtime_disable()
immediately after, it aborts the pending idle request. This leaves the 
platform device permanently locked in the active state. Could this be avoided 
by ensuring the device suspends synchronously before disabling runtime PM?

> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-qcom-sa8255p-emac-v14-0-ec212b747a7d@oss.qualcomm.com?part=3

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic