Re: [PATCH net-next v3 7/8] net: dsa: soce: Add VLAN offload support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: dsa: soce: needs_standalone_vlan_filtering drops standalone traffic after bridge leave
- [High] net: dsa: soce: VLAN isolation bypass due to missing hardware rollback on write failure
- [Medium] net: dsa: soce: -EOPNOTSUPP fails to reject unsupported VLAN configurations
--

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

net: dsa: soce: Add VLAN offload support
    
Add VLAN filtering and membership offload for switch configurations that
implement the Port VLAN synthesis option.

> diff --git a/drivers/net/dsa/soce/soce_dsa_core.c b/drivers/net/dsa/soce/soce_dsa_core.c
> index d391b11b94ad6..f93ce9da8400c 100644
> --- a/drivers/net/dsa/soce/soce_dsa_core.c
> +++ b/drivers/net/dsa/soce/soce_dsa_core.c

[ ... ]

> @@ -162,6 +200,178 @@ static void soce_sw_disable_tagging(struct soce_dsa_local *local)

[ ... ]

> +static int soce_vlan_write_entry(struct soce_priv *priv, u16 vid)
> +{
> +	struct soce_dsa_local *local = &priv->local;
> +	void __iomem *base = local->base_addr;
> +	u32 cpu_ports;
> +	u32 untagged;
> +	u32 members;
> +	int ret;
> +
> +	/* The CPU port must be a tagged member of every active VLAN so
> +	 * tagged frames can reach the conduit.
> +	 */
> +	cpu_ports = dsa_cpu_ports(&priv->ds);
> +	members = priv->vlan_members[vid];
> +	if (members)
> +		members |= cpu_ports;
> +	untagged = priv->vlan_untagged[vid] & ~cpu_ports;
> +
> +	soce_vlan_select_vid(local, vid);
> +	writel(members, base + SOCE_VLAN_MEMBER_PORTS_OFFSET);
> +	writel(SOCE_VLAN_MEMBER_CTRL_WRITE,
> +	       base + SOCE_VLAN_MEMBER_CTRL_OFFSET);
> +	ret = soce_vlan_wait_for_write(local, SOCE_VLAN_MEMBER_CTRL_OFFSET,
> +				       SOCE_VLAN_MEMBER_CTRL_WRITE);
> +	if (ret)
> +		return ret;
> +
> +	writel(untagged, base + SOCE_VLAN_UNTAG_PORTS_OFFSET);
> +	writel(SOCE_VLAN_UNTAG_CTRL_WRITE,
> +	       base + SOCE_VLAN_UNTAG_CTRL_OFFSET);
> +
> +	return soce_vlan_wait_for_write(local, SOCE_VLAN_UNTAG_CTRL_OFFSET,
> +					SOCE_VLAN_UNTAG_CTRL_WRITE);
> +}

[ ... ]

