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

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

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 vlan support
> 
> Add support for vlanaware bridge. We reserve vid 4095 for standalone
> mode, to implement fdb-isolation. A vlan-unaware bridge uses vid 0.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index 599e589c4ec3..046c95a72242 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> [ ... ]
> > @@ -535,11 +536,54 @@ 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);
> 
> In other places, the driver avoids calling egress functions on the npi
> port because it conflicts with cpu port header extraction. Since this
> unconditionally applies the vlan configuration to both ingress and egress,
> could this overwrite the npi port's configuration when the state is
> synchronized?
> 

It is true we avoid this, but the vlan_filtering is not called by DSA core for
the cpu port, only user ports.

> > +
> > +     return 0;
> > +}
> > +
> > +static int lan9645x_port_vlan_add(struct dsa_switch *ds, int port,
> [ ... ]
> > 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 000000000000..0459932a626c
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> [ ... ]
> > +static void lan9645x_vlan_hw_wr(struct lan9645x *lan9645x, u16 vid)
> > +{
> > +     struct lan9645x_vlan *v = &lan9645x->vlans[vid];
> > +     bool cpu_dis = !(v->portmask & BIT(CPU_PORT));
> > +     u32 val;
> > +
> > +     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(v->portmask, lan9645x, ANA_VLAN_PORT_MASK);
> > +     lan_wr(VLANACCESS_CMD_WRITE, lan9645x, ANA_VLANACCESS);
> 
> Are these registers written with raw values rather than using their respective
> set macros? This circumvents the register abstraction and might risk failures
> if the hardware layout changes.
> 

Yes, they are single field registers. I can add the macro.

> > +
> > +     if (lan9645x_vlan_wait_for_completion(lan9645x))
> > +             dev_err(lan9645x->dev, "Vlan set mask failed\n");
> > +}
> [ ... ]
> > +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));
> > +
> > +     lan_rmw(REW_PORT_VLAN_CFG_PORT_TPID_SET(ETH_P_8021AD) |
> > +             REW_PORT_VLAN_CFG_PORT_VID_SET(port_vid),
> > +             REW_PORT_VLAN_CFG_PORT_TPID |
> > +             REW_PORT_VLAN_CFG_PORT_VID,
> > +             lan9645x, REW_PORT_VLAN_CFG(p->chip_port));
> > +}
> 
> Setting the port tpid to 0x88a8 causes untagged ingress frames to egress trunk
> ports with 802.1ad tags. Could this break interoperability with standard
> networks that expect 0x8100 tags?
> 

No that is not how it works. I think the comment which is left out explains this
clearly. 0x8100 tags are unaffected by this, but it means an stag is not rewritten
to a ctag.

> > +static void lan9645x_vlan_port_apply_ingress(struct lan9645x_port *p)
> > +{
> [ ... ]
> > +     /* Drop frames with multicast source address */
> > +     val = ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(1);
> > +     if (p->vlan_aware && !pvid)
> > +             /* If port is vlan-aware and tagged, drop untagged and priority
> > +              * tagged frames.
> > +              */
> > +             val |= ANA_DROP_CFG_DROP_UNTAGGED_ENA_SET(1) |
> > +                    ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_SET(1) |
> > +                    ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_SET(1);
> > +
> > +     lan_wr(val, lan9645x, ANA_DROP_CFG(p->chip_port));
> > +}
> 
> Does this overwrite the entire 32-bit register and silently clear other
> initialized bitfields? Would it be safer to use a read-modify-write here
> instead?
> 

Yes this is intentional. I prefer this over writing all fields with rmw which is
slower.

> [ ... ]
> > +static int lan9645x_vlan_cpu_add(struct lan9645x_port *p, u16 vid, bool pvid,
> > +                              bool untagged)
> > +{
> > +     struct lan9645x_vlan *v;
> > +
> > +     v = lan9645x_vlan_port_modify(p, vid, pvid, untagged);
> 
> Is it intentional that this bypasses the bounds check on the vlan id?
> 

No, I will move the cpu action below the bounds check.

> > +     v->portmask |= BIT(CPU_PORT) | BIT(p->chip_port);
> > +     lan9645x_vlan_hw_wr(p->lan9645x, vid);
> > +     lan9645x_vlan_port_apply_ingress(p);
> > +
> > +     return 0;
> > +}
> > +
> > +int lan9645x_vlan_port_add_vlan(struct lan9645x_port *p, u16 vid, bool pvid,
> > +                             bool untagged, struct netlink_ext_ack *extack)
> > +{
> [ ... ]
> > +void lan9645x_vlan_port_del_vlan(struct lan9645x_port *p, u16 vid)
> > +{
> > +     struct lan9645x *lan9645x = p->lan9645x;
> > +     struct lan9645x_vlan *v;
> > +
> > +     if (!vid)
> > +             return;
> > +
> > +     v = lan9645x_vlan_port_modify(p, vid, false, false);
> 
> There is a check for the maximum vlan id when adding a vlan, but no bounds
> check when deleting one. Could userspace use this to delete the reserved
> vid 4095 and break standalone forwarding?
> 

I will add the bounds check for delete.

> > +     v->portmask &= ~BIT(p->chip_port);
> > +
> > +     if (p->chip_port == lan9645x->npi)
> > +             v->portmask &= ~BIT(CPU_PORT);
> > +
> > +     lan9645x_vlan_hw_wr(lan9645x, vid);
> [ ... ]
> > +void lan9645x_vlan_init(struct lan9645x *lan9645x)
> > +{
> [ ... ]
> > +     for (vid = 1; vid < VLAN_N_VID; vid++)
> > +             lan9645x_vlan_hw_wr(lan9645x, vid);
> 
> If the indirect table access times out, the write function prints an error
> but does not return a status code. Will this cause the initialization loop
> to stall for the full timeout duration 4095 consecutive times if the
> hardware is unresponsive?
> 

Yes, I can return the err from vlan_hw_wr and return it early from vlan_init.

> > +
> > +     /* Set all the ports + cpu to be part of HOST_PVID and UNAWARE_PVID */
> > +     lan9645x->vlans[HOST_PVID].portmask = all_ports;