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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi again,

On 9/1/26 17:01, Coia Prant wrote:
> The current XPCS creation logic in stmmac_pcs_setup() is problematic
> for several reasons.
> 
> 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. This is confusing for developers
> who expect that specifying a pcs-handle in their device tree should be
> sufficient to enable the PCS.
> 
> 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 during its own initialization,
> for example in a pcs_init() callback or during probe. The platform
> driver has no way to prevent this override because the common code
> runs unconditionally after the platform-specific initialization.
> 
> After commit 93f84152e4ae ("net: stmmac: clean up
> stmmac_mac_select_pcs()"), the common code no longer falls back to
> priv->hw->phylink_pcs if select_pcs() is not set. This change
> reinforces that each platform must manage its own PCS life cycle
> explicitly, but the XPCS creation code in stmmac_pcs_setup() was not
> updated to match this new expectation, leaving a gap where platform
> drivers have no clean way to take control of XPCS creation.
> 
> Address all of these issues by introducing pcs_init() and pcs_exit()
> callbacks in plat_stmmacenet_data. These callbacks give platform
> drivers full control over when and how the XPCS is created, configured,
> and destroyed. The common stmmac_pcs_setup() and stmmac_pcs_clean()
> functions are simplified to just call these callbacks, removing the
> confusing and error-prone XPCS creation logic from the common code.
> 
> Platforms that do not need an XPCS simply leave the callbacks as NULL
> and no change in behavior occurs. Platforms that do need an XPCS can
> now create it with the exact configuration they require, including
> wrapping it with custom phylink_pcs_ops when necessary.
> 
> 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(). In their pcs_exit() implementations, the
> pointer to the destroyed PCS is explicitly set to NULL to avoid
> dangling pointer references.
> 
> Signed-off-by: Coia Prant <coiaprant@gmail.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 44 +++++++++++++++++--
>  .../stmicro/stmmac/dwmac-renesas-gbeth.c      |  7 ++-
>  .../net/ethernet/stmicro/stmmac/dwmac-rzn1.c  |  7 ++-
>  .../ethernet/stmicro/stmmac/dwmac-socfpga.c   |  7 ++-
>  .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 37 +++-------------
>  5 files changed, 61 insertions(+), 41 deletions(-)
> 
> 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;
> +	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);
> +	}

Sorry I had a second look at that after a good night's sleep, and actually here
we may regress. The original logic checked for "pcs-handle" presence, then
for the pcs mask, but if no PCS is found we didn't error out, we returned 0.

This is making it mandatory to have a PCS. Please handle gracefully the "there's no
PCS" case :(


Maxime