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

[Coia Prant]

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

No. This change is intentional. pcs-xpcs-rk is a platform-specific
driver and should be built as a separate module, not linked into
the composite pcs_xpcs module. There are no undefined references.

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

No. Read remapping is performed by hardware. The VEND2 registers
reflect the settings of MII0; the corresponding bits for MII1-3 are
read-only. This is a hardware feature, not a bug.


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

You are correct. I followed the same pattern as pcs-xpcs-plat.c,
but I agree this could be a problem. I will add a .remove callback
in v2 that calls pm_runtime_force_suspend() before devres cleanup
runs, ensuring the clock enable count is balanced before clk_put().

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

This would only happen with a misconfigured device tree. However,
for robustness, I will add a compatible string check in v2 before
dereferencing drvdata.

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

No. of_find_device_by_node() increments the reference count of the
device. The supplier cannot be unbound between of_find_device_by_node()
and put_device() because the reference count is held.

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

You are correct. I will use pm_ptr() to wrap the PM ops in v2.

Thanks,
Coia