* Re: [PATCH net-next v4 5/9] net: dsa: lan9645x: add bridge support
2026-05-06 1:46 ` [PATCH net-next v4 5/9] net: dsa: lan9645x: add bridge support Jakub Kicinski
@ 2026-05-12 7:24 ` Jens Emil Schulz Ostergaard
0 siblings, 0 replies; 2+ messages in thread
From: Jens Emil Schulz Ostergaard @ 2026-05-12 7:24 UTC (permalink / raw)
To: Jakub Kicinski
Cc: UNGLinuxDriver, andrew, olteanv, davem, edumazet, pabeni, horms,
robh, krzk+dt, conor+dt, woojung.huh, linux, Steen.Hegelund,
daniel.machon, linux-kernel, netdev, devicetree
On Tue, 2026-05-05 at 18:46 -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 patch adds hardware offload support for a single bridge device in
> the lan9645x DSA driver. It introduces the fwd_domain_lock mutex and an
> ordered workqueue, and wires up the bridge-integration switch ops.
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index 6fd66ea67cfd..70f6a11f0753 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > @@ -67,7 +67,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);
> > }
>
> [ ... ]
>
> > @@ -133,6 +135,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,
>
> [ ... ]
>
> > @@ -239,6 +243,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;
> > +
>
> Does this error path leak the fwd_domain_lock initialization and the
> NPI port setup?
>
> When alloc_ordered_workqueue() fails, lan9645x_setup() returns -ENOMEM
> directly. In DSA, dsa_switch_setup() handles a failed ds->ops->setup()
> by jumping past the teardown label:
>
> err = ds->ops->setup(ds);
> if (err < 0)
> goto unregister_notifier;
>
> So lan9645x_teardown() is never invoked on the setup failure path, and
> the mutex_destroy(&lan9645x->fwd_domain_lock) and
> lan9645x_npi_port_deinit() calls it contains are skipped.
>
> With CONFIG_DEBUG_MUTEXES or lockdep enabled, the mutex debug state is
> left dangling against devm-managed memory, and the NPI port hardware
> configuration performed by lan9645x_tag_npi_setup() is left in place
> until the next reset_switch().
>
> Should the failure path of lan9645x_setup() unwind the mutex_init()
> and lan9645x_tag_npi_setup() it performed earlier?
>
I will make sure to unwind this in the error path.
> > ds->mtu_enforcement_ingress = true;
> > ds->assisted_learning_on_cpu_port = true;
> > ds->fdb_isolation = true;
> > @@ -257,6 +266,272 @@ 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(msecs ? age_secs : 0),
> > + ANA_AUTOAGE_AGE_PERIOD,
> > + lan9645x, ANA_AUTOAGE);
> > + return 0;
> > +}
>
> Can this silently truncate user-supplied ageing times that exceed the
> hardware field?
>
> ANA_AUTOAGE_AGE_PERIOD is GENMASK(20, 1), so the maximum representable
> age_secs is 2^20 - 1 = 1048575. ANA_AUTOAGE_AGE_PERIOD_SET() expands
> to FIELD_PREP(), whose runtime path in include/linux/bitfield.h is:
>
> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)
>
> which masks the value without any range check or warning.
>
> The driver does not populate ds->ageing_time_min or ds->ageing_time_max,
> and dsa_switch_ageing_time() gates its -ERANGE checks on those being
> non-zero:
>
> if (ds->ageing_time_min && ageing_time < ds->ageing_time_min)
> return -ERANGE;
> if (ds->ageing_time_max && ageing_time > ds->ageing_time_max)
> return -ERANGE;
>
> so a large ageing time set via "ip link set br0 type bridge ageing_time
> <large>" reaches this callback and is silently truncated to a short
> period. Should lan9645x set ageing_time_min/max so out-of-range values
> are rejected instead?
>
That is a good idea, I will initialize ageing_time_max.
> [ ... ]
>
> > +static void lan9645x_host_flood_work_fn(struct work_struct *work)
> > +{
> > + struct lan9645x_port *p = container_of(work, struct lan9645x_port,
> > + host_flood_work);
> > + struct lan9645x *lan9645x = p->lan9645x;
> > +
> > + mutex_lock(&lan9645x->fwd_domain_lock);
> > + __lan9645x_port_mark_host_flood(lan9645x, p->chip_port,
> > + p->host_flood_uc, p->host_flood_mc);
> > + __lan9645x_port_set_host_flood(lan9645x);
> > + mutex_unlock(&lan9645x->fwd_domain_lock);
> > +}
> > +
> > +/* 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_port *p;
> > +
> > + p = lan9645x_to_port(lan9645x, port);
> > +
> > + p->host_flood_uc = uc;
> > + p->host_flood_mc = mc;
> > + queue_work(lan9645x->owq, &p->host_flood_work);
> > +}
>
> Can the worker observe a mixed (uc, mc) tuple that no caller requested?
>
> The atomic-context writer stores p->host_flood_uc and p->host_flood_mc
> as two independent unsynchronized writes and then queues the work. The
> worker takes fwd_domain_lock and reads both, but the writer never takes
> that lock (it cannot — it is called in atomic context, and fwd_domain_lock
> is a mutex).
>
> If a second update arrives between the worker's two reads, the worker
> can program PGID_UC/MC/MCIPV4/MCIPV6 on the CPU port with a combination
> that was never requested by either call. The state converges on the
> next work run triggered by the second queue_work(), but the hardware is
> transiently programmed with a tuple nobody asked for.
>
> Would it be safer to bundle both booleans into a single atomic word, or
> to use a spinlock that can be taken in atomic context to protect the
> pair of stores?
>
Yes I will pack these into an u8 and use {READ,WRITE}_ONCE.
> > +
> > +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)
>
> [ ... ]
^ permalink raw reply [flat|nested] 2+ messages in thread