| Message ID | 20200926173108.1230014-3-vladimir.oltean@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Generic adjustment for flow dissector in DSA | expand |
On Sat, Sep 26, 2020 at 08:30:54PM +0300, Vladimir Oltean wrote: > Currently DSA assumes that taggers don't mess with the destination MAC > address of the frames on RX. That is not always the case. Some DSA > headers are placed before the Ethernet header (ocelot), and others > simply mangle random bytes from the destination MAC address (sja1105 > with its incl_srcpt option). ... > So give drivers the possibility to signal that their tagging protocol > will get randomly dropped otherwise, and let DSA deal with fixing that. > @@ -317,6 +317,13 @@ struct dsa_switch { > */ > bool mtu_enforcement_ingress; > > + /* Some tagging protocols either mangle or shift the destination MAC > + * address, in which case the DSA master would drop packets on ingress > + * if what it understands out of the destination MAC address is not in > + * its RX filter. > + */ > + bool promisc_on_master; > + > size_t num_ports; > }; Hi Vladimir I actually think this is a property of the tagger, not the DSA driver. In fact, DSA drivers never handle actual frames, they are all about the control plane, not the data plane. So i think this bool should be in the tagger structure, dsa_device_ops. Andrew
On Sat, Sep 26, 2020 at 07:47:00PM +0200, Andrew Lunn wrote: > Hi Vladimir > > I actually think this is a property of the tagger, not the DSA > driver. In fact, DSA drivers never handle actual frames, they are all > about the control plane, not the data plane. So i think this bool > should be in the tagger structure, dsa_device_ops. This is actually a good comment that will simplify my future work a little bit. I need to add a Kconfig-selectable tagger for the Felix driver (tag_ocelot.c vs a future tag_ocelot_8021q.c) and, predictably enough, one tagger needs promisc on master while the other doesn't. If you have no other comments on this series, I'm thinking about resending it right away with this change, since I made a big blunder and the people in Cc on the tagger patches were not actually copied to my emails, since I have supresscc=all in my .gitconfig.
diff --git a/include/net/dsa.h b/include/net/dsa.h index d16057c5987a..70571b179d05 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -317,6 +317,13 @@ struct dsa_switch { */ bool mtu_enforcement_ingress; + /* Some tagging protocols either mangle or shift the destination MAC + * address, in which case the DSA master would drop packets on ingress + * if what it understands out of the destination MAC address is not in + * its RX filter. + */ + bool promisc_on_master; + size_t num_ports; }; diff --git a/net/dsa/master.c b/net/dsa/master.c index 61615ebc70e9..c12cbcdd54b1 100644 --- a/net/dsa/master.c +++ b/net/dsa/master.c @@ -259,6 +259,19 @@ static void dsa_netdev_ops_set(struct net_device *dev, dev->dsa_ptr->netdev_ops = ops; } +static void dsa_master_set_promiscuity(struct net_device *dev, int inc) +{ + struct dsa_port *cpu_dp = dev->dsa_ptr; + struct dsa_switch *ds = cpu_dp->ds; + + if (!ds->promisc_on_master) + return; + + rtnl_lock(); + dev_set_promiscuity(dev, inc); + rtnl_unlock(); +} + static ssize_t tagging_show(struct device *d, struct device_attribute *attr, char *buf) { @@ -314,9 +327,12 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp) dev->dsa_ptr = cpu_dp; lockdep_set_class(&dev->addr_list_lock, &dsa_master_addr_list_lock_key); + + dsa_master_set_promiscuity(dev, 1); + ret = dsa_master_ethtool_setup(dev); if (ret) - return ret; + goto out_err_reset_promisc; dsa_netdev_ops_set(dev, &dsa_netdev_ops); @@ -329,6 +345,8 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp) out_err_ndo_teardown: dsa_netdev_ops_set(dev, NULL); dsa_master_ethtool_teardown(dev); +out_err_reset_promisc: + dsa_master_set_promiscuity(dev, -1); return ret; } @@ -338,6 +356,7 @@ void dsa_master_teardown(struct net_device *dev) dsa_netdev_ops_set(dev, NULL); dsa_master_ethtool_teardown(dev); dsa_master_reset_mtu(dev); + dsa_master_set_promiscuity(dev, -1); dev->dsa_ptr = NULL;
Currently DSA assumes that taggers don't mess with the destination MAC address of the frames on RX. That is not always the case. Some DSA headers are placed before the Ethernet header (ocelot), and others simply mangle random bytes from the destination MAC address (sja1105 with its incl_srcpt option). The DSA master goes to promiscuous mode automatically when the slave devices go too (such as when enslaved to a bridge), but in standalone mode this is a problem that needs to be dealt with. So give drivers the possibility to signal that their tagging protocol will get randomly dropped otherwise, and let DSA deal with fixing that. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- include/net/dsa.h | 7 +++++++ net/dsa/master.c | 21 ++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-)