On 4/30/20 10:50 AM, Vladimir Oltean wrote: > Hi Nikolay, Roopa, > > On Wed, 29 Apr 2020 at 19:33, Nikolay Aleksandrov > <[email protected]> wrote: >> >> +CC Roopa >> >> On 29/04/2020 19:27, Nikolay Aleksandrov wrote: >>> On 29/04/2020 19:19, Vladimir Oltean wrote: >>>> From: Florian Fainelli <[email protected]> >>>> >>>> Commit 8db0a2ee2c63 ("net: bridge: reject DSA-enabled master netdevices >>>> as bridge members") added a special check in br_if.c in order to check >>>> for a DSA master network device with a tagging protocol configured. This >>>> was done because back then, such devices, once enslaved in a bridge >>>> would become inoperative and would not pass DSA tagged traffic anymore >>>> due to br_handle_frame returning RX_HANDLER_CONSUMED. >>>> >>>> But right now we have valid use cases which do require bridging of DSA >>>> masters. One such example is when the DSA master ports are DSA switch >>>> ports themselves (in a disjoint tree setup). This should be completely >>>> equivalent, functionally speaking, from having multiple DSA switches >>>> hanging off of the ports of a switchdev driver. So we should allow the >>>> enslaving of DSA tagged master network devices. >>>> >>>> Make br_handle_frame() return RX_HANDLER_PASS in order to call into the >>>> DSA specific tagging protocol handlers, and lift the restriction from >>>> br_add_if. >>>> >>>> Signed-off-by: Florian Fainelli <[email protected]> >>>> Signed-off-by: Vladimir Oltean <[email protected]> >>>> --- >>>> net/bridge/br_if.c | 4 +--- >>>> net/bridge/br_input.c | 4 +++- >>>> 2 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >>>> index ca685c0cdf95..e0fbdb855664 100644 >>>> --- a/net/bridge/br_if.c >>>> +++ b/net/bridge/br_if.c >>>> @@ -18,7 +18,6 @@ >>>> #include <linux/rtnetlink.h> >>>> #include <linux/if_ether.h> >>>> #include <linux/slab.h> >>>> -#include <net/dsa.h> >>>> #include <net/sock.h> >>>> #include <linux/if_vlan.h> >>>> #include <net/switchdev.h> >>>> @@ -571,8 +570,7 @@ int br_add_if(struct net_bridge *br, struct net_device >>>> *dev, >>>> */ >>>> if ((dev->flags & IFF_LOOPBACK) || >>>> dev->type != ARPHRD_ETHER || dev->addr_len != ETH_ALEN || >>>> - !is_valid_ether_addr(dev->dev_addr) || >>>> - netdev_uses_dsa(dev)) >>>> + !is_valid_ether_addr(dev->dev_addr)) >>>> return -EINVAL; >>>> >>>> /* No bridging of bridges */ >>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c >>>> index d5c34f36f0f4..396bc0c18cb5 100644 >>>> --- a/net/bridge/br_input.c >>>> +++ b/net/bridge/br_input.c >>>> @@ -17,6 +17,7 @@ >>>> #endif >>>> #include <linux/neighbour.h> >>>> #include <net/arp.h> >>>> +#include <net/dsa.h> >>>> #include <linux/export.h> >>>> #include <linux/rculist.h> >>>> #include "br_private.h" >>>> @@ -263,7 +264,8 @@ rx_handler_result_t br_handle_frame(struct sk_buff >>>> **pskb) >>>> struct sk_buff *skb = *pskb; >>>> const unsigned char *dest = eth_hdr(skb)->h_dest; >>>> >>>> - if (unlikely(skb->pkt_type == PACKET_LOOPBACK)) >>>> + if (unlikely(skb->pkt_type == PACKET_LOOPBACK) || >>>> + netdev_uses_dsa(skb->dev)) >>>> return RX_HANDLER_PASS; >>>> >>>> if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) >>>> >>> >>> Yet another test in fast-path for all packets. >>> Since br_handle_frame will not be executed at all for such devices I'd >>> suggest >>> to look into a scheme that avoid installing rx_handler and thus prevents >>> br_handle_frame >>> to be called in the frist place. In case that is not possible then we can >>> discuss adding >>> one more test in fast-path. >>> >>> Actually you can just add a dummy rx_handler that simply returns >>> RX_HANDLER_PASS for >>> these devices and keep rx_handler_data so all br_port_get_* will continue >>> working. >>> >>> Thanks, >>> Nik >>> > > Actually both of those are problematic, since br_port_get_check_rcu > and br_port_get_check_rtnl check for the rx_handler pointer against > br_handle_frame. > Actually a plain old revert of 8db0a2ee2c63 works just fine for what I > need it to, not sure if there's any point in making this any more > complicated than that. > What do you think?
We would still be leaving single switch fabric with no nested tagging non functional if we allow a DSA master to be enslaved into the bridge I believe, unless something changed in how handlers are processed between 8db0a2ee2c63 and now. >From what you described to me, your Ocelot/Felix interfaces are DSA slaves from one side, and DSA master for the sja1105 network devices. We could change the netdev_uses_dsa() into something slightly more elaborate, which is reflective of whether there is nested tagging being used. -- Florian
