Re: [PATCH net-next v3 8/8] net: dsa: motorcomm: Add SerDes PCS

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] PHY_INTERFACE_MODE_100BASEX is advertised as a supported PCS
  interface and the SerDes is actually programmed to…
- [Medium] For SGMII with in-band signalling enabled,
  yt921x_serdes_config() sets YT921X_PORT_CTRL_PAUSE_AN whenever…
- [Low] yt921x_phylink_pcs_get_state() logs an unconditional dev_err() on
  every register-read failure (pcs-921x.c:132-137).

--

net: dsa: motorcomm: Add SerDes PCS

This adds a phylink PCS implementation for the yt921x SerDes ports, moving
the SerDes mode selection out of yt921x_port_config() into pcs_config(),
and wires up in-band negotiation through the internal MDIO interface.

A few questions about the new PCS code below.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 491ba1edfa3cd..530bea77ac70a 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -3511,6 +3512,10 @@ yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
>  	if (ps == YT921X_SPEED_NUM)
>  		return -EINVAL;
>  
> +	mask = YT921X_PORT_SPEED_M | YT921X_PORT_TX_MAC_EN |
> +	       YT921X_PORT_RX_MAC_EN | YT921X_PORT_TX_PAUSE |
> +	       YT921X_PORT_RX_PAUSE | YT921X_PORT_DUPLEX_FULL |
> +	       YT921X_PORT_CTRL_LINK_AN;
>  	ctrl = YT921X_PORT_SPEED(ps);
>  	if (duplex == DUPLEX_FULL)
>  		ctrl |= YT921X_PORT_DUPLEX_FULL;
> @@ -3519,7 +3524,9 @@ yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
>  	if (rx_pause)
>  		ctrl |= YT921X_PORT_RX_PAUSE;
>  	ctrl |= YT921X_PORT_RX_MAC_EN | YT921X_PORT_TX_MAC_EN;
> +	if (pp->serdes && pp->inband)
> +		ctrl |= YT921X_PORT_CTRL_LINK_AN;
> +	res = yt921x_reg_update_bits(priv, YT921X_PORTn_CTRL(port), mask, ctrl);
>  	if (res)
>  		return res;

[ ... ]

> @@ -3637,6 +3622,29 @@ yt921x_port_config(struct yt921x_priv *priv, int port, unsigned int mode,
>  	return -EINVAL;
>  }
>  
> +static struct phylink_pcs *
> +yt921x_phylink_mac_select_pcs(struct phylink_config *config,
> +			      phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct yt921x_priv *priv = to_yt921x_priv(dp->ds);
> +	const struct yt921x_info *info = priv->info;
> +	int port = dp->index;
> +
> +	if (!(BIT(port) & info->serdes_mask))
> +		return NULL;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_100BASEX:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		return &priv->ports[port].pcs;
> +	default:
> +		return NULL;
> +	}
> +}
> +

[ ... ]

> @@ -4266,6 +4281,23 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
>  		return -ENODEV;
>  	}
>  
> +	mask = priv->info->serdes_mask;
> +	for_each_set_bit(port, &mask, YT921X_PORT_NUM) {
> +		struct yt921x_port *pp = &priv->ports[port];
> +
> +		pp->pcs.ops = &yt921x_phylink_pcs_ops;
> +		pp->pcs.poll = true;
> +
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  pp->pcs.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_100BASEX,
> +			  pp->pcs.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +			  pp->pcs.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			  pp->pcs.supported_interfaces);
> +	}
> +

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/pcs-921x.c b/drivers/net/dsa/motorcomm/pcs-921x.c
> new file mode 100644
> index 0000000000000..dcf7397cd2413
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/pcs-921x.c
> @@ -0,0 +1,235 @@

[ ... ]

> +static int
> +yt921x_serdes_config(struct yt921x_priv *priv, int port, unsigned int neg_mode,
> +		     phy_interface_t interface,
> +		     const unsigned long *advertising, bool permit_pause_to_mac)
> +{
> +	bool inband = neg_mode & PHYLINK_PCS_NEG_INBAND;
> +	struct yt921x_port *pp = &priv->ports[port];

[ ... ]

> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +		ctrl = YT921X_SERDES_MODE_SGMII;
> +		break;
> +	case PHY_INTERFACE_MODE_100BASEX:
> +		ctrl = YT921X_SERDES_MODE_100BASEX;
> +		break;
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		ctrl = YT921X_SERDES_MODE_1000BASEX;
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		ctrl = YT921X_SERDES_MODE_2500BASEX;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	res = yt921x_reg_update_bits(priv, YT921X_SERDESn(port),
> +				     YT921X_SERDES_MODE_M, ctrl);
> +	if (res)
> +		return res;
> +
> +	mask = YT921X_PORT_CTRL_LINK_AN | YT921X_PORT_CTRL_PAUSE_AN;
> +	ctrl = 0;
> +	if (inband)
> +		ctrl |= YT921X_PORT_CTRL_LINK_AN;
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED && permit_pause_to_mac)
> +		ctrl |= YT921X_PORT_CTRL_PAUSE_AN;

[Severity: Medium]
Should YT921X_PORT_CTRL_PAUSE_AN also be gated on the interface here?

