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

[Vasilij Strassheim]

On Thu, 2026-09-24 at 10:40 +0000, sashiko-bot@xxxxxxxxxx wrote:
> 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@xxxxxxxxxxxxx>
> 
> 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.
> 

The error is propagated through dsa_port_vlan_add(). In my tests VLAN
upper configuration was rejected in this path. Anyway, I will use
-EINVAL instead to avoid exceptions.

> > +
> > +	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.
> 

Yes, this might become a problem.
I will fix this with best-effort rollback to the previous configuration
(like mxl862xx), and also check that both table commands have completed
before modifying the selector or data registers.

> [ ... ]
> 
> > +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?

Same as above, I will change it to -EINVAL.

> 
> [ ... ]
> 
> > @@ -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?
> 

Yes, I will fix this by preserving standalone VLAN membership and
restoring the default PVID when the port leaves the bridge.