Re: [PATCH net-next v6 01/11] net: stmmac: move XPCS lifetime management to platform drivers

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

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

- [Medium] stmmac_pcs_setup()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c:426) loses the only…

Pre-existing issues:
- [Medium] Clock reference leak in the XPCS constructor now invoked from
  intel_mgbe_pcs_init(): xpcs_init_clks() (drivers/net/pcs/pcs-…

--

net: stmmac: move XPCS lifetime management to platform drivers

This moves XPCS creation and destruction out of the common
stmmac_pcs_setup()/stmmac_pcs_clean() and into plat->pcs_init()/
plat->pcs_exit(), converts the Intel mGbE glue to create its own XPCS,
and makes the renesas-gbeth, rzn1 and socfpga pcs_exit() callbacks clear
priv->hw->phylink_pcs after destroying the PCS.

[Severity: Medium]
This isn't a bug introduced by this patch, but since the XPCS constructor
call now lives here, it may be worth noting: can the failure path inside
xpcs_init_clks() in drivers/net/pcs/pcs-xpcs.c leak the clk references?

xpcs_init_clks() takes the references and, if enabling fails, returns
without putting them back:

	ret = clk_bulk_get_optional(dev, DW_XPCS_NUM_CLKS, xpcs->clks);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to get clocks\n");

	ret = clk_bulk_prepare_enable(DW_XPCS_NUM_CLKS, xpcs->clks);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to enable clocks\n");

xpcs_create() then takes the error path:

	ret = xpcs_init_clks(xpcs);
	if (ret)
		goto out_free_data;

out_free_data calls xpcs_free_data() (mdio_device_put() plus kfree(xpcs)),
so xpcs_clear_clks() - the only caller of clk_bulk_put() - never runs and
the xpcs->clks array is freed with the references still held.

intel_mgbe_pcs_init() only receives an ERR_PTR here, so it has no way to
release them. Would adding clk_bulk_put() on the failed
clk_bulk_prepare_enable() path in xpcs_init_clks() be the right fix?

[ ... ]

[Severity: Medium]
This removes the only generic reader of the documented "pcs-handle"
property (and of mdio_bus_data->pcs_mask). For a glue driver that does not
install plat->pcs_init, stmmac_pcs_setup() now returns 0 before looking at
the firmware node at all.

Does that make "pcs-handle" a silent no-op for the plain Synopsys
compatibles handled by dwmac-generic.c?

	static const struct of_device_id dwmac_generic_match[] = {
		{ .compatible = "snps,dwmac-4.10a"},
		{ .compatible = "snps,dwmac"},
		{ .compatible = "snps,dwxgmac-2.10"},

dwmac_generic_probe() sets no pcs_init, while "pcs-handle" is defined
generically in Documentation/devicetree/bindings/net/ethernet-controller.yaml
and imported unrestricted by snps,dwmac.yaml. Previously such a node got an
XPCS via xpcs_create_fwnode(); now priv->hw->xpcs stays NULL and the
former probe diagnostic ("No xPCS found" on -EPROBE_DEFER/-ENODEV) is gone
with nothing in its place. Should the bindings be updated, or should a
generic pcs_init helper keep honouring the property?

The commit message says:

    First, if a device tree specifies a "pcs-handle" but no select_pcs()
    callback is provided by the platform driver, the created XPCS is never
    used.

Is that accurate? priv->hw->xpcs has two consumers besides select_pcs().
stmmac_phylink_setup() uses it to widen the interface masks:

	/* If we have an xpcs, it defines which PHY interfaces are supported. */
	if (priv->hw->xpcs)
		pcs = xpcs_to_phylink_pcs(priv->hw->xpcs);
	else
		pcs = priv->hw->phylink_pcs;

	if (pcs)
		phy_interface_or(config->supported_interfaces, ...

and stmmac_init_phy() uses it to skip PHY attachment:

	if (priv->hw->xpcs &&
	    xpcs_get_an_mode(priv->hw->xpcs, mode) == DW_AN_C73)
		return 0;

The commit message also says:

    Platforms that do not need an XPCS simply leave the callbacks as NULL
    and no change in behavior occurs.

For a platform that relied on the generic "pcs-handle" path, does behavior
change - narrowed config->supported_interfaces (and config->lpi_interfaces
when priv->dma_cap.eee is set), plus an unconditional PHY attach where the
DW_AN_C73 bypass used to apply?

There appear to be no in-tree DTS files pairing "snps,dwmac" with an
"snps,dw-xpcs" node today (RZ/N1, RZ/T2H, SoCFPGA and Rockchip all go
through glue that sets pcs_init, and pcs_mask is set only by dwmac-intel,
which this patch converts), and the gap is not filled later in the series -
at the end of the series stmmac_mdio.c still only dispatches to
plat->pcs_init and "pcs-handle" is parsed only by dwmac-intel,
dwmac-renesas-gbeth, dwmac-rzn1 and the new dwmac-rk. Could the commit
message state plainly that generic device tree XPCS attachment is being
removed?

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