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

[Coia Prant]

<sashiko-bot at kernel.org> 于2026年9月16日周三 22:50写道：
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] net: pcs: UAF in xpcs_rk_create due to lockless drvdata access
> - [Low] kconfig: unmet PM_GENERIC_DOMAINS dependency via select
> - [Low] net: pcs: integer underflow in EEE multiplier calculation
> --
>
> commit 45fa9f088b9d4ae656f2c67e864a462b4d229815
> Author: Coia Prant <coiaprant at gmail.com>
> net: pcs: xpcs: add Rockchip RK3568 platform glue driver
>
> This commit adds a Rockchip RK3568 platform glue driver for the Synopsys
> DesignWare XPCS controller. It creates a virtual MDIO bus to translate MDIO
> operations to APB3 register accesses and derives the EEE multiplier at runtime.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf
> 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
> [ ... ]
> > +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 selecting PM_GENERIC_DOMAINS without inheriting its PM dependency cause
> unmet direct dependency warnings?
>
> The config PM_GENERIC_DOMAINS has a strict dependency on PM. If
> PCS_XPCS_ROCKCHIP is enabled on a platform where PM is disabled, won't this
> force PM_GENERIC_DOMAINS on and trigger Kconfig warnings?

PM is already selected by ARCH_ROCKCHIP, so PM_GENERIC_DOMAINS has its
dependency satisfied on the target platform. On COMPILE_TEST the driver
is not run on hardware, so the unmet dependency does not matter.

> > +     help
> > +       Support for XPCS controller on Rockchip RK356x SoC.
> [ ... ]
> > 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)
> > +{
> [ ... ]
> > +     /*
> > +      * Compute the multiplier for the EEE clock so that
> > +      * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
> > +      *
> > +      * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
> > +      * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
> > +      * The reset value is 0 (200 MHz), but derive the value at runtime to
> > +      * stay correct if the mux is changed by a board.
> > +      *
> > +      * Use a 64-bit intermediate: on 32-bit builds, 100 * 200000000
> > +      * does not fit in unsigned long. Clamp to the 4-bit
> > +      * DW_VR_MII_EEE_MULT_FACT_100NS field.
> > +      */
> > +     rate = clk_get_rate(pxpcs->eee_clk);
> > +     if (!rate)
> > +             return dev_err_probe(dev, -EINVAL, "Invalid EEE clock rate\n");
> > +
> > +     mult = DIV_ROUND_CLOSEST_ULL(100ULL * rate, NSEC_PER_SEC) - 1;
> > +     pxpcs->eee_mult_fact = min_t(u64, mult, 15);
>
> [Severity: Low]
> Does this calculation underflow if the EEE clock rate is very low?
>
> If the board configuration provides a clock rate under 5 MHz, the division
> yields 0. Subtracting 1 causes an unsigned underflow to UINT64_MAX.
>
> Won't the subsequent min_t() clamping logic evaluate UINT64_MAX < 15 as false,
> incorrectly assigning the maximum multiplier of 15 instead of 0?

clk_xpcs_eee is muxed between gpll200 (200 MHz) and cpll125 (125 MHz),
so the rate can never approach the 5 MHz underflow threshold. The
existing !rate check is sufficient.

> > +     return 0;
> > +}
> [ ... ]
> > +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> > +{
> [ ... ]
> > +     if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> > +             put_device(&pdev->dev);
> > +             return ERR_PTR(-ENOMEM);
> > +     }
> > +
> > +     pxpcs = platform_get_drvdata(pdev);
> > +     if (!pxpcs || !pxpcs->bus) {
>
> [Severity: High]
> Can this lockless access to the supplier's drvdata cause a use-after-free?
>
> If the supplier driver is concurrently unbound via sysfs, the supplier might
> have already passed the consumer unbind checks before this device link was
> established.
>
> If the supplier then proceeds to devres cleanup and frees the pxpcs memory,
> won't dereferencing pxpcs->bus access freed memory?
>
> Should the consumer hold device_lock(&pdev->dev) while validating the supplier
> state and reading its drvdata to prevent this race?

The link is established before reading the drvdata, so the window you
describe does not exist: device_link_add() rejects a supplier that is
being unbound and returns NULL, and the caller defers before touching
platform_get_drvdata().

I also removed the explicit device_link_remove() calls on the failure
paths. DL_FLAG_AUTOREMOVE_CONSUMER already releases the link when the
consumer device is destroyed, which covers all probe failure cases, and
manual removal adds complexity for no benefit.