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

[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: dsa: lan9645x: add bridge support

This commit adds support for hardware offloading of the bridge in the
lan9645x DSA driver. It enables a single bridge device to be used with
the switch.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index a0908cbf89c7..599e589c4ec3 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> @@ -145,6 +155,8 @@ static int lan9645x_setup(struct dsa_switch *ds)
>  		return err;
>  	}
>  
> +	mutex_init(&lan9645x->fwd_domain_lock);
> +
>  	/* Link Aggregation Mode: NETDEV_LAG_HASH_L2 */
>  	lan_wr(ANA_AGGR_CFG_AC_SMAC_ENA |
>  	       ANA_AGGR_CFG_AC_DMAC_ENA,
[ ... ]
> @@ -240,6 +252,11 @@ static int lan9645x_setup(struct dsa_switch *ds)
>  
>  	lan9645x_port_set_tail_drop_wm(lan9645x);
>  
> +	lan9645x->owq = alloc_ordered_workqueue("%s-owq", 0,
> +						dev_name(lan9645x->dev));
> +	if (!lan9645x->owq)
> +		return -ENOMEM;
> +

Is it possible that the hardware remains initialized if the call to
alloc_ordered_workqueue fails? Because the setup callback returns an error,
the DSA core will skip the teardown callback. This would bypass
lan9645x_npi_port_deinit, potentially leaving the conduit netdev in a
corrupted state with hardware tagging enabled.

Also in lan9645x_setup, the initialization loop for PGID_ENTRIES iterates
through index 88:

for (int i = 0; i < PGID_ENTRIES; ++i)
        lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
               lan9645x, ANA_PGID_CFG(i));

Could this leave the CPU port's source configuration in an undefined state?
The CPU port is port 9, which has its source analysis register at PGID_SRC
(80) + 9 = 89, but the loop terminates before reaching it.

> @@ -258,6 +275,271 @@ static void lan9645x_port_phylink_get_caps(struct dsa_switch *ds, int port,
>  	lan9645x_phylink_get_caps(ds->priv, port, config);
>  }
>  
> +static int lan9645x_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> +{
> +	u32 age_secs = max(1, msecs / MSEC_PER_SEC / 2);
> +	struct lan9645x *lan9645x = ds->priv;
> +
> +	/* Entry is must suffer two aging scans before it is removed, so it is
> +	 * aged after 2*AGE_PERIOD, and the unit is in seconds.
> +	 * An age period of 0 disables automatic aging.
> +	 */
> +	lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(age_secs),
> +		ANA_AUTOAGE_AGE_PERIOD,
> +		lan9645x, ANA_AUTOAGE);
> +	return 0;
> +}

Does this function properly disable automatic aging when requested? When the
Linux bridge passes msecs = 0 to disable aging, the max macro evaluates to 1.
Since writing 1 configures an aggressive 2-second aging period, it appears
this might not fulfill the user's intent.

[ ... ]
> +static void __lan9645x_port_set_host_flood(struct lan9645x *lan9645x, int port,
> +					   bool uc, bool mc)
> +{
> +	bool mc_ena, uc_ena;
> +
> +	lockdep_assert_held(&lan9645x->fwd_domain_lock);
> +
> +	/* We want promiscuous and all_multi to affect standalone ports, for
> +	 * debug and test purposes.
> +	 *
> +	 * However, the linux bridge is incredibly eager to put bridged ports in
> +	 * promiscuous mode.
> +
> +	 * This is unfortunate since lan9645x flood masks are global and not per
> +	 * ingress port. When some port triggers unknown uc/mc to the CPU, the
> +	 * traffic from any port is forwarded to the CPU.
> +	 *
> +	 * If the host CPU is weak, this can cause tremendous stress. Therefore,
> +	 * we compromise by ignoring this host flood request for bridged ports.
> +	 */
> +	if (lan9645x_port_is_bridged(lan9645x_to_port(lan9645x, port)))
> +		return;

Will ignoring the host flood request for bridged ports break IPv6 Neighbor
Discovery? IPv6 ND relies on multicast, and the PGID_MC and PGID_MCIPV6 masks
do not include the CPU port by default. By ignoring the bridge's request to
enable multicast flooding, the CPU port may fail to receive ICMPv6 ND packets.

[ ... ]
> +/* Called in atomic context */
> +static void lan9645x_port_set_host_flood(struct dsa_switch *ds, int port,
> +					 bool uc, bool mc)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct lan9645x_host_flood_work *w;
> +
> +	w = kzalloc_obj(*w, GFP_ATOMIC);

Is kzalloc_obj an available kernel function? Using it might cause a build
error, as the standard API would be kzalloc(sizeof(*w), GFP_ATOMIC).

> +	if (!w)
> +		return;

Are there consequences to returning silently when the allocation fails?
If this allocation fails under memory pressure, the network stack will still
assume the configuration was successfully applied, leaving the hardware state
out of sync.

[ ... ]
> +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)
> +{
[ ... ]
> +	/* The bridge puts ports in IFF_ALLMULTI before calling
> +	 * port_bridge_join, so clean up before the port is marked as bridged.
> +	 */
> +	__lan9645x_port_set_host_flood(lan9645x, port, false, false);
> +	lan9645x->bridge_mask |= BIT(p->chip_port);
> +
> +	mutex_unlock(&lan9645x->fwd_domain_lock);
> +
> +	/* Later: stp_state_set updates forwarding */
> +
> +	return 0;
> +}

