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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 3 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In drivers/net/pcs/pcs-xpcs-rk.c, xpcs_rk_system_suspend()
  relies solely on device_set_wakeup_path() to keep the PD_PIPE power…
- [Medium] xpcs_rk_mdio_write_remapping() (drivers/net/pcs/pcs-xpcs-
  rk.c:137-150) forces writes of DW_VR_MII_AN_CTRL,…
- [Medium] xpcs_rk_create() (drivers/net/pcs/pcs-xpcs-rk.c:537-546)
  dereferences the struct device_link * returned by device_link_add()…
- [Medium] Concern: xpcs_rk_create() validates the supplier with an
  unlocked read of the driver-core-internal field…
- [Low] Concern: xpcs_rk_create() is EXPORT_SYMBOL_GPL'ed and published
  in a new header include/linux/pcs/pcs-xpcs-rk.h with no in-tree caller…
- [Low] In xpcs_rk_create() (drivers/net/pcs/pcs-xpcs-rk.c:518-558) the
  comment justifying the device_link handling misstates the API…

--

net: pcs: xpcs: add Rockchip RK3568 platform glue driver

This adds a Rockchip RK3568 glue driver for the DesignWare XPCS that
exposes the APB3-mapped CSRs through a virtual MDIO bus, splits the
generic platform glue into its own PCS_XPCS_PLATFORM module, and adds
power-management handling for the shared PD_PIPE power domain.

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/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..35ee980a759e5
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
> @@ -0,0 +1,619 @@

[ ... ]

> +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;
> +	}
> +
> +	/*
> +	 * Reads are redirected by hardware to the port's read-only mirror;
> +	 * only writes have to be targeted at MII (see the write path).
> +	 */
> +	switch (addr) {
> +	case 0:
> +		return ROCKCHIP_MMD_MII;
> +	case 1:
> +		return ROCKCHIP_MMD_MII1;
> +	case 2:
> +		return ROCKCHIP_MMD_MII2;
> +	case 3:
> +		return ROCKCHIP_MMD_MII3;
> +	default:
> +		return -ENODEV;
> +	}
> +}
> +
> +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;
> +	}
> +
> +	/*
> +	 * These registers physically live only in MII (the management port).
> +	 * Ports 1-3 expose read-only mirrors of these bits, so writes must
> +	 * always target MII; the read path remaps per address and the
> +	 * hardware redirects to the port's mirror.
> +	 */
> +	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: Medium]
Reads and writes of the same (addr, reg) pair resolve to different MMDs for
addresses 1..3.  xpcs_rk_mdio_write_remapping() sends AN_CTRL, AN_INTR_STS,
EEE_MCTRL0/1 and DIG_CTRL2 to ROCKCHIP_MMD_MII before the per-address switch
is reached, while xpcs_rk_mdio_read_remapping() returns ROCKCHIP_MMD_MII1/2/3
for those same registers.  Since xpcs_rk_addr_format() folds the MMD into the
APB CSR index, the two accesses land on different physical offsets.

The unmodified core does read-modify-write on exactly these registers, e.g.
in xpcs_config_aneg_c37_sgmii():

	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, mask, val);
	...
	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);

and in xpcs_get_state_c37_sgmii():

	an_intr = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
	if (an_intr & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
		an_intr &= ~DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, an_intr);

For port 1..3, is the value read from the port mirror then written back into
port 0's register, and is the autoneg-complete latch that was observed a
different one from the latch that gets acknowledged?

Two related questions on the same mapping:

AN_CTRL (PCS mode / TX_CONFIG) and the EEE registers become one shared
instance for all four ports with no serialization, yet the commit message
says ports 0..3 can be routed to GMAC0 or GMAC1 independently.  How is
cross-port clobbering avoided when two MACs configure their PCS?

DW_VR_MII_DIG_CTRL1, which holds VR_RST plus 2G5_EN/MAC_AUTO_SW and is
modified by the core in xpcs_config_aneg_c37_sgmii() and
xpcs_config_2500basex(), is not in the list and therefore goes to the
per-port MMD.  Does that contradict the comment's claim that such vendor
registers "physically live only in MII"?

