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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit message supporting evidence: the list "every DTS that
  pairs a dwmac node with a PCS goes through a glue that sets pcs_init()…
- [Low] Concern that the commit message overstates the second ("more
  critically") problem — the unconditional `priv->hw->xpcs = xpcs;`…

--

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() code and into the platform glue
pcs_init()/pcs_exit() callbacks. The Intel mGbE glue gains 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.

Two observations on the commit message below.

>     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 is the "more critically" framing accurate for the
tree as it stands before this patch?

The description of the pre-patch code is correct: stmmac_pcs_setup() ran

	if (priv->plat->pcs_init) {
		ret = priv->plat->pcs_init(priv);
	...
	priv->hw->xpcs = xpcs;

with xpcs still NULL on the pcs_init() branch.

However, none of the pre-patch in-tree pcs_init() implementations store
an XPCS: renesas_gmac_pcs_init(), rzn1_dwmac_pcs_init() and
socfpga_dwmac_pcs_init() all assign priv->hw->phylink_pcs, not
priv->hw->xpcs, so no shipping platform was actually hit by the
overwrite.

The first platform that stores an XPCS from pcs_init() is
intel_mgbe_pcs_init(), added by this very patch. Would it be clearer to
state that the overwrite is a latent design gap that becomes relevant
once a glue starts creating its own XPCS, rather than an existing
failure?

>     Note that this also removes the generic "pcs-handle" parsing from the
>     common code. A glue that does not set pcs_init() now leaves
>     priv->hw->xpcs as NULL, and "pcs-handle" becomes a no-op for it. No
>     in-tree platform relies on this path: every DTS that pairs a dwmac node
>     with a PCS goes through a glue that sets pcs_init() (Intel, Renesas,
>     RZ/N1, SoCFPGA, Rockchip).

[Severity: Low]
Should Rockchip be in that list at this point in the series? At this
commit, drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c has no PCS
handling at all, there is no pcs_init(), pcs_exit(), select_pcs(), xpcs
or pcs_mask reference in that file, and no Rockchip dwmac DTS uses
"pcs-handle".

The underlying claim does check out for the other entries: the only
in-tree setter of mdio_bus_data->pcs_mask is intel_mgbe_common_data(),
which this patch also gives pcs_init()/pcs_exit(), and the only dwmac DTS
users of "pcs-handle" are the rzn1 and renesas-gbeth nodes, both of which
already set pcs_init().

The Rockchip glue does gain rk_pcs_init()/rk_pcs_exit()/rk_select_pcs()
later in this same series ("net: stmmac: dwmac-rk: add SGMII support for
RK3568"), so the list is accurate for the finished series. Would it be
worth wording the parenthetical to reflect what is true at this commit,
or reordering so the Rockchip support lands first?

For reference, the code the two comments refer to:

> 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)
> +{

[ ... ]

> +	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;
>  }

[ ... ]

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