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

Jakub Kicinski [off-list ref] 于2026年9月13日周日 04:13写道：

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: move XPCS lifetime management to platform drivers

This removes the generic XPCS creation and destruction code from
stmmac_pcs_setup() and stmmac_pcs_clean(), leaving those functions as
plain dispatchers to plat->pcs_init() and plat->pcs_exit().  The Intel
mGbE glue gains its own pcs_init()/pcs_exit() implementations, 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 changelog and on the removed common code below.

[Severity: Medium]
Three statements in the commit message do not seem to match the tree
this patch applies to.

First:

    Address all of these issues by introducing pcs_init() and pcs_exit()
    callbacks in plat_stmmacenet_data.

Are these callbacks actually introduced here?  Both function pointers
already exist in struct plat_stmmacenet_data at the baseline:

        int (*pcs_init)(struct stmmac_priv *priv);
        void (*pcs_exit)(struct stmmac_priv *priv);

and this patch does not touch include/linux/stmmac.h at all.  The
baseline stmmac_pcs_setup() and stmmac_pcs_clean() already call them.

You're right. These callbacks already exist. My patch simplifies the
dispatch, not introduces them. I'll fix the wording.

Second:

    Second, and more critically, when stmmac_pcs_setup() fails to create
    an XPCS (either because no pcs-handle is present and no pcs_mask is
    configured), it falls through to the else branch and leaves
    priv->hw->xpcs as NULL. This will silently override any XPCS that a
    platform driver may have already set up

Is that the branch that causes the override?  In the baseline that arm
returns immediately:

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

so no XPCS pointer can be clobbered there.  The clobber only happens on
the plat->pcs_init branch, where the local xpcs stays NULL and the
unconditional priv->hw->xpcs = xpcs; afterwards overwrites whatever
pcs_init() stored.  Could the changelog describe that path instead?

You're right. The else branch returns immediately. The clobber happens on
the plat->pcs_init branch. I'll fix the changelog.

Third:

    Existing platform drivers (intel, rzn1, socfpga) are updated to use
    the new callbacks by moving their XPCS creation and cleanup logic into
    pcs_init() and pcs_exit().

rzn1 and socfpga already used pcs_init()/pcs_exit() before this patch,
and neither creates an XPCS: rzn1_dwmac_pcs_init() calls miic_create()
and socfpga_dwmac_pcs_init() calls lynx_pcs_create_mdiodev().  The only
change for them is the added NULL assignment.  The patch also modifies
dwmac-renesas-gbeth.c, which the changelog does not mention.  Should
that list be corrected to intel only, plus a note about the pointer
clearing in renesas-gbeth, rzn1 and socfpga?

Okay, I will update it.

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index f5f9fa67ecd77..fd5f01c8941c1 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 = NULL;

[Severity: Low]
This isn't a bug, but is the NULL initializer still reachable here?
Both arms of the if/else below assign xpcs unconditionally, from
xpcs_create_fwnode() or xpcs_create_mdiodev(), and neither returns NULL.
In the common code this was moved from, the initializer mattered because
a fall-through left xpcs NULL and the call was guarded by
if (xpcs) xpcs_config_eee_mult_fact(...); that guard is now gone.

You're right. I'll drop it.

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
index 19f34e18bfef2..9af32c26f9c14 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c

