[PATCH v3 net-next 0/2] net: tc: dsa: Implement offload of matchall for bridged DSA ports

[PATCH v3 net-next 0/2] net: tc: dsa: Implement offload of matchall for bridged DSA ports

[Mattias Forsblad]

Greetings,

This series implements offloading of tc matchall filter to HW
for bridged DSA ports.

Background
When using a non-VLAN filtering bridge we want to be able to drop
traffic directed to the CPU port so that the CPU doesn't get unnecessary loaded.
This is specially important when we have disabled learning on user ports.

A sample configuration could be something like this:

       br0
      /   \
   swp0   swp1

ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
ip link set swp0 master br0
ip link set swp1 master br0
ip link set swp0 type bridge_slave learning off
ip link set swp1 type bridge_slave learning off
ip link set swp0 up
ip link set swp1 up
ip link set br0 up

After discussions here: https://lore.kernel.org/netdev/YjMo9xyoycXgSWXS@shredder/
it was advised to use tc to set an ingress filter that could then
be offloaded to HW, like so:

tc qdisc add dev br0 clsact
tc filter add dev br0 ingress pref 1 proto all matchall action drop

Limitations
If there is tc rules on a bridge and all the ports leave the bridge
and then joins the bridge again, the indirect framwork doesn't seem
to reoffload them at join. The tc rules need to be torn down and
re-added.

The first part of this serie uses the flow indirect framework to
setup monitoring of tc qdisc and filters added to a bridge.
The second part offloads the matchall filter to HW for Marvell
switches.

RFC -> v1: Monitor bridge join/leave and re-evaluate offloading (Vladimir Oltean)
v2: Fix code standard compliance (Jakub Kicinski)
v3: Fix warning from kernel test robot (<lkp@intel.com>)

Mattias Forsblad (2):
  net: tc: dsa: Add the matchall filter with drop action for bridged DSA
    ports.
  net: dsa: Implement tc offloading for drop target.

 drivers/net/dsa/mv88e6xxx/chip.c |  23 +++-
 include/net/dsa.h                |  14 ++
 net/dsa/dsa2.c                   |   5 +
 net/dsa/dsa_priv.h               |   3 +
 net/dsa/port.c                   |   2 +
 net/dsa/slave.c                  | 224 ++++++++++++++++++++++++++++++-
 6 files changed, 266 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v3 net-next 1/2] net: tc: dsa: Add the matchall filter with drop action for bridged DSA ports.

[Mattias Forsblad]

Use the flow indirect framework on bridged DSA ports to be
able to set up offloading of matchall filter with drop target.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 include/net/dsa.h  |  14 +++
 net/dsa/dsa2.c     |   5 +
 net/dsa/dsa_priv.h |   3 +
 net/dsa/port.c     |   2 +
 net/dsa/slave.c    | 224 ++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 244 insertions(+), 4 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 934958fda962..0ddfce552002 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -171,6 +171,9 @@ struct dsa_switch_tree {
 
 	/* Track the largest switch index within a tree */
 	unsigned int last_switch;
+
+	/* For tc indirect bookkeeping */
+	struct list_head tc_indr_block_list;
 };
 
 /* LAG IDs are one-based, the dst->lags array is zero-based */
