Add support for hardware offloading of the bridge. We support a single
bridge device.
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
Signed-off-by: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
---
Changes in v2:
- variable name consistency
- port_set_learning use stp_state before writing to hw
- add set_host_flood for selftests, which need promic/all_multi on
standalone interfaces
---
drivers/net/dsa/microchip/lan9645x/lan9645x_main.c | 291 +++++++++++++++++++++
drivers/net/dsa/microchip/lan9645x/lan9645x_main.h | 18 ++
2 files changed, 309 insertions(+)
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
@@ -28,6 +28,14 @@ static const char *lan9645x_resource_names[NUM_TARGETS + 1] = {
[NUM_TARGETS] = NULL,
};
+struct lan9645x_host_flood_work {
+ struct work_struct work;
+ struct lan9645x *lan9645x;
+ int port;
+ bool uc;
+ bool mc;
+};
+
static int lan9645x_tag_npi_setup(struct dsa_switch *ds)
{
struct dsa_port *dp, *first_cpu_dp = NULL;
@@ -61,7 +69,9 @@ static void lan9645x_teardown(struct dsa_switch *ds)
{
struct lan9645x *lan9645x = ds->priv;
+ destroy_workqueue(lan9645x->owq);
lan9645x_npi_port_deinit(lan9645x, lan9645x->npi);
+ mutex_destroy(&lan9645x->fwd_domain_lock);
}
static int lan9645x_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
@@ -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;
+
ds->mtu_enforcement_ingress = true;
ds->assisted_learning_on_cpu_port = true;
ds->fdb_isolation = true;
@@ -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;
+}
+
+static int lan9645x_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack)
+{
+ if (flags.mask &
+ ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD))
+ return -EINVAL;
+
+ return 0;
+}
+
+static void lan9645x_port_pgid_set(struct lan9645x *lan9645x, u16 pgid,
+ int chip_port, bool enabled)
+{
+ u32 reg_msk, port_msk;
+
+ WARN_ON(chip_port > CPU_PORT);
+
+ port_msk = ANA_PGID_PGID_SET(enabled ? BIT(chip_port) : 0);
+ reg_msk = ANA_PGID_PGID_SET(BIT(chip_port));
+
+ lan_rmw(port_msk, reg_msk, lan9645x, ANA_PGID(pgid));
+}
+
+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));
+}
+
+static int lan9645x_port_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags f,
+ struct netlink_ext_ack *extack)
+{
+ struct lan9645x *lan9645x = ds->priv;
+
+ if (WARN_ON(port == lan9645x->npi))
+ return -EINVAL;
+
+ if (f.mask & BR_LEARNING)
+ lan9645x_port_set_learning(lan9645x, port,
+ !!(f.val & BR_LEARNING));
+
+ if (f.mask & BR_FLOOD)
+ lan9645x_port_pgid_set(lan9645x, PGID_UC, port,
+ !!(f.val & BR_FLOOD));
+
+ if (f.mask & BR_MCAST_FLOOD) {
+ bool ena = !!(f.val & BR_MCAST_FLOOD);
+
+ lan9645x_port_pgid_set(lan9645x, PGID_MC, port, ena);
+ lan9645x_port_pgid_set(lan9645x, PGID_MCIPV4, port, ena);
+ lan9645x_port_pgid_set(lan9645x, PGID_MCIPV6, port, ena);
+ }
+
+ if (f.mask & BR_BCAST_FLOOD)
+ lan9645x_port_pgid_set(lan9645x, PGID_BC, port,
+ !!(f.val & BR_BCAST_FLOOD));
+
+ return 0;
+}
+
+static void lan9645x_update_fwd_mask(struct lan9645x *lan9645x)
+{
+ struct lan9645x_port *p;
+ struct dsa_port *dp;
+
+ lockdep_assert_held(&lan9645x->fwd_domain_lock);
+
+ /* Updates the source port PGIDs, making sure frames from p
+ * are only forwarded to ports q != p, where q is relevant to forward
+ */
+ dsa_switch_for_each_available_port(dp, lan9645x->ds) {
+ u32 mask = 0;
+
+ p = lan9645x_to_port(lan9645x, dp->index);
+
+ if (lan9645x_port_is_bridged(p)) {
+ mask = lan9645x->bridge_mask &
+ lan9645x->bridge_fwd_mask & ~BIT(dp->index);
+ }
+
+ lan_wr(mask, lan9645x, ANA_PGID(PGID_SRC + dp->index));
+ }
+}
+
+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;
+
+ if (uc)
+ lan9645x->host_flood_uc_mask |= BIT(port);
+ else
+ lan9645x->host_flood_uc_mask &= ~BIT(port);
+
+ if (mc)
+ lan9645x->host_flood_mc_mask |= BIT(port);
+ else
+ lan9645x->host_flood_mc_mask &= ~BIT(port);
+
+ uc_ena = !!lan9645x->host_flood_uc_mask;
+ lan9645x_port_pgid_set(lan9645x, PGID_UC, CPU_PORT, uc_ena);
+
+ mc_ena = !!lan9645x->host_flood_mc_mask;
+ lan9645x_port_pgid_set(lan9645x, PGID_MC, CPU_PORT, mc_ena);
+ lan9645x_port_pgid_set(lan9645x, PGID_MCIPV4, CPU_PORT, mc_ena);
+ lan9645x_port_pgid_set(lan9645x, PGID_MCIPV6, CPU_PORT, mc_ena);
+}
+
+static void lan9645x_host_flood_work_fn(struct work_struct *work)
+{
+ struct lan9645x_host_flood_work *w =
+ container_of(work, struct lan9645x_host_flood_work, work);
+
+ mutex_lock(&w->lan9645x->fwd_domain_lock);
+ __lan9645x_port_set_host_flood(w->lan9645x, w->port, w->uc, w->mc);
+ mutex_unlock(&w->lan9645x->fwd_domain_lock);
+ kfree(w);
+}
+
+/* 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);
+ if (!w)
+ return;
+
+ INIT_WORK(&w->work, lan9645x_host_flood_work_fn);
+ w->lan9645x = lan9645x;
+ w->port = port;
+ w->uc = uc;
+ w->mc = mc;
+ queue_work(lan9645x->owq, &w->work);
+}
+
+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);
+
+ if (lan9645x->bridge && lan9645x->bridge != bridge.dev) {
+ NL_SET_ERR_MSG_MOD(extack, "Only one bridge supported");
+ return -EBUSY;
+ }
+
+ mutex_lock(&lan9645x->fwd_domain_lock);
+ /* First bridged port sets bridge dev */
+ if (!lan9645x->bridge_mask)
+ lan9645x->bridge = bridge.dev;
+
+ /* 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;
+}
+
+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);
+}
+
+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);
+}
+
static const struct dsa_switch_ops lan9645x_switch_ops = {
.get_tag_protocol = lan9645x_get_tag_protocol,
@@ -271,6 +553,15 @@ static const struct dsa_switch_ops lan9645x_switch_ops = {
/* MTU */
.port_change_mtu = lan9645x_change_mtu,
.port_max_mtu = lan9645x_get_max_mtu,
+
+ /* Bridge integration */
+ .set_ageing_time = lan9645x_set_ageing_time,
+ .port_pre_bridge_flags = lan9645x_port_pre_bridge_flags,
+ .port_bridge_flags = lan9645x_port_bridge_flags,
+ .port_bridge_join = lan9645x_port_bridge_join,
+ .port_bridge_leave = lan9645x_port_bridge_leave,
+ .port_stp_state_set = lan9645x_port_bridge_stp_state_set,
+ .port_set_host_flood = lan9645x_port_set_host_flood,
};
static int lan9645x_request_target_regmaps(struct lan9645x *lan9645x)
diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
index d8bdcb8a92ed..42048e16bb47 100644
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
@@ -155,6 +155,11 @@ struct lan9645x {
struct dsa_switch *ds;
struct regmap *rmap[NUM_TARGETS];
+ u32 host_flood_uc_mask;
+ u32 host_flood_mc_mask;
+
+ struct workqueue_struct *owq;
+
int shared_queue_sz;
/* NPI chip_port */
@@ -163,6 +168,12 @@ struct lan9645x {
u8 num_phys_ports;
struct lan9645x_port **ports;
+ /* Forwarding Database */
+ struct net_device *bridge; /* Only support single bridge */
+ u16 bridge_mask; /* Mask for bridged ports */
+ u16 bridge_fwd_mask; /* Mask for forwarding bridged ports */
+ struct mutex fwd_domain_lock; /* lock forwarding configuration */
+
int num_port_dis;
bool dd_dis;
bool tsn_dis;
@@ -172,6 +183,8 @@ struct lan9645x_port {
struct lan9645x *lan9645x;
u8 chip_port;
+ u8 stp_state;
+ bool learn_ena;
bool rx_internal_delay;
bool tx_internal_delay;
@@ -224,6 +237,11 @@ static inline struct lan9645x_port *lan9645x_to_port(struct lan9645x *lan9645x,
return lan9645x->ports[port];
}
+static inline bool lan9645x_port_is_bridged(struct lan9645x_port *p)
+{
+ return p && (p->lan9645x->bridge_mask & BIT(p->chip_port));
+}
+
static inline struct regmap *lan_tgt2rmap(struct lan9645x *lan9645x,
enum lan9645x_target t, int tinst)
{
--
2.52.0
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.
On Sun, 2026-03-29 at 12:56 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> 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.
I do not think it is common practice to undo switch register writes in the
error path here. We reset the switch registers in .setup. If we exit
here there is nothing to teardown.
>
> 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.
>
No as I mentioned in another mail this OBEY_VLAN does not make sense for
pgid 89.
> > @@ -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.
>
Yes that is right. I will change it to:
lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(msecs ? age_secs : 0),
ANA_AUTOAGE_AGE_PERIOD,
lan9645x, ANA_AUTOAGE);
> [ ... ]
> > +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.
>
No that is handled by other mechanisms. There are explicit handles to make sure
such traffic reaches the cpu.
> [ ... ]
> > +/* 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.
>
But we can not return any error in the dsa op to the network stack, so I think
this is our only option.
> [ ... ]
> > +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.
>
Yes this is a bug. I will split flood requests and register writes, and make sure
the requested state is restored on bridge leave.
> > +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.
>
Yes this is a bug, I need to use a zero mask for the src pgid when a port is
in non-forwarding STP state.
> > +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.
No I believe DSA core takes care of this with dsa_port_clear_brport_flags.
© 2016 - 2026 Red Hat, Inc.