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

[Coia Prant]

<sashiko-bot at kernel.org> 于2026年9月19日周六 04:48写道：
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] net: pcs: use-after-free in xpcs_rk_create() during concurrent supplier unbind
> - [Low] kconfig: unmet dependency warning due to PM_GENERIC_DOMAINS select without PM
> --
>
> commit 7e2d67535ce6f3173f2984680b0a77b61caaf964
> Author: Coia Prant <coiaprant at gmail.com>
>
> net: pcs: xpcs: add Rockchip RK3568 platform glue driver
>
> This commit adds a platform glue driver for the Synopsys DesignWare XPCS
> on Rockchip RK3568 SoCs. It implements a virtual MDIO bus to translate
> MDIO operations into APB3 memory-mapped register accesses.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 59, CRU_CLKSEL_CON29)
> 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/Kconfig b/drivers/net/pcs/Kconfig
> > --- a/drivers/net/pcs/Kconfig
> > +++ b/drivers/net/pcs/Kconfig
> [ ... ]
> > +config PCS_XPCS_ROCKCHIP
> > +     tristate "Rockchip XPCS controller support"
> > +     default ARCH_ROCKCHIP
> > +     depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> > +     select GENERIC_PHY
> > +     select PM_GENERIC_DOMAINS
>
> [Severity: Low]
> Will this cause a Kconfig unmet direct dependency warning during compilation
> when PM is not enabled?
>
> Because PCS_XPCS_ROCKCHIP has a COMPILE_TEST fallback in its depends on
> list, it can be enabled on architectures without CONFIG_PM. Forcefully
> selecting PM_GENERIC_DOMAINS (which depends on PM) without inheriting
> that dependency could lead to a configuration mismatch.
>
> Could this Kconfig entry add a dependency on PM, or only conditionally
> select PM_GENERIC_DOMAINS?

Thanks for the review. You're right, I was only considering the real
Rockchip build. COMPILE_TEST allows the symbol to be enabled on any
architecture, including ones with CONFIG_PM=n, and in that case
select PM_GENERIC_DOMAINS would force it on against its own depends on
PM.

I'll change the select to be conditional:

    select PM_GENERIC_DOMAINS if PM

With PM=n, PM_GENERIC_DOMAINS stays off and there is no unmet
dependency. The driver still compiles because dev_pm_genpd_rpm_always_on()
is a stub returning -EOPNOTSUPP in that configuration, and COMPILE_TEST
does not run on hardware anyway.

> > diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> [ ... ]
> > +     /*
> > +      * Pin the supplier before reading its drvdata: device_link_add()
> > +      * refuses to create a managed link while the supplier is being
> > +      * unbound, so if it succeeds the drvdata cannot be freed under us.
> > +      * The link is released automatically when the consumer device is
> > +      * destroyed (DL_FLAG_AUTOREMOVE_CONSUMER), which covers all probe
> > +      * failure paths, so no explicit device_link_remove() is needed.
> > +      */
> > +     if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> > +             put_device(&pdev->dev);
> > +             return ERR_PTR(-EPROBE_DEFER);
> > +     }
> > +
> > +     pxpcs = platform_get_drvdata(pdev);
> > +     if (!pxpcs || !pxpcs->bus) {
>
> [Severity: High]
> Is this sequence in xpcs_rk_create() safe against a concurrent supplier
> unbind?
>
> The comment assumes device_link_add() returns NULL if the supplier is
> unbinding, but it actually succeeds and creates a link with a state like
> DL_STATE_SUPPLIER_UNBIND or DL_STATE_DORMANT.
>
> If userspace concurrently unbinds the XPCS supplier device while this
> consumer MAC driver is probing, reading the drvdata via
> platform_get_drvdata(pdev) might yield a pointer to devres memory that
> has already been freed by the supplier's remove path.
>
> Should this code acquire the device lock on the supplier, or otherwise
> verify the device is fully bound before dereferencing its drvdata?

Thanks for the review. This is a valid concern and I've fixed it in v8.

You're right that device_link_add() does not fail when the supplier is
being unbound. Looking at device_link_init_status(), a supplier in
DL_DEV_UNBINDING state produces a link with DL_STATE_SUPPLIER_UNBIND,
and the function returns non-NULL. The check at the top of
device_link_add() is device_pm_initialized(), not device_is_bound(), so
a supplier that is mid-unbind passes it.

The drvdata is now read under the supplier's device_lock, with a
device_is_bound() check:

        device_lock(&pdev->dev);
        if (!device_is_bound(&pdev->dev)) {
                device_unlock(&pdev->dev);
                put_device(&pdev->dev);
                return ERR_PTR(-EPROBE_DEFER);
        }
        pxpcs = platform_get_drvdata(pdev);
        device_unlock(&pdev->dev);

device_lock() serializes with __device_release_driver(), which also
takes the supplier's device lock while running its remove path, and
device_is_bound() confirms the driver is still attached before we read
its data. If the supplier is unbinding, we return -EPROBE_DEFER and
retry.

The device link is still established first so that the PM ordering and
the automatic cleanup on consumer removal are in place, but it is the
lock plus the bound check that keeps the drvdata valid, not the link
itself. I've updated the comment to say that.

Thanks,
Coia