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

[Jakub Kicinski <kuba@kernel.org>]

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.

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?

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?

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

> +	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;
> +}
> +
> +static void intel_mgbe_pcs_exit(struct stmmac_priv *priv)
> +{
> +	if (!priv->hw->xpcs)
> +		return;
> +
> +	xpcs_destroy(priv->hw->xpcs);
> +	priv->hw->xpcs = NULL;
> +}
> +

[ ... ]

> @@ -733,6 +767,8 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
>  	    plat->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
>  		plat->mdio_bus_data->pcs_mask = BIT_U32(INTEL_MGBE_XPCS_ADDR);
>  		plat->default_an_inband = true;
> +		plat->pcs_init = intel_mgbe_pcs_init;
> +		plat->pcs_exit = intel_mgbe_pcs_exit;
>  		plat->select_pcs = intel_mgbe_select_pcs;
>  	}
>  
> 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?

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

>  
> -	if (xpcs)
> -		xpcs_config_eee_mult_fact(xpcs, priv->plat->mult_fact_100ns);
> -
> -	priv->hw->xpcs = xpcs;
> -
>  	return 0;
>  }
>  

[ ... ]