Re: [PATCH net-next v13 5/9] net: dsa: lan9645x: add vlan support

[Jens Emil Schulz Ostergaard]

On Fri, 2026-10-02 at 21:14 +0000, netdev-bot+sashiko@xxxxxxxxxx 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 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] On a VLAN-aware bridged port, frames the host sends without a
>   tag can leave the switch tagged, sometimes with the wrong VID.
> - [Medium] On VLAN-aware ports, lan9645x_vlan_port_apply_ingress() sets
>   ANA_VLAN_CFG_VLAN_POP_CNT=1, so the hardware removes the outer VLAN…
> - [Low] The commit message says the patch adds "VLAN support for VLAN-
>   aware and VLAN-unaware bridges", but none of the bridged code paths…
> - [Low] Errors from VLAN table programming are not passed back to
>   callers.
> 
> --
> 
> net: dsa: lan9645x: add vlan support
> 
> This commit adds VLAN table management to the lan9645x DSA driver. It
> configures ports for VLAN-aware and VLAN-unaware bridges, and reserves
> HOST_PVID (4095) for standalone ports. It also registers the
> port_vlan_filtering, port_vlan_add and port_vlan_del ops, all serialized
> by a new fwd_domain_lock.
> 
> > Add VLAN support for VLAN-aware and VLAN-unaware bridges.
> 
> [Severity: Low]
> Does this description match what the commit does on its own?
> 
> At this point in the series nothing writes lan9645x->bridge_mask, and
> lan9645x_switch_ops has no .port_bridge_join. That means
> lan9645x_port_is_bridged() always returns false:
> 
> static inline bool lan9645x_port_is_bridged(struct lan9645x_port *p)
> {
>         return p->lan9645x->bridge_mask & BIT(p->chip_port);
> }
> 
> It also means DSA never offloads a bridge, so .port_vlan_filtering,
> .port_vlan_add and .port_vlan_del are never called for user ports. The
> only thing that takes effect here is the standalone HOST_PVID setup done
> from .port_setup.
> 
> Bridge offload, and clearing HOST_PVID membership on bridge join, arrive
> with the later "net: dsa: lan9645x: add bridge support" patch. That
> ordering is only explained in the v12 changelog below the "---", and git
> am drops that part.
> 
> Could the commit message say that the bridged paths only become
> reachable once the bridge support patch is applied?
> 
> > 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..6d28a7cf1442b
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> > @@ -0,0 +1,402 @@
> 
> [ ... ]
> 
> > +static void
> > +lan9645x_vlan_port_apply_egress(struct lan9645x_port *p,
> > +                             struct lan9645x_vlan_port_info *info)
> > +{
> > +     struct lan9645x *lan9645x = p->lan9645x;
> > +     enum lan9645x_vlan_port_tag tag_cfg;
> > +     u16 port_vid = UNAWARE_PVID;
> > +
> > +     if (!lan9645x_port_is_bridged(p)) {
> > +             tag_cfg = LAN9645X_TAG_NO_PVID_NO_UNAWARE;
> > +             port_vid = HOST_PVID;
> > +     } else if (p->vlan_aware) {
> 
> [ ... ]
> 
> > +             if (info->untagged == 1 && info->tagged) {
> > +                     tag_cfg = LAN9645X_TAG_NO_PVID_NO_UNAWARE;
> > +                     port_vid = info->untagged_vid;
> > +             } else if (info->untagged) {
> > +                     tag_cfg = LAN9645X_TAG_DISABLED;
> > +             } else {
> > +                     tag_cfg = LAN9645X_TAG_ALL;
> > +             }
> 
> [Severity: Medium]
> Can untagged frames from the host leave a VLAN-aware bridged port tagged,
> and with the wrong VID?
> 
> For an untagged host frame on a VLAN-filtering bridge,
> lan9645x_xmit_get_vlan_info() in net/dsa/tag_lan9645x.c puts the bridge
> device's pvid into the IFH:
> 
>         } else {
>                 rcu_read_lock();
>                 br_vlan_get_pvid_rcu(br, &tci);
>                 rcu_read_unlock();
>                 *vlan_tci = tci;
>         }
> 
> The rewriter mode here, however, is chosen from the port's own untagged
> VID:
> 
> - With one untagged VLAN X plus tagged VLANs, TAG_NO_PVID_NO_UNAWARE with
>   PORT_VID=X leaves only VID X or VID 0 untagged.
> - With no untagged VLANs, TAG_ALL tags every frame, including VID 0.
> 
> The two sides only agree when br0's pvid equals the port's untagged VID.
> Two examples where they don't:
> 
> (a) swp0 has "vid 10 pvid untagged" plus tagged vid 20, and br0 self
> pvid is 1. br_handle_vlan() strips host traffic on br0.10 for egress on
> swp0, and the same happens to VLAN 10 traffic forwarded in software from
> a foreign bridge port. The tagger then puts VID 1 in the IFH, so the
> frame goes out tagged with VID 1 instead of untagged in VLAN 10.
> 
> (b) On a trunk port with no untagged VLANs, untagged frames go out tagged
> with br0's pvid. If br0 has no pvid, they go out priority-tagged with
> VID 0. STP BPDUs from br_send_bpdu() and LLDP sent on swp0 are examples.
> 
> This depends on the rewriter using IFH.TCI as the classified VID when
> BYPASS=1, and on the TAG_CFG semantics described in the
> lan9645x_vlan_port_tag comment. I could not check either against the
> datasheet. ocelot and tag_ocelot use a similar pairing, so this may be a
> known limitation.
> 
> The code becomes reachable once the bridge support patch lands, and it
> is unchanged at the end of the series. Also, LAN9645X_TAG_NO_UNAWARE is
> defined but never used.
> 

Correct, this is the same issue as the one raised on the tag driver
patch, and the rewriter does use IFH.TCI as the classified VID with
BYPASS set. v14 fixes both sides: the tag driver no longer uses the
bridge pvid, and ports with only tagged VLANs use TAG_NO_UNAWARE instead
of TAG_ALL.


> > +     } else {
> > +             tag_cfg = LAN9645X_TAG_DISABLED;
> > +     }
> 
> [ ... ]
> 
> > +static void lan9645x_vlan_port_apply_ingress(struct lan9645x_port *p)
> > +{
> 
> [ ... ]
> 
> > +     val = ANA_VLAN_CFG_VLAN_VID_SET(pvid) |
> > +           ANA_VLAN_CFG_VLAN_TAG_TYPE_SET(0);
> > +     if (p->vlan_aware)
> > +             val |= ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
> > +                    ANA_VLAN_CFG_VLAN_POP_CNT_SET(1);
> 
> [Severity: Medium]
> Does popping the outer tag here conflict with the pvid check in
> lan9645x_rcv()?
> 
> With VLAN_POP_CNT=1 on VLAN-aware ports, the classified VID survives
> only in the IFH. lan9645x_rcv() in net/dsa/tag_lan9645x.c skips
> restoring it when it equals the port pvid:
> 
>                 if ((vlan_tci & VLAN_VID_MASK) != port_pvid)
>                         __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
>                                                vlan_tci);
> 
> That is fine for untagged frames. Consider a double-tagged frame
> [0x8100 VID 100][0x8100 VID 200] that arrives on a VLAN-aware port with
> pvid 100 and is sent to the CPU (broadcast, host flood or trap). The
> hardware classifies it to VID 100 and pops the outer tag. IFH TCI is
> 100, which equals the pvid, so no hwaccel tag is set.
> 
> The software bridge then finds the inner tag 200 in the payload and puts
> the frame in VLAN 200, while the hardware forwarded it in VLAN 100.
> 
> Could this cause local delivery (for example to br0.200), or software
> forwarding in a different VLAN than the hardware used?
> br_allowed_ingress() still restricts this to VIDs the port is a member
> of.
> 

