Re: [PATCH net-next v5 4/4] net: dsa: mxl862xx: add support for SerDes ports

[Daniel Golle <daniel@makrotopia.org>]

On Thu, Jun 11, 2026 at 02:10:44PM +0100, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> net: dsa: mxl862xx: add support for SerDes ports
> 
> The MxL862xx has two XPCS/SerDes interfaces. This commit implements phylink
> PCS operations using the firmware's XPCS API and registers a PCS instance
> for each SerDes interface and QSGMII/10G_QXGMII sub-ports.
> 
> > diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-phylink.c b/drivers/net/dsa/mxl862xx/mxl862xx-phylink.c
> > --- a/drivers/net/dsa/mxl862xx/mxl862xx-phylink.c
> > +++ b/drivers/net/dsa/mxl862xx/mxl862xx-phylink.c
> [ ... ]
> > @@ -7,20 +7,401 @@
> >   * Copyright (C) 2025 Daniel Golle <daniel@makrotopia.org>
> >   */
> >  
> > +#include <linux/bitfield.h>
> > +#include <linux/mutex.h>
> >  #include <linux/phylink.h>
> >  #include <net/dsa.h>
> >  
> >  #include "mxl862xx.h"
> > +#include "mxl862xx-api.h"
> > +#include "mxl862xx-cmd.h"
> > +#include "mxl862xx-host.h"
> >  #include "mxl862xx-phylink.h"
> >  
> >  void mxl862xx_phylink_get_caps(struct dsa_switch *ds, int port,
> >  			       struct phylink_config *config)
> >  {
> > +	struct mxl862xx_priv *priv = ds->priv;
> > +
> >  	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 |
> >  				   MAC_100 | MAC_1000 | MAC_2500FD;
> >  
> > -	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > -		  config->supported_interfaces);
> > +	switch (port) {
> > +	case 1 ... 8:
> > +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > +			  config->supported_interfaces);
> > +		break;
> > +	case 9:
> > +	case 13:
> > +		__set_bit(PHY_INTERFACE_MODE_SGMII, config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_1000BASEX, config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_2500BASEX, config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_10GBASER, config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_10GKR, config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_USXGMII, config->supported_interfaces);
> > +		fallthrough;
> > +	case 10 ... 12:
> > +	case 14 ... 16:
> > +		if (!MXL862XX_FW_VER_MIN(priv, 1, 0, 84))
> > +			break;
> 
> [Severity: High]
> Since the firmware version check is performed after the supported interface bits
> are set for ports 9 and 13, won't SGMII and other modes remain advertised on
> older firmware versions?
> 
> Because mxl862xx_phylink_mac_select_pcs() returns NULL for ports 9-16 on older
> firmware, phylink would accept the configuration without a PCS, potentially
> leaving the port silently unconfigured.

This is intentional. The firmware configures the SerDes interfaces to the
modes defined in a on-flash configuration. This means the CPU port typically
works fine if DT reflects what is in the on-flash configuration without
needing the driver to do anything for that.
On the other hand, if we error out here, the driver will fail to probe
because it cannot bring up the CPU port. The problem is that this would
also prevent firmware updates via devlink (which are going to be introduced
in a later follow series), which require the driver to at least probe.
A dev_warn() is issued in this case, so the user is informed about SerDes PCS
configuration being unsupported on the old firmware version.