@@ -81,8 +81,11 @@ static int renesas_gmac_pcs_init(struct stmmac_priv *priv)

 static void renesas_gmac_pcs_exit(struct stmmac_priv *priv)
 {
-     if (priv->hw->phylink_pcs)
-             miic_destroy(priv->hw->phylink_pcs);
+     if (!priv->hw->phylink_pcs)
+             return;
+
+     miic_destroy(priv->hw->phylink_pcs);
+     priv->hw->phylink_pcs = NULL;
 }

 static struct phylink_pcs *renesas_gmac_select_pcs(struct stmmac_priv *priv,

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
index 13634965bc19a..01df4776edb3f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c

@@ -35,8 +35,11 @@ static int rzn1_dwmac_pcs_init(struct stmmac_priv *priv)

 static void rzn1_dwmac_pcs_exit(struct stmmac_priv *priv)
 {
-     if (priv->hw->phylink_pcs)
-             miic_destroy(priv->hw->phylink_pcs);
+     if (!priv->hw->phylink_pcs)
+             return;
+
+     miic_destroy(priv->hw->phylink_pcs);
+     priv->hw->phylink_pcs = NULL;
 }

 static struct phylink_pcs *rzn1_dwmac_select_pcs(struct stmmac_priv *priv,

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1d7f0a57d2889..6d4bc1fe8f751 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c

@@ -539,8 +539,11 @@ static int socfpga_dwmac_pcs_init(struct stmmac_priv *priv)

 static void socfpga_dwmac_pcs_exit(struct stmmac_priv *priv)
 {
-     if (priv->hw->phylink_pcs)
-             lynx_pcs_destroy(priv->hw->phylink_pcs);
+     if (!priv->hw->phylink_pcs)
+             return;
+
+     lynx_pcs_destroy(priv->hw->phylink_pcs);
+     priv->hw->phylink_pcs = NULL;
 }

 static struct phylink_pcs *socfpga_dwmac_select_pcs(struct stmmac_priv *priv,

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index afe98ff5bdcb0..d2f77f0c223a7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c

@@ -426,36 +426,15 @@ 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;
-     }

[Severity: Medium]
Does this drop the generic handling of the "pcs-handle" property?  After
this change, any snps,dwmac node whose firmware points "pcs-handle" at a
Synopsys XPCS, but whose glue driver does not set plat->pcs_init, gets
priv->hw->xpcs == NULL and no message at all, since the function just
returns 0.  "pcs-handle" is still a documented generic property in
Documentation/devicetree/bindings/net/ethernet-controller.yaml.

The changelog explains this is harmless because:

    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?  The core consumes priv->hw->xpcs in two places that
do not go through plat->select_pcs():

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_phy_setup() {
        /* 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, ...);
}

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_init_phy() {
        if (priv->hw->xpcs &&
            xpcs_get_an_mode(priv->hw->xpcs, mode) == DW_AN_C73)
                return 0;
}

Without the XPCS, supported_interfaces stays limited to
plat->phy_interface and the DW_AN_C73 "no PHY" path is lost, so
stmmac_init_phy() can now fail where it previously succeeded.

Related, for Intel mGbE the callbacks are only installed inside the
SGMII/1000BASEX branch of intel_mgbe_common_data(), while the removed
common code created an XPCS from "pcs-handle" regardless of interface
mode.  Should a warning be emitted when "pcs-handle" is present but no
pcs_init() claims it, or should the changelog state the behaviour change
explicitly?

This removal is intentional. Phylink relies on select_pcs() to obtain the
PCS. Without that callback, the PCS is effectively never used, as phylink
has no way to obtain it. Requiring pcs_init() to be set is a way to ensure
that only platforms that know they need a PCS will create one.

The two places in stmmac_main.c that read priv->hw->xpcs directly
(stmmac_phy_setup and stmmac_init_phy) do so under the assumption that
the platform has already set up a working PCS. A platform with
"pcs-handle" but no pcs_init()/select_pcs() has never had a functional
PCS.

However, to make this behaviour change explicit and avoid silent failures,
I'll add a warning or a comment in the changelog describing the
requirement. If you prefer, I can also emit a diagnostic when
"pcs-handle" is present but no pcs_init() claims it.

quoted

+     ret = priv->plat->pcs_init(priv);
      if (ret)
              return dev_err_probe(priv->device, ret, "No xPCS found\n");

[Severity: Low]
This is a pre-existing wording problem and not introduced by this patch,
but with the generic XPCS code gone this is now the only diagnostic in
stmmac_pcs_setup(), and three of the four pcs_init() implementers do not
create an XPCS:

  socfpga_dwmac_pcs_init()  -> lynx_pcs_create_mdiodev()
  rzn1_dwmac_pcs_init()     -> miic_create()
  renesas_gmac_pcs_init()   -> miic_create()

So a miic_create() or lynx_pcs_create_mdiodev() failure, including
-EPROBE_DEFER, is reported as "No xPCS found".  Would something like
"failed to initialise PCS" be more accurate here?

I'll change it to "Failed to initialize PCS".

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy