diff mbox series

[RFC,net-next,2/5] net: dsa: propagate flags down towards drivers

Message ID 20230117185714.3058453-3-netdev@kapio-technology.com
State New
Headers
Series ATU and FDB synchronization on locked ports |

Commit Message

Hans Schultz Jan. 17, 2023, 6:57 p.m. UTC 
  Dynamic FDB flag needs to be propagated through the DSA layer to be
added to drivers.
Use a u16 for fdb flags for future use, so that other flags can also be
sent the same way without having to change function interfaces.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 include/net/dsa.h |  2 ++
 net/dsa/port.c    | 28 ++++++++++++++++------------
 net/dsa/port.h    |  8 ++++----
 net/dsa/slave.c   | 17 +++++++++++++----
 net/dsa/switch.c  | 18 ++++++++++++------
 net/dsa/switch.h  |  1 +
 6 files changed, 48 insertions(+), 26 deletions(-)

Comments

Vladimir Oltean Jan. 17, 2023, 11:17 p.m. UTC | #1 
On Tue, Jan 17, 2023 at 07:57:11PM +0100, Hans J. Schultz wrote:
> Dynamic FDB flag needs to be propagated through the DSA layer to be
> added to drivers.
> Use a u16 for fdb flags for future use, so that other flags can also be
> sent the same way without having to change function interfaces.
> 
> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> ---
> @@ -3364,6 +3368,7 @@ static int dsa_slave_fdb_event(struct net_device *dev,
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
>  	bool host_addr = fdb_info->is_local;
>  	struct dsa_switch *ds = dp->ds;
> +	u16 fdb_flags = 0;
>  
>  	if (ctx && ctx != dp)
>  		return 0;
> @@ -3410,6 +3415,9 @@ static int dsa_slave_fdb_event(struct net_device *dev,
>  		   orig_dev->name, fdb_info->addr, fdb_info->vid,
>  		   host_addr ? " as host address" : "");
>  
> +	if (fdb_info->is_dyn)
> +		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
> +

Hmm, I don't think this is going to work with the assisted_learning_on_cpu_port
feature ("if (switchdev_fdb_is_dynamically_learned(fdb_info))"). The reason being
that a "dynamically learned" FDB entry (defined as this):

static inline bool
switchdev_fdb_is_dynamically_learned(const struct switchdev_notifier_fdb_info *fdb_info)
{
	return !fdb_info->added_by_user && !fdb_info->is_local;
}

is also dynamic in the DSA_FDB_FLAG_DYNAMIC sense. But we install a
static FDB entry for it on the CPU port.

And in your follow-up patch 3/5, you make all drivers except mv88e6xxx
ignore all DSA_FDB_FLAG_DYNAMIC entries (including the ones snooped from
address learning on software interfaces). So this breaks those drivers
which don't implement DSA_FDB_FLAG_DYNAMIC but do set ds->assisted_learning_on_cpu_port
to true.

I think you also want to look at the added_by_user flag to disambiguate
between a dynamic FDB entry added from learning (which it's ok to
offload as static, because software ageing will remove it) and one added
by the user.

>  	INIT_WORK(&switchdev_work->work, dsa_slave_switchdev_event_work);
>  	switchdev_work->event = event;
>  	switchdev_work->dev = dev;
> @@ -3418,6 +3426,7 @@ static int dsa_slave_fdb_event(struct net_device *dev,
>  	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
>  	switchdev_work->vid = fdb_info->vid;
>  	switchdev_work->host_addr = host_addr;
> +	switchdev_work->fdb_flags = fdb_flags;
>  
>  	dsa_schedule_work(&switchdev_work->work);
>
Hans Schultz Jan. 18, 2023, 10:35 p.m. UTC | #2 
On 2023-01-18 00:17, Vladimir Oltean wrote:
> On Tue, Jan 17, 2023 at 07:57:11PM +0100, Hans J. Schultz wrote:
>> Dynamic FDB flag needs to be propagated through the DSA layer to be
>> added to drivers.
>> Use a u16 for fdb flags for future use, so that other flags can also 
>> be
>> sent the same way without having to change function interfaces.
>> 
>> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
>> ---
>> @@ -3364,6 +3368,7 @@ static int dsa_slave_fdb_event(struct net_device 
>> *dev,
>>  	struct dsa_port *dp = dsa_slave_to_port(dev);
>>  	bool host_addr = fdb_info->is_local;
>>  	struct dsa_switch *ds = dp->ds;
>> +	u16 fdb_flags = 0;
>> 
>>  	if (ctx && ctx != dp)
>>  		return 0;
>> @@ -3410,6 +3415,9 @@ static int dsa_slave_fdb_event(struct net_device 
>> *dev,
>>  		   orig_dev->name, fdb_info->addr, fdb_info->vid,
>>  		   host_addr ? " as host address" : "");
>> 
>> +	if (fdb_info->is_dyn)
>> +		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
>> +
> 
> Hmm, I don't think this is going to work with the 
> assisted_learning_on_cpu_port
> feature ("if (switchdev_fdb_is_dynamically_learned(fdb_info))"). The
> reason being
> that a "dynamically learned" FDB entry (defined as this):
> 
> static inline bool
> switchdev_fdb_is_dynamically_learned(const struct
> switchdev_notifier_fdb_info *fdb_info)
> {
> 	return !fdb_info->added_by_user && !fdb_info->is_local;
> }
> 
> is also dynamic in the DSA_FDB_FLAG_DYNAMIC sense. But we install a
> static FDB entry for it on the CPU port.
> 
> And in your follow-up patch 3/5, you make all drivers except mv88e6xxx
> ignore all DSA_FDB_FLAG_DYNAMIC entries (including the ones snooped 
> from
> address learning on software interfaces). So this breaks those drivers
> which don't implement DSA_FDB_FLAG_DYNAMIC but do set
> ds->assisted_learning_on_cpu_port
> to true.

I am not sure I understand you entirely.
 From my standpoint I see it as so: that until now any fdb entry coming 
to port_fdb_add() (or port_fdb_del()) are seen as static entries. And 
this changes nothing with respect to those static entries as how drivers 
handle them.
When the new dynamic flag is true, all drivers will ignore it in patch 
#3, so basically nothing will change by that. Then in patch #5 the 
dynamic flag is handled by the mv88e6xxx driver.

I don't know the assisted_learning_on_cpu_port feature you mention, but 
there has still not been anything but static entries going towards 
port_fdb_add() yet...

> 
> I think you also want to look at the added_by_user flag to disambiguate
> between a dynamic FDB entry added from learning (which it's ok to
> offload as static, because software ageing will remove it) and one 
> added
> by the user.
> 
>>  	INIT_WORK(&switchdev_work->work, dsa_slave_switchdev_event_work);
>>  	switchdev_work->event = event;
>>  	switchdev_work->dev = dev;
>> @@ -3418,6 +3426,7 @@ static int dsa_slave_fdb_event(struct net_device 
>> *dev,
>>  	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
>>  	switchdev_work->vid = fdb_info->vid;
>>  	switchdev_work->host_addr = host_addr;
>> +	switchdev_work->fdb_flags = fdb_flags;
>> 
>>  	dsa_schedule_work(&switchdev_work->work);
>>
Vladimir Oltean Jan. 18, 2023, 11:01 p.m. UTC | #3 
On Wed, Jan 18, 2023 at 11:35:08PM +0100, netdev@kapio-technology.com wrote:
> I am not sure I understand you entirely.
> From my standpoint I see it as so: that until now any fdb entry coming to
> port_fdb_add() (or port_fdb_del()) are seen as static entries. And this
> changes nothing with respect to those static entries as how drivers handle
> them.

This is true; it is implicit that the port_fdb_add() and port_fdb_del()
DSA methods request switches to operate on static FDB entries (in hardware).

> When the new dynamic flag is true, all drivers will ignore it in patch #3,
> so basically nothing will change by that.

This is not true, because it assumes that DSA never called port_fdb_add()
up until now for bridge FDB entries with the BR_FDB_STATIC flag unset,
which is incorrect (it did).

So what will change is that drivers which used to react to those bridge
FDB entries will stop doing so.

> Then in patch #5 the dynamic flag is handled by the mv88e6xxx driver.
> 
> I don't know the assisted_learning_on_cpu_port feature you mention, but
> there has still not been anything but static entries going towards
> port_fdb_add() yet...

For starters, you can read the commit message of the patch that
introduced it, which is d5f19486cee7 ("net: dsa: listen for
SWITCHDEV_{FDB,DEL}_ADD_TO_DEVICE on foreign bridge neighbors").
Hans Schultz Jan. 22, 2023, 11:08 a.m. UTC | #4 
On 2023-01-19 00:01, Vladimir Oltean wrote:
> On Wed, Jan 18, 2023 at 11:35:08PM +0100, netdev@kapio-technology.com 
> wrote:

>> When the new dynamic flag is true, all drivers will ignore it in patch 
>> #3,
>> so basically nothing will change by that.
> 
> This is not true, because it assumes that DSA never called 
> port_fdb_add()
> up until now for bridge FDB entries with the BR_FDB_STATIC flag unset,
> which is incorrect (it did).
> 
> So what will change is that drivers which used to react to those bridge
> FDB entries will stop doing so.
> 

So the solution to this problem could be to only set the is_dyn flag in 
combination with the added_by_user flag. So an 'and' operation with the 
two in br_switchdev_fdb_populate()?
diff mbox series

Patch

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 96086289aa9b..f697d56e52d5 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -818,6 +818,8 @@  static inline bool dsa_port_tree_same(const struct dsa_port *a,
 	return a->ds->dst == b->ds->dst;
 }
 
+#define DSA_FDB_FLAG_DYNAMIC		BIT(0)
+
 typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
 			      bool is_static, void *data);
 struct dsa_switch_ops {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 67ad1adec2a2..65b8c4470d59 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -976,12 +976,13 @@  int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu)
 }
 
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid)
+		     u16 vid, u16 fdb_flags)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.fdb_flags = fdb_flags,
 		.db = {
 			.type = DSA_DB_BRIDGE,
 			.bridge = *dp->bridge,
@@ -999,12 +1000,13 @@  int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 }
 
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid)
+		     u16 vid, u16 fdb_flags)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.fdb_flags = fdb_flags,
 		.db = {
 			.type = DSA_DB_BRIDGE,
 			.bridge = *dp->bridge,
@@ -1019,12 +1021,13 @@  int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 
 static int dsa_port_host_fdb_add(struct dsa_port *dp,
 				 const unsigned char *addr, u16 vid,
-				 struct dsa_db db)
+				 u16 fdb_flags, struct dsa_db db)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.fdb_flags = fdb_flags,
 		.db = db,
 	};
 