Correct, same issue as on the tag driver patch. v14 restores the classified VID
as a hwaccel tag whenever a tag was popped, so in the example the
inner tag 200 stays in the payload and the bridge classifies the frame
to VLAN 100, as the hardware did. This also preserves the PCP of
priority tagged frames and of frames tagged with the pvid.

> [ ... ]
> 
> > +int lan9645x_vlan_port_add_vlan(struct lan9645x_port *p, u16 vid, bool pvid,
> > +                             bool untagged, struct netlink_ext_ack *extack)
> > +{
> 
> [ ... ]
> 
> > +     lan9645x_vlan_hw_wr(lan9645x, vid);
> > +     lan9645x_vlan_port_apply_ingress(p);
> > +     lan9645x_vlan_port_apply_egress(p, &info);
> > +
> > +     return 0;
> > +}
> 
> [Severity: Low]
> Should the return value of lan9645x_vlan_hw_wr() be checked here, before
> the software state is committed?
> 
> This path updates lan9645x->vlans[] and p->pvid, reprograms the port
> ingress and egress configuration, and returns 0 even when the table
> write failed. lan9645x_vlan_port_del_vlan(), lan9645x_vlan_cpu_add(),
> lan9645x_vlan_cpu_del() and lan9645x_vlan_set_hostmode() do the same.
> 
> Inside lan9645x_vlan_hw_wr(), the ANA_VLANTIDX, ANA_VLAN_PORT_MASK and
> ANA_VLANACCESS writes go through lan_wr(), which only does WARN_ON_ONCE()
> on a regmap error. The engine is also not checked for IDLE before a new
> command is issued.
> 
> When this fails, the shadow table and the hardware can diverge while
> switchdev believes the change succeeded. For the CPU port, DSA caches the
> VID as installed, so later adds of the same VID only bump a refcount and
> never restore the missing membership.
> 
> The comment in lan9645x_vlan_hw_wr() says a timeout most likely means
> the register bus is dead, which limits the practical impact. lan_wr()
> swallowing errors also predates this patch.
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2%40microchip.com