> +static int soce_vlan_setup(struct dsa_switch *ds)
> +{
> +	struct soce_priv *priv = ds->priv;
> +	struct soce_dsa_local *local;
> +	struct dsa_port *dp;
> +	int ret;
> +
> +	local = &priv->local;
> +
> +	if (!priv->features.port_vlan)
> +		return 0;
> +
> +	ret = soce_vlan_reset(local);
> +	if (ret) {
> +		dev_err(ds->dev, "failed to reset VLAN configuration: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* Default every port to PVID 1, unfiltered, so standalone
> +	 * forwarding keeps working before any bridge VLAN is configured.
> +	 */
> +	scoped_guard(mutex, &priv->vlan_lock) {
> +		dsa_switch_for_each_available_port(dp, ds) {
> +			priv->port_pvid[dp->index] = 1;
> +			soce_vlan_config_port(priv, dp->index, false);
> +		}
> +		soce_vlan_set_enabled(local, true);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> @@ -224,6 +443,130 @@ static enum dsa_tag_protocol soce_get_tag_protocol(struct dsa_switch *ds,

[ ... ]

> +static int soce_port_vlan_add(struct dsa_switch *ds, int port,
> +			      const struct switchdev_obj_port_vlan *vlan,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct soce_priv *priv = ds->priv;
> +	u32 port_mask = BIT(port);
> +	u32 *untagged_ports;
> +	u32 old_untagged;
> +	u32 old_members;
> +	bool untagged;
> +	u32 *members;
> +	int ret;
> +
> +	untagged_ports = priv->vlan_untagged;
> +	members = priv->vlan_members;
> +
> +	if (!priv->features.port_vlan) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Port VLAN support is not implemented in the switch core");
> +		return -EOPNOTSUPP;
> +	}

[Severity: Medium]
Since -EOPNOTSUPP is explicitly masked by the switchdev and DSA cores, will
this return value be ignored instead of rejecting the configuration? It
looks like this would allow the operation to succeed in the network stack,
causing hardware to silently strip VLAN tags on CPU ingress without
including them in the DSA tag, which could break the network configuration.

> +
> +	if (!vlan->vid)
> +		return 0;
> +
> +	untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +
> +	scoped_guard(mutex, &priv->vlan_lock) {
> +		old_members = members[vlan->vid];
> +		old_untagged = untagged_ports[vlan->vid];
> +
> +		members[vlan->vid] |= port_mask;
> +		if (untagged)
> +			untagged_ports[vlan->vid] |= port_mask;
> +		else
> +			untagged_ports[vlan->vid] &= ~port_mask;
> +
> +		ret = soce_vlan_write_entry(priv, vlan->vid);
> +		if (ret) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "failed to update VLAN hardware tables");
> +			dev_err(ds->dev,
> +				"failed to add VLAN %u on port %d: %d\n",
> +				vlan->vid, port, ret);
> +			members[vlan->vid] = old_members;
> +			untagged_ports[vlan->vid] = old_untagged;
> +			return ret;
> +		}

[Severity: High]
If the second write for the untagged mask in soce_vlan_write_entry() fails
and returns an error, the first write for the member mask has already been
committed to the hardware. 

Is it safe to only rollback the software state here? By leaving the hardware
member mask modified, the switch might forward frames for this VLAN to the
port, leading to a VLAN isolation bypass while the OS believes the port is
isolated.

[ ... ]

> +static int soce_port_vlan_filtering(struct dsa_switch *ds, int port,
> +				    bool vlan_filtering,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct soce_priv *priv = ds->priv;
> +
> +	if (!priv->features.port_vlan) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Port VLAN support is not implemented in the switch core");
> +		return -EOPNOTSUPP;
> +	}

[Severity: Medium]
Similarly to soce_port_vlan_add(), will returning -EOPNOTSUPP here fail to
reject unsupported VLAN configurations since it gets masked by the DSA core?

[ ... ]

> @@ -278,8 +623,35 @@ static int soce_sw_probe(struct platform_device *pdev)

[ ... ]

> +	/* Force VLAN uppers always through the callbacks, so cores without
> +	 * Port VLAN feature can reject them instead of silently dropping
> +	 * VLAN frames.
> +	 */
> +	ds->needs_standalone_vlan_filtering = true;

[Severity: High]
Does enabling needs_standalone_vlan_filtering cause complete loss of standalone
network traffic when a port leaves a bridge?

When a port is removed from a VLAN-unaware bridge, the DSA core will call
dsa_port_vlan_filtering(dp, true), forcing the hardware into ingress filtering
mode.

Since the bridge flushes VLANs on leave, the port is removed from the hardware
member mask for VLAN 1. However, earlier in soce_vlan_setup(), the driver
assigns PVID 1 but never adds the port to the hardware member mask for VLAN 1.
Will all subsequent untagged standalone traffic be classified to PVID 1 and
dropped by the ingress filter because the port is no longer a member?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-devel-vstrassheim-soce-dsa-ml-v3-0-ddebafcb9ba7@linutronix.de?part=7