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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Tue, 2026-08-11 at 12:32 +0200, Paolo Abeni 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.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> --- Patch [6]: [PATCH net-next v11 6/9] net: dsa: lan9645x: add vlan support ---
> commit 48ea20348797be1134c296e1f56d305fbc50bbe3
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
> 
> net: dsa: lan9645x: add vlan support
> 
> This adds a new lan9645x_vlan.c implementing port_vlan_filtering,
> port_vlan_add and port_vlan_del, backed by a software shadow of the
> hardware VLAN table in lan9645x->vlans[]. VID 4095 is reserved as the
> port VLAN of standalone ports and VID 0 is used for VLAN-unaware bridge
> ports.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index c145d28ffbb99..3281726284dc2 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> 
> [ ... ]
> 
> > @@ -521,6 +524,7 @@ static int lan9645x_port_bridge_join(struct dsa_switch *ds, int port,
> > 
> >       lan9645x->bridge_mask |= BIT(p->chip_port);
> >       __lan9645x_port_set_host_flood(lan9645x);
> > +     lan9645x_vlan_clear_hostmode(p);
> > 
> >       mutex_unlock(&lan9645x->fwd_domain_lock);
> > 
> 
> [ ... ]
> 
> > @@ -576,11 +580,52 @@ static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
> > +static int lan9645x_port_vlan_add(struct dsa_switch *ds, int port,
> > +                               const struct switchdev_obj_port_vlan *vlan,
> > +                               struct netlink_ext_ack *extack)
> > +{
> > +     struct lan9645x *lan9645x = ds->priv;
> > +     struct lan9645x_port *p;
> > +     bool pvid, untagged;
> > +
> > +     p = lan9645x_to_port(lan9645x, port);
> > +     pvid = !!(vlan->flags & BRIDGE_VLAN_INFO_PVID);
> > +     untagged = !!(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
> > +
> > +     return lan9645x_vlan_port_add_vlan(p, vlan->vid, pvid, untagged,
> > +                                        extack);
> > +}
> 
> [ ... ]
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
> > index a0cd1707e1149..248c4ab59f1a8 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
> 
> [ ... ]
> 
> > @@ -156,6 +157,17 @@ enum lan9645x_vlan_port_tag {
> >       LAN9645X_TAG_ALL = 3,
> >  };
> > 
> > +struct lan9645x_vlan {
> > +     u32 portmask: 10, /* ports 0-8 + CPU port module */
> > +         untagged: 9, /* ports 0-8 */
> > +         src_chk: 1,
> > +         mir: 1,
> > +         lrn_dis: 1,
> > +         prv_vlan: 1,
> > +         fld_dis: 1,
> > +         s_fwd_ena: 1;
> > +};
> > +
> 
> [ ... ]
> 
> > @@ -180,6 +192,9 @@ struct lan9645x {
> >       u16 bridge_fwd_mask; /* Mask for forwarding bridged ports */
> >       struct mutex fwd_domain_lock; /* lock forwarding configuration */
> > 
> > +     /* VLAN entries */
> > +     struct lan9645x_vlan vlans[VLAN_N_VID];
> > +
> >       int num_port_dis;
> >  };
> 
> [ ... ]
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> > index 0642b59419c25..0548904dc4bd2 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> > @@ -210,5 +210,8 @@ int lan9645x_port_setup(struct dsa_switch *ds, int port)
> >               ANA_PORT_CFG_PORTID_VAL,
> >               lan9645x, ANA_PORT_CFG(p->chip_port));
> > 
> > +     if (p->chip_port != lan9645x->npi)
> > +             lan9645x_vlan_set_hostmode(p);
> > +
> >       return 0;
> >  }
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> > new file mode 100644
> > index 0000000000000..e2116132b4d48
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> 
> [ ... ]
> 
> > +static int lan9645x_vlan_hw_wr(struct lan9645x *lan9645x, u16 vid)
> > +{
> > +     struct lan9645x_vlan *v = &lan9645x->vlans[vid];
> > +     bool cpu_dis = !(v->portmask & BIT(lan9645x->num_phys_ports));
> > +     u32 val;
> > +     int err;
> > +
> > +     val = ANA_VLANTIDX_VLAN_PGID_CPU_DIS_SET(cpu_dis) |
> > +           ANA_VLANTIDX_V_INDEX_SET(vid) |
> > +           ANA_VLANTIDX_VLAN_SEC_FWD_ENA_SET(v->s_fwd_ena) |
> > +           ANA_VLANTIDX_VLAN_FLOOD_DIS_SET(v->fld_dis) |
> > +           ANA_VLANTIDX_VLAN_PRIV_VLAN_SET(v->prv_vlan) |
> > +           ANA_VLANTIDX_VLAN_LEARN_DISABLED_SET(v->lrn_dis) |
> > +           ANA_VLANTIDX_VLAN_MIRROR_SET(v->mir) |
> > +           ANA_VLANTIDX_VLAN_SRC_CHK_SET(v->src_chk);
> > +
> > +     lan_wr(val, lan9645x, ANA_VLANTIDX);
> > +     lan_wr(ANA_VLAN_PORT_MASK_VLAN_PORT_MASK_SET(v->portmask),
> > +            lan9645x, ANA_VLAN_PORT_MASK);
> > +     lan_wr(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_WRITE),
> > +            lan9645x, ANA_VLANACCESS);
> 
> [Severity: High]
> [Finding: main-5-5]
> [Sources: opus-5]
> Should this three-register indirect transaction be serialized by a lock?
> 
> The MAC table engine in this driver is protected by mact_lock:
> 
>         struct mutex mact_lock; /* serialize mac table register access */
> 
> but the VLAN table access engine has no equivalent, and there is no
> lockdep_assert_held() here.
> 
> lan9645x_vlan_set_hostmode() is now called from .port_setup:
> 
> lan9645x_port_setup()
>     if (p->chip_port != lan9645x->npi)
>         lan9645x_vlan_set_hostmode(p)
>             lan9645x_vlan_hw_wr(p->lan9645x, HOST_PVID)
> 
> and .port_setup runs from the DSA probe path without rtnl_lock and
> without fwd_domain_lock:
> 
> net/dsa/devlink.c:dsa_port_devlink_setup() {
>         if (ds->ops->port_setup) {
>                 err = ds->ops->port_setup(ds, dp->index);
>         ...
> }
> 
> dsa_port_devlink_setup() runs at the start of dsa_port_setup(), before
> dsa_user_create() registers this port's netdev, while the netdevs of the
> ports already processed are live. So while swp1..swpN are still going
> through lan9645x_port_setup(), userspace can enslave swp0:
> 
> CPU0 (probe, no locks held)          CPU1 (rtnl, bridge join / vlan add)
> lan9645x_vlan_set_hostmode()         lan9645x_port_vlan_add()
>   lan9645x_vlan_hw_wr(.., 4095)        lan9645x_vlan_port_add_vlan()
>                                          lan9645x_vlan_hw_wr(.., 100)
> 
> Can the two transactions interleave like this?
> 
> CPU1: lan_wr(V_INDEX=100,   ANA_VLANTIDX)
> CPU0: lan_wr(V_INDEX=4095,  ANA_VLANTIDX)
> CPU0: lan_wr(mask(4095),    ANA_VLAN_PORT_MASK)
> CPU0: lan_wr(CMD_WRITE,     ANA_VLANACCESS)
> CPU1: lan_wr(mask(100),     ANA_VLAN_PORT_MASK)
> CPU1: lan_wr(CMD_WRITE,     ANA_VLANACCESS)
> 
> That would commit mask(100) into the HOST_PVID (4095) entry and leave
> vid 100 without a member mask, i.e. either standalone ports lose their
> isolation domain or the new VLAN blackholes traffic.
> 
> There is also the shadow update itself. lan9645x_vlan_set_hostmode() and
> lan9645x_vlan_clear_hostmode() both do a non-atomic read-modify-write of
> the same u32 bitfield word:
> 
>         p->lan9645x->vlans[HOST_PVID].portmask |= BIT(p->chip_port);
>         ...
>         p->lan9645x->vlans[HOST_PVID].portmask &= ~BIT(p->chip_port);
> 
> portmask:10 shares the word with untagged:9 and the flag bits, and
> lan9645x_port_bridge_join() calls lan9645x_vlan_clear_hostmode() under
> fwd_domain_lock while lan9645x_port_setup() calls
> lan9645x_vlan_set_hostmode() with no lock at all, so the lock sets do not
> intersect. Can one of these updates be lost?
> 

Yes, this is right, I will move vlan modifications under the existing
fwd_domain_lock.

> > +
> > +     /* The VLAN access engine completes in a fixed ~1us vs the polling
> > +      * timeout of 100_000 us. A timeout here therefore likely means the
> > +      * register bus itself is dead, not that the VLAN op failed. There is no
> > +      * meaningful recovery at runtime, so this function logs via dev_err()
> > +      * and runtime callers discard the return value. Only
> > +      * lan9645x_vlan_init() treats this as fatal so that probe fails early
> > +      * on a broken bus.
> > +      */
> > +     err = lan9645x_vlan_wait_for_completion(lan9645x);
> > +     if (err)
> > +             dev_err(lan9645x->dev, "Vlan set mask failed\n");
> > +
> > +     return err;
> > +}
> 
> [ ... ]
> 
> > +int lan9645x_vlan_port_add_vlan(struct lan9645x_port *p, u16 vid, bool pvid,
> > +                             bool untagged, struct netlink_ext_ack *extack)
> > +{
> 
> [ ... ]
> 
> > +     lan9645x_vlan_port_get_info(lan9645x, p->chip_port, &info);
> > +
> > +     if (info.untagged > 1 && info.tagged) {
> > +             *v = old_vlan;
> > +             p->pvid = old_pvid;
> > +             NL_SET_ERR_MSG_MOD(extack, "Only support 1 untagged port VLAN");
> > +             return -EBUSY;
> > +     }
> 
> [Severity: Low]
> [Finding: main-1-1]
> [Sources: opus-5]
> This isn't a bug, but could the commit message mention this restriction?
> 
> The commit message says only "Add VLAN support for VLAN-aware and
> VLAN-unaware bridges", while this rejects an otherwise legal bridge
> configuration: a second
> 
>         bridge vlan add dev swpN vid B untagged
> 
> fails with -EBUSY once the port already has one untagged VLAN plus at
> least one tagged VLAN.
> 
> The changelog also notes an expected selftest failure with respect to
> ctag/stag conformance, but that text is below the --- line and is dropped
> when the patch is applied, so nothing about either limitation survives in
> the tree.
> 

I can mention these restrictions in the commit message, that is a good idea.
Both are hardware limitations.

The stag/ctag limitation is that the switch considers both stagged and
ctagged frames as tagged, and the conformance requires you accept 1 as
tagged and the other as untagged.

I see ocelot has a workaround for this using the VCAP. We may be able
to do the same once the required features land, but not in this series.

> > +
> > +     lan9645x_vlan_hw_wr(lan9645x, vid);
> > +     lan9645x_vlan_port_apply_ingress(p);
> > +     lan9645x_vlan_port_apply_egress(p, &info);
> > +
> > +     return 0;
> > +}
> 
> [ ... ]
> 
> > +void lan9645x_vlan_set_hostmode(struct lan9645x_port *p)
> > +{
> > +     p->vlan_aware = false;
> > +     p->lan9645x->vlans[HOST_PVID].portmask |= BIT(p->chip_port);
> > +     lan9645x_vlan_hw_wr(p->lan9645x, HOST_PVID);
> > +     lan9645x_vlan_port_apply(p);
> > +}
> > +
> > +void lan9645x_vlan_clear_hostmode(struct lan9645x_port *p)
> > +{
> > +     p->lan9645x->vlans[HOST_PVID].portmask &= ~BIT(p->chip_port);
> > +     lan9645x_vlan_hw_wr(p->lan9645x, HOST_PVID);
> > +     lan9645x_vlan_port_apply(p);
> > +}
> 
> [ ... ]
> --
> This is an AI-generated review.
> 

Thanks,
Emil