Hi Vladimir,

On 8/23/21 1:25 AM, Vladimir Oltean wrote:

On Sun, Aug 22, 2021 at 11:11:21PM +0000, Alvin Šipraga wrote:

quoted

quoted

quoted

+ *    KEEP       | preserve packet VLAN tag format

What does it mean to preserve packet VLAN tag format? Trying to
understand if the sane thing is to clear or set this bit. Does it mean
to strip the VLAN tag on egress if the VLAN is configured as
egress-untagged on the port?

I suppose you mean "Does it mean _don't_ strip the VLAN tag on egress..."?

I'm not sure what the semantics of this KEEP are. When I configure the
ports to be egress-untagged, the packets leave the port untagged. When I
configure the ports without egress-untagged, the packets leave the port
tagged. This is with the code as you see it - so KEEP=0. If I am to
hazard a guess, maybe it overrides any port-based egress-untagged
setting. I will run some tests tomorrow.

Ok, then it makes sense to set KEEP=0 and not override the port settings.

OK, glad you agree.

quoted

quoted

quoted

+	*p = htons(~(1 << 15) & BIT(dp->index));

I am deeply confused by this line.

~(1 << 15) is GENMASK(14, 0)
By AND-ing it with BIT(dp->index), what do you gain?

Deliberate verbosity for the human who was engaged in writing the
tagging driver to begin with, but obviously stupid. I'll remove.

I wouldn't say "stupid", but it's non-obvious, hard to read and at the same time pointless.
I had to take out the abacus to see if I'm missing something.

quoted

quoted

quoted

+	/* Ignore FID_EN, FID, PRI_EN, PRI, KEEP, LEARN_DIS */
+	p = (__be16 *)(tag + 4);

Delete then?

Deliberate verbosity again - but I figure any half-decent compiler will
optimize this out to begin with. I thought it serves as a perfectly fine
"add stuff here" notice together with the comment, but I can remove in v2.

Keeping just the comment is fine, but having the line of code is pretty
pointless. Just like any half-decent compiler will optimize it out, any
developer with half a brain will figure out what to do to parse
FID_EN ... LEARN_DIS thanks to the other comments.

Point well made :-) I'll clean up in v2. Thanks!

quoted

quoted

quoted

+
+	/* Ignore ALLOW; parse TX (switch->CPU) */
+	p = (__be16 *)(tag + 6);
+	tmp = ntohs(*p);
+	port = tmp & 0xf; /* Port number is the LSB 4 bits */
+
+	skb->dev = dsa_master_find_slave(dev, 0, port);
+	if (!skb->dev) {
+		netdev_dbg(dev, "could not find slave for port %d\n", port);
+		return NULL;
+	}
+
+	/* Remove tag and recalculate checksum */
+	skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
+
+	dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
+
+	skb->offload_fwd_mark = 1;

At the very least, please use

	dsa_default_offload_fwd_mark(skb);

which does the right thing when the port is not offloading the bridge.

Sure. Can you elaborate on what you mean by "at the very least"? Can it
be improved even further?

The elaboration is right below. skb->offload_fwd_mark should be set to
zero for packets that have been forwarded only to the host (like packets
that have hit a trapping rule). I guess the switch will denote this
piece of info through the REASON code.

Yes, I think it will be communicated in REASON too. I haven't gotten to 
deciphering the contents of this field since it has not been needed so 
far: the ports are fully isolated and all bridging is done in software.

This allows the software bridge data path to know to not flood packets
that have already been flooded by the switch in its hardware data path.

Control packets can still be re-forwarded by the software data path,
even if the switch has trapped/not forwarded them, through the
"group_fwd_mask" option in "man ip-link").

Since the driver doesn't support any offloading right now (ports are 
isolated), would you be OK with me just setting 
dsa_default_offload_fwd_mark(skb) for now? I will revisit this in the 
future when I have more time at work to implement some of the offloading 
features, but it's not something I can commit to in the near future.

quoted

quoted

Also tell us more about REASON and ALLOW. Is there a bit in the RX tag
which denotes that the packet was forwarded only to the host?

As I wrote to Andrew, REASON is undocumented and I have not investigated
this field yet. I have addressed ALLOW upstairs in this email, but
suffice to say I am not sure.

On xmit, you have. On rcv (switch->CPU), I am not sure whether the
switch will ever set ALLOW to 1, and what is the meaning of that.

I think ALLOW is only relevant on xmit (CPU->switch). I can make it more 
clear in the comment in v2.