Re: [PATCH net-next v10 5/6] net: stmmac: dwmac-rk: add SGMII support for RK3568

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

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

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] SGMII cannot work at all, and the XPCS leaks on every probe.
- [Medium] With phy-mode="sgmii", rk_gmac_probe() clears
  plat_dat->set_clk_tx_rate and sets default_an_inband=true, but that…
- [Medium] The commit message's account of in-band vs out-of-band SGMII
  is wrong in three ways.
- [Medium] xpcs_rk_create(), which the new rk_pcs_init() calls, assumes
  that 'supplier->links.status == DL_DEV_DRIVER_BOUND' right after…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  rk_gmac_exit() assumes driver data already exists.
- [Medium] This problem wasn't introduced by this patch, but
  rk_gmac_resume() ignores the return value of rk_gmac_powerup() and…

--

net: stmmac: dwmac-rk: add SGMII support for RK3568

This patch adds RK3568 SGMII support to dwmac-rk. It adds a set_to_sgmii()
GRF callback, plus pcs_init/pcs_exit/select_pcs callbacks that create the
XPCS with xpcs_rk_create(). For SGMII it also enables clocks earlier in
rk_gmac_powerup(), sets default_an_inband and clears set_clk_tx_rate.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)

On RK3568, the MAC clock is fixed at 125 MHz and cannot be dynamically
changed by the stmmac core's set_clk_tx_rate callback.  In-band mode
works because the PCS handles rate adaptation internally.  Out-of-band
mode does not work because the MAC would need to change the clock rate
to 125/12.5/1.25 MHz for 1000/100/10 Mbps respectively, and the clock
is fixed.

[Severity: Medium]
Are these rates correct? The clk_tx_i kernel-doc in include/linux/stmmac.h
says "SGMII 125MHz SerDes clock divided by 1, 5, or 50". Also,
rgmii_clock(), which rk_set_clk_mac_speed() uses, returns
125/25/2.5 MHz.

The reason given for in-band working and out-of-band failing also doesn't
seem to match the code:

- rk_gmac_probe() sets set_clk_tx_rate to NULL for every SGMII
  configuration, so the MAC clock is handled the same way in both modes.
- The XPCS is told the speed in both modes: through MAC_AUTO_SW in
  xpcs_config_aneg_c37_sgmii() for in-band, and through a BMCR write in
  xpcs_link_up_sgmii_1000basex() for out-of-band.

Enable default_an_inband for SGMII and disable the generic stmmac
set_clk_tx_rate callback.

The callback that gets cleared is rk_set_clk_tx_rate(). That is the
Rockchip-specific one, which ends up in rk_set_clk_mac_speed() on rk3568,
not a generic stmmac callback.

Could the commit message be updated on these points?

[ ... ]

[Severity: Medium]
Can xpcs_rk_create() race with the XPCS driver binding? It does this:

