| Message ID | 20241201074212.2833277-1-andrew@andrewstrohman.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] dsa: Make offloading optional on per port basis | expand |
Sun, Dec 01, 2024 at 08:42:11AM CET, andrew@andrewstrohman.com wrote: >The author has a couple use cases for this: > >1) Creating a sniffer, or ethernet tap, by bridging two or more >non-offloaded ports to the bridge, and tcpdump'ing the member >ports. Along the same lines, it would be nice to have the ability >to temporarily disable offloading to sniff all traffic for debugging. > >2) Work around bugs in the hardware switch or use features >that are only available in software. > >DSA drivers can be modified to remove their port_bridge_join() >dsa_switch_ops member to accomplish this. But, it would be better >to make it this runtime configurable, and configurable on a per port >basis. > >The key to signaling that a port is not offloading is by >ensuring dp->bridge == NULL. With this, the VLAN and FDB >operations that affect hardware (ie port_fdb_add, port_vlan_del, etc) >will not run. dsa_user_fdb_event() will bail if !dp->bridge. >dsa_user_port_obj_add() checks dsa_port_offloads_bridge_port(), >and dsa_user_host_vlan_add() checks !dp->bridge. > >By being configurable on a per port basis (as opposed to switch-wide), >we can have some subset of a switch's ports offloading and others not. > >While this approach is generic, and therefore will be available for all >dsa switches, I have only tested this on a mt7530 switch. It may not be >possible or feasible to disable offloading on other switches. > >A flags member was added to the dsa user port netdev private data structure >in order to facilitate adding future dsa specific flags more easily. >IFLA_VLAN_FLAGS was used as an example when implementing the flags member. > >Signed-off-by: Andy Strohman <andrew@andrewstrohman.com> Why is this DSA specific? Plus, you say you want to disable offloading in general (DSA_FLAG_OFFLOADING_DISABLED), but you check the flag only when joining bridge. I mean, shouldn't this be rather something exposed by some common UAPI? Btw, isn't NETIF_F_HW_L2FW_DOFFLOAD what you are looking for?
On Mon, Dec 02, 2024 at 10:27:25AM +0100, Jiri Pirko wrote: > Why is this DSA specific? Plus, you say you want to disable offloading > in general (DSA_FLAG_OFFLOADING_DISABLED), but you check the flag only > when joining bridge. I mean, shouldn't this be rather something exposed > by some common UAPI? I agree with this. The proposed functionality isn't DSA specific, and thus, the UAPI to configure it shouldn't be made so. > Btw, isn't NETIF_F_HW_L2FW_DOFFLOAD what you are looking for? Is it? macvlan uses NETIF_F_HW_L2FW_DOFFLOAD to detect presence of netdev_ops->ndo_dfwd_add_station(). Having to even consider macvlan offload and its implications just because somebody decided to monopolize the "l2-fwd-offload" name seems at least bizarre in my opinion.
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index f7f638f25020..c5e89064d48c 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -1972,9 +1972,19 @@ enum { IFLA_DSA_CONDUIT, /* Deprecated, use IFLA_DSA_CONDUIT instead */ IFLA_DSA_MASTER = IFLA_DSA_CONDUIT, + IFLA_DSA_FLAGS, __IFLA_DSA_MAX, }; #define IFLA_DSA_MAX (__IFLA_DSA_MAX - 1) +struct ifla_dsa_flags { + __u32 flags; + __u32 mask; +}; + +enum dsa_flags { + DSA_FLAG_OFFLOADING_DISABLED = 0x1, +}; + #endif /* _UAPI_LINUX_IF_LINK_H */ diff --git a/net/dsa/netlink.c b/net/dsa/netlink.c index 1332e56349e5..376ba00957fe 100644 --- a/net/dsa/netlink.c +++ b/net/dsa/netlink.c @@ -9,13 +9,35 @@ static const struct nla_policy dsa_policy[IFLA_DSA_MAX + 1] = { [IFLA_DSA_CONDUIT] = { .type = NLA_U32 }, + [IFLA_DSA_FLAGS] = { .len = sizeof(struct ifla_dsa_flags) }, }; +static int dsa_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask) +{ + struct dsa_user_priv *p = netdev_priv(dev); + u32 old_flags = p->flags; + + /* For now, we only support make these changes when the port is not a member + * of a bridge (ie in standalone mode). If the user wants to alter these flags + * for ports that are currently members of a bridge need to first remove the + * interface from the bridge. Then they can add interface back + * after making their desired flag changes. + */ + + if (netif_is_bridge_port(dev)) + return -EBUSY; + + p->flags = (old_flags & ~mask) | (flags & mask); + + return 0; +} + static int dsa_changelink(struct net_device *dev, struct nlattr *tb[], struct nlattr *data[], struct netlink_ext_ack *extack) { int err; + struct ifla_dsa_flags *flags; if (!data) return 0; @@ -32,6 +54,12 @@ static int dsa_changelink(struct net_device *dev, struct nlattr *tb[], if (err) return err; } + if (data[IFLA_DSA_FLAGS]) { + flags = nla_data(data[IFLA_DSA_FLAGS]); + err = dsa_dev_change_flags(dev, flags->flags, flags->mask); + if (err) + return err; + } return 0; } @@ -45,10 +73,20 @@ static size_t dsa_get_size(const struct net_device *dev) static int dsa_fill_info(struct sk_buff *skb, const struct net_device *dev) { struct net_device *conduit = dsa_user_to_conduit(dev); + struct dsa_user_priv *dsa = netdev_priv(dev); + struct ifla_dsa_flags f; + if (nla_put_u32(skb, IFLA_DSA_CONDUIT, conduit->ifindex)) return -EMSGSIZE; + if (dsa->flags) { + f.flags = dsa->flags; + f.mask = ~0; + if (nla_put(skb, IFLA_DSA_FLAGS, sizeof(f), &f)) + return -EMSGSIZE; + } + return 0; } diff --git a/net/dsa/switch.c b/net/dsa/switch.c index 3d2feeea897b..f0bb354c3961 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -83,9 +83,10 @@ static int dsa_switch_bridge_join(struct dsa_switch *ds, struct dsa_notifier_bridge_info *info) { int err; + struct dsa_user_priv *priv = netdev_priv(info->dp->user); if (info->dp->ds == ds) { - if (!ds->ops->port_bridge_join) + if (!ds->ops->port_bridge_join || priv->flags & DSA_FLAG_OFFLOADING_DISABLED) return -EOPNOTSUPP; err = ds->ops->port_bridge_join(ds, info->dp->index, diff --git a/net/dsa/user.h b/net/dsa/user.h index 016884bead3c..846af7ed819b 100644 --- a/net/dsa/user.h +++ b/net/dsa/user.h @@ -33,6 +33,8 @@ struct dsa_user_priv { /* TC context */ struct list_head mall_tc_list; + + u32 flags; }; void dsa_user_mii_bus_init(struct dsa_switch *ds); diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h index 8516c1ccd57a..0982b309b09c 100644 --- a/tools/include/uapi/linux/if_link.h +++ b/tools/include/uapi/linux/if_link.h @@ -1970,6 +1970,7 @@ enum { IFLA_DSA_CONDUIT, /* Deprecated, use IFLA_DSA_CONDUIT instead */ IFLA_DSA_MASTER = IFLA_DSA_CONDUIT, + IFLA_DSA_FLAGS, __IFLA_DSA_MAX, };
The author has a couple use cases for this: 1) Creating a sniffer, or ethernet tap, by bridging two or more non-offloaded ports to the bridge, and tcpdump'ing the member ports. Along the same lines, it would be nice to have the ability to temporarily disable offloading to sniff all traffic for debugging. 2) Work around bugs in the hardware switch or use features that are only available in software. DSA drivers can be modified to remove their port_bridge_join() dsa_switch_ops member to accomplish this. But, it would be better to make it this runtime configurable, and configurable on a per port basis. The key to signaling that a port is not offloading is by ensuring dp->bridge == NULL. With this, the VLAN and FDB operations that affect hardware (ie port_fdb_add, port_vlan_del, etc) will not run. dsa_user_fdb_event() will bail if !dp->bridge. dsa_user_port_obj_add() checks dsa_port_offloads_bridge_port(), and dsa_user_host_vlan_add() checks !dp->bridge. By being configurable on a per port basis (as opposed to switch-wide), we can have some subset of a switch's ports offloading and others not. While this approach is generic, and therefore will be available for all dsa switches, I have only tested this on a mt7530 switch. It may not be possible or feasible to disable offloading on other switches. A flags member was added to the dsa user port netdev private data structure in order to facilitate adding future dsa specific flags more easily. IFLA_VLAN_FLAGS was used as an example when implementing the flags member. Signed-off-by: Andy Strohman <andrew@andrewstrohman.com> --- include/uapi/linux/if_link.h | 10 ++++++++ net/dsa/netlink.c | 38 ++++++++++++++++++++++++++++++ net/dsa/switch.c | 3 ++- net/dsa/user.h | 2 ++ tools/include/uapi/linux/if_link.h | 1 + 5 files changed, 53 insertions(+), 1 deletion(-)