@@ -1042,11 +1045,11 @@  int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
 		.dp = dp,
 	};
 
-	return dsa_port_host_fdb_add(dp, addr, vid, db);
+	return dsa_port_host_fdb_add(dp, addr, vid, 0, db);
 }
 
-int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
-				 const unsigned char *addr, u16 vid)
+int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
+				 u16 vid, u16 fdb_flags)
 {
 	struct net_device *master = dsa_port_to_master(dp);
 	struct dsa_db db = {
@@ -1065,17 +1068,18 @@  int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
 			return err;
 	}
 
-	return dsa_port_host_fdb_add(dp, addr, vid, db);
+	return dsa_port_host_fdb_add(dp, addr, vid, fdb_flags, db);
 }
 
 static int dsa_port_host_fdb_del(struct dsa_port *dp,
 				 const unsigned char *addr, u16 vid,
-				 struct dsa_db db)
+				 u16 fdb_flags, struct dsa_db db)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.fdb_flags = fdb_flags,
 		.db = db,
 	};
 
@@ -1093,11 +1097,11 @@  int dsa_port_standalone_host_fdb_del(struct dsa_port *dp,
 		.dp = dp,
 	};
 
-	return dsa_port_host_fdb_del(dp, addr, vid, db);
+	return dsa_port_host_fdb_del(dp, addr, vid, 0, db);
 }
 
