Re: [PATCH v2 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Probe fails unconditionally on systems without a PM domain or with CONFIG_PM_GENERIC_DOMAINS disabled.
- [High] Premature `pm_runtime_force_suspend()` in `remove()` breaks PHY teardown, causes SErrors, and permanently disables PM runtime on rebind.
- [High] Concurrent unbinding of the XPCS driver during `xpcs_rk_create` leads to a Use-After-Free of `pxpcs` and its MDIO bus.
- [High] Unchecked return value of `device_link_add()` allows Use-After-Free on supplier unbind.
- [Low] Unconditional assignment of `dev_pm_ops` and missing `pm_ptr()` wrapper causes unused function compiler warnings.
--

--- Patch [7]: [PATCH v2 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver ---
commit 749227defaf729e0f171dc87722abef8ebe69759
Author: Coia Prant <coiaprant@gmail.com>
net: pcs: xpcs: add Rockchip RK3568 platform glue driver

This commit introduces a platform glue driver for the Synopsys DesignWare XPCS
controller found on Rockchip RK3568 SoCs. It translates MDIO operations to
APB3 register accesses via a virtual MDIO bus. The generic xpcs driver then
creates a phylink_pcs instance on top of this bus.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)

diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
index 0000000000000..36c1557b83367
--- /dev/null
+++ b/drivers/net/pcs/pcs-xpcs-rk.c
@@ -0,0 +1,538 @@
[ ... ]
> +static int xpcs_rk_serdes_phy_poweron(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +	int ret;
> +
> +	ret = dev_pm_genpd_rpm_always_on(dev, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to power on power-domains\n");
> +		return ret;
> +	}

[Severity: High]
Does this code unintentionally fail the probe on systems without a PM domain
or when PM generic domains are disabled?

When CONFIG_PM_GENERIC_DOMAINS is disabled, dev_pm_genpd_rpm_always_on()
returns -EOPNOTSUPP. If the device has no power domain (which appears to be
optional), it returns -ENODEV.

Because the probe is aborted on any non-zero return value, the driver will
fail to load on these hardware configurations or in COMPILE_TEST builds.
Should -ENODEV and -EOPNOTSUPP be explicitly ignored here?

[ ... ]
> +static void xpcs_rk_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_force_suspend(&pdev->dev);
> +}

[Severity: High]
Can this break PHY teardown and disable PM runtime permanently upon rebind?

Because the MDIO bus is registered using devm_mdiobus_register(), devres
will unregister it after xpcs_rk_remove() returns. However,
pm_runtime_force_suspend() is called here first, cutting the clock and
incrementing the PM runtime disable depth.

When devres subsequently unregisters the MDIO bus, attached PHY devices
are unbound, which triggers MDIO accesses for teardown. These accesses
will call pm_runtime_resume_and_get(), which fails with -EACCES because
PM runtime is disabled. This failure can cause a synchronous external abort
if the clock is cut during access.

Additionally, the double-disable (here and in devm_pm_runtime_enable cleanup)
leaves the disable depth greater than zero on rebind.

[ ... ]
> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
[ ... ]
> +	pdev = of_find_device_by_node(pcs_np);
> +	of_node_put(pcs_np);
> +	if (!pdev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	pxpcs = platform_get_drvdata(pdev);
> +	if (!pxpcs || !pxpcs->bus) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);

[Severity: High]
Is it possible for the XPCS driver to be unbound concurrently while
platform_get_drvdata() is reading the data?

The device structure itself is pinned by of_find_device_by_node(), but
without holding the device lock, a concurrent sysfs unbind could trigger
devres cleanup. This would free pxpcs and unregister the MDIO bus right
before xpcs_create_mdiodev() accesses pxpcs->bus, leading to a
use-after-free.

Should the device lock be held to verify the driver is still bound before
proceeding?

> +	if (IS_ERR(xpcs)) {
> +		put_device(&pdev->dev);
> +		return xpcs;
> +	}
> +
> +	device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

[Severity: High]
Does this ignore the return value of device_link_add() if the link creation
fails?

If device_link_add() fails (for example, due to a memory allocation failure),
no link is established. Later, if the XPCS supplier unbinds, the driver
core will not automatically unbind the MAC consumer. The XPCS devres
cleanup will free the MDIO bus while the MAC driver is still bound and
potentially using it, which can cause a use-after-free.

Should the return value of device_link_add() be checked?

[ ... ]
> +static const struct dev_pm_ops xpcs_rk_pm_ops = {
> +	SET_RUNTIME_PM_OPS(xpcs_rk_pm_runtime_suspend,
> +			   xpcs_rk_pm_runtime_resume,
> +			   NULL)
> +};
> +
> +static struct platform_driver xpcs_rk_driver = {
> +	.probe = xpcs_rk_probe,
> +	.remove = xpcs_rk_remove,
> +	.driver = {
> +		.name = "rk_xpcs-dwxpcs",
> +		.pm = &xpcs_rk_pm_ops,

[Severity: Low]
Will this generate unused function warnings when CONFIG_PM is disabled?

The xpcs_rk_pm_runtime_suspend() and xpcs_rk_pm_runtime_resume() callbacks
are defined as static. When CONFIG_PM is not set, SET_RUNTIME_PM_OPS expands
to nothing, leaving the static functions unreferenced.

Since xpcs_rk_pm_ops is unconditionally assigned to .pm without the
pm_ptr() wrapper, the struct is always referenced, but the callbacks inside
might not be. Should RUNTIME_PM_OPS() and pm_ptr() be used instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801142244.679437-2-coiaprant@gmail.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy