| Message ID | 20190410005700.31582-12-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | NXP SJA1105 DSA driver | expand |
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>
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
diff --git a/include/net/dsa.h b/include/net/dsa.h index 91375bcf2cfb..97325a412156 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -156,6 +156,12 @@ struct dsa_port { * Original copy of the master netdev net_device_ops */ const struct net_device_ops *orig_ndo_ops; + + /* 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; }; struct dsa_switch { @@ -502,7 +508,7 @@ struct net_device *dsa_dev_to_net_device(struct device *dev); static inline bool netdev_uses_dsa(struct net_device *dev) { #if IS_ENABLED(CONFIG_NET_DSA) - return dev->dsa_ptr && dev->dsa_ptr->rcv; + return dev->dsa_ptr && dev->dsa_ptr->uses_tag_protocol; #endif return false; } diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index f4277ee314da..e800bbf9d183 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -585,6 +585,13 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master) dp->tag_ops = tag_ops; dp->master = master; dp->dst = dst; + /* Initially tell DSA to filter all traffic on the master net + * device in eth_type_trans unconditionally if we have a + * packet_type handler (true for all tagging protocols except + * DSA_TAG_PROTO_NONE). Then drivers can later change this + * property. + */ + dp->uses_tag_protocol = !!dp->rcv; return 0; } diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c index cb42939db776..6450c2deded9 100644 --- a/net/dsa/legacy.c +++ b/net/dsa/legacy.c @@ -161,6 +161,13 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, /* Few copies for faster access in master receive hot path */ dst->cpu_dp->rcv = dst->cpu_dp->tag_ops->rcv; dst->cpu_dp->dst = dst; + /* Initially tell DSA to filter all traffic on the master net + * device in eth_type_trans unconditionally if we have a + * packet_type handler (true for all tagging protocols except + * DSA_TAG_PROTO_NONE). Then drivers can later change this + * property. + */ + dst->cpu_dp->uses_tag_protocol = !!dst->cpu_dp->rcv; } memcpy(ds->rtable, cd->rtable, sizeof(ds->rtable));
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> --- Changes in v2: Patch is new. include/net/dsa.h | 8 +++++++- net/dsa/dsa2.c | 7 +++++++ net/dsa/legacy.c | 7 +++++++ 3 files changed, 21 insertions(+), 1 deletion(-)