Re: [PATCH net-next v2 6/9] net: dsa: lan9645x: add vlan support
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-03-29 19:56:35
Also in:
linux-devicetree, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -535,11 +536,54 @@ static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ + return 0; +} + +static int lan9645x_port_vlan_add(struct dsa_switch *ds, int port,
[ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ + if (lan9645x_vlan_wait_for_completion(lan9645x)) + dev_err(lan9645x->dev, "Vlan set mask failed\n"); +}
[ ... ]
quoted hunk ↗ jump to hunk
+static void +lan9645x_vlan_port_apply_egress(struct lan9645x_port *p, + struct lan9645x_vlan_port_info *info) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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?
+static void lan9645x_vlan_port_apply_ingress(struct lan9645x_port *p)
+{[ ... ]
quoted hunk ↗ jump to hunk
+ /* 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? [ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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) +{
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ + /* Set all the ports + cpu to be part of HOST_PVID and UNAWARE_PVID */ + lan9645x->vlans[HOST_PVID].portmask = all_ports;