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

From: Vladimir Oltean
Date: Wed Apr 10 2019 - 17:52:51 EST

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@xxxxxxxxx>
---

[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@xxxxxxxxx>


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