Re: [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag

[Vladimir Oltean]

On Sun, Aug 22, 2021 at 11:11:21PM +0000, Alvin Šipraga wrote:
> >> + *    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.

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

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

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

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").

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