drivers/net/pcs/pcs-xpcs-rk.c:xpcs_rk_create() {
    ...
	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
    ...
	if (READ_ONCE(link->supplier->links.status) != DL_DEV_DRIVER_BOUND) {
    ...
}

fw_devlink does not parse "pcs-handle", so nothing orders the supplier
ahead of this probe. Suppose the XPCS device has no driver when
device_link_add() runs:

- The link starts in DL_STATE_DORMANT.
- If the XPCS driver binds before the READ_ONCE() check,
  device_links_driver_bound() moves the link to DL_STATE_AVAILABLE, not
  DL_STATE_CONSUMER_PROBE.
- The DL_DEV_DRIVER_BOUND check still passes.

device_links_unbind_consumers() only waits for links in
DL_STATE_CONSUMER_PROBE. Could a concurrent unbind of the XPCS driver then
free the devm-managed pxpcs and mii_bus while xpcs_create_mdiodev() is
still using them?

Also, if the stmmac probe then succeeds, wouldn't
device_links_driver_bound() for the consumer hit
WARN_ON(link->status != DL_STATE_CONSUMER_PROBE)?

[Severity: High]
Does this XPCS pointer survive after the callback returns? On the pcs_init
branch, stmmac_pcs_setup() never sets its local xpcs, and then writes it
back unconditionally:

drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c:stmmac_pcs_setup() {
	struct dw_xpcs *xpcs = NULL;
	...
	if (priv->plat->pcs_init) {
		ret = priv->plat->pcs_init(priv);
	} else if (fwnode_property_present(devnode, "pcs-handle")) {
	...
	priv->hw->xpcs = xpcs;

	return 0;
}

So once rk_pcs_init() returns, priv->hw->xpcs is NULL again.

In stmmac_phylink_setup(), priv->hw->xpcs and priv->hw->phylink_pcs are
then both NULL, so the XPCS supported_interfaces are never merged.
rk_get_interfaces() has already set the RGMII and RMII bits for rk3568,
so PHY_INTERFACE_MODE_SGMII never ends up in supported_interfaces.

With default_an_inband set, phylink_parse_mode() takes the in-band path
and phylink_validate() fails. Doesn't that make phylink_create(), and so
the whole probe, fail with -EINVAL on every phy-mode = "sgmii" board?

Even if probe got past that, rk_select_pcs() would return NULL. Also,
rk_pcs_exit() and stmmac_pcs_clean() both return early when
priv->hw->xpcs is NULL, so xpcs_destroy() never runs. Is the dw_xpcs,
together with its mdio_device reference and clocks, leaked on every probe
attempt?

The other pcs_init users (socfpga, rzn1, renesas) avoid this by storing
their PCS in priv->hw->phylink_pcs.

Because of this, the commit message statement that SGMII "comes from the
XPCS's own supported_interfaces, merged by stmmac_phylink_setup()" does
not currently hold.

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but rk_gmac_resume() ignores
the error returned here:

drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_gmac_resume() {
	...
	if (!device_may_wakeup(dev))
		rk_gmac_powerup(bsp_priv);

	return 0;
}

If the clock enable, a GRF write or the phy regulator fails during
resume, rk_gmac_powerup() returns an error and the clocks are disabled.

Won't stmmac_resume() then go on to reinitialise the MAC with its clocks
off?

[ ... ]

[Severity: Medium]
default_an_inband only picks the default. Does anything stop phylink from
running this link out-of-band?

- A fixed-link node gives MLO_AN_FIXED.
- If the PHY reports only LINK_INBAND_DISABLE: xpcs_inband_caps() returns
  LINK_INBAND_DISABLE | LINK_INBAND_ENABLE for SGMII, so
  phylink_pcs_neg_mode() selects PHYLINK_PCS_NEG_OUTBAND.

rk_select_pcs() returns the XPCS whatever the neg_mode is. With
set_clk_tx_rate cleared, the MAC clock stays at 125 MHz.

According to the commit message, 10/100 Mbps would then have working TX
and failing RX. The link would still be reported up, and nothing would be
logged.

Could the driver reject this or warn about it, for example by checking the
neg_mode or limiting the MAC capabilities? Right now it is only documented
in the commit message.

[Severity: Medium]
This is a pre-existing issue, but rk_gmac_exit(), which is installed as
plat_dat->exit in this function, assumes the driver data has already been
set:

drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_gmac_exit() {
	struct stmmac_priv *priv = netdev_priv(dev_get_drvdata(dev));
	...
	if (priv->plat->phy_node && bsp_priv->integrated_phy)
	...
}

stmmac_dvr_probe() calls plat->init before __stmmac_dvr_probe(), and
calls plat->exit if anything fails afterwards.

Suppose __stmmac_dvr_probe() fails before dev_set_drvdata(), for example
because devm_alloc_etherdev_mqs() returns NULL. Would rk_gmac_exit() then
dereference a priv derived from a NULL net_device?

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