Re: [PATCH 20/20] net: dsa: xilinx: trap link-local control frames to the CPU port

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Mixing goto-based error handling with scoped guard cleanups in the same function.
- [Medium] Incomplete hardware state rollback on error in VLAN configuration functions.
- [Medium] Error path in xlnx_tsn_setup bypasses trap removal, leaking hardware state.
- [Medium] FDB additions and deletions for control MACs are silently ignored, deceiving the software bridge.
--

--- Patch [20]: [PATCH 20/20] net: dsa: xilinx: trap link-local control frames to the CPU port ---
commit 56b303f76151a63e86634cce7c4b44f8de4578a9
Author: Nagadheeraj Rottela <[email protected]>

net: dsa: xilinx: trap link-local control frames to the CPU port

This commit installs CAM entries to trap link-local control frames (STP and
LLDP) to the CPU port. It keeps these traps in sync with native VIDs during
VLAN configuration changes and updates FDB operations to handle these
control MACs.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 7e5cf77cfa66a..e35dbcdfcbda2 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c
[ ... ]
> @@ -712,6 +742,9 @@ static int xlnx_tsn_port_fdb_add(struct dsa_switch *ds, int port,
>  	u8 portlist;
>  	int ret;
>  
> +	if (xlnx_tsn_addr_is_ctrl_trap(addr))
> +		return 0;
> +
>  	if (!vid)
>  		vid = TSN_SW_DEFAULT_VID;

[Severity: Medium]
If the hardware cannot apply the requested FDB offloads for control MACs,
should this return an error like -EOPNOTSUPP instead of 0?

Returning 0 here could deceive the software bridge and userspace into
believing the rules were successfully applied to the hardware when they
were actually ignored.

> @@ -733,6 +766,9 @@ static int xlnx_tsn_port_fdb_del(struct dsa_switch *ds, int port,
>  	u8 portlist;
>  	int ret;
>  
> +	if (xlnx_tsn_addr_is_ctrl_trap(addr))
> +		return 0;
> +
>  	if (!vid)
>  		vid = TSN_SW_DEFAULT_VID;

[Severity: Medium]
Similarly, should xlnx_tsn_port_fdb_del() return an error instead of 0 to 
accurately reflect that the hardware FDB wasn't modified?

[ ... ]
> @@ -1004,15 +1123,23 @@ static int xlnx_tsn_port_vlan_filtering(struct dsa_switch *ds, int port,
>  		ret = xlnx_tsn_port_state_cycle(sw, dp->index,
>  						TSN_PORT_STATE_BLOCKING);
>  		if (ret)
> -			return ret;
> +			goto restore;
>  
>  		ret = xlnx_tsn_port_state_cycle(sw, dp->index,
>  						TSN_PORT_STATE_FLUSH);
>  		if (ret)
> -			return ret;
> +			goto restore;
>  	}
>  
> +	ret = xlnx_tsn_sync_ctrl_traps(sw);
> +	if (ret)
> +		goto restore;
> +
>  	return 0;
> +
> +restore:
> +	sw->vlan_aware = old_vlan_aware;
> +	return ret;
>  }

[Severity: Medium]
Does xlnx_tsn_port_vlan_filtering() now mix goto-based error handling with 
guard() declarations?

The function begins with:

	guard(mutex)(&sw->indirect_lock);

According to include/linux/cleanup.h, functions should either use scope-based 
cleanup for all resources or convert none of them. Mixing goto labels and 
guard() in the same function can lead to complex ownership issues.

Additionally, if we hit the goto restore path, are the previously executed 
hardware register changes (like PVID, learning control, or CAM entries) left 
unreverted? This seems like it could leave the hardware and software views 
desynchronized on error.

> @@ -1061,6 +1188,7 @@ static int xlnx_tsn_port_vlan_add(struct dsa_switch *ds, int port,
>  		sw->pvid[port] = vlan->vid;
>  		sw->pvid_untagged[port] = untagged;
>  		xlnx_tsn_apply_pvid(sw, port);
> +		return xlnx_tsn_sync_ctrl_traps(sw);
>  	}

[Severity: Medium]
If xlnx_tsn_sync_ctrl_traps() fails, will the hardware and internal PVID state 
be left out of sync with the bridge's view? 

The error is returned directly to the caller, but the hardware changes just 
made by xlnx_tsn_apply_pvid() are not rolled back.

> @@ -1101,6 +1229,7 @@ static int xlnx_tsn_port_vlan_del(struct dsa_switch *ds, int port,
>  		sw->pvid[port] = TSN_SW_DEFAULT_VID;
>  		sw->pvid_untagged[port] = false;
>  		xlnx_tsn_apply_pvid(sw, port);
> +		return xlnx_tsn_sync_ctrl_traps(sw);
>  	}
>  
>  	return 0;

[Severity: Medium]
Similar to the addition path, if xlnx_tsn_sync_ctrl_traps() fails here, is the 
removed PVID state left unreverted while an error is returned?

[ ... ]
> @@ -1276,9 +1420,19 @@ static int xlnx_tsn_setup(struct dsa_switch *ds)
>  			return ret;
>  	}
>  
> +	/* Trap link-local control frames (STP, LLDP) to the CPU port.
> +	 * Without this, a frame arriving on one wire port would be
> +	 * flooded out the other instead of reaching the host bridge.
> +	 */
> +	scoped_guard(mutex, &sw->indirect_lock) {
> +		ret = xlnx_tsn_sync_ctrl_traps(sw);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = xlnx_tsn_mdio_register_all(sw);
>  	if (ret)
> -		return ret;
> +		goto err_traps;

[Severity: Medium]
Does the failure path inside the scoped_guard bypass the cleanup block?

If xlnx_tsn_sync_ctrl_traps() fails after partially installing traps, 
returning directly skips the xlnx_tsn_remove_ctrl_traps() cleanup that 
happens at the err_traps label below. Could this leak hardware CAM entries?

Also, just like in xlnx_tsn_port_vlan_filtering(), this function mixes 
scoped_guard() with goto labels (err_traps, err_mdio, etc.), which the cleanup 
subsystem guidelines advise against.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20