Keeping the full email body untrimmed for extra context for the newly
added people.
On Mon, Oct 14, 2024 at 09:22:07AM +0300, Nikolay Aleksandrov wrote:
> On 14/10/2024 09:18, Nikolay Aleksandrov wrote:
> > On 13/10/2024 21:55, Eric Woudstra wrote:
> >> In network setup as below:
> >>
> >> fastpath bypass
> >> .----------------------------------------.
> >> / \
> >> | IP - forwarding |
> >> | / \ v
> >> | / wan ...
> >> | /
> >> | |
> >> | |
> >> | brlan.1
> >> | |
> >> | +-------------------------------+
> >> | | vlan 1 |
> >> | | |
> >> | | brlan (vlan-filtering) |
> >> | | +---------------+
> >> | | | DSA-SWITCH |
> >> | | vlan 1 | |
> >> | | to | |
> >> | | untagged 1 vlan 1 |
> >> | +---------------+---------------+
> >> . / \
> >> ----->wlan1 lan0
> >> . .
> >> . ^
> >> ^ vlan 1 tagged packets
> >> untagged packets
> >>
> >> Now that DEV_PATH_MTK_WDMA is added to nft_dev_path_info() the forward
> >> path is filled also when ending with the mediatek wlan1, info.indev not
> >> NULL now in nft_dev_forward_path(). This results in a direct transmit
> >> instead of a neighbor transmit. This is how it should be, But this fails.
> >>
> >> br_vlan_fill_forward_path_mode() sets DEV_PATH_BR_VLAN_UNTAG_HW when
> >> filling in from brlan.1 towards wlan1. But it should be set to
> >> DEV_PATH_BR_VLAN_UNTAG in this case. Using BR_VLFLAG_ADDED_BY_SWITCHDEV
> >> is not correct. The dsa switchdev adds it as a foreign port.
> >>
> >> Use BR_VLFLAG_TAGGING_BY_SWITCHDEV to make sure DEV_PATH_BR_VLAN_UNTAG is
> >> set when there is a dsa-switch inside the bridge.
> >>
> >> Signed-off-by: Eric Woudstra <ericwouds@xxxxxxxxx>
> >> ---
> >> net/bridge/br_private.h | 1 +
> >> net/bridge/br_vlan.c | 18 +++++++++++++++++-
> >> 2 files changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >> index 8da7798f9368..7d427214cc7c 100644
> >> --- a/net/bridge/br_private.h
> >> +++ b/net/bridge/br_private.h
> >> @@ -180,6 +180,7 @@ enum {
> >> BR_VLFLAG_MCAST_ENABLED = BIT(2),
> >> BR_VLFLAG_GLOBAL_MCAST_ENABLED = BIT(3),
> >> BR_VLFLAG_NEIGH_SUPPRESS_ENABLED = BIT(4),
> >> + BR_VLFLAG_TAGGING_BY_SWITCHDEV = BIT(5),
> >> };
> >>
> >> /**
> >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> >> index 1830d7d617cd..b7877724b969 100644
> >> --- a/net/bridge/br_vlan.c
> >> +++ b/net/bridge/br_vlan.c
> >> @@ -3,6 +3,7 @@
> >> #include <linux/netdevice.h>
> >> #include <linux/rtnetlink.h>
> >> #include <linux/slab.h>
> >> +#include <net/dsa.h>
> >> #include <net/switchdev.h>
> >>
> >> #include "br_private.h"
> >> @@ -100,6 +101,19 @@ static void __vlan_flags_commit(struct net_bridge_vlan *v, u16 flags)
> >> __vlan_flags_update(v, flags, true);
> >> }
> >>
> >> +static inline bool br_vlan_tagging_by_switchdev(struct net_bridge *br)
> >
> > no inline in .c files and also constify br
> >
> >> +{
> >> +#if IS_ENABLED(CONFIG_NET_DSA)
> >> + struct net_bridge_port *p;
> >> +
> >> + list_for_each_entry(p, &br->port_list, list) {
> >> + if (dsa_user_dev_check(p->dev))
> >
> > I don't think this can change at runtime, so please keep a counter in
> > the bridge and don't walk the port list on every vlan add.
> >
>
> you can use an internal bridge opt (check br_private.h) with a private opt
> that's set when such device is added as a port, no need for a full counter
> obviously
To continue on Nikolay's line of thought...
Can you abstractly describe which functional behavior do you need the
bridge port to perform, rather than "it needs to be a DSA user port"?
switchdev_bridge_port_offload() has a mechanism to inform the bridge
core of extra abilities (like tx_fwd_offload). Perhaps you could modify
the DSA drivers you need to set a similar bit to inform the bridge of
their presence and ability. That would also work when the bridge port is
a LAG over a DSA user port.
Also, please also CC DSA maintainers when you use DSA API outside
net/dsa/ and drivers/net/dsa/. I am in the process of revamping the
public DSA API and would like to be in touch with changes as they are
made.
> >> + return false;
> >> + }
> >> +#endif
> >> + return true;
> >> +}
> >> +
> >> static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> >> struct net_bridge_vlan *v, u16 flags,
> >> struct netlink_ext_ack *extack)
> >> @@ -113,6 +127,8 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> >> if (err == -EOPNOTSUPP)
> >> return vlan_vid_add(dev, br->vlan_proto, v->vid);
> >> v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV;
> >> + if (br_vlan_tagging_by_switchdev(br))
> >> + v->priv_flags |= BR_VLFLAG_TAGGING_BY_SWITCHDEV;
> >> return err;
> >> }
> >>
> >> @@ -1491,7 +1507,7 @@ int br_vlan_fill_forward_path_mode(struct net_bridge *br,
> >>
> >> if (path->bridge.vlan_mode == DEV_PATH_BR_VLAN_TAG)
> >> path->bridge.vlan_mode = DEV_PATH_BR_VLAN_KEEP;
> >> - else if (v->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV)
> >> + else if (v->priv_flags & BR_VLFLAG_TAGGING_BY_SWITCHDEV)
> >> path->bridge.vlan_mode = DEV_PATH_BR_VLAN_UNTAG_HW;
> >> else
> >> path->bridge.vlan_mode = DEV_PATH_BR_VLAN_UNTAG;
> >