> 
> [ ... ]
> > +static void mxl862xx_pcs_disable(struct phylink_pcs *pcs)
> > +{
> > +	struct mxl862xx_pcs *mpcs = pcs_to_mxl862xx_pcs(pcs);
> > +	struct mxl862xx_xpcs_pcs_disable dis = {};
> > +	struct mxl862xx_priv *priv = mpcs->priv;
> > +
> > +	dis.port_id = mpcs->serdes_id;
> > +
> > +	/* The SerDes is shared across QSGMII/QUSXGMII sub-ports; only
> > +	 * power it down once the last active sub-port goes away. Hold
> > +	 * serdes_lock across the count and the power-down so a sibling
> > +	 * sub-port enable cannot race the transition to zero.
> > +	 */
> > +	mutex_lock(&priv->serdes_lock);
> > +	if (--priv->serdes_refcount[mpcs->serdes_id] == 0)
> > +		MXL862XX_API_WRITE(priv, MXL862XX_XPCS_PCS_DISABLE, dis);
> > +	mutex_unlock(&priv->serdes_lock);
> > +}
> 
> [ ... ]
> > +static int mxl862xx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> > +			       phy_interface_t interface,
> > +			       const unsigned long *advertising,
> > +			       bool permit_pause_to_mac)
> > +{
> > +	struct mxl862xx_pcs *mpcs = pcs_to_mxl862xx_pcs(pcs);
> > +	struct mxl862xx_priv *priv = mpcs->priv;
> > +	struct mxl862xx_xpcs_pcs_cfg cfg = {};
> > +	int if_mode, lane, ret, adv;
> > +
> > +	if_mode = mxl862xx_xpcs_if_mode(interface);
> > +	if (if_mode < 0) {
> > +		dev_err(priv->ds->dev, "unsupported interface: %s\n",
> > +			phy_modes(interface));
> > +		return if_mode;
> > +	}
> > +
> > +	/* The XPCS bringup is per-instance and idempotent in the
> > +	 * firmware: every QSGMII/QUSXGMII sub-port may call pcs_config
> > +	 * and the firmware will skip the bringup if the requested mode
> > +	 * matches the cached one, then update MAC pause for the
> > +	 * sub-port indicated by @usx_subport.
> > +	 */
> > +	lane = (interface == PHY_INTERFACE_MODE_10G_QXGMII) ?
> > +	       MXL862XX_XPCS_USX_QUAD : MXL862XX_XPCS_USX_SINGLE;
> > +
> > +	cfg.mode = cpu_to_le16(FIELD_PREP(MXL862XX_XPCS_CFG_PORT_ID,
> > +					  mpcs->serdes_id) |
> > +			       FIELD_PREP(MXL862XX_XPCS_CFG_USX_SUBPORT,
> > +					  mpcs->slot) |
> > +			       FIELD_PREP(MXL862XX_XPCS_CFG_USX_LANE_MODE, lane) |
> > +			       FIELD_PREP(MXL862XX_XPCS_CFG_INTERFACE, if_mode) |
> > +			       FIELD_PREP(MXL862XX_XPCS_CFG_NEG_MODE,
> > +					  mxl862xx_xpcs_neg_mode(neg_mode)) |
> > +			       FIELD_PREP(MXL862XX_XPCS_CFG_ROLE,
> > +					  MXL862XX_XPCS_ROLE_MAC) |
> > +			       FIELD_PREP(MXL862XX_XPCS_CFG_PERMIT_PAUSE,
> > +					  permit_pause_to_mac));
> > +
> > +	if (neg_mode & PHYLINK_PCS_NEG_INBAND) {
> > +		adv = phylink_mii_c22_pcs_encode_advertisement(interface,
> > +							       advertising);
> > +		if (adv >= 0)
> > +			cfg.advertising.cl37 = cpu_to_le16(adv);
> > +	}
> > +
> > +	ret = MXL862XX_API_READ(priv, MXL862XX_XPCS_PCS_CONFIG, cfg);
> 
> [Severity: High]
> Can mxl862xx_pcs_disable() on one sub-port race with mxl862xx_pcs_config() on
> another sub-port, potentially leaving the shared XPCS powered down?
> 
> mxl862xx_pcs_disable() synchronizes the hardware disable command using
> serdes_lock, but it seems mxl862xx_pcs_config() issues the firmware config
> command (which implicitly powers up the hardware) without holding
> serdes_lock. If mxl862xx_pcs_disable() executes its firmware command right
> after mxl862xx_pcs_config() executes, could this cause a permanent link failure
> on the newly configured sub-port?

Yes, mxl862xx_pcs_config() needs to hold the lock as well, I'll fix that.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = (s16)le16_to_cpu(cfg.result);
> > +	if (ret < 0)
> > +		return mxl862xx_xpcs_errno(ret);
> > +
> > +	mpcs->interface = interface;
> > +	return ret > 0 ? 1 : 0;
> > +}
> > +
> > +static void mxl862xx_pcs_get_state(struct phylink_pcs *pcs,
> > +				   unsigned int neg_mode,
> > +				   struct phylink_link_state *state)
> > +{
> > +	struct mxl862xx_pcs *mpcs = pcs_to_mxl862xx_pcs(pcs);
> > +	struct mxl862xx_priv *priv = mpcs->priv;
> > +	struct mxl862xx_xpcs_pcs_state st = {};
> > +	int if_mode, lane, ret;
> > +	u32 mode;
> > +	u16 bmsr;
> > +
> > +	if_mode = mxl862xx_xpcs_if_mode(state->interface);
> > +	if (if_mode < 0)
> > +		return;
> 
> [Severity: High]
> Does returning early here without setting state->link = 0 cause issues?

In practise this is not a problem as phylink prevents the function from
being called with any other interface mode than the ones defined in the
capabilities. If at all, then we should issue a WARN here.

> 
> Phylink initializes state->link = 1 before calling pcs_get_state(), expecting
> the callback to clear it if the link is down or an error occurs. Leaving it
> as 1 might cause phylink to process uninitialized parameters.
> 
> > +
> > +	lane = (state->interface == PHY_INTERFACE_MODE_10G_QXGMII) ?
> > +	       MXL862XX_XPCS_USX_QUAD : MXL862XX_XPCS_USX_SINGLE;
> > +
> > +	st.mode = cpu_to_le32(FIELD_PREP(MXL862XX_XPCS_ST_PORT_ID,
> > +					 mpcs->serdes_id) |
> > +			      FIELD_PREP(MXL862XX_XPCS_ST_INTERFACE, if_mode) |
> > +			      FIELD_PREP(MXL862XX_XPCS_ST_USX_SUBPORT,
> > +					 mpcs->slot) |
> > +			      FIELD_PREP(MXL862XX_XPCS_ST_USX_LANE_MODE, lane));
> > +
> > +	ret = MXL862XX_API_READ(priv, MXL862XX_XPCS_PCS_GET_STATE, st);
> > +	if (ret)
> > +		return;
> 
> [Severity: High]
> Could the early return here also leave state->link = 1 incorrectly when the
> firmware command fails?

Yes, and that's intentional. A failing firmware command doesn't mean that the
link went down -- it means that there has been a problem with the MDIO bus
communication with the firmware, or the specific firmware command returning an
error. As pcs-polling will retry pcs_get_state() I figure that just not touching
state->link in this case is the best thing to do.