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

[netdev-bot+sashiko]

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.

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

>     Second, and more critically, when a platform driver sets pcs_init()
>     and creates an XPCS inside that callback, the common code afterwards
>     still runs unconditionally and overwrites priv->hw->xpcs with the
>     local xpcs variable, which stays NULL. The platform driver has no way
>     to prevent this override because the common code runs after the
>     platform-specific initialization.

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

> 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);
> +
> +	xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> +
> +	priv->hw->xpcs = xpcs;
> +	return 0;
> +}
> +

[ ... ]

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

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