[PATCH net-next v3 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[Coia Prant]

<sashiko-bot at kernel.org> 于2026年9月2日周三 23:04写道：
> > 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().

ARCH_ROCKCHIP already selects CONFIG_PM.

> > 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.

Sure, I will revert it in next version.

> > 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.

Yes, I will fix it in next version.

> [ ... ]
> > +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.

Yes, I will fix it in next version.

> [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.

Yes, I will fix it in next version.

> [ ... ]
> > +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.

of_find_device_by_node() increments the reference count of pdev->dev,
so the supplier cannot be unbound between the lock release and
device_link_add(). The put_device() is only called after device_link_add()
completes.