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

[Alvin Šipraga]

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

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

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

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

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