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

[off-list ref] 于2026年9月15日周二 04:24写道：

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

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

- [Medium] Commit message mischaracterizes the removal of the generic
  XPCS creation as removing dead code.
- [Medium] DT binding vs implementation mismatch: after this patch
  nothing in the stmmac core reads the generic 'pcs-handle' property,…
- [Low] The commit message describes two defects, one framed as "more
  critically" (the common code overwriting priv->hw->xpcs after a…

--

net: stmmac: move XPCS lifetime management to platform drivers

The generic XPCS creation in stmmac_pcs_setup() (pcs-handle lookup and
mdio_bus_data->pcs_mask based xpcs_create_mdiodev()) is removed, leaving
only the plat->pcs_init() / plat->pcs_exit() dispatch. The Intel mGbE glue
grows its own pcs_init()/pcs_exit() that create and destroy the XPCS, and
the renesas-gbeth, rzn1 and socfpga pcs_exit() callbacks now clear
priv->hw->phylink_pcs after destroying the PCS.

A few questions on the commit message and on the removal of the generic
path.

quoted

    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. The phylink framework requires select_pcs() to actually return
    the PCS to the core, so the pcs-handle property becomes effectively
    useless without the matching callback.

[Severity: Medium]
Is "the created XPCS is never used" accurate?  An XPCS stored in
priv->hw->xpcs has two consumers that do not go through select_pcs().

stmmac_phylink_setup() in stmmac_main.c uses it to widen the phylink
interface sets:

        /* 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 decide whether to attach a PHY at all:

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

So for a dwmac instance with a pcs-handle but no plat->pcs_init, does this
change narrow config->supported_interfaces (and config->lpi_interfaces) and
alter the PHY-attach decision, rather than just dropping something unused?
Would it be worth spelling that behavioural change out in the commit
message?

The XPCS's runtime functionality is entirely driven by phylink through
pcs_config() / pcs_get_state() / pcs_link_up(), all of which require
the pl->pcs pointer returned by mac_select_pcs(). Without that callback,
pl->pcs stays NULL and none of them are ever called.

The two reads in stmmac_main.c are one-shot setup-time side effects.
They do not exercise the PCS, so I consider the statement accurate in
the sense that matters here.

[Severity: Low]
This isn't a bug, but could this paragraph say that no in-tree platform is
affected today?

All three pre-existing pcs_init() implementations store their PCS in
priv->hw->phylink_pcs, not priv->hw->xpcs:

  renesas_gmac_pcs_init()  -> priv->hw->phylink_pcs = miic_create(...)
  rzn1_dwmac_pcs_init()    -> priv->hw->phylink_pcs = miic_create(...)
  socfpga_dwmac_pcs_init() -> priv->hw->phylink_pcs = pcs

so the old "priv->hw->xpcs = xpcs;" on the pcs_init branch overwrote NULL
with NULL.  intel_mgbe_pcs_init() added here is the first in-tree pcs_init()
that stores an XPCS, which makes this a refactor that removes a latent trap
rather than a fix for observable breakage.

The wording is intentional. All in-tree pcs_init() implementations
except Intel store their PCS in priv->hw->phylink_pcs, so the old
assignment was a latent trap rather than observable breakage. Calling
it out as critical reflects the risk it would have posed for the first
driver that stored an XPCS there.

quoted

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]
After this change, does anything in the stmmac core still read the generic
"pcs-handle" property?

The property is still documented generically in
Documentation/devicetree/bindings/net/ethernet-controller.yaml:

  pcs-handle:
    $ref: /schemas/types.yaml#/definitions/phandle-array
    items:
      maxItems: 1
    description:
      Specifies a reference to a node representing a PCS PHY device on a MDIO
      bus to link with an external PHY (phy-handle) if exists.

and snps,dwmac.yaml $refs ethernet-controller.yaml and ends with
"additionalProperties: true", so pcs-handle stays schema-valid on every
dwmac node regardless of which glue is bound.

On a glue that does not set plat->pcs_init, stmmac_pcs_setup() now returns 0
without looking at the property, priv->hw->xpcs stays NULL, and nothing is
logged - whereas the removed code would at least fail probe with
"No xPCS found" for a bad phandle.  Should the binding be updated to say
pcs-handle is only honoured by specific glue drivers, or should the core
keep a generic path (or emit a warning) so the two agree?

The same question applies to struct stmmac_mdio_bus_data.pcs_mask.
stmmac_mdio_register() still treats it as "a PCS lives at these MDIO
addresses" and masks them out of the bus scan:

        new_bus->phy_mask = mdio_bus_data->phy_mask | mdio_bus_data->pcs_mask;

but no core code creates a PCS for those addresses any more, only
dwmac-intel's private intel_mgbe_pcs_init().  Does a glue that sets
pcs_mask without pcs_init now end up with that address neither scanned as a
PHY nor instantiated as a PCS?

This is intentional. Only platforms that set pcs_init() are considered
to support a PCS. A glue that sets pcs-handle or pcs_mask without
pcs_init() is misconfigured, and the new behaviour makes that explicit
by not creating anything.