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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Asymmetric MDIO read/write remapping for VEND2 control registers corrupts port 0 and breaks configuration for ports 1-3.
- [High] Accessing the supplier device's `drvdata` without synchronization before establishing the device link causes a Use-After-Free race condition.
- [Medium] Clock `csr_clk` leaks if the device is removed while its runtime PM state is active.
- [Medium] `xpcs_rk_create` extracts driver data from an arbitrary device node without validating its compatible string or bound driver, leading to type confusion.
- [Low] The Makefile modifications incorrectly split the composite `pcs_xpcs` module into multiple independent modules, causing `modpost` build failures.
- [Low] The `dev_pm_ops` structure is assigned unconditionally to the driver's `.pm` field, causing dead code when `CONFIG_PM` is disabled.
--

--- Patch [7]: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver ---
commit c0159daf5cb4d2e6a0fbbe7a38525a8e230e6d93
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/Makefile b/drivers/net/pcs/Makefile
> index 4f7920618b900..c809b7f942a51 100644
> --- 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 change cause build failures when CONFIG_PCS_XPCS=m?

Replacing the composite module assignment (pcs_xpcs-y) with individual
object assignments causes Kbuild to build separate modules instead of one
composite module, which can lead to undefined references during modpost.

> +obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
> +obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o

[ ... ]

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..ed969a38d544d
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
[ ... ]
> +static int xpcs_rk_mdio_read_remapping(int addr, int dev, int reg)
> +{
> +	switch (dev) {
> +	case MDIO_MMD_PMAPMD:
> +		return ROCKCHIP_MMD_PMAPMD;
> +	case MDIO_MMD_VEND2:
> +		break;
> +	default:
> +		return -ENXIO;
> +	}
> +
> +	switch (addr) {
> +	case 0:
> +		return ROCKCHIP_MMD_MII;
> +	case 1:
> +		return ROCKCHIP_MMD_MII1;
[ ... ]
> +static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
> +{
> +	switch (dev) {
> +	case MDIO_MMD_PMAPMD:
> +		return ROCKCHIP_MMD_PMAPMD;
> +	case MDIO_MMD_VEND2:
> +		break;
> +	default:
> +		return -ENXIO;
> +	}
> +
> +	/* Writable only on MII */
> +	switch (reg) {
> +	case DW_VR_MII_AN_CTRL:
> +	case DW_VR_MII_AN_INTR_STS:
> +	case DW_VR_MII_EEE_MCTRL0:
> +	case DW_VR_MII_EEE_MCTRL1:
> +	case DW_VR_MII_DIG_CTRL2:
> +		return ROCKCHIP_MMD_MII;
> +	default:
> +		break;
> +	}

[Severity: High]
Can this asymmetric remapping between reads and writes cause configuration
corruption on port 0?

In xpcs_rk_mdio_write_remapping(), writes to these VEND2 registers are
unconditionally mapped to ROCKCHIP_MMD_MII (port 0). However,
xpcs_rk_mdio_read_remapping() lacks an equivalent redirect for reads.

Because the generic XPCS driver uses read-modify-write operations on VEND2
control registers for ports 1-3, this will result in reading from the target
port's MMD but overwriting port 0's configuration.

> +
> +	switch (addr) {
> +	case 0:

[ ... ]

> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
> +	struct device *dev = &pxpcs->pdev->dev;
> +	int ret;
> +
> +	pxpcs->csr_clk = devm_clk_get(dev, "csr");
> +	if (IS_ERR(pxpcs->csr_clk))
> +		return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
> +					 "Failed to get CSR clock\n");
> +
> +	pm_runtime_set_suspended(dev);
> +	ret = devm_pm_runtime_enable(dev);

[Severity: Medium]
Will this leak the csr_clk if the device is removed while active?

The generic devm_pm_runtime_enable() runs pm_runtime_disable() on cleanup,
which cancels pending suspends but does not synchronously suspend an active
device.

Since this driver lacks a .remove callback to manually suspend the device,
xpcs_rk_pm_runtime_suspend() might never be invoked on unbind, leaving the
clock prepare and enable counts unbalanced.

> +	if (ret) {
> +		dev_err(dev, "Failed to enable runtime-PM\n");
> +		return ret;
> +	}

[ ... ]

> +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);

[Severity: Medium]
Could this blind cast lead to type confusion?

The code extracts driver data from the platform device associated with
pcs_np without validating if the device is actually compatible with
rockchip,rk3568-xpcs or bound to this driver.

If a misconfigured device tree points to a different type of device (like
an I2C controller), casting its drvdata to struct dw_xpcs_rk * and
dereferencing it could crash the system.

> +	if (!pxpcs || !pxpcs->bus) {

[Severity: High]
Is there a use-after-free race condition when accessing drvdata here?

This checks and dereferences pxpcs->bus before establishing a device link
to lock the supplier.

If a user concurrently unbinds the XPCS supplier driver (for example, via
sysfs), devres will free the dw_xpcs_rk structure and clear the drvdata.
The consumer driver might then dereference the freed pxpcs->bus before
device_link_add() is called.

> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);

[ ... ]

> +static struct platform_driver xpcs_rk_driver = {
> +	.probe = xpcs_rk_probe,
> +	.driver = {
> +		.name = "rk_xpcs-dwxpcs",
> +		.pm = &xpcs_rk_pm_ops,

[Severity: Low]
Will this assignment cause dead code when CONFIG_PM is disabled?

Assigning the dev_pm_ops structure unconditionally to the .pm field without
the pm_ptr() wrapper results in dead code and possible compiler warnings
when CONFIG_PM is not set, which violates the PM subsystem guidelines.

> +		.of_match_table = xpcs_rk_of_ids,
> +	},
> +};
> +module_platform_driver(xpcs_rk_driver);

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

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