Re: [PATCH v4 7/7] net: dsa: mediatek: support EN751221 switch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] net: dsa: mt7530: missing MT7530_CHG_TRAP setup for ID_EN751221
- [High] net: dsa: mt7530: recursive reg_mutex lockdep class in MMIO-MDIO hierarchy
- [Medium] net: dsa: mt7530: swapped default_taps in en751221_trgmii_pair_setup()
- [Medium] net: dsa: mt7530: missing child_bus mdio_lock in core_write() and core_rmw()
- [Medium] net: dsa: mt7530: race with peer phylink in mt7530_cal_interswitch_trgmii()
--

commit fcbb4fdaccb60b7ebd8ef125ae65f04744031511
Author: Caleb James DeLisle <cjd@cjdns.fr>

net: dsa: mediatek: support EN751221 switch

This patch adds support for the EcoNet EN751221 on-die MT7530 switch and the
optional external MCM MT7530 switch, along with startup calibration for the
TRGMII link connecting them.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 24e4485624d39..447c80455460e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -49,6 +49,9 @@ static const struct mt7530_mib_desc mt7530_mib[] = {
>  static int
>  mt753x_ctrl_phy_addr(struct mt7530_priv *priv)
>  {
> +	if (priv->id == ID_EN751221)
> +		return 12;
> +
>  	if (WARN_ON_ONCE(!priv->mdiodev))
>  		return 0;
>  
> @@ -349,12 +352,19 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>  	regmap_read(priv->regmap, MT753X_MTRAP, &xtal);
>  	xtal &= MT7530_XTAL_MASK;
>  
> +	/* EN751221 on-die does not report clock speed in MTRAP */
> +	if (priv->id == ID_EN751221)
> +		xtal = MT7530_XTAL_25MHZ;
> +
>  	if (xtal == MT7530_XTAL_25MHZ)
>  		ssc_delta = 0x57;
>  	else
>  		ssc_delta = 0x87;
>  
> -	if (priv->id == ID_MT7621) {
> +	if (priv->id == ID_EN751221_EXT || priv->id == ID_EN751221) {
> +		/* PLL frequency: 362.5Mhz */
> +		ncpo1 = 0x1d00;
> +	} else if (priv->id == ID_MT7621) {

[Severity: Medium]
When mt7530_mac_config() calls mt7530_setup_port6() on ID_EN751221, it invokes
core_clear(), core_write(), and core_set() on control PHY address 12.

On the MMIO-based ID_EN751221 switch, priv->bus is NULL and core_write() and
core_rmw() fall back to priv->child_bus:

drivers/net/dsa/mt7530.c:mt7530_mutex_lock() {
	if (priv->bus)
		mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
}

drivers/net/dsa/mt7530.c:core_write() {
	...
	if (!bus)
		bus = priv->child_bus;

	if (WARN_ON_ONCE(!bus))
		return;

	mt7530_mutex_lock(priv);

	/* Write the desired MMD Devad */
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);
	...
}

Because mt7530_mutex_lock() and mt7530_mutex_unlock() only check priv->bus,
priv->child_bus->mdio_lock is not acquired when priv->bus is NULL. While
mt7531_ind_c22_phy_read() and mt7531_ind_c22_phy_write() acquire
priv->reg_mutex for each individual Clause 22 transfer, can the multi-step
MII_MMD_CTRL / MII_MMD_DATA sequence and read-modify-write in core_write() and
core_rmw() interleave with concurrent MDIO transactions on priv->child_bus
(such as phylib polling or regmap accesses to the downstream ID_EN751221_EXT
switch)?

[ ... ]

> @@ -471,6 +481,251 @@ mt7531_pll_setup(struct mt7530_priv *priv)
>  	usleep_range(25, 35);
>  }
>  
> +static bool
> +en751221_trgmii_cal_ok(struct mt7530_priv *rx, u32 reg)
> +{
> +	u32 val = 0;
> +
> +	regmap_read(rx->regmap, reg, &val);
> +	regmap_write(rx->regmap, reg, val | EDGE_CHK);
> +	regmap_write(rx->regmap, reg, val & ~EDGE_CHK);
> +	regmap_read(rx->regmap, reg, &val);
> +
> +	return FIELD_GET(RD_VALUE_MASK, val) == TGMII_TD_PAT &&
> +	       !FIELD_GET(RD_ERR_MASK, val);
> +}
> +
> +static void
> +en751221_trgmii_calibrate_direction(struct mt7530_priv *tx,
> +				    struct mt7530_priv *rx,
> +				    u8 *default_taps)
> +{

[ ... ]

> +		if (best_tap > -1) {
> +			regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
> +					   RD_TAP(best_tap));
> +
> +			log[best_tap] = 'X';
> +
> +			dev_info(rx->dev,
> +				 "TRGMII lane %d: %s tap %d old %d\n",
> +				 channel, log, best_tap, default_taps[channel]);
> +		} else {
> +			regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
> +					   RD_TAP(default_taps[channel]));
> +
> +			dev_warn(rx->dev,
> +				 "TRGMII lane %d: %s calibration failed, preserving tap %u\n",
> +				 channel, log, default_taps[channel]);
> +		}

[ ... ]

> +static void
> +en751221_trgmii_pair_setup(struct mt7530_priv *ext, int ext_port,
> +			   struct mt7530_priv *ondie, int ondie_port)
> +{

[ ... ]

> +	/* BOTH: Collect and then zero every RX TAP */
> +	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++) {
> +		reg_val = 0;
> +		regmap_read(ext->regmap, MT7530_TRGMII_RD(channel), &reg_val);
> +		default_taps_ext[channel] = FIELD_GET(RD_TAP_MASK, reg_val);
> +		regmap_clear_bits(ext->regmap, MT7530_TRGMII_RD(channel),
> +				  RD_TAP_MASK);
> +
> +		reg_val = 0;
> +		regmap_read(ondie->regmap, MT7530_TRGMII_RD(channel), &reg_val);
> +		default_taps_ondie[channel] = FIELD_GET(RD_TAP_MASK, reg_val);
> +		regmap_clear_bits(ondie->regmap, MT7530_TRGMII_RD(channel),
> +				  RD_TAP_MASK);
> +	}
> +
> +	en751221_trgmii_calibrate_direction(ondie, ext, default_taps_ondie);
> +	en751221_trgmii_calibrate_direction(ext, ondie, default_taps_ext);

[Severity: Medium]
Are default_taps_ondie and default_taps_ext swapped in these two calls to
en751221_trgmii_calibrate_direction()?

In en751221_trgmii_calibrate_direction(tx, rx, default_taps), default_taps is
used to log the previous RX tap value on rx->dev and to restore rx->regmap's
MT7530_TRGMII_RD(channel) register if calibration fails. Passing
default_taps_ondie when rx is ext (and default_taps_ext when rx is ondie) uses
the peer switch's saved RX taps instead of rx's own saved taps.

> +
> +	dev_info(ondie->dev, "TRGMII inter-switch link initialized\n");
> +}
> +
> +static void
> +mt7530_cal_interswitch_trgmii(struct dsa_port *dp)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +	struct dsa_switch *peer_ds;
> +	struct dsa_port *peer_dp;
> +	struct mt7530_priv *peer;
> +	struct mt7530_priv *priv;
> +	struct dsa_link *dl;
> +	u32 pmsr = 0;
> +
> +	priv = ds->priv;
> +
> +	if (priv->id != ID_EN751221_EXT)
> +		return;
> +
> +	if (dp->type != DSA_PORT_TYPE_DSA)
> +		return;
> +
> +	regmap_read(priv->regmap, MT7530_PMSR_P(dp->index), &pmsr);
> +
> +	if (!(pmsr & PMSR_LINK))
> +		return;
> +
> +	list_for_each_entry(dl, &ds->dst->rtable, list) {
> +		if (dl->dp != dp)
> +			continue;
> +
> +		peer_dp = dl->link_dp;
> +
> +		if (peer_dp->type != DSA_PORT_TYPE_DSA)
> +			continue;
> +
> +		peer_ds = dl->link_dp->ds;
> +
> +		if (!of_device_is_compatible(peer_ds->dev->of_node,
> +					     "econet,en751221-switch"))
> +			continue;
> +
> +		peer = peer_ds->priv;
> +
> +		if (peer->id != ID_EN751221)
> +			continue;
> +
> +		en751221_trgmii_pair_setup(priv, dp->index, peer,
> +					   peer_dp->index);
> +	}
> +}

