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

[Coia Prant]

<netdev-bot+sashiko@xxxxxxxxxx> 于2026年9月18日周五 02:38写道：
>
> 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.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > index f5f9fa67ecd77..4308dccbf2570 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > @@ -603,13 +603,47 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
> >       plat->mdio_bus_data->needs_reset = true;
> >  }
> >
> > +static int intel_mgbe_pcs_init(struct stmmac_priv *priv)
> > +{
> > +     struct fwnode_handle *devnode, *pcsnode;
> > +     struct dw_xpcs *xpcs;
> > +     int addr;
> > +
> > +     devnode = dev_fwnode(priv->device);
> > +
> > +     if (fwnode_property_present(devnode, "pcs-handle")) {
> > +             pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
> > +             xpcs = xpcs_create_fwnode(pcsnode);
> > +             fwnode_handle_put(pcsnode);
> > +     } else {
> > +             addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
> > +             xpcs = xpcs_create_mdiodev(priv->mii, addr);
> > +     }
> > +
> > +     if (IS_ERR(xpcs))
> > +             return PTR_ERR(xpcs);
>
> [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?

This is a pre-existing bug in pcs-xpcs.c and is not introduced by this
series. It belongs to the XPCS core driver and should be fixed
separately.

> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> > index afe98ff5bdcb0..7396b68899c66 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> > @@ -426,35 +426,14 @@ int stmmac_mdio_reset(struct mii_bus *bus)
> >  int stmmac_pcs_setup(struct net_device *ndev)
> >  {
> >       struct stmmac_priv *priv = netdev_priv(ndev);
> > -     struct fwnode_handle *devnode, *pcsnode;
> > -     struct dw_xpcs *xpcs = NULL;
> > -     int addr, ret;
> > -
> > -     devnode = dev_fwnode(priv->device);
> > -
> > -     if (priv->plat->pcs_init) {
> > -             ret = priv->plat->pcs_init(priv);
> > -     } else if (fwnode_property_present(devnode, "pcs-handle")) {
> > -             pcsnode = fwnode_find_reference(devnode, "pcs-handle", 0);
> > -             xpcs = xpcs_create_fwnode(pcsnode);
> > -             fwnode_handle_put(pcsnode);
> > -             ret = PTR_ERR_OR_ZERO(xpcs);
> > -     } else if (priv->plat->mdio_bus_data &&
> > -                priv->plat->mdio_bus_data->pcs_mask) {
> > -             addr = ffs(priv->plat->mdio_bus_data->pcs_mask) - 1;
> > -             xpcs = xpcs_create_mdiodev(priv->mii, addr);
> > -             ret = PTR_ERR_OR_ZERO(xpcs);
> > -     } else {
> > +     int ret;
> > +
> > +     if (!priv->plat->pcs_init)
> >               return 0;
> > -     }
> >
> > +     ret = priv->plat->pcs_init(priv);
> >       if (ret)
> > -             return dev_err_probe(priv->device, ret, "No xPCS found\n");
> > -
> > -     if (xpcs)
> > -             xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> > -
> > -     priv->hw->xpcs = xpcs;
> > +             return dev_err_probe(priv->device, ret, "Failed to initialize PCS\n");
> >
> >       return 0;
> >  }
>
> [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?

You're right that this removes the only generic reader of the
"pcs-handle" property, and that dwmac-generic.c no longer gets an XPCS
from it. This is intentional: PCS handling is now the responsibility of
each platform's glue driver via pcs_init(). No in-tree platform relies
on the generic path. I'll state this explicitly in the commit message.

I'll also reword the "created XPCS is never used" sentence to
acknowledge that priv->hw->xpcs is still consulted by
stmmac_phylink_setup() and stmmac_init_phy(), even without select_pcs().