-int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
-				 const unsigned char *addr, u16 vid)
+int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
+				 u16 vid, u16 fdb_flags)
 {
 	struct net_device *master = dsa_port_to_master(dp);
 	struct dsa_db db = {
@@ -1112,7 +1116,7 @@  int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
 			return err;
 	}
 
-	return dsa_port_host_fdb_del(dp, addr, vid, db);
+	return dsa_port_host_fdb_del(dp, addr, vid, fdb_flags, db);
 }
 
 int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
diff --git a/net/dsa/port.h b/net/dsa/port.h
index 9c218660d223..0a4f487e3c05 100644
--- a/net/dsa/port.h
+++ b/net/dsa/port.h
@@ -47,17 +47,17 @@  int dsa_port_vlan_msti(struct dsa_port *dp,
 		       const struct switchdev_vlan_msti *msti);
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu);
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid);
+		     u16 vid, u16 fdb_flags);
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid);
+		     u16 vid, u16 fdb_flags);
 int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
 				     const unsigned char *addr, u16 vid);
 int dsa_port_standalone_host_fdb_del(struct dsa_port *dp,
 				     const unsigned char *addr, u16 vid);
 int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-				 u16 vid);
+				 u16 vid, u16 fdb_flags);
 int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-				 u16 vid);
+				 u16 vid, u16 fdb_flags);
 int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 			 u16 vid);
 int dsa_port_lag_fdb_del(struct dsa_port *dp, const unsigned char *addr,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index aab79c355224..b6381baa1ff3 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -39,6 +39,7 @@  struct dsa_switchdev_event_work {
 	 */
 	unsigned char addr[ETH_ALEN];
 	u16 vid;
+	u16 fdb_flags;
 	bool host_addr;
 };
 
@@ -3295,6 +3296,7 @@  static void dsa_slave_switchdev_event_work(struct work_struct *work)
 		container_of(work, struct dsa_switchdev_event_work, work);
 	const unsigned char *addr = switchdev_work->addr;
 	struct net_device *dev = switchdev_work->dev;
+	u16 fdb_flags = switchdev_work->fdb_flags;
 	u16 vid = switchdev_work->vid;
 	struct dsa_switch *ds;
 	struct dsa_port *dp;
@@ -3306,11 +3308,12 @@  static void dsa_slave_switchdev_event_work(struct work_struct *work)
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 		if (switchdev_work->host_addr)
-			err = dsa_port_bridge_host_fdb_add(dp, addr, vid);
+			err = dsa_port_bridge_host_fdb_add(dp, addr,
+							   vid, fdb_flags);
 		else if (dp->lag)
 			err = dsa_port_lag_fdb_add(dp, addr, vid);
 		else
-			err = dsa_port_fdb_add(dp, addr, vid);
+			err = dsa_port_fdb_add(dp, addr, vid, fdb_flags);
 		if (err) {
 			dev_err(ds->dev,
 				"port %d failed to add %pM vid %d to fdb: %d\n",
@@ -3322,11 +3325,12 @@  static void dsa_slave_switchdev_event_work(struct work_struct *work)
 
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
 		if (switchdev_work->host_addr)
-			err = dsa_port_bridge_host_fdb_del(dp, addr, vid);
+			err = dsa_port_bridge_host_fdb_del(dp, addr,
+							   vid, fdb_flags);
 		else if (dp->lag)
 			err = dsa_port_lag_fdb_del(dp, addr, vid);
 		else
-			err = dsa_port_fdb_del(dp, addr, vid);
+			err = dsa_port_fdb_del(dp, addr, vid, fdb_flags);
 		if (err) {
 			dev_err(ds->dev,
 				"port %d failed to delete %pM vid %d from fdb: %d\n",
@@ -3364,6 +3368,7 @@  static int dsa_slave_fdb_event(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	bool host_addr = fdb_info->is_local;
 	struct dsa_switch *ds = dp->ds;
+	u16 fdb_flags = 0;
 
 	if (ctx && ctx != dp)
 		return 0;
@@ -3410,6 +3415,9 @@  static int dsa_slave_fdb_event(struct net_device *dev,
 		   orig_dev->name, fdb_info->addr, fdb_info->vid,
 		   host_addr ? " as host address" : "");
 
+	if (fdb_info->is_dyn)
+		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
+
 	INIT_WORK(&switchdev_work->work, dsa_slave_switchdev_event_work);
 	switchdev_work->event = event;
 	switchdev_work->dev = dev;
@@ -3418,6 +3426,7 @@  static int dsa_slave_fdb_event(struct net_device *dev,
 	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
 	switchdev_work->vid = fdb_info->vid;
 	switchdev_work->host_addr = host_addr;
+	switchdev_work->fdb_flags = fdb_flags;
 
 	dsa_schedule_work(&switchdev_work->work);
 
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index d5bc4bb7310d..9b0d6ce0f7da 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -239,7 +239,7 @@  static int dsa_port_do_mdb_del(struct dsa_port *dp,
 }
 
 static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-			       u16 vid, struct dsa_db db)
+			       u16 vid, u16 fdb_flags, struct dsa_db db)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_mac_addr *a;
@@ -283,7 +283,7 @@  static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 }
 
 static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-			       u16 vid, struct dsa_db db)
+			       u16 vid, u16 fdb_flags, struct dsa_db db)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_mac_addr *a;
@@ -410,7 +410,9 @@  static int dsa_switch_host_fdb_add(struct dsa_switch *ds,
 								info->db);
 			} else {
 				err = dsa_port_do_fdb_add(dp, info->addr,
-							  info->vid, info->db);
+							  info->vid,
+							  info->fdb_flags,
+							  info->db);
 			}
 			if (err)
 				break;
