On Sun, Jun 25, 2023 at 02:42:27PM +0200, Simon Horman wrote:

+ Dan Carpenter

On Sun, Jun 25, 2023 at 01:53:39PM +0200, Pawel Dembicki wrote:

quoted

This patch is simple implementation of 8021q tagging in vsc73xx driver.
At this moment devices with DSA_TAG_PROTO_NONE are useless. VSC73XX
family doesn't provide any tag support for external ethernet ports.

The only way is vlan-based tagging. It require constant hardware vlan
filtering. VSC73XX family support provider bridging but QinQ only without
fully implemented 802.1AD. It allow only doubled 0x8100 TPID.

In simple port mode QinQ is enabled to preserve forwarding vlan tagged
frames.

Tag driver introduce most simple funcionality required for proper taging
support.

Reviewed-by: Linus Walleij <redacted>
Signed-off-by: Pawel Dembicki <redacted>

...

quoted

+static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port,
+			     int *switch_id, int *vbid, u16 *vid)
+{
+	if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK))
+		return dsa_8021q_rcv(skb, source_port, switch_id, vbid);
+
+	/* Try our best with imprecise RX */
+	*vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
+}
+
+static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb,
+				   struct net_device *netdev)
+{
+	int src_port = -1, switch_id = -1, vbid = -1;
+	u16 vid;
+
+	if (skb_vlan_tag_present(skb))
+		/* Normal traffic path. */
+		vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid);
+
+	if (vbid >= 1)
+		skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);
+	else if (src_port == -1 || switch_id == -1)
+		skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid);

Hi Pawel,

Smatch warns that vid may be used uninitialised here.
And it's not clear to me why that cannot be the case.

The problem is only when skb_vlan_tag_present() is false.

If vsc73xx_vlan_rcv() is called then actually it's fine.  Either vbid,
src_port and switch_id will be set or vid will be initialized.  Smatch
thinks the vbid is set to 0-7, src_port is set to 0-15 and
switch_id is set to 0-7.  Smatch kind of ignores the switch_id == -1
condition because it's known to be false given that we already checked
src_port == -1.

But the concern again is the skb_vlan_tag_present() is false and then
vbid, src_port and switch_id would all be set to -1.

regards,
dan carpenter