Re: [PATCH net-next v3 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] Hardware clock gated while MDIO bus is still active, leading to SError panic
- [High] TOCTOU race in xpcs_rk_create() allows use-after-free of MDIO bus
- [High] Missing PM dependency causes SError crash when CONFIG_PM is disabled
- [High] Unbalanced pm_runtime_disable() depth permanently breaks device on rebind
- [Low] Build failure due to split composite module when CONFIG_PCS_XPCS=m
--

commit 813e502fc8133a43782c6cdc92f677a818bf8605
Author: Coia Prant <coiaprant@gmail.com>

net: pcs: xpcs: add Rockchip RK3568 platform glue driver
    
The RK3568 SoC integrates a Synopsys DesignWare XPCS accessed via APB3
memory-mapped registers. This driver provides the glue logic to make the
XPCS accessible to the generic pcs-xpcs core.
    
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf

> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -12,6 +12,29 @@ config PCS_XPCS
[ ... ]
> +config PCS_XPCS_ROCKCHIP
> +	tristate "Rockchip XPCS controller support"
> +	default ARCH_ROCKCHIP
> +	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)

[Severity: High]
Is this missing a PM dependency?

If the kernel is built with CONFIG_PM disabled, pm_runtime_resume_and_get()
returns 0 without enabling the hardware clock. This could lead to a
synchronous external abort (SError) when the unclocked APB registers are
subsequently accessed in xpcs_rk_read_reg().

> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,10 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Makefile for Linux PCS drivers
>  
> -pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-plat.o \
> -				   pcs-xpcs-nxp.o pcs-xpcs-wx.o
> -
> -obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
> +obj-$(CONFIG_PCS_XPCS) += pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o

[Severity: Low]
Does this syntax create separate modules instead of a composite module?

Because pcs-xpcs.c references functions like nxp_sja1105_sgmii_pma_config
defined in pcs-xpcs-nxp.c without EXPORT_SYMBOL being present, creating
separate modules will break symbol resolution and cause modpost linkage
failures when CONFIG_PCS_XPCS=m.

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
[ ... ]
> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
[ ... ]
> +	pm_runtime_set_suspended(dev);
> +	ret = devm_pm_runtime_enable(dev);

[Severity: High]
This devm action schedules pm_runtime_disable() to run during devres
cleanup on device removal.

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

[Severity: High]
Will calling pm_runtime_force_suspend() here cause an unbalanced disable
depth?

Since pm_runtime_force_suspend() internally calls pm_runtime_disable(), the
disable depth is incremented twice during removal (once here, and once
during the devres cleanup). When the driver is rebound, it will only be
decremented once, leaving the depth at 1 and permanently breaking runtime
PM hardware access.

[Severity: High]
Can a concurrent sysfs MDIO access race with xpcs_rk_remove() and crash the
system?

Because the MDIO bus is registered with devm_mdiobus_register(), it remains
active and accessible until devres cleanup runs after xpcs_rk_remove()
returns. Calling pm_runtime_force_suspend() gates the hardware clock while
the MDIO bus is still exposed to userspace. A concurrent sysfs read could
increment the usage counter, call into xpcs_rk_read_reg(), and execute
readl() on unclocked hardware, triggering an SError panic.

[ ... ]
> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
[ ... ]
> +	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
> +	device_unlock(&pdev->dev);
> +	if (IS_ERR(xpcs)) {
> +		put_device(&pdev->dev);
> +		return xpcs;
> +	}
> +
> +	if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {

[Severity: High]
Is there a race window between device_unlock() and device_link_add()?

If a concurrent unbind of the supplier proceeds after the lock is dropped
but before device_link_add() creates the dependency, the devm-managed
pxpcs->bus could be torn down and freed. This would allow the MAC driver
to continue probing with a freed MDIO bus, leading to a use-after-free.

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