Re: [PATCH net-next v5 5/9] net: dsa: lan9645x: add bridge support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Race condition in `lan9645x_port_set_learning` leads to desynchronized hardware learning configuration.
- [High] Stale STP forwarding state causes port to bypass STP and temporarily forward traffic upon joining a bridge, creating Layer 2 loops.
--

commit 4eb2af719b5c6bdd492ea4fe37e94ac02ff39b74
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

net: dsa: lan9645x: add bridge support

This commit adds hardware offloading support for a single bridge device
on the lan9645x switch.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> +static void lan9645x_port_set_learning(struct lan9645x *lan9645x, int port,
> +				       bool enabled)
> +{
> +	struct lan9645x_port *p = lan9645x_to_port(lan9645x, port);
> +
> +	p->learn_ena = enabled;
> +
> +	enabled = enabled && (p->stp_state == BR_STATE_LEARNING ||
> +			      p->stp_state == BR_STATE_FORWARDING);
> +
> +	lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(enabled), ANA_PORT_CFG_LEARN_ENA,
> +		lan9645x, ANA_PORT_CFG(port));
> +}

Can this race with lan9645x_port_bridge_stp_state_set() and result in
desynchronized hardware learning configuration?

lan9645x_port_set_learning() modifies p->learn_ena and reads p->stp_state
without holding lan9645x->fwd_domain_lock. Meanwhile,
lan9645x_port_bridge_stp_state_set() updates p->stp_state and reads
p->learn_ena while holding fwd_domain_lock.

Since lan9645x_port_bridge_flags() executes under rtnl_lock, but
lan9645x_port_bridge_stp_state_set() can execute asynchronously, could an
STP state change to BR_STATE_LEARNING interrupt lan9645x_port_set_learning()?

If so, the hardware ANA_PORT_CFG_LEARN_ENA register might be written with 0
(from the stale BR_STATE_BLOCKING state) immediately after the STP change
wrote 1, permanently disabling hardware learning despite software indicating
it is enabled.

[ ... ]
> +static int lan9645x_port_bridge_join(struct dsa_switch *ds, int port,
> +				     struct dsa_bridge bridge,
> +				     bool *tx_fwd_offload,
> +				     struct netlink_ext_ack *extack)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct lan9645x_port *p;
> +
> +	p = lan9645x_to_port(lan9645x, port);
> +
> +	mutex_lock(&lan9645x->fwd_domain_lock);
> +	if (lan9645x->bridge && lan9645x->bridge != bridge.dev) {
> +		mutex_unlock(&lan9645x->fwd_domain_lock);
> +		NL_SET_ERR_MSG_MOD(extack, "Only one bridge supported");
> +		return -EBUSY;
> +	}
> +
> +	/* First bridged port sets bridge dev */
> +	if (!lan9645x->bridge_mask)
> +		lan9645x->bridge = bridge.dev;
> +
> +	lan9645x->bridge_mask |= BIT(p->chip_port);
> +	__lan9645x_port_set_host_flood(lan9645x);
> +
> +	mutex_unlock(&lan9645x->fwd_domain_lock);
> +
> +	/* Later: stp_state_set updates forwarding */
> +
> +	return 0;
> +}

Does this leave the port in a stale STP forwarding state upon joining the
bridge, potentially creating Layer 2 loops?

Standalone ports are placed in BR_STATE_FORWARDING by DSA, which sets
their bit in lan9645x->bridge_fwd_mask via
lan9645x_port_bridge_stp_state_set().

When a port joins a bridge, this function adds it to lan9645x->bridge_mask
but does not clear the stale bridge_fwd_mask bit or reset its internal
stp_state.

The bridge core queues the port's initial BR_STATE_BLOCKING state
asynchronously. If another port's STP state update is processed during this
window, lan9645x_update_fwd_mask() is triggered.

Since the newly joined port is in both bridge_mask and bridge_fwd_mask,
will the hardware be immediately programmed to forward its traffic, bypassing
the STP listening/learning phases?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-dsa_lan9645x_switch_driver_base-v5-0-968fbf34ffa3@microchip.com?part=5