Re: [PATCH v2 net-next 11/22] net: dsa: Allow drivers to modulate between presence and absence of tagging

[Vladimir Oltean <olteanv@gmail.com>]

On 4/10/19 5:17 AM, Florian Fainelli wrote:
> 
> 
> On 4/9/2019 5:56 PM, Vladimir Oltean wrote:
>> Frames get processed by DSA and redirected to switch port net devices
>> based on the ETH_P_XDSA multiplexed packet_type handler found by the
>> network stack when calling eth_type_trans().
>>
>> The running assumption is that once the DSA .rcv function is called, DSA
>> is always able to decode the switch tag in order to change the skb->dev
>> from its master.
>>
>> However there are tagging protocols (such as the new
>> DSA_TAG_PROTO_SJA1105) where this assumption is not completely true,
>> since switch tagging piggybacks on the absence of a vlan_filtering
>> bridge.
>>
>> Having DSA receive untagged traffic would put it in an impossible
>> situation: the eth_type_trans() function would invoke the DSA .rcv(),
>> which could not change skb->dev, then eth_type_trans() would be invoked
>> again, which again would call the DSA .rcv, and the packet would never
>> be able to exit the DSA filter and would spiral in a loop until the
>> whole system dies.
>>
>> This happens because eth_type_trans() doesn't actually look at the skb
>> (so as to identify a potential tag) when it deems it as being
>> ETH_P_XDSA. It just checks whether skb->dev has a DSA private pointer
>> installed (therefore it's a DSA master) and that there exists a .rcv
>> callback (everybody except DSA_TAG_PROTO_NONE has that). This is
>> understandable as there are many switch tags out there, and exhaustively
>> checking for all of them is far from ideal.
>>
>> The solution lies in the observation that a more nuanced check can be
>> made when eth_type_trans() determines that switch tagging is used or
>> not. In a way, this reverts patch "717ffbfb28ac net: dsa: remove
>> dsa_uses_tagged_protocol", but instead of adding it back as a DSA
>> function, it is now a boolean property. This is because the driver might
>> actually know better when it can and can't support switch tagging.
>>
>> With this patch, all tagging protocols can morph at runtime into the
>> DSA_TAG_PROTO_NONE on receive, by setting cpu_dp->uses_tag_protocol = 0.
>> This permits them to at least terminate traffic through the master net
>> device. Their .rcv callback no longer even gets called in this mode.
>>
>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>> ---
> 
> [snip]
> 
>> +	/* Property used to allow traffic at runtime to bypass the DSA
>> +	 * filter in eth_type_trans and be processed as regular on the
>> +	 * master net device.
>> +	 */
>> +	bool			uses_tag_protocol;
> 
> This gets used in the hot path can you make sure this is part of the
> first cache line of a dsa_port? pahole is a good tool for determining
> where a member is in a given structure. With that:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 

Can you give a bit more context about what makes the first cache line 
special?

Also I am not all that satisfied with my own solution here, maybe you 
could share a thought? Basically the SJA1105 *can* actually do a very 
crude form of switch tagging, but only for MAC filtered traffic, and it 
actually mangles bytes 4 and 5 of the DMAC in the process (sort of 
01-80-C2-XX-XX-00). (the irony is that then I can configure it to send 
me a follow-up "meta frame" where it gives me back the value of my XX-XX 
bytes it overwrote).
So receiving MAC filtered frames can be made to not require the 8021Q 
switch tagging. But the fundamental problem is that when I set 
uses_tag_protocol = 0 because I can no longer process 8021Q tags, I 
inherently break my reception of MAC filtered traffic too. Having to 
terminate traffic on the master port is not a big deal, but turning the 
switch back into an unmanaged brick is not that nice, especially since 
it's an unnecessary loss.
Ideally what I would like to have is a way to let DSA (drivers) classify 
skb's and determine whether they can decode a port from them 
(individually) or not.

Thanks,
-Vladimir