[Severity: Medium]
Can mt7530_cal_interswitch_trgmii() race with the peer port's phylink_resolve()
work item?

When dsa_tree_setup_ports() enables the DSA ports on both switches, each port
schedules its own phylink_resolve() work item holding only its own
dp->pl->state_mutex. In mt753x_phylink_mac_link_up(), priv->regmap's
MT753X_PMCR_P(dp->index) has just been written with PMCR_FORCE_LNK right
before calling mt7530_cal_interswitch_trgmii(dp), so checking PMSR_LINK on
priv->regmap checks the local port rather than peer->regmap's
MT7530_PMSR_P(peer_dp->index).

While en751221_trgmii_pair_setup() forces MT753X_PMCR_P(ondie_port) down and
sleeps in usleep_range(5000, 6000) twice during TX/RX reset, could peer_dp's
phylink_resolve() run concurrently on another worker and call
mt753x_phylink_mac_link_up() on ondie_port mid-reset? Also, when
en751221_trgmii_pair_setup() writes mcr_up to MT753X_PMCR_P on both ports,
does it overwrite PMCR_FORCE_TX_FC_EN and PMCR_FORCE_RX_FC_EN configured by
mt753x_phylink_mac_link_up()?

> @@ -1459,7 +1714,8 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>  
>  	mutex_unlock(&priv->reg_mutex);
>  
> -	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
> +	    priv->id != ID_EN751221 && priv->id != ID_EN751221_EXT)
>  		return 0;
>  
>  	if (port == 5)
> @@ -1486,7 +1742,8 @@ mt7530_port_disable(struct dsa_switch *ds, int port)
>  
>  	mutex_unlock(&priv->reg_mutex);
>  
> -	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
> +	    priv->id != ID_EN751221 && priv->id != ID_EN751221_EXT)
>  		return;