@@ -438,7 +440,9 @@  static int dsa_switch_host_fdb_del(struct dsa_switch *ds,
 								info->db);
 			} else {
 				err = dsa_port_do_fdb_del(dp, info->addr,
-							  info->vid, info->db);
+							  info->vid,
+							  info->fdb_flags,
+							  info->db);
 			}
 			if (err)
 				break;
@@ -457,7 +461,8 @@  static int dsa_switch_fdb_add(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_add)
 		return -EOPNOTSUPP;
 
-	return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->db);
+	return dsa_port_do_fdb_add(dp, info->addr, info->vid,
+				   info->fdb_flags, info->db);
 }
 
 static int dsa_switch_fdb_del(struct dsa_switch *ds,
@@ -469,7 +474,8 @@  static int dsa_switch_fdb_del(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_del)
 		return -EOPNOTSUPP;
 
-	return dsa_port_do_fdb_del(dp, info->addr, info->vid, info->db);
+	return dsa_port_do_fdb_del(dp, info->addr, info->vid,
+				   info->fdb_flags, info->db);
 }
 
 static int dsa_switch_lag_fdb_add(struct dsa_switch *ds,
diff --git a/net/dsa/switch.h b/net/dsa/switch.h
index 15e67b95eb6e..824ad7950785 100644
--- a/net/dsa/switch.h
+++ b/net/dsa/switch.h
@@ -55,6 +55,7 @@  struct dsa_notifier_fdb_info {
 	const struct dsa_port *dp;
 	const unsigned char *addr;
 	u16 vid;
+	u16 fdb_flags;
 	struct dsa_db db;
 };