@@ -212,6 +215,7 @@ static inline int dsa_lag_id(struct dsa_switch_tree *dst,
 enum dsa_port_mall_action_type {
 	DSA_PORT_MALL_MIRROR,
 	DSA_PORT_MALL_POLICER,
+	DSA_PORT_MALL_DROP,
 };
 
 /* TC mirroring entry */
@@ -220,6 +224,11 @@ struct dsa_mall_mirror_tc_entry {
 	bool ingress;
 };
 
+/* TC drop entry */
+struct dsa_mall_drop_tc_entry {
+	bool enable;
+};
+
 /* TC port policer entry */
 struct dsa_mall_policer_tc_entry {
 	u32 burst;
@@ -234,6 +243,7 @@ struct dsa_mall_tc_entry {
 	union {
 		struct dsa_mall_mirror_tc_entry mirror;
 		struct dsa_mall_policer_tc_entry policer;
+		struct dsa_mall_drop_tc_entry drop;
 	};
 };
 
@@ -241,6 +251,8 @@ struct dsa_bridge {
 	struct net_device *dev;
 	unsigned int num;
 	bool tx_fwd_offload;
+	u8 local_rcv:1;
+	u8 local_rcv_effective:1;
 	refcount_t refcount;
 };
 
@@ -1034,6 +1046,8 @@ struct dsa_switch_ops {
 	int	(*port_policer_add)(struct dsa_switch *ds, int port,
 				    struct dsa_mall_policer_tc_entry *policer);
 	void	(*port_policer_del)(struct dsa_switch *ds, int port);
+	int	(*bridge_local_rcv)(struct dsa_switch *ds, int port,
+				    struct dsa_mall_drop_tc_entry *drop);
 	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
 				 enum tc_setup_type type, void *type_data);
 
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ca6af86964bc..e87ceb841a70 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -247,6 +247,9 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
 	INIT_LIST_HEAD(&dst->list);
 	list_add_tail(&dst->list, &dsa_tree_list);
 
+	INIT_LIST_HEAD(&dst->tc_indr_block_list);
+	dsa_setup_bridge_tc_indr(dst);
+
 	kref_init(&dst->refcount);
 
 	return dst;
@@ -254,6 +257,8 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
 
 static void dsa_tree_free(struct dsa_switch_tree *dst)
 {
+	dsa_cleanup_bridge_tc_indr(dst);
+
 	if (dst->tag_ops)
 		dsa_tag_driver_put(dst->tag_ops);
 	list_del(&dst->list);
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 5d3f4a67dce1..456bcbe730ba 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -320,6 +320,9 @@ void dsa_slave_setup_tagger(struct net_device *slave);
 int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
 int dsa_slave_manage_vlan_filtering(struct net_device *dev,
 				    bool vlan_filtering);
+int dsa_setup_bridge_tc_indr(struct dsa_switch_tree *dst);
+void dsa_cleanup_bridge_tc_indr(struct dsa_switch_tree *dst);
+bool dsa_slave_dev_check(const struct net_device *dev);
 
 static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
 {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 32d472a82241..0c4522cc9eae 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -361,6 +361,8 @@ static int dsa_port_bridge_create(struct dsa_port *dp,
 	refcount_set(&bridge->refcount, 1);
 
 	bridge->dev = br;
+	bridge->local_rcv = 1;
+	bridge->local_rcv_effective = 1;
 
 	bridge->num = dsa_bridge_num_get(br, ds->max_num_bridges);
 	if (ds->max_num_bridges && !bridge->num) {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 41c69a6e7854..62219210d3ea 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1246,6 +1246,67 @@ dsa_slave_add_cls_matchall_mirred(struct net_device *dev,
 	return err;
 }
 
+static int dsa_slave_bridge_foreign_if_check(struct net_device *dev,
+					     struct dsa_mall_drop_tc_entry *drop)
+{
+	struct net_device *lower_dev;
+	struct dsa_port *dp = NULL;
+	bool foreign_if = false;
+	struct list_head *iter;
+
+	/* Check port types in this bridge */
+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
+		if (dsa_slave_dev_check(lower_dev))
+			dp = dsa_slave_to_port(lower_dev);
+		else
+			foreign_if = true;
+	}
+
+	/* Offload only if we have requested it and the bridge only
+	 * contains dsa ports
+	 */
+	if (!dp || !dp->bridge)
+		return 0;
+
+	if (!foreign_if)
+		dp->bridge->local_rcv_effective = dp->bridge->local_rcv;
+	else
+		dp->bridge->local_rcv_effective = 1;
+
+	return dp->ds->ops->bridge_local_rcv(dp->ds, dp->index, drop);
+}
+
+static int
+dsa_slave_add_cls_matchall_drop(struct net_device *dev,
+				struct tc_cls_matchall_offload *cls,
+				bool ingress)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_mall_tc_entry *mall_tc_entry;
+	struct dsa_mall_drop_tc_entry *drop;
+	struct dsa_switch *ds = dp->ds;
+	int err;
+
+	if (!ds->ops->bridge_local_rcv)
+		return -EOPNOTSUPP;
+
+	mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL);
+	if (!mall_tc_entry)
+		return -ENOMEM;
+
+	mall_tc_entry->cookie = cls->cookie;
+	mall_tc_entry->type = DSA_PORT_MALL_DROP;
+	drop = &mall_tc_entry->drop;
+	drop->enable = true;
+	dp->bridge->local_rcv = 0;
+	err = dsa_slave_bridge_foreign_if_check(dp->bridge->dev, drop);
+
+	list_add_tail(&mall_tc_entry->list, &p->mall_tc_list);
+
+	return err;
+}
+
 static int
 dsa_slave_add_cls_matchall_police(struct net_device *dev,
 				  struct tc_cls_matchall_offload *cls,
@@ -1320,6 +1381,9 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 	else if (flow_offload_has_one_action(&cls->rule->action) &&
 		 cls->rule->action.entries[0].id == FLOW_ACTION_POLICE)
 		err = dsa_slave_add_cls_matchall_police(dev, cls, ingress);
+	else if (flow_offload_has_one_action(&cls->rule->action) &&
+		 cls->rule->action.entries[0].id == FLOW_ACTION_DROP)
+		err = dsa_slave_add_cls_matchall_drop(dev, cls, ingress);
 
 	return err;
 }
@@ -1347,6 +1411,13 @@ static void dsa_slave_del_cls_matchall(struct net_device *dev,
 		if (ds->ops->port_policer_del)
 			ds->ops->port_policer_del(ds, dp->index);
 		break;
+	case DSA_PORT_MALL_DROP:
+		if (!dp->bridge)
+			return;
+		dp->bridge->local_rcv = 1;
+		mall_tc_entry->drop.enable = false;
+		dsa_slave_bridge_foreign_if_check(dp->bridge->dev, &mall_tc_entry->drop);
+		break;
 	default:
 		WARN_ON(1);
 	}
@@ -1430,7 +1501,8 @@ static int dsa_slave_setup_tc_cls_flower(struct net_device *dev,
 	}
 }
 
-static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
+static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type,
+				       void *cls,
 				       void *cb_priv, bool ingress)
 {
 	struct net_device *dev = cb_priv;
@@ -1440,9 +1512,9 @@ static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 
 	switch (type) {
 	case TC_SETUP_CLSMATCHALL:
-		return dsa_slave_setup_tc_cls_matchall(dev, type_data, ingress);
+		return dsa_slave_setup_tc_cls_matchall(dev, cls, ingress);
 	case TC_SETUP_CLSFLOWER:
-		return dsa_slave_setup_tc_cls_flower(dev, type_data, ingress);
+		return dsa_slave_setup_tc_cls_flower(dev, cls, ingress);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -1514,6 +1586,133 @@ static int dsa_slave_setup_ft_block(struct dsa_switch *ds, int port,
 	return master->netdev_ops->ndo_setup_tc(master, TC_SETUP_FT, type_data);
 }
 
+static LIST_HEAD(dsa_slave_block_indr_cb_list);
+
+struct dsa_slave_indr_block_cb_priv {
+	struct dsa_switch_tree *dst;
+	struct net_device *bridge;
+	struct list_head list;
+};
+
+static int dsa_slave_setup_bridge_block_cb(enum tc_setup_type type,
+					   void *type_data,
+					   void *cb_priv)
+{
+	struct dsa_slave_indr_block_cb_priv *priv = cb_priv;
+	struct tc_cls_matchall_offload *cls;
+	struct dsa_port *dp;
+	int ret = 0;
+
+	cls = (struct tc_cls_matchall_offload *)type_data;
+	list_for_each_entry(dp, &priv->dst->ports, list) {
+		if (!dp->bridge || !dp->slave)
+			continue;
+
+		if (dp->bridge->dev != priv->bridge)
+			continue;
+
+		ret += dsa_slave_setup_tc_block_cb(type, cls, dp->slave, true);
+	}
+
+	return ret;
+}
+
+static struct dsa_slave_indr_block_cb_priv *
+dsa_slave_tc_indr_block_cb_lookup(struct dsa_switch_tree *dst, struct net_device *netdev)
+{
+	struct dsa_slave_indr_block_cb_priv *cb_priv;
+
+	list_for_each_entry(cb_priv, &dst->tc_indr_block_list, list)
+		if (cb_priv->bridge == netdev)
+			return cb_priv;
+
+	return NULL;
+}
+
+static void dsa_slave_setup_tc_indr_rel(void *cb_priv)
+{
+	struct dsa_slave_indr_block_cb_priv *priv = cb_priv;
+
+	list_del(&priv->list);
+	kfree(priv);
+}
+
+static int
+dsa_slave_setup_bridge_tc_indr_block(struct net_device *netdev, struct Qdisc *sch,
+				     struct dsa_switch_tree *dst,
+				     struct flow_block_offload *f, void *data,
+				     void (*cleanup)(struct flow_block_cb *block_cb))
+{
+	struct dsa_slave_indr_block_cb_priv *cb_priv;
+	struct flow_block_cb *block_cb;
+
+	if (f->binder_type != FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
+		return -EOPNOTSUPP;
+
+	switch (f->command) {
+	case FLOW_BLOCK_BIND:
+		cb_priv = kmalloc(sizeof(*cb_priv), GFP_KERNEL);
+		if (!cb_priv)
+			return -ENOMEM;
+
+		cb_priv->bridge = netdev;
+		cb_priv->dst = dst;
+		list_add(&cb_priv->list, &dst->tc_indr_block_list);
+
+		block_cb = flow_indr_block_cb_alloc(dsa_slave_setup_bridge_block_cb,
+						    cb_priv, cb_priv,
+						    dsa_slave_setup_tc_indr_rel, f,
+						    netdev, sch, data, cb_priv, cleanup);
+		if (IS_ERR(block_cb)) {
+			list_del(&cb_priv->list);
+			kfree(cb_priv);
+			return PTR_ERR(block_cb);
+		}
+
+		flow_block_cb_add(block_cb, f);
+		list_add_tail(&block_cb->driver_list, &dsa_slave_block_indr_cb_list);
+		break;
+	case FLOW_BLOCK_UNBIND:
+		cb_priv = dsa_slave_tc_indr_block_cb_lookup(dst, netdev);
+		if (!cb_priv)
+			return -ENOENT;
+
+		block_cb = flow_block_cb_lookup(f->block,
+						dsa_slave_setup_bridge_block_cb,
+						cb_priv);
+		if (!block_cb)
+			return -ENOENT;
+
+		flow_indr_block_cb_remove(block_cb, f);
+		list_del(&block_cb->driver_list);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int dsa_slave_setup_bridge_tc_indr_cb(struct net_device *netdev, struct Qdisc *sch,
+					     void *cb_priv,
+					     enum tc_setup_type type, void *type_data,
+					     void *data,
+					     void (*cleanup)(struct flow_block_cb *block_cb))
+{
+	if (!netdev || !netif_is_bridge_master(netdev))
+		return -EOPNOTSUPP;
+
+	switch (type) {
+	case TC_SETUP_BLOCK:
+		return dsa_slave_setup_bridge_tc_indr_block(netdev, sch, cb_priv,
+						     type_data, data, cleanup);
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type,
 			      void *type_data)
 {
@@ -1535,6 +1734,17 @@ static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type,
 	return ds->ops->port_setup_tc(ds, dp->index, type, type_data);
 }
 
+int dsa_setup_bridge_tc_indr(struct dsa_switch_tree *dst)
+{
+	return flow_indr_dev_register(dsa_slave_setup_bridge_tc_indr_cb, dst);
+}
+
+void dsa_cleanup_bridge_tc_indr(struct dsa_switch_tree *dst)
+{
+	flow_indr_dev_unregister(dsa_slave_setup_bridge_tc_indr_cb,
+				 dst, dsa_slave_setup_tc_indr_rel);
+}
+
 static int dsa_slave_get_rxnfc(struct net_device *dev,
 			       struct ethtool_rxnfc *nfc, u32 *rule_locs)
 {
@@ -2717,7 +2927,12 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 
 		break;
 	}
-	case NETDEV_CHANGEUPPER:
+	case NETDEV_CHANGEUPPER: {
+		struct netdev_notifier_changeupper_info *info = ptr;
+
+		if (netif_is_bridge_master(info->upper_dev))
+			dsa_slave_bridge_foreign_if_check(info->upper_dev, NULL);
+
 		if (dsa_slave_dev_check(dev))
 			return dsa_slave_changeupper(dev, ptr);
 
@@ -2725,6 +2940,7 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 			return dsa_slave_lag_changeupper(dev, ptr);
 
 		break;
+	}
 	case NETDEV_CHANGELOWERSTATE: {
 		struct netdev_notifier_changelowerstate_info *info = ptr;
 		struct dsa_port *dp;
-- 
2.25.1

[PATCH v3 net-next 2/2] net: dsa: Implement tc offloading for drop target.

[Mattias Forsblad]

Add the ability to handle tc matchall drop HW offloading for Marvell
switches.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd02902..84e319520d36 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1436,7 +1436,7 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 	 * bridge group.
 	 */
 	dsa_switch_for_each_port(other_dp, ds)
-		if (other_dp->type == DSA_PORT_TYPE_CPU ||
+		if ((other_dp->type == DSA_PORT_TYPE_CPU && dp->bridge->local_rcv_effective) ||
 		    other_dp->type == DSA_PORT_TYPE_DSA ||
 		    dsa_port_bridge_same(dp, other_dp))
 			pvlan |= BIT(other_dp->index);
@@ -6439,6 +6439,26 @@ static void mv88e6xxx_port_mirror_del(struct dsa_switch *ds, int port,
 	mutex_unlock(&chip->reg_lock);
 }
 
+static int mv88e6xxx_bridge_local_rcv(struct dsa_switch *ds, int port,
+				      struct dsa_mall_drop_tc_entry *drop)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct dsa_port *dp;
+	int err;
+
+	dp = dsa_to_port(ds, port);
+	if (!dp)
+		return -EINVAL;
+
+	mutex_lock(&chip->reg_lock);
+
+	err = mv88e6xxx_bridge_map(chip, *dp->bridge);
+
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
 static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 					   struct switchdev_brport_flags flags,
 					   struct netlink_ext_ack *extack)
@@ -6837,6 +6857,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_mdb_del           = mv88e6xxx_port_mdb_del,
 	.port_mirror_add	= mv88e6xxx_port_mirror_add,
 	.port_mirror_del	= mv88e6xxx_port_mirror_del,
+	.bridge_local_rcv	= mv88e6xxx_bridge_local_rcv,
 	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
 	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
 	.port_hwtstamp_set	= mv88e6xxx_port_hwtstamp_set,
-- 
2.25.1

Re: [PATCH v3 net-next 0/2] net: tc: dsa: Implement offload of matchall for bridged DSA ports

[Jakub Kicinski]

On Mon,  4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote:
> Limitations
> If there is tc rules on a bridge and all the ports leave the bridge
> and then joins the bridge again, the indirect framwork doesn't seem
> to reoffload them at join. The tc rules need to be torn down and
> re-added.

You should unregister your callback when last DSA port leaves and
re-register when first joins. That way you'll get replay.

Also the code needs to check the matchall is highest prio.

Re: [PATCH v3 net-next 0/2] net: tc: dsa: Implement offload of matchall for bridged DSA ports

[Mattias Forsblad]

On 2022-04-06 03:09, Jakub Kicinski wrote:
> On Mon,  4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote:
>> Limitations
>> If there is tc rules on a bridge and all the ports leave the bridge
>> and then joins the bridge again, the indirect framwork doesn't seem
>> to reoffload them at join. The tc rules need to be torn down and
>> re-added.
> 
> You should unregister your callback when last DSA port leaves and
> re-register when first joins. That way you'll get replay.
> 

So I've tried that and it partially works. I get the FLOW_BLOCK_BIND
callback but tcf_action_reoffload_cb() bails out here (tc_act_bind() == 1):

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/act_api.c?h=v5.18-rc1#n1819

B.c. that flag is set here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/cls_api.c?h=v5.18-rc1#n3088

I cannot say I fully understand this logic. Can you perhaps advise?

> Also the code needs to check the matchall is highest prio.

Isn't sufficient with this check?

	else if (flow_offload_has_one_action(&cls->rule->action) &&
		 cls->rule->action.entries[0].id == FLOW_ACTION_DROP)
		err = dsa_slave_add_cls_matchall_drop(dev, cls, ingress);

If it only has one action is must be the highest priority or am I 
missing something?

Re: [PATCH v3 net-next 0/2] net: tc: dsa: Implement offload of matchall for bridged DSA ports

[Mattias Forsblad]

On 2022-04-06 11:24, Mattias Forsblad wrote:
> On 2022-04-06 03:09, Jakub Kicinski wrote:
>> On Mon,  4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote:
>>> Limitations
>>> If there is tc rules on a bridge and all the ports leave the bridge
>>> and then joins the bridge again, the indirect framwork doesn't seem
>>> to reoffload them at join. The tc rules need to be torn down and
>>> re-added.
>>
>> You should unregister your callback when last DSA port leaves and
>> re-register when first joins. That way you'll get replay.
>>
> 
> So I've tried that and it partially works. I get the FLOW_BLOCK_BIND
> callback but tcf_action_reoffload_cb() bails out here (tc_act_bind() == 1):
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/act_api.c?h=v5.18-rc1#n1819
> 
> B.c. that flag is set here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/cls_api.c?h=v5.18-rc1#n3088
> 
> I cannot say I fully understand this logic. Can you perhaps advise?
> 

I cannot see that tcf_block_playback_offloads() -> mall_reoffload() is called at all in
this case.

Re: [PATCH v3 net-next 0/2] net: tc: dsa: Implement offload of matchall for bridged DSA ports

[Jakub Kicinski]

On Wed, 6 Apr 2022 11:24:46 +0200 Mattias Forsblad wrote:
> On 2022-04-06 03:09, Jakub Kicinski wrote:
> > On Mon,  4 Apr 2022 12:48:24 +0200 Mattias Forsblad wrote:  
> >> Limitations
> >> If there is tc rules on a bridge and all the ports leave the bridge
> >> and then joins the bridge again, the indirect framwork doesn't seem
> >> to reoffload them at join. The tc rules need to be torn down and
> >> re-added.  
> > 
> > You should unregister your callback when last DSA port leaves and
> > re-register when first joins. That way you'll get replay.
> >   
> 
> So I've tried that and it partially works. I get the FLOW_BLOCK_BIND
> callback but tcf_action_reoffload_cb() bails out here (tc_act_bind() == 1):
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/act_api.c?h=v5.18-rc1#n1819
> 
> B.c. that flag is set here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/cls_api.c?h=v5.18-rc1#n3088
> 
> I cannot say I fully understand this logic. Can you perhaps advise?

tcf_action_reoffload_cb() is for action-as-first-class-citizen offload.
I think you should get the reply thru tcf_block_playback_offloads().
But I haven't really kept up with the TC offloads, non-zero chance
they got broken :/

> > Also the code needs to check the matchall is highest prio.  
> 
> Isn't sufficient with this check?
> 
> 	else if (flow_offload_has_one_action(&cls->rule->action) &&
> 		 cls->rule->action.entries[0].id == FLOW_ACTION_DROP)
> 		err = dsa_slave_add_cls_matchall_drop(dev, cls, ingress);
> 
> If it only has one action is must be the highest priority or am I 
> missing something?

That just checks there is a single action on the rule.
There could be multiple rules, adding something like:

	if (flow->common.prio != 1)
		goto bail;

is what I had in mind.

Re: [PATCH v3 net-next 1/2] net: tc: dsa: Add the matchall filter with drop action for bridged DSA ports.

[Vladimir Oltean]

Please remove the "tc: " prefix from the commit message, you're not
modifying anything in that subsystem. Also remove the full stop at the
end of the commit message.

On Mon, Apr 04, 2022 at 12:48:25PM +0200, Mattias Forsblad wrote:
> Use the flow indirect framework on bridged DSA ports to be
> able to set up offloading of matchall filter with drop target.
> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  include/net/dsa.h  |  14 +++
>  net/dsa/dsa2.c     |   5 +
>  net/dsa/dsa_priv.h |   3 +
>  net/dsa/port.c     |   2 +
>  net/dsa/slave.c    | 224 ++++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 244 insertions(+), 4 deletions(-)
> 
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index ca6af86964bc..e87ceb841a70 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -247,6 +247,9 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
>  	INIT_LIST_HEAD(&dst->list);
>  	list_add_tail(&dst->list, &dsa_tree_list);
>  
> +	INIT_LIST_HEAD(&dst->tc_indr_block_list);
> +	dsa_setup_bridge_tc_indr(dst);
> +
>  	kref_init(&dst->refcount);
>  
>  	return dst;
> @@ -254,6 +257,8 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
>  
>  static void dsa_tree_free(struct dsa_switch_tree *dst)
>  {
> +	dsa_cleanup_bridge_tc_indr(dst);
> +
>  	if (dst->tag_ops)
>  		dsa_tag_driver_put(dst->tag_ops);
>  	list_del(&dst->list);
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 5d3f4a67dce1..456bcbe730ba 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -320,6 +320,9 @@ void dsa_slave_setup_tagger(struct net_device *slave);
>  int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
>  int dsa_slave_manage_vlan_filtering(struct net_device *dev,
>  				    bool vlan_filtering);
> +int dsa_setup_bridge_tc_indr(struct dsa_switch_tree *dst);
> +void dsa_cleanup_bridge_tc_indr(struct dsa_switch_tree *dst);
> +bool dsa_slave_dev_check(const struct net_device *dev);
>  
>  static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
>  {
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 32d472a82241..0c4522cc9eae 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -361,6 +361,8 @@ static int dsa_port_bridge_create(struct dsa_port *dp,
>  	refcount_set(&bridge->refcount, 1);
>  
>  	bridge->dev = br;
> +	bridge->local_rcv = 1;

Instead of 1 and 0 you should use true and false.

> +	bridge->local_rcv_effective = 1;
>  
>  	bridge->num = dsa_bridge_num_get(br, ds->max_num_bridges);
>  	if (ds->max_num_bridges && !bridge->num) {
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 41c69a6e7854..62219210d3ea 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1246,6 +1246,67 @@ dsa_slave_add_cls_matchall_mirred(struct net_device *dev,
>  	return err;
>  }
>  
> +static int dsa_slave_bridge_foreign_if_check(struct net_device *dev,
> +					     struct dsa_mall_drop_tc_entry *drop)
> +{
> +	struct net_device *lower_dev;
> +	struct dsa_port *dp = NULL;
> +	bool foreign_if = false;
> +	struct list_head *iter;
> +
> +	/* Check port types in this bridge */
> +	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +		if (dsa_slave_dev_check(lower_dev))
> +			dp = dsa_slave_to_port(lower_dev);

This is subtly buggy, because "dp" may have a NULL dp->bridge (software
forwarding), which is effectively equivalent to "foreign_if = true" in
that it requires sending traffic to the CPU. Yet you don't set
"foreign_if = true" when you detect such a port.

> +		else
> +			foreign_if = true;

And this is really buggy, because the bridge port may be an offloaded
LAG device which doesn't require forwarding to the CPU, yet you mark it
as foreign_if = true.

This is actually more complicated to treat, because not only do you need
to dig deeper through the lowers of the bridge port itself, but you have
to monitor CHANGEUPPER events where info->upper_dev isn't a bridge at all.
Just consider the case where a DSA port joins a LAG which is already a
bridge port, in a bridge with foreign interfaces.

> +	}
> +
> +	/* Offload only if we have requested it and the bridge only
> +	 * contains dsa ports
> +	 */
> +	if (!dp || !dp->bridge)
> +		return 0;

And this is subtly buggy too, because you only look at the last dp you
see. But in a mixed bridge with offloaded and unoffloaded DSA interfaces,
you effectively fail to update dp->bridge->local_rcv_effective, because
the dp->bridge of the last dp may be NULL, yet you've walked through
non-NULL dp->bridge structures which you've ignored.

> +
> +	if (!foreign_if)
> +		dp->bridge->local_rcv_effective = dp->bridge->local_rcv;
> +	else
> +		dp->bridge->local_rcv_effective = 1;
> +
> +	return dp->ds->ops->bridge_local_rcv(dp->ds, dp->index, drop);

Why does this function take an "int port" as argument, if the port you
provide is just one of many? Pass the struct dsa_bridge as argument.
Not to mention this should be a cross-chip notifier, maybe a cross-tree
notifier. And that you should only call ds->ops->bridge_local_rcv() only
if "A && B" changes in logical value.

> +}
> +
> +static int
> +dsa_slave_add_cls_matchall_drop(struct net_device *dev,
> +				struct tc_cls_matchall_offload *cls,
> +				bool ingress)
> +{
> +	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_mall_tc_entry *mall_tc_entry;
> +	struct dsa_mall_drop_tc_entry *drop;
> +	struct dsa_switch *ds = dp->ds;
> +	int err;
> +
> +	if (!ds->ops->bridge_local_rcv)
> +		return -EOPNOTSUPP;
> +
> +	mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL);
> +	if (!mall_tc_entry)
> +		return -ENOMEM;
> +
> +	mall_tc_entry->cookie = cls->cookie;
> +	mall_tc_entry->type = DSA_PORT_MALL_DROP;
> +	drop = &mall_tc_entry->drop;
> +	drop->enable = true;
> +	dp->bridge->local_rcv = 0;
> +	err = dsa_slave_bridge_foreign_if_check(dp->bridge->dev, drop);

Please check error before adding to list, and free mall_tc_entry.
In fact we have no reason to call dsa_slave_bridge_foreign_if_check(),
we need a smaller sub-function that doesn't uselessly walk again through
the lowers of dp->bridge->dev, as nothing changed there.

> +
> +	list_add_tail(&mall_tc_entry->list, &p->mall_tc_list);
> +
> +	return err;
> +}
> +
>  static int
>  dsa_slave_add_cls_matchall_police(struct net_device *dev,
>  				  struct tc_cls_matchall_offload *cls,
> @@ -1320,6 +1381,9 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
>  	else if (flow_offload_has_one_action(&cls->rule->action) &&
>  		 cls->rule->action.entries[0].id == FLOW_ACTION_POLICE)
>  		err = dsa_slave_add_cls_matchall_police(dev, cls, ingress);
> +	else if (flow_offload_has_one_action(&cls->rule->action) &&
> +		 cls->rule->action.entries[0].id == FLOW_ACTION_DROP)
> +		err = dsa_slave_add_cls_matchall_drop(dev, cls, ingress);
>  
>  	return err;
>  }
> @@ -1347,6 +1411,13 @@ static void dsa_slave_del_cls_matchall(struct net_device *dev,
>  		if (ds->ops->port_policer_del)
>  			ds->ops->port_policer_del(ds, dp->index);
>  		break;
> +	case DSA_PORT_MALL_DROP:
> +		if (!dp->bridge)
> +			return;
> +		dp->bridge->local_rcv = 1;
> +		mall_tc_entry->drop.enable = false;
> +		dsa_slave_bridge_foreign_if_check(dp->bridge->dev, &mall_tc_entry->drop);
> +		break;
>  	default:
>  		WARN_ON(1);
>  	}
> @@ -1430,7 +1501,8 @@ static int dsa_slave_setup_tc_cls_flower(struct net_device *dev,
>  	}
>  }
>  
> -static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
> +static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type,
> +				       void *cls,
>  				       void *cb_priv, bool ingress)
>  {
>  	struct net_device *dev = cb_priv;
> @@ -1440,9 +1512,9 @@ static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>  
>  	switch (type) {
>  	case TC_SETUP_CLSMATCHALL:
> -		return dsa_slave_setup_tc_cls_matchall(dev, type_data, ingress);
> +		return dsa_slave_setup_tc_cls_matchall(dev, cls, ingress);
>  	case TC_SETUP_CLSFLOWER:
> -		return dsa_slave_setup_tc_cls_flower(dev, type_data, ingress);
> +		return dsa_slave_setup_tc_cls_flower(dev, cls, ingress);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -1514,6 +1586,133 @@ static int dsa_slave_setup_ft_block(struct dsa_switch *ds, int port,
>  	return master->netdev_ops->ndo_setup_tc(master, TC_SETUP_FT, type_data);
>  }
>  
> +static LIST_HEAD(dsa_slave_block_indr_cb_list);
> +
> +struct dsa_slave_indr_block_cb_priv {
> +	struct dsa_switch_tree *dst;
> +	struct net_device *bridge;
> +	struct list_head list;
> +};
> +
> +static int dsa_slave_setup_bridge_block_cb(enum tc_setup_type type,
> +					   void *type_data,
> +					   void *cb_priv)
> +{
> +	struct dsa_slave_indr_block_cb_priv *priv = cb_priv;
> +	struct tc_cls_matchall_offload *cls;
> +	struct dsa_port *dp;
> +	int ret = 0;
> +
> +	cls = (struct tc_cls_matchall_offload *)type_data;
> +	list_for_each_entry(dp, &priv->dst->ports, list) {
> +		if (!dp->bridge || !dp->slave)
> +			continue;
> +
> +		if (dp->bridge->dev != priv->bridge)
> +			continue;
> +
> +		ret += dsa_slave_setup_tc_block_cb(type, cls, dp->slave, true);
> +	}
> +
> +	return ret;
> +}
> +
> +static struct dsa_slave_indr_block_cb_priv *
> +dsa_slave_tc_indr_block_cb_lookup(struct dsa_switch_tree *dst, struct net_device *netdev)
> +{
> +	struct dsa_slave_indr_block_cb_priv *cb_priv;
> +
> +	list_for_each_entry(cb_priv, &dst->tc_indr_block_list, list)
> +		if (cb_priv->bridge == netdev)
> +			return cb_priv;
> +
> +	return NULL;
> +}
> +
> +static void dsa_slave_setup_tc_indr_rel(void *cb_priv)
> +{
> +	struct dsa_slave_indr_block_cb_priv *priv = cb_priv;
> +
> +	list_del(&priv->list);
> +	kfree(priv);
> +}
> +
> +static int
> +dsa_slave_setup_bridge_tc_indr_block(struct net_device *netdev, struct Qdisc *sch,
> +				     struct dsa_switch_tree *dst,
> +				     struct flow_block_offload *f, void *data,
> +				     void (*cleanup)(struct flow_block_cb *block_cb))
> +{
> +	struct dsa_slave_indr_block_cb_priv *cb_priv;
> +	struct flow_block_cb *block_cb;
> +
> +	if (f->binder_type != FLOW_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
> +		return -EOPNOTSUPP;
> +
> +	switch (f->command) {
> +	case FLOW_BLOCK_BIND:
> +		cb_priv = kmalloc(sizeof(*cb_priv), GFP_KERNEL);
> +		if (!cb_priv)
> +			return -ENOMEM;
> +
> +		cb_priv->bridge = netdev;
> +		cb_priv->dst = dst;
> +		list_add(&cb_priv->list, &dst->tc_indr_block_list);
> +
> +		block_cb = flow_indr_block_cb_alloc(dsa_slave_setup_bridge_block_cb,
> +						    cb_priv, cb_priv,
> +						    dsa_slave_setup_tc_indr_rel, f,
> +						    netdev, sch, data, cb_priv, cleanup);
> +		if (IS_ERR(block_cb)) {
> +			list_del(&cb_priv->list);
> +			kfree(cb_priv);
> +			return PTR_ERR(block_cb);
> +		}
> +
> +		flow_block_cb_add(block_cb, f);
> +		list_add_tail(&block_cb->driver_list, &dsa_slave_block_indr_cb_list);
> +		break;
> +	case FLOW_BLOCK_UNBIND:
> +		cb_priv = dsa_slave_tc_indr_block_cb_lookup(dst, netdev);
> +		if (!cb_priv)
> +			return -ENOENT;
> +
> +		block_cb = flow_block_cb_lookup(f->block,
> +						dsa_slave_setup_bridge_block_cb,
> +						cb_priv);
> +		if (!block_cb)
> +			return -ENOENT;
> +
> +		flow_indr_block_cb_remove(block_cb, f);
> +		list_del(&block_cb->driver_list);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dsa_slave_setup_bridge_tc_indr_cb(struct net_device *netdev, struct Qdisc *sch,
> +					     void *cb_priv,
> +					     enum tc_setup_type type, void *type_data,
> +					     void *data,
> +					     void (*cleanup)(struct flow_block_cb *block_cb))
> +{
> +	if (!netdev || !netif_is_bridge_master(netdev))
> +		return -EOPNOTSUPP;
> +
> +	switch (type) {
> +	case TC_SETUP_BLOCK:
> +		return dsa_slave_setup_bridge_tc_indr_block(netdev, sch, cb_priv,
> +						     type_data, data, cleanup);
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
>  static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type,
>  			      void *type_data)
>  {
> @@ -1535,6 +1734,17 @@ static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type,
>  	return ds->ops->port_setup_tc(ds, dp->index, type, type_data);
>  }
>  
> +int dsa_setup_bridge_tc_indr(struct dsa_switch_tree *dst)
> +{
> +	return flow_indr_dev_register(dsa_slave_setup_bridge_tc_indr_cb, dst);

I wish I could help you here but unfortunately I'm no flow offload
expert either. existing_qdiscs_register() looks interesting, and I see
it was added in commit 74fc4f828769 ("net: Fix offloading indirect
devices dependency on qdisc order creation"), however I just see that
the flow block is bound, not that the filters are reoffloaded.
My elementary intuition says that the logic you're searching for simply
has not been written, but you should ask Eli Cohen or Ido Schimmel or
Jiri Pirko or Pablo Neira or Baowen Zheng about what to do, since the
extra logic is probably not trivial.

Under normal conditions I would have taken a deeper look at this, but it
would take me an absurd amount of time in this case, and the truth is
that I have a lot more emails to get up to speed and respond to. Sorry.

> +}
> +
> +void dsa_cleanup_bridge_tc_indr(struct dsa_switch_tree *dst)
> +{
> +	flow_indr_dev_unregister(dsa_slave_setup_bridge_tc_indr_cb,
> +				 dst, dsa_slave_setup_tc_indr_rel);
> +}
> +
>  static int dsa_slave_get_rxnfc(struct net_device *dev,
>  			       struct ethtool_rxnfc *nfc, u32 *rule_locs)
>  {
> @@ -2717,7 +2927,12 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  
>  		break;
>  	}
> -	case NETDEV_CHANGEUPPER:
> +	case NETDEV_CHANGEUPPER: {
> +		struct netdev_notifier_changeupper_info *info = ptr;
> +
> +		if (netif_is_bridge_master(info->upper_dev))
> +			dsa_slave_bridge_foreign_if_check(info->upper_dev, NULL);

I don't like this. The function returns an error code yet you are
ignoring it. I can see why that is, so please could you take this patch,
which I had in my tree for some undisclosed reason, and add it to your
series prior to yours? You may make adjustments to it as you wish.

-----------------------------[ cut here ]-----------------------------
From 9d9b9c145c3edf5e14711eed70fd96f02c733f0d Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 28 Mar 2022 16:34:13 +0300
Subject: [PATCH] net: dsa: walk through all changeupper notifier functions

Traditionally, DSA has had a single netdev notifier handling function
for each device type.

For the sake of code cleanliness, we would like to introduce more
handling functions which do one thing, but the conditions for entering
these functions start to overlap. Example: a handling function which
tracks whether any bridges contain both DSA and non-DSA interfaces.
Either this is placed before dsa_slave_changeupper(), case in which it
will prevent that function from executing, or we place it after
dsa_slave_changeupper(), case in which we will prevent it from
executing. The other alternative is to ignore errors from the new
handling function (not ideal).

To support this usage, we need to change the pattern. In the new model,
we enter all notifier handling sub-functions, and exit with NOTIFY_DONE
if there is nothing to do. This allows the sub-functions to be
relatively free-form and independent from each other.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f47048a624fb..f87109e7696d 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2463,6 +2463,9 @@ static int dsa_slave_changeupper(struct net_device *dev,
 	struct netlink_ext_ack *extack;
 	int err = NOTIFY_DONE;
 
+	if (!dsa_slave_dev_check(dev))
+		return err;
+
 	extack = netdev_notifier_info_to_extack(&info->info);
 
 	if (netif_is_bridge_master(info->upper_dev)) {
@@ -2517,6 +2520,9 @@ static int dsa_slave_prechangeupper(struct net_device *dev,
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 
+	if (!dsa_slave_dev_check(dev))
+		return NOTIFY_DONE;
+
 	if (netif_is_bridge_master(info->upper_dev) && !info->linking)
 		dsa_port_pre_bridge_leave(dp, info->upper_dev);
 	else if (netif_is_lag_master(info->upper_dev) && !info->linking)
@@ -2537,6 +2543,9 @@ dsa_slave_lag_changeupper(struct net_device *dev,
 	int err = NOTIFY_DONE;
 	struct dsa_port *dp;
 
+	if (!netif_is_lag_master(dev))
+		return err;
+
 	netdev_for_each_lower_dev(dev, lower, iter) {
 		if (!dsa_slave_dev_check(lower))
 			continue;
@@ -2566,6 +2575,9 @@ dsa_slave_lag_prechangeupper(struct net_device *dev,
 	int err = NOTIFY_DONE;
 	struct dsa_port *dp;
 
+	if (!netif_is_lag_master(dev))
+		return err;
+
 	netdev_for_each_lower_dev(dev, lower, iter) {
 		if (!dsa_slave_dev_check(lower))
 			continue;
@@ -2687,22 +2699,29 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		if (err != NOTIFY_DONE)
 			return err;
 
-		if (dsa_slave_dev_check(dev))
-			return dsa_slave_prechangeupper(dev, ptr);
+		err = dsa_slave_prechangeupper(dev, ptr);
+		if (notifier_to_errno(err))
+			return err;
 
-		if (netif_is_lag_master(dev))
-			return dsa_slave_lag_prechangeupper(dev, ptr);
+		err = dsa_slave_lag_prechangeupper(dev, ptr);
+		if (notifier_to_errno(err))
+			return err;
 
 		break;
 	}
-	case NETDEV_CHANGEUPPER:
-		if (dsa_slave_dev_check(dev))
-			return dsa_slave_changeupper(dev, ptr);
+	case NETDEV_CHANGEUPPER: {
+		int err;
+
+		err = dsa_slave_changeupper(dev, ptr);
+		if (notifier_to_errno(err))
+			return err;
 
-		if (netif_is_lag_master(dev))
-			return dsa_slave_lag_changeupper(dev, ptr);
+		err = dsa_slave_lag_changeupper(dev, ptr);
+		if (notifier_to_errno(err))
+			return err;
 
 		break;
+	}
 	case NETDEV_CHANGELOWERSTATE: {
 		struct netdev_notifier_changelowerstate_info *info = ptr;
 		struct dsa_port *dp;
-----------------------------[ cut here ]-----------------------------

> +
>  		if (dsa_slave_dev_check(dev))
>  			return dsa_slave_changeupper(dev, ptr);
>  
> @@ -2725,6 +2940,7 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  			return dsa_slave_lag_changeupper(dev, ptr);
>  
>  		break;
> +	}
>  	case NETDEV_CHANGELOWERSTATE: {
>  		struct netdev_notifier_changelowerstate_info *info = ptr;
>  		struct dsa_port *dp;
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 934958fda962..0ddfce552002 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -171,6 +171,9 @@ struct dsa_switch_tree {
>  
>  	/* Track the largest switch index within a tree */
>  	unsigned int last_switch;
> +
> +	/* For tc indirect bookkeeping */
> +	struct list_head tc_indr_block_list;
>  };
>  
>  /* LAG IDs are one-based, the dst->lags array is zero-based */
> @@ -212,6 +215,7 @@ static inline int dsa_lag_id(struct dsa_switch_tree *dst,
>  enum dsa_port_mall_action_type {
>  	DSA_PORT_MALL_MIRROR,
>  	DSA_PORT_MALL_POLICER,
> +	DSA_PORT_MALL_DROP,
>  };
>  
>  /* TC mirroring entry */
> @@ -220,6 +224,11 @@ struct dsa_mall_mirror_tc_entry {
>  	bool ingress;
>  };
>  
> +/* TC drop entry */
> +struct dsa_mall_drop_tc_entry {
> +	bool enable;
> +};
> +
>  /* TC port policer entry */
>  struct dsa_mall_policer_tc_entry {
>  	u32 burst;
> @@ -234,6 +243,7 @@ struct dsa_mall_tc_entry {
>  	union {
>  		struct dsa_mall_mirror_tc_entry mirror;
>  		struct dsa_mall_policer_tc_entry policer;
> +		struct dsa_mall_drop_tc_entry drop;
>  	};
>  };
>  
> @@ -241,6 +251,8 @@ struct dsa_bridge {
>  	struct net_device *dev;
>  	unsigned int num;
>  	bool tx_fwd_offload;
> +	u8 local_rcv:1;
> +	u8 local_rcv_effective:1;

I think there's value in tracking foreign interfaces as a completely
independent process from just making the "local_rcv" effective or not.
So that would affect the naming.

Would you mind working with this change?

-----------------------------[ cut here ]-----------------------------
From fe9088bbc9d341bee6170494b9860c6f21f4c8e2 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Thu, 7 Apr 2022 00:11:37 +0300
Subject: [PATCH] net: dsa: track whether bridges have foreign interfaces in
 them

There are certain optimizations which can be done for bridges where all
bridge ports are offloaded DSA interfaces. For instance, there is no
reason to enable flooding towards the CPU, given some extra checks (the
switch supports unicast and multicast filtering, the ports aren't
promiscuous - the bridge makes them promiscuous anyway, which we need
to change - etc).

As a preparation for those optimizations, create a function called
dsa_bridge_foreign_dev_update() which updates a new boolean of struct
dsa_bridge called "have_foreign".

Note that when dsa_port_bridge_create() is first called,
dsa_bridge_foreign_dev_update() is not called. It is called slightly
later (still under rtnl_mutex), leading to some DSA switch callbacks
(->port_bridge_join) being called with a potentially not up-to-date
"have_foreign" property. This can be changed if necessary.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h  |  3 +-
 net/dsa/dsa_priv.h |  1 +
 net/dsa/port.c     |  6 ++++
 net/dsa/slave.c    | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f2352d82e37b..0ea45a4acc80 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -240,8 +240,9 @@ struct dsa_mall_tc_entry {
 struct dsa_bridge {
 	struct net_device *dev;
 	unsigned int num;
-	bool tx_fwd_offload;
 	refcount_t refcount;
+	u8 tx_fwd_offload:1;
+	u8 have_foreign:1;
 };
 
 struct dsa_port {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 5d3f4a67dce1..d610776ecd76 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -320,6 +320,7 @@ void dsa_slave_setup_tagger(struct net_device *slave);
 int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
 int dsa_slave_manage_vlan_filtering(struct net_device *dev,
 				    bool vlan_filtering);
+int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev);
 
 static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
 {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index af9a815c2639..55fc54a78870 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -656,8 +656,14 @@ int dsa_port_lag_join(struct dsa_port *dp, struct net_device *lag_dev,
 	if (err)
 		goto err_bridge_join;
 
+	err = dsa_bridge_foreign_dev_update(bridge_dev);
+	if (err)
+		goto err_foreign_update;
+
 	return 0;
 
+err_foreign_update:
+	dsa_port_bridge_leave(dp, bridge_dev);
 err_bridge_join:
 	dsa_port_notify(dp, DSA_NOTIFIER_LAG_LEAVE, &info);
 err_lag_join:
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f87109e7696d..12a4b8dda493 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2595,6 +2595,18 @@ dsa_slave_lag_prechangeupper(struct net_device *dev,
 	return err;
 }
 
+static int dsa_bridge_changelower(struct net_device *dev,
+				  struct netdev_notifier_changeupper_info *info)
+{
+	int err;
+
+	if (!netif_is_bridge_master(info->upper_dev))
+		return NOTIFY_DONE;
+
+	err = dsa_bridge_foreign_dev_update(info->upper_dev);
+	return notifier_from_errno(err);
+}
+
 static int
 dsa_prevent_bridging_8021q_upper(struct net_device *dev,
 				 struct netdev_notifier_changeupper_info *info)
@@ -2720,6 +2732,10 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		if (notifier_to_errno(err))
 			return err;
 
+		err = dsa_bridge_changelower(dev, ptr);
+		if (notifier_to_errno(err))
+			return err;
+
 		break;
 	}
 	case NETDEV_CHANGELOWERSTATE: {
@@ -2874,6 +2890,64 @@ static bool dsa_foreign_dev_check(const struct net_device *dev,
 	return true;
 }
 
+int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev)
+{
+	struct net_device *first_slave = NULL, *lower;
+	bool have_foreign = false, dig_deeper = false;
+	struct dsa_bridge *bridge = NULL;
+	struct list_head *iter;
+
+	netdev_for_each_lower_dev(bridge_dev, lower, iter) {
+		if (dsa_slave_dev_check(lower)) {
+			struct dsa_port *dp = dsa_slave_to_port(lower);
+
+			if (dsa_port_offloads_bridge_dev(dp, bridge_dev)) {
+				bridge = dp->bridge;
+				if (!first_slave)
+					first_slave = lower;
+			} else {
+				/* Unoffloaded bridge port requires software
+				 * forwarding too. In effect this is the same
+				 * as a foreign interface.
+				 */
+				have_foreign = true;
+			}
+		} else if (netif_is_lag_master(lower)) {
+			/* If the bridge port is a LAG, we don't know for sure
+			 * whether it's foreign or not, so we'll have to go
+			 * through a second pass, after we find at least one
+			 * DSA slave interface.
+			 */
+			dig_deeper = true;
+		} else {
+			have_foreign = true;
+		}
+
+		/* No need to continue if we've found all we need to know */
+		if (bridge && have_foreign)
+			goto update;
+	}
+
+	/* Bridge with no DSA interface in it. */
+	if (!bridge)
+		return NOTIFY_DONE;
+
+	if (dig_deeper) {
+		netdev_for_each_lower_dev(bridge_dev, lower, iter) {
+			if (dsa_foreign_dev_check(first_slave, lower)) {
+				have_foreign = true;
+				break;
+			}
+		}
+	}
+
+update:
+	bridge->have_foreign = have_foreign;
+	/* TODO update all other consumers of this information */
+
+	return NOTIFY_DONE;
+}
+
 static int dsa_slave_fdb_event(struct net_device *dev,
 			       struct net_device *orig_dev,
 			       unsigned long event, const void *ctx,
-----------------------------[ cut here ]-----------------------------

>  	refcount_t refcount;
>  };
>  
> @@ -1034,6 +1046,8 @@ struct dsa_switch_ops {
>  	int	(*port_policer_add)(struct dsa_switch *ds, int port,
>  				    struct dsa_mall_policer_tc_entry *policer);
>  	void	(*port_policer_del)(struct dsa_switch *ds, int port);
> +	int	(*bridge_local_rcv)(struct dsa_switch *ds, int port,
> +				    struct dsa_mall_drop_tc_entry *drop);
>  	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
>  				 enum tc_setup_type type, void *type_data);
>  
> -- 
> 2.25.1
> 

Re: [PATCH v3 net-next 2/2] net: dsa: Implement tc offloading for drop target.

[Vladimir Oltean]

Again, please use an adequate commit prefix. In this case that is
"net: dsa: mv88e6xxx: ".

On Mon, Apr 04, 2022 at 12:48:26PM +0200, Mattias Forsblad wrote:
> Add the ability to handle tc matchall drop HW offloading for Marvell
> switches.
> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 64f4fdd02902..84e319520d36 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1436,7 +1436,7 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>  	 * bridge group.
>  	 */
>  	dsa_switch_for_each_port(other_dp, ds)
> -		if (other_dp->type == DSA_PORT_TYPE_CPU ||
> +		if ((other_dp->type == DSA_PORT_TYPE_CPU && dp->bridge->local_rcv_effective) ||

In the light of the idea that we should keep dsa_bridge :: have_foreign
an independent variable, maybe there should be a static inline
dsa_bridge_local_rcv(const struct dsa_bridge *bridge) helper which
returns bridge->have_foreign || bridge->local_rcv. Then you could use
that here.

Also note that said dsa_bridge_local_rcv() function returns a loop
invariant, so you should consider caching the result before using it in
dsa_switch_for_each_port().

>  		    other_dp->type == DSA_PORT_TYPE_DSA ||
>  		    dsa_port_bridge_same(dp, other_dp))
>  			pvlan |= BIT(other_dp->index);
> @@ -6439,6 +6439,26 @@ static void mv88e6xxx_port_mirror_del(struct dsa_switch *ds, int port,
>  	mutex_unlock(&chip->reg_lock);
>  }
>  
> +static int mv88e6xxx_bridge_local_rcv(struct dsa_switch *ds, int port,
> +				      struct dsa_mall_drop_tc_entry *drop)

I think you should ask yourself some questions about passing the "drop"
argument to ->bridge_local_rcv then never using it...

> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct dsa_port *dp;
> +	int err;
> +
> +	dp = dsa_to_port(ds, port);
> +	if (!dp)
> +		return -EINVAL;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	err = mv88e6xxx_bridge_map(chip, *dp->bridge);
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return err;
> +}
> +
>  static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>  					   struct switchdev_brport_flags flags,
>  					   struct netlink_ext_ack *extack)
> @@ -6837,6 +6857,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
>  	.port_mdb_del           = mv88e6xxx_port_mdb_del,
>  	.port_mirror_add	= mv88e6xxx_port_mirror_add,
>  	.port_mirror_del	= mv88e6xxx_port_mirror_del,
> +	.bridge_local_rcv	= mv88e6xxx_bridge_local_rcv,
>  	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
>  	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
>  	.port_hwtstamp_set	= mv88e6xxx_port_hwtstamp_set,
> -- 
> 2.25.1
> 

Re: [PATCH v3 net-next 1/2] net: tc: dsa: Add the matchall filter with drop action for bridged DSA ports.

[Vladimir Oltean]

On Thu, Apr 07, 2022 at 02:01:35AM +0300, Vladimir Oltean wrote:
> > +static int dsa_slave_bridge_foreign_if_check(struct net_device *dev,
> > +					     struct dsa_mall_drop_tc_entry *drop)
> > +{
> > +	struct net_device *lower_dev;
> > +	struct dsa_port *dp = NULL;
> > +	bool foreign_if = false;
> > +	struct list_head *iter;
> > +
> > +	/* Check port types in this bridge */
> > +	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> > +		if (dsa_slave_dev_check(lower_dev))
> > +			dp = dsa_slave_to_port(lower_dev);
> 
> This is subtly buggy, because "dp" may have a NULL dp->bridge (software
> forwarding), which is effectively equivalent to "foreign_if = true" in
> that it requires sending traffic to the CPU. Yet you don't set
> "foreign_if = true" when you detect such a port.
> 
> > +		else
> > +			foreign_if = true;
> 
> And this is really buggy, because the bridge port may be an offloaded
> LAG device which doesn't require forwarding to the CPU, yet you mark it
> as foreign_if = true.
> 
> This is actually more complicated to treat, because not only do you need
> to dig deeper through the lowers of the bridge port itself, but you have
> to monitor CHANGEUPPER events where info->upper_dev isn't a bridge at all.
> Just consider the case where a DSA port joins a LAG which is already a
> bridge port, in a bridge with foreign interfaces.
> 
> > +	}
> > +
> > +	/* Offload only if we have requested it and the bridge only
> > +	 * contains dsa ports
> > +	 */
> > +	if (!dp || !dp->bridge)
> > +		return 0;
> 
> And this is subtly buggy too, because you only look at the last dp you
> see. But in a mixed bridge with offloaded and unoffloaded DSA interfaces,
> you effectively fail to update dp->bridge->local_rcv_effective, because
> the dp->bridge of the last dp may be NULL, yet you've walked through
> non-NULL dp->bridge structures which you've ignored.

Edit: the implementation I had posted yesterday is buggy too, because if
the bridge contains no direct DSA slave interface, just LAG interfaces
offloaded by DSA interfaces, we'll fail to get a hold of a dp->bridge.

It seems like the most straightforward thing to do is to find the bridge
from our list of bridges rather than deducing it from the bridge device
lowers.

-----------------------------[ cut here ]-----------------------------
From f980ba7ac63c527fd2b9a674e2249d4308ac1620 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Thu, 7 Apr 2022 00:11:37 +0300
Subject: [PATCH] net: dsa: track whether bridges have foreign interfaces in
 them

There are certain optimizations which can be done for bridges where all
bridge ports are offloaded DSA interfaces. For instance, there is no
reason to enable flooding towards the CPU, given some extra checks (the
switch supports unicast and multicast filtering, the ports aren't
promiscuous - the bridge makes them promiscuous anyway, which we need
to change - etc).

As a preparation for those optimizations, create a function called
dsa_bridge_foreign_dev_update() which updates a new boolean of struct
dsa_bridge called "have_foreign".

Note that when dsa_port_bridge_create() is first called,
dsa_bridge_foreign_dev_update() is not called. It is called slightly
later (still under rtnl_mutex), leading to some DSA switch callbacks
(->port_bridge_join) being called with a potentially not up-to-date
"have_foreign" property. This can be changed if necessary.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h  |  3 ++-
 net/dsa/dsa_priv.h |  1 +
 net/dsa/port.c     |  7 +++++++
 net/dsa/slave.c    | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f2352d82e37b..0ea45a4acc80 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -240,8 +240,9 @@ struct dsa_mall_tc_entry {
 struct dsa_bridge {
 	struct net_device *dev;
 	unsigned int num;
-	bool tx_fwd_offload;
 	refcount_t refcount;
+	u8 tx_fwd_offload:1;
+	u8 have_foreign:1;
 };
 
 struct dsa_port {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 5d3f4a67dce1..d610776ecd76 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -320,6 +320,7 @@ void dsa_slave_setup_tagger(struct net_device *slave);
 int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
 int dsa_slave_manage_vlan_filtering(struct net_device *dev,
 				    bool vlan_filtering);
+int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev);
 
 static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
 {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index af9a815c2639..cbee564e1c22 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -656,8 +656,15 @@ int dsa_port_lag_join(struct dsa_port *dp, struct net_device *lag_dev,
 	if (err)
 		goto err_bridge_join;
 
+	err = dsa_bridge_foreign_dev_update(bridge_dev);
+	if (err)
+		goto err_foreign_update;
+
 	return 0;
 
+err_foreign_update:
+	dsa_port_pre_bridge_leave(dp, bridge_dev);
+	dsa_port_bridge_leave(dp, bridge_dev);
 err_bridge_join:
 	dsa_port_notify(dp, DSA_NOTIFIER_LAG_LEAVE, &info);
 err_lag_join:
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f87109e7696d..ce213b93ec05 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2595,6 +2595,18 @@ dsa_slave_lag_prechangeupper(struct net_device *dev,
 	return err;
 }
 
+static int dsa_bridge_changelower(struct net_device *dev,
+				  struct netdev_notifier_changeupper_info *info)
+{
+	int err;
+
+	if (!netif_is_bridge_master(info->upper_dev))
+		return NOTIFY_DONE;
+
+	err = dsa_bridge_foreign_dev_update(info->upper_dev);
+	return notifier_from_errno(err);
+}
+
 static int
 dsa_prevent_bridging_8021q_upper(struct net_device *dev,
 				 struct netdev_notifier_changeupper_info *info)
@@ -2720,6 +2732,10 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		if (notifier_to_errno(err))
 			return err;
 
+		err = dsa_bridge_changelower(dev, ptr);
+		if (notifier_to_errno(err))
+			return err;
+
 		break;
 	}
 	case NETDEV_CHANGELOWERSTATE: {
@@ -2874,6 +2890,42 @@ static bool dsa_foreign_dev_check(const struct net_device *dev,
 	return true;
 }
 
+int dsa_bridge_foreign_dev_update(struct net_device *bridge_dev)
+{
+	struct net_device *first_slave = NULL, *lower;
+	struct dsa_bridge *bridge = NULL;
+	struct dsa_switch_tree *dst;
+	bool have_foreign = false;
+	struct list_head *iter;
+	struct dsa_port *dp;
+
+	list_for_each_entry(dst, &dsa_tree_list, list) {
+		dsa_tree_for_each_user_port(dp, dst) {
+			if (dsa_port_offloads_bridge_dev(dp, bridge_dev)) {
+				bridge = dp->bridge;
+				first_slave = dp->slave;
+				break;
+			}
+		}
+	}
+
+	/* Bridge with no DSA interface in it */
+	if (!bridge)
+		return 0;
+
+	netdev_for_each_lower_dev(bridge_dev, lower, iter) {
+		if (dsa_foreign_dev_check(first_slave, lower)) {
+			have_foreign = true;
+			break;
+		}
+	}
+
+	bridge->have_foreign = have_foreign;
+	/* TODO update all other consumers of this information */
+
+	return 0;
+}
+
 static int dsa_slave_fdb_event(struct net_device *dev,
 			       struct net_device *orig_dev,
 			       unsigned long event, const void *ctx,
-----------------------------[ cut here ]-----------------------------