Re: [PATCH net-next v11 6/9] net: dsa: lan9645x: add vlan support

On Thu, 2026-08-06 at 14:38 +0000, [email protected] wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Code Fix needed in lan9645x_port_vlan_filtering to explicitly ignore the NPI port, preventing REW_TAG_CFG corruption.
> 
> Pre-existing issues:
> - [High] The MAC table dump function `lan9645x_mact_dsa_dump` acquires a sleepable mutex and performs sleep-polling within an atomic RCU read-side critical section.
> --
> 
> --- Patch [6]: [PATCH net-next v11 6/9] net: dsa: lan9645x: add vlan support ---
> commit 7eb00c9a8e8dbdc6e13b0734d6fc7a8d54af0360
> Author: Jens Emil Schulz Østergaard <[email protected]>
> Subject: net: dsa: lan9645x: add vlan support
> 
> This commit adds VLAN support for VLAN-aware and VLAN-unaware bridges.
> VID 4095 is reserved for standalone ports and VID 0 is used for
> VLAN-unaware bridge ports to isolate their FDB domains.
> 
> > 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
> [ ... ]
> > @@ -576,11 +580,52 @@ static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
> [ ... ]
> > +static int lan9645x_port_vlan_filtering(struct dsa_switch *ds, int port,
> > +                                     bool enabled,
> > +                                     struct netlink_ext_ack *extack)
> > +{
> > +     struct lan9645x *lan9645x = ds->priv;
> > +     struct lan9645x_port *p;
> > +
> > +     p = lan9645x_to_port(lan9645x, port);
> > +     p->vlan_aware = enabled;
> > +     lan9645x_vlan_port_apply(p);
> 
> [Severity: High]
> Does this code inadvertently apply standard VLAN configurations to the NPI
> (CPU) port?
> 
> When the bridge's VLAN filtering state changes, the DSA core propagates the
> change to all ports in the bridge tree, including the CPU port. Here,


It does not. .port_vlan_filtering is only ever called for a user port.

There is a single invocation in the core, in dsa_port_vlan_filtering():

      err = ds->ops->port_vlan_filtering(ds, dp->index, vlan_filtering,
                                         extack);

and its callers all pass the port whose bridge membership or switchdev
attribute changed: the SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING handler,
dsa_port_switchdev_sync_attrs() on bridge join, and
dsa_port_reset_vlan_filtering() on bridge leave. Never a CPU or DSA
port.

In the vlan_filtering_is_global case the core iterates
dsa_switch_for_each_user_port() and calls
dsa_user_manage_vlan_filtering(), which takes a struct net_device *user
and only flips NETIF_F_HW_VLAN_CTAG_FILTER and walks vlan_for_each(). It
does not re-enter the driver op.


> lan9645x_port_vlan_filtering() unconditionally executes
> lan9645x_vlan_port_apply() which proceeds to modify the egress tagging.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> [ ... ]
> > +static void
> > +lan9645x_vlan_port_apply_egress(struct lan9645x_port *p,
> > +                             struct lan9645x_vlan_port_info *info)
> > +{
> [ ... ]
> > +     lan_rmw(REW_TAG_CFG_TAG_TPID_CFG_SET(3) |
> > +             REW_TAG_CFG_TAG_CFG_SET(tag_cfg),
> > +             REW_TAG_CFG_TAG_TPID_CFG |
> > +             REW_TAG_CFG_TAG_CFG,
> > +             lan9645x, REW_TAG_CFG(p->chip_port));
> 
> [Severity: High]
> Is it safe to rewrite the egress tag configuration for the NPI port here?


The NPI port never reaches this function. Every path to
lan9645x_vlan_port_apply_egress() either diverts the NPI port first or
is a user-port-only DSA op:

- lan9645x_vlan_port_add_vlan() and _del_vlan() check p->chip_port ==
  lan9645x->npi and divert to lan9645x_vlan_cpu_add() /
  lan9645x_vlan_cpu_del(). Those deliberately call only
  lan9645x_vlan_port_apply_ingress(), never the egress half, precisely
  because the CPU port needs its VLAN classification updated for host
  VLANs while REW must be left as lan9645x_npi_port_init() configured
  it. That is why the apply step is split in two.

- lan9645x_vlan_set_hostmode() is called from
  lan9645x_port_bridge_leave() and from lan9645x_port_setup(). The
  latter is called for the CPU port but it is guarded by
   p->chip_port != lan9645x->npi

- lan9645x_vlan_clear_hostmode() is called only from
  lan9645x_port_bridge_join(), and the NPI port is never bridged.

- lan9645x_port_vlan_filtering() is only ever invoked by the core for a
  user port, as mentioned above.


> 
> Writing the dynamically evaluated egress tag configuration to the
> REW_TAG_CFG register erases the mandatory LAN9645X_TAG_DISABLED
> configuration that is normally set during CPU port initialization.
> 
> If IFH extraction and packet rewriting conflict, a pushed VLAN tag could
> overwrite the end of the extraction header with the top 4 bytes of the DMAC,
> breaking CPU traffic. Should lan9645x_port_vlan_filtering() explicitly
> ignore the NPI port?
> 
> [Severity: High]
> This is a pre-existing issue, but does the MAC table dump function sleep
> while holding rcu_read_lock()?
> 
> A user-space request to dump the Forwarding Database (e.g., via
> bridge fdb show) invokes the netlink rtnl_fdb_dump() handler. This handler
> takes rcu_read_lock() and iterates over devices, calling ndo_fdb_dump().
> DSA passes this down to the driver's lan9645x_mact_dsa_dump().
> 
> Inside the RCU critical section, the driver acquires mact_lock (a sleepable
> mutex) and uses lan9645x_mac_wait_for_completion() to poll the hardware,
> which uses a regmap poll with a non-zero sleep interval:
> 
> drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c:lan9645x_mact_dsa_dump() {
>         ...
>         mutex_lock(&lan9645x->mact_lock);
>         ...
>         err = lan9645x_mac_wait_for_completion(lan9645x, &maca);
>         ...
> }
> 
> Could this trigger a "scheduling while atomic" kernel panic or cause a
> deadlock?
> 

This is a false positive. No RCU is held here. The RCU section inside br_fdb_dump
at net/bridge/br_fdb.c:1062 takes rcu_read_lock() for its own br->fdb_list walk
and drops it before returning.

So RCU is not held when the ports ndo_fdb_dump is called, and dsa_user_fdb_dump
is never nested inside the bridges rcu critical section.

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a@microchip.com?part=6