[ ... ]

> +	/*
> +	 * Establish the device link before reading the supplier's drvdata.
> +	 * device_link_add() does not fail on a supplier that is unbinding:
> +	 * it creates the link in DL_STATE_SUPPLIER_UNBIND. Whether the link
> +	 * actually protects the drvdata depends on the supplier's state at
> +	 * creation time.
> +	 *
> +	 * Check link->supplier->links.status right after creation. If the
> +	 * supplier was DL_DEV_DRIVER_BOUND, the link is in
> +	 * DL_STATE_CONSUMER_PROBE and device_links_unbind_consumers() will
> +	 * wait for this probe to finish before unbinding the supplier, so
> +	 * the drvdata stays valid for the rest of the function. Any other
> +	 * state means the supplier is not usable yet; defer and retry.
> +	 *
> +	 * The link is released automatically when the consumer device is
> +	 * destroyed (DL_FLAG_AUTOREMOVE_CONSUMER), so no explicit
> +	 * device_link_remove() is needed on the failure paths.
> +	 */

[Severity: Low]
Two details in this comment in xpcs_rk_create() look inaccurate.

include/linux/device.h describes the flag as:

	/* Remove the link automatically on consumer driver unbind. */

so is "released automatically when the consumer device is destroyed" the
right wording?  The core drops such links from __device_links_no_driver() on
consumer probe failure or driver unbind, not at device destruction.

The claim that a DL_DEV_DRIVER_BOUND supplier implies DL_STATE_CONSUMER_PROBE
only holds while the consumer is DL_DEV_PROBING:

drivers/base/core.c:device_link_init_status() {
	case DL_DEV_DRIVER_BOUND:
		switch (consumer->links.status) {
		case DL_DEV_PROBING:
			link->status = DL_STATE_CONSUMER_PROBE;
	...
}

An already-bound consumer gets DL_STATE_ACTIVE and anything else gets
DL_STATE_AVAILABLE.  Could the comment (or the kernel-doc of the exported
helper) state that xpcs_rk_create() must be called from the consumer's probe?

> +	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +	if (!link) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	if (READ_ONCE(link->supplier->links.status) != DL_DEV_DRIVER_BOUND) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}

[Severity: Medium]
Is it valid to dereference the pointer returned by device_link_add() here?
DL_FLAG_STATELESS is not passed, so this is a managed link, and the
kernel-doc above device_link_add() says:

 * If that flag is not set, however, the caller of this function is handing the
 * management of the link over to the driver core entirely and its return value
 * can only be used to check whether or not the link is present.

No kref is taken for managed links (kref_get() only happens on the stateless
path), so the caller owns no reference on the link object, which the core can
free from device_link_drop_managed() -> kref_put(&link->kref,
__device_link_del) or from device_del() -> device_links_purge().

Since link->supplier is just &pdev->dev, and this function already holds a
reference on pdev from of_find_device_by_node(), would reading
pdev->dev.links.status instead give the same result without touching the
link object?

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

[Severity: Medium]
This is a check on a driver-core-internal field (links.status) followed by
use of the supplier's devm-owned data (pxpcs->bus, pxpcs->eee_mult_fact) and
registration of an MDIO device on that bus.

For the in-tree caller the window does look closed: rk_pcs_init() runs from
stmmac_pcs_setup() in __stmmac_dvr_probe(), so the consumer is DL_DEV_PROBING
and the new link is DL_STATE_CONSUMER_PROBE, which makes the supplier wait:

drivers/base/core.c:device_links_unbind_consumers() {
	if (status == DL_STATE_CONSUMER_PROBE) {
		device_links_write_unlock();

		wait_for_device_probe();
		goto start;
	}
}

But xpcs_rk_create() is exported and does not document that precondition.
For a caller outside the consumer's probe the link would be
DL_STATE_AVAILABLE or DL_STATE_ACTIVE, nothing would wait for it, and the
supplier's devres could be released while pxpcs->bus is in use.  Also note
that xpcs_rk_create_data() publishes drvdata before bus and eee_mult_fact
exist:

	pxpcs->pdev = pdev;

	dev_set_drvdata(&pdev->dev, pxpcs);