For PHY_INTERFACE_MODE_SGMII the Cisco in-band configuration word carries
only link, speed and duplex - there are no pause capability bits - so the
resolved pause can only come from the attached PHY via the tx_pause/rx_pause
arguments of mac_link_up().

permit_pause_to_mac is true by default, since phylink_create() initialises
link_config.pause to MLO_PAUSE_AN and phylink_major_config() passes:

    err = phylink_pcs_config(pl->pcs, pl->pcs_neg_mode, state,
                             !!(pl->link_config.pause & MLO_PAUSE_AN));

so an SGMII port with in-band enabled ends up with PAUSE_AN set. At the same
time yt921x_port_up() now programs the software-resolved bits with a masked
update whose mask does not contain YT921X_PORT_CTRL_PAUSE_AN:

    mask = YT921X_PORT_SPEED_M | YT921X_PORT_TX_MAC_EN |
           YT921X_PORT_RX_MAC_EN | YT921X_PORT_TX_PAUSE |
           YT921X_PORT_RX_PAUSE | YT921X_PORT_DUPLEX_FULL |
           YT921X_PORT_CTRL_LINK_AN;

so PAUSE_AN stays set while TX_PAUSE/RX_PAUSE are written from software.
Does that leave flow control taking its value from in-band fields that SGMII
never transmits?

mvpp2_gmac_pcs_config() only applies permit_pause_to_mac in the 802.3z branch
and explicitly not for SGMII, for the same reason.

> +	res = yt921x_reg_update_bits(priv, YT921X_PORTn_CTRL(port), mask, ctrl);
> +	if (res)
> +		return res;

[ ... ]

> +static unsigned int
> +yt921x_phylink_pcs_inband_caps(struct phylink_pcs *pcs,
> +			       phy_interface_t interface)
> +{
> +	return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
> +}
> +
> +static void
> +yt921x_phylink_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			     struct phylink_link_state *state)
> +{

[ ... ]

> +	res = yt921x_intif_read(priv, port, MII_BMSR, &bmsr);
> +	if (res)
> +		goto out;
> +
> +	res = yt921x_intif_read(priv, port, MII_LPA, &lpa);
> +
> +out:
> +	mutex_unlock(&priv->reg_lock);
> +
> +	if (res) {
> +		dev_err(dev, "Failed to %s PCS port %d: %i\n", "get state of",
> +			port, res);
> +		state->link = false;
> +		return;
> +	}

[Severity: Low]
Would dev_err_ratelimited(), or logging only on a state change, be better
here?

yt921x_dsa_setup() registers the PCS with pp->pcs.poll = true, and phylink
arms a one second timer for MLO_AN_INBAND ports:

drivers/net/phy/phylink.c:phylink_pcs_poll_start() {
	if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND)
		mod_timer(&pl->link_poll, jiffies + HZ);
}

phylink_resolve() also calls phylink_mac_pcs_get_state() a second time when
the link reads down. If the internal MDIO/SMI access keeps failing (bus
wedged, chip held in reset), does this print the same message roughly twice
per second per SerDes port indefinitely?

The error paths in yt921x_phylink_pcs_an_restart() and
yt921x_phylink_pcs_disable() are bounded by administrative events, but this
one is on a periodic path.

> +
> +	phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
> +}

[Severity: Medium]
Can a 100BASE-X port ever come up with this decode path?

PHY_INTERFACE_MODE_100BASEX is set in pp.pcs.supported_interfaces by
yt921x_dsa_setup(), yt921x_phylink_mac_select_pcs() returns this PCS for it,
and yt921x_serdes_config() programs YT921X_SERDES_MODE_100BASEX, but
phylink_mii_c22_pcs_decode_state() has no case for it:

drivers/net/phy/phylink.c:phylink_mii_c22_pcs_decode_state() {
	state->link = !!(bmsr & BMSR_LSTATUS);
	...
	switch (state->interface) {
	case PHY_INTERFACE_MODE_1000BASEX:
	...
	case PHY_INTERFACE_MODE_2500BASEX:
	...
	case PHY_INTERFACE_MODE_SGMII:
	...
	default:
		state->link = false;
		break;
	}
}

so the BMSR_LSTATUS bit just read from MII_BMSR is discarded and the link is
forced down on every poll.

One way to reach this without any in-band DT setting is an SFP cage on a
SerDes port: phylink_sfp_config_optical() picks
PHY_INTERFACE_MODE_100BASEX for a 100BASE-FX module and
phylink_sfp_set_config() selects MLO_AN_INBAND, after which
phylink_resolve() uses pcs_get_state() as the only link source.

On a related note, yt921x_phylink_pcs_inband_caps() ignores its interface
argument and reports LINK_INBAND_ENABLE for 100BASE-X as well.
phylink_get_inband_type() returns INBAND_NONE for that mode, so
BMCR_ANENABLE is not actually armed, but
phylink_validate_pcs_inband_autoneg() consults these caps, so
"ethtool -s ... autoneg on" is accepted for a mode with no in-band
negotiation. Should the 100BASE-X case return LINK_INBAND_DISABLE only, or
be dropped from supported_interfaces until the state decoding handles it?

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