[Severity: High]
Do the updates to MT753X_MTRAP for ID_EN751221 in mt7530_port_enable(),
mt7530_port_disable(), mt7530_setup_port5(), and mt7531_setup_common() take
effect without MT7530_CHG_TRAP being set?

In mt753x_table, ID_EN751221 uses mt7988_setup() -> mt7531_setup_common().
Unlike mt7530_setup(), which sets MT7530_CHG_TRAP in MT753X_MTRAP after
resetting the switch so that trap bits can be modified by software,
mt7988_setup() asserts and deasserts priv->rstc and then calls
mt7531_setup_common() without setting MT7530_CHG_TRAP in MT753X_MTRAP.

[ ... ]

> @@ -3817,6 +4122,28 @@ const struct mt753x_info mt753x_table[] = {
>  		.phy_write_c45 = mt7531_ind_c45_phy_write,
>  		.mac_port_get_caps = en7528_mac_port_get_caps,
>  	},
> +	[ID_EN751221] = {
> +		.id = ID_EN751221,
> +		.pcs_ops = &mt7530_pcs_ops,
> +		.sw_setup = mt7988_setup,
> +		.phy_read_c22 = mt7531_ind_c22_phy_read,
> +		.phy_write_c22 = mt7531_ind_c22_phy_write,
> +		.phy_read_c45 = mt7531_ind_c45_phy_read,
> +		.phy_write_c45 = mt7531_ind_c45_phy_write,
> +		.mac_port_get_caps = en751221_mac_port_get_caps,
> +		.mac_port_config = mt7530_mac_config,
> +	},
> +	[ID_EN751221_EXT] = {
> +		.id = ID_EN751221_EXT,
> +		.pcs_ops = &mt7530_pcs_ops,
> +		.sw_setup = mt7530_setup,
> +		.phy_read_c22 = mt7530_phy_read_c22,
> +		.phy_write_c22 = mt7530_phy_write_c22,
> +		.phy_read_c45 = mt7530_phy_read_c45,
> +		.phy_write_c45 = mt7530_phy_write_c45,
> +		.mac_port_get_caps = mt7530_mac_port_get_caps,
> +		.mac_port_config = mt7530_mac_config,
> +	},

[Severity: High]
When ID_EN751221_EXT is registered as an MDIO device on ID_EN751221's internal
MDIO bus (ondie->child_bus), both switches initialize priv->reg_mutex in
mt7530_probe_common() with the same lockdep class key.

When an operation on the downstream ID_EN751221_EXT switch holds
ext->reg_mutex and accesses ext->regmap, the regmap MDIO callbacks invoke the
parent switch's child_bus operations, which acquire ondie->reg_mutex:

mt7530_setup() (on ext)
  mt7530_port_disable()
    mutex_lock(&ext->reg_mutex)
    regmap_update_bits(ext->regmap, ...)
      mt7530_regmap_write()
        bus->write() (on ondie->child_bus)
          mt753x_phy_write_c22()
            mt7531_ind_c22_phy_write()
              mutex_lock(&ondie->reg_mutex)

Will this nested acquisition of ondie->reg_mutex while ext->reg_mutex is held
trigger a lockdep recursive locking warning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029184202.1859843-1-cjd@cjdns.fr?part=7