Re: [PATCH net-next v4 8/8] net: dsa: soce: Add VLAN membership table support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net: dsa: soce: broken VID 1 untagging in
  swip_vlan_write_entry()
--

commit 25c6cf2ff3f136587d102371c25a636e1af52a26
Author: Vasilij Strassheim <v.strassheim@linutronix.de>

net: dsa: soce: Add VLAN membership table support

This commit adds VLAN membership table programming for SoC-e switch cores
that implement the Port VLAN synthesis option, initializing VLAN-capable
cores with PVID 1 and updating per-VID hardware membership and untagging
masks on VLAN add and delete operations.

> diff --git a/drivers/net/dsa/soce-swip.c b/drivers/net/dsa/soce-swip.c
> index 70fc2da91dd25..7cc3f4abc691d 100644
> --- a/drivers/net/dsa/soce-swip.c
> +++ b/drivers/net/dsa/soce-swip.c
[ ... ]
> @@ -179,6 +212,185 @@ static void swip_stp_disable(struct swip_dsa_local *local)
[ ... ]
> +static void swip_vlan_config_port(struct swip_priv *priv, int port)
> +{
> +	struct swip_dsa_local *local = &priv->local;
> +	void __iomem *base = local->vlan;
> +	u32 regval;
> +
> +	swip_vlan_select_port(local, port);
> +	writel(SWIP_VLAN_PORT_MODE_HYBRID, base + SWIP_VLAN_PORT_MODE_OFFSET);
> +	writel(SWIP_VLAN_PORT_TYPE_UNAWARE, base + SWIP_VLAN_PORT_TYPE_OFFSET);
> +	writel(FIELD_PREP(SWIP_VLAN_PORT_VLAN_PVID_MASK,
> +			  SWIP_STANDALONE_PVID),
> +	       base + SWIP_VLAN_PORT_VLAN_OFFSET);
> +	writel(0, base + SWIP_VLAN_PORT_INGR_FILTER_OFFSET);
> +	writel(SWIP_VLAN_PORT_INGR_ACCEPT_ALL,
> +	       base + SWIP_VLAN_PORT_INGR_ACCEPT_OFFSET);
> +
> +	regval = readl(base + SWIP_VLAN_PORT_EGR_TAG_OFFSET);
> +	regval &= ~SWIP_VLAN_PORT_EGR_TAG_MASK;
> +	regval |= FIELD_PREP(SWIP_VLAN_PORT_EGR_TAG_MASK,
> +			     SWIP_VLAN_PORT_EGR_TAG_UNTAG_PORT);
> +	writel(regval, base + SWIP_VLAN_PORT_EGR_TAG_OFFSET);
> +}
> +
> +static int swip_vlan_write_entry(struct swip_priv *priv, u16 vid)
> +{
> +	struct swip_dsa_local *local = &priv->local;
> +	void __iomem *base = local->vlan;
> +	struct dsa_port *dp;
> +	u32 standalone = 0;
> +	u32 cpu_ports;
> +	u32 untagged;
> +	u32 members;
> +	int ret;
> +
> +	cpu_ports = dsa_cpu_ports(&priv->ds);
> +	/* Bridge VLAN deletion must not remove standalone membership. */
> +	if (vid == SWIP_STANDALONE_PVID) {
> +		dsa_switch_for_each_user_port(dp, &priv->ds) {
> +			if (!dsa_port_bridge_dev_get(dp))
> +				standalone |= BIT(dp->index);
> +		}
> +	}
> +	members = priv->vlan_members[vid] | standalone;
> +	/* CPU membership preserves VLAN delivery to the conduit. */
> +	if (members)
> +		members |= cpu_ports;
> +	/* Explicit VLAN configuration takes precedence over default untagging. */
> +	untagged = standalone & ~priv->vlan_members[vid] & ~cpu_ports;

[Severity: High]
Does this clear hardware VID 1 untagging whenever a port joins a software
bridge (or adds a VID 1 VLAN upper), causing all untagged frames transmitted
out of that port to egress tagged with VID 1 on the wire?

In swip_vlan_config_port(), every port including the CPU port is configured
with SWIP_VLAN_PORT_TYPE_UNAWARE, PVID = SWIP_STANDALONE_PVID (1), and
SWIP_VLAN_PORT_EGR_TAG_UNTAG_PORT. When the CPU transmits an untagged frame
via sdsa_xmit(), SDSA_TAG_VLAN_BIT is not set, so the switch classifies the
frame into the CPU port's PVID (VID 1) and strips the tag on egress from a
user port only if BIT(port) is set in SWIP_VLAN_UNTAG_PORTS_OFFSET for VID 1.

Because swip_switch_ops does not implement .port_bridge_join, joining a
Linux bridge takes the software fallback path:

  dsa_user_changeupper()
    -> dsa_port_bridge_join()
      -> dsa_switch_bridge_join() -> returns -EOPNOTSUPP
      -> dsa_port_bridge_destroy() -> sets dp->bridge = NULL

As a result, dp->bridge is always NULL and dsa_port_bridge_dev_get(dp) in
swip_vlan_write_entry() is always NULL for every port.

When br_add_if() -> nbp_vlan_init() -> nbp_vlan_add() adds the bridge's
default_pvid (VID 1, untagged), __vlan_vid_add() sees that the port is not
switchdev-offloaded and falls back to vlan_vid_add(dev, br->vlan_proto, 1).
Because swip_probe() sets ds->needs_standalone_vlan_filtering =
priv->features.port_vlan, the user port has NETIF_F_HW_VLAN_CTAG_FILTER set,
so vlan_vid_add() calls:

  dsa_user_vlan_rx_add_vid()
    -> dsa_port_vlan_add()
      -> swip_port_vlan_add()

with vid = 1 and flags = 0 (since ndo_vlan_rx_add_vid does not carry
BRIDGE_VLAN_INFO_UNTAGGED).

swip_port_vlan_add() sets BIT(port) in priv->vlan_members[1], and
swip_vlan_write_entry() then computes:

  untagged = standalone & ~priv->vlan_members[vid] & ~cpu_ports;

which clears BIT(port) in SWIP_VLAN_UNTAG_PORTS_OFFSET for VID 1. Once that
bit is cleared, untagged frames sent on the bridge or port are classified
into VID 1 at the CPU port and egress the user port with an 802.1Q VID 1 tag
instead of untagged.

Conversely, because dsa_port_bridge_dev_get(dp) is always NULL, deleting
VID 1 from the bridged port (for example via bridge vlan del dev swp0 vid 1)
clears BIT(port) in priv->vlan_members[1], which leaves BIT(port) set in
standalone and restores the port to both SWIP_VLAN_MEMBER_PORTS_OFFSET and
SWIP_VLAN_UNTAG_PORTS_OFFSET for VID 1.

Also, because dp->bridge is always NULL and
ds->needs_standalone_vlan_filtering is false when !priv->features.port_vlan,
can swip_port_vlan_add() or swip_port_vlan_del() ever be reached when
priv->features.port_vlan is false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-devel-vstrassheim-soce-dsa-ml-v4-0-02f3cde2afa1@linutronix.de?part=8