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? Thanks, -Vladimir