so the read here relies entirely on the supplier being fully bound.  Would
it be preferable to document the probe-context requirement, or to avoid
inspecting links.status from a driver altogether?

> +EXPORT_SYMBOL_GPL(xpcs_rk_create);

[Severity: Low]
At this commit xpcs_rk_create() has no in-tree caller, and the new header
include/linux/pcs/pcs-xpcs-rk.h declares it without a
!CONFIG_PCS_XPCS_ROCKCHIP stub.  The caller does arrive later in the same
series (rk_pcs_init() in drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
calls it and is installed as plat_dat->pcs_init for
PHY_INTERFACE_MODE_SGMII), and DWMAC_ROCKCHIP does "select
PCS_XPCS_ROCKCHIP", which forces the tristate to at least the selector's
value, so there is no y/m link mismatch either.  Noting it only because the
symbol is unused at this point in the series.

[ ... ]

> +static int xpcs_rk_system_suspend(struct device *dev)
> +{
> +	/*
> +	 * Keep the PD_PIPE power domain on during system suspend.
> +	 *
> +	 * PD_PIPE is shared with SATA/PCIe and would be powered down by
> +	 * genpd once all its consumers are suspended, killing the SerDes
> +	 * and breaking MAC WoL.  Mark the XPCS as part of the wakeup path
> +	 * so genpd keeps the domain on.  Unconditional because the XPCS
> +	 * core has no callback to convey the MAC WoL state.
> +	 */
> +	device_set_wakeup_path(dev);
> +	return 0;
> +}
> +
> +static int xpcs_rk_system_resume(struct device *dev)
> +{
> +	return 0;
> +}

[Severity: High]
Does device_set_wakeup_path() actually keep PD_PIPE powered on RK3568?  The
commit message states:

	genpd then leaves the domain powered, because the Rockchip power
	domain driver sets GENPD_FLAG_ACTIVE_WAKEUP on PD_PIPE, which makes
	genpd check the wakeup path of its consumers during system suspend.

PD_PIPE is registered without active wakeup in
drivers/pmdomain/rockchip/pm-domains.c:

	[RK3568_PD_PIPE]	= DOMAIN_RK3568("pipe", BIT(8), BIT(11), false, false),

The last argument is the wakeup flag, so pd_info->active_wakeup stays false
and GENPD_FLAG_ACTIVE_WAKEUP is never set for this domain.  genpd only
honours the consumer wakeup path when that flag is present:

drivers/pmdomain/core.c:genpd_finish_suspend() {
	if (device_awake_path(dev) && genpd_is_active_wakeup(genpd) &&
	    !device_out_band_wakeup(dev))
		return 0;
}

Without the flag, does genpd fall through to genpd_sync_power_off() and power
PD_PIPE down, which is exactly the situation the probe comment warns about
("The domain must be powered on before any register access, otherwise the SoC
will trigger a synchronous external abort")?

dev_pm_genpd_rpm_always_on() does not appear to cover the system-suspend path
either; genpd_sync_power_off() only consults GENPD_FLAG_ALWAYS_ON:

drivers/pmdomain/core.c:genpd_sync_power_off() {
	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
		return;
}

If the domain is power-cycled, what restores the hardware on the way back up?
phy_init() and phy_power_on() are called once from xpcs_rk_probe() via
xpcs_rk_serdes_phy_poweron(), xpcs_rk_system_resume() returns 0 without
re-initialising the SerDes or the PCS, and
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c has no dev_pm_ops at all.
Does that leave the combphy and XPCS at power-on-reset defaults (no SerDes
configuration, no SerDes TX clock) after resume while the driver, the xpcs
core and phylink still assume the programmed configuration is live, and MAC
WoL, the stated reason for the mechanism, non-functional?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922200336.2201212-1-coiaprant%40gmail.com