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

[Coia Prant]

Jakub Kicinski <kuba@xxxxxxxxxx> 于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.

> > 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.

> > 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.

> >
> > +     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".