Could clearing the host flood masks here lead to lost hardware state when the
port leaves the bridge? If a user previously enabled IFF_PROMISC or
IFF_ALLMULTI on the standalone port, the software netdev flags remain set.
When lan9645x_port_bridge_leave is called, it does not restore the hardware's
promiscuous or multicast state to match the unchanged software flags.

> +static void lan9645x_port_bridge_stp_state_set(struct dsa_switch *ds, int port,
> +					       u8 state)
> +{
> +	struct lan9645x *lan9645x;
> +	struct lan9645x_port *p;
> +	bool learn_ena;
> +
> +	lan9645x = ds->priv;
> +	p = lan9645x_to_port(lan9645x, port);
> +
> +	mutex_lock(&lan9645x->fwd_domain_lock);
> +
> +	p->stp_state = state;
> +
> +	if (state == BR_STATE_FORWARDING)
> +		lan9645x->bridge_fwd_mask |= BIT(p->chip_port);
> +	else
> +		lan9645x->bridge_fwd_mask &= ~BIT(p->chip_port);
> +
> +	learn_ena = (state == BR_STATE_LEARNING ||
> +		     state == BR_STATE_FORWARDING) && p->learn_ena;
> +
> +	lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena),
> +		ANA_PORT_CFG_LEARN_ENA, lan9645x,
> +		ANA_PORT_CFG(p->chip_port));
> +
> +	lan9645x_update_fwd_mask(lan9645x);
> +	mutex_unlock(&lan9645x->fwd_domain_lock);
> +}

Are ingress data frames correctly dropped for ports in non-forwarding STP
states? When the port is removed from bridge_fwd_mask,
lan9645x_update_fwd_mask calculates the source PGID mask as
bridge_mask & bridge_fwd_mask & ~BIT(dp->index).

This evaluates to the set of all other forwarding ports in the bridge. Since
ingress forwarding is not explicitly disabled on the port (for example, by
setting ANA_PORT_CFG_PORTID_VAL to NONE), data frames received on a blocking
or learning port can enter the analyzer and be forwarded.

> +static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
> +				       struct dsa_bridge bridge)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct lan9645x_port *p;
> +
> +	p = lan9645x_to_port(lan9645x, port);
> +
> +	mutex_lock(&lan9645x->fwd_domain_lock);
> +
> +	lan9645x->bridge_mask &= ~BIT(p->chip_port);
> +
> +	/* Last port leaving clears bridge dev */
> +	if (!lan9645x->bridge_mask)
> +		lan9645x->bridge = NULL;
> +
> +	lan9645x_update_fwd_mask(lan9645x);
> +
> +	mutex_unlock(&lan9645x->fwd_domain_lock);
> +}

Should p->learn_ena be reset when a port leaves the bridge?

During port_bridge_flags, p->learn_ena is set to true. When leaving the
bridge, this flag remains true. The DSA core will transition the leaving
port to BR_STATE_FORWARDING, which calls lan9645x_port_bridge_stp_state_set
and leaves hardware learning enabled. This can pollute the shared FDB with
MAC addresses from standalone ports, leading to silent packet drops if
bridged ports attempt to forward traffic to them.