diff mbox series

[RFC,net-next,1/3] net: dsa: don't use switchdev_notifier_fdb_info in dsa_switchdev_event_work

Message ID 20201108131953.2462644-2-olteanv@gmail.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Offload learnt bridge addresses to DSA | expand

Commit Message

Vladimir Oltean Nov. 8, 2020, 1:19 p.m. UTC
Currently DSA doesn't add FDB entries on the CPU port, because it only
does so through switchdev events added_by_user and associated with a
DSA net_device, and there are none of those for the CPU port.

But actually FDB entries towards the CPU port make sense for some use
cases where certain addresses need to be processed in software, and in
that case we need to call dsa_switchdev_event_work.

There is just one problem with the existing code: it passes a structure
in dsa_switchdev_event_work which was retrieved directly from switchdev,
so it contains a net_device. We need to generalize the contents to
something that covers the CPU port as well: the "ds, port" tuple is fine
for that.

Note that the new procedure for notifying the successful FDB offload is
inspired from the rocker model.

Also, nothing was being done if added_by_user was false. Let's check for
that a lot earlier, and don't actually bother to schedule the whole
workqueue for nothing.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/dsa_priv.h |  12 ++++++
 net/dsa/slave.c    | 100 ++++++++++++++++++++++-----------------------
 2 files changed, 62 insertions(+), 50 deletions(-)

Comments

Vladimir Oltean Nov. 8, 2020, 11:57 p.m. UTC | #1
On Sun, Nov 08, 2020 at 03:19:51PM +0200, Vladimir Oltean wrote:
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 59c80052e950..30db8230e30b 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2062,72 +2062,62 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -struct dsa_switchdev_event_work {
> -	struct work_struct work;
> -	struct switchdev_notifier_fdb_info fdb_info;
> -	struct net_device *dev;
> -	unsigned long event;
> -};
> +static void
> +dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
> +{
> +	struct dsa_switch *ds = switchdev_work->ds;
> +	struct dsa_port *dp = dsa_to_port(ds, switchdev_work->port);
> +	struct switchdev_notifier_fdb_info info;
> +
> +	if (!dsa_is_user_port(ds, dp->index))
> +		return;
> +
> +	info.addr = switchdev_work->addr;
> +	info.vid = switchdev_work->vid;
> +	info.offloaded = true;
> +	call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED,
> +				 dp->slave, &info.info, NULL);
> +}
>  
>  static void dsa_slave_switchdev_event_work(struct work_struct *work)
>  {
>  	struct dsa_switchdev_event_work *switchdev_work =
>  		container_of(work, struct dsa_switchdev_event_work, work);
> -	struct net_device *dev = switchdev_work->dev;
> -	struct switchdev_notifier_fdb_info *fdb_info;
> -	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	struct dsa_switch *ds = switchdev_work->ds;
> +	struct dsa_port *dp;
>  	int err;
>  
> +	dp = dsa_to_port(ds, switchdev_work->port);
> +
>  	rtnl_lock();
>  	switch (switchdev_work->event) {
>  	case SWITCHDEV_FDB_ADD_TO_DEVICE:
> -		fdb_info = &switchdev_work->fdb_info;
> -		if (!fdb_info->added_by_user)
> -			break;
> -
> -		err = dsa_port_fdb_add(dp, fdb_info->addr, fdb_info->vid);
> +		err = dsa_port_fdb_add(dp, switchdev_work->addr,
> +				       switchdev_work->vid);
>  		if (err) {
> -			netdev_dbg(dev, "fdb add failed err=%d\n", err);
> +			dev_dbg(ds->dev, "port %d fdb add failed err=%d\n",
> +				dp->index, err);
>  			break;
>  		}
> -		fdb_info->offloaded = true;
> -		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
> -					 &fdb_info->info, NULL);
> +		dsa_fdb_offload_notify(switchdev_work);
>  		break;
>  
>  	case SWITCHDEV_FDB_DEL_TO_DEVICE:
> -		fdb_info = &switchdev_work->fdb_info;
> -		if (!fdb_info->added_by_user)
> -			break;
> -
> -		err = dsa_port_fdb_del(dp, fdb_info->addr, fdb_info->vid);
> +		err = dsa_port_fdb_del(dp, switchdev_work->addr,
> +				       switchdev_work->vid);
>  		if (err) {
> -			netdev_dbg(dev, "fdb del failed err=%d\n", err);
> -			dev_close(dev);
> +			dev_dbg(ds->dev, "port %d fdb del failed err=%d\n",
> +				dp->index, err);
> +			if (dsa_is_user_port(ds, dp->index))
> +				dev_close(dp->slave);

Not sure that this dev_close() serves any real purpose, it stands in the
way a little. It was introduced "to indicate inconsistent situation".

commit c9eb3e0f870105242a15a5e628ed202cf32afe0d
Author: Arkadi Sharshevsky <arkadis@mellanox.com>
Date:   Sun Aug 6 16:15:42 2017 +0300

    net: dsa: Add support for learning FDB through notification

    Add support for learning FDB through notification. The driver defers
    the hardware update via ordered work queue. In case of a successful
    FDB add a notification is sent back to bridge.

    In case of hw FDB del failure the static FDB will be deleted from
    the bridge, thus, the interface is moved to down state in order to
    indicate inconsistent situation.

I hope it's ok to only close a net device if it exists, I can't think of
anything smarter.

>  		}
>  		break;
>  	}
>  	rtnl_unlock();
>  
> -	kfree(switchdev_work->fdb_info.addr);
>  	kfree(switchdev_work);
> -	dev_put(dev);
> -}
> -
> -static int
> -dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
> -				  switchdev_work,
> -				  const struct switchdev_notifier_fdb_info *
> -				  fdb_info)
> -{
> -	memcpy(&switchdev_work->fdb_info, fdb_info,
> -	       sizeof(switchdev_work->fdb_info));
> -	switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
> -	if (!switchdev_work->fdb_info.addr)
> -		return -ENOMEM;
> -	ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
> -			fdb_info->addr);
> -	return 0;
> +	if (dsa_is_user_port(ds, dp->index))
> +		dev_put(dp->slave);
>  }

The reference counting is broken here. It doesn't line up with the
dev_hold(dev) done in the last patch, which is on a non-DSA interface.
Anyway I think a net_device refcount is way too much for what we need
here, which is to ensure that DSA doesn't get unbound. I think I'll just
simplify to get_device(ds->dev) and put_device(ds->dev).
diff mbox series

Patch

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 12998bf04e55..03671ed984a1 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -73,6 +73,18 @@  struct dsa_notifier_mtu_info {
 	int mtu;
 };
 
+struct dsa_switchdev_event_work {
+	struct dsa_switch *ds;
+	int port;
+	struct work_struct work;
+	unsigned long event;
+	/* Specific for SWITCHDEV_FDB_ADD_TO_DEVICE and
+	 * SWITCHDEV_FDB_DEL_TO_DEVICE
+	 */
+	unsigned char addr[ETH_ALEN];
+	u16 vid;
+};
+
 struct dsa_slave_priv {
 	/* Copy of CPU port xmit for faster access in slave transmit hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 59c80052e950..30db8230e30b 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2062,72 +2062,62 @@  static int dsa_slave_netdevice_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-struct dsa_switchdev_event_work {
-	struct work_struct work;
-	struct switchdev_notifier_fdb_info fdb_info;
-	struct net_device *dev;
-	unsigned long event;
-};
+static void
+dsa_fdb_offload_notify(struct dsa_switchdev_event_work *switchdev_work)
+{
+	struct dsa_switch *ds = switchdev_work->ds;
+	struct dsa_port *dp = dsa_to_port(ds, switchdev_work->port);
+	struct switchdev_notifier_fdb_info info;
+
+	if (!dsa_is_user_port(ds, dp->index))
+		return;
+
+	info.addr = switchdev_work->addr;
+	info.vid = switchdev_work->vid;
+	info.offloaded = true;
+	call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED,
+				 dp->slave, &info.info, NULL);
+}
 
 static void dsa_slave_switchdev_event_work(struct work_struct *work)
 {
 	struct dsa_switchdev_event_work *switchdev_work =
 		container_of(work, struct dsa_switchdev_event_work, work);
-	struct net_device *dev = switchdev_work->dev;
-	struct switchdev_notifier_fdb_info *fdb_info;
-	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = switchdev_work->ds;
+	struct dsa_port *dp;
 	int err;
 
+	dp = dsa_to_port(ds, switchdev_work->port);
+
 	rtnl_lock();
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
-		fdb_info = &switchdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
-			break;
-
-		err = dsa_port_fdb_add(dp, fdb_info->addr, fdb_info->vid);
+		err = dsa_port_fdb_add(dp, switchdev_work->addr,
+				       switchdev_work->vid);
 		if (err) {
-			netdev_dbg(dev, "fdb add failed err=%d\n", err);
+			dev_dbg(ds->dev, "port %d fdb add failed err=%d\n",
+				dp->index, err);
 			break;
 		}
-		fdb_info->offloaded = true;
-		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
-					 &fdb_info->info, NULL);
+		dsa_fdb_offload_notify(switchdev_work);
 		break;
 
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
-		fdb_info = &switchdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
-			break;
-
-		err = dsa_port_fdb_del(dp, fdb_info->addr, fdb_info->vid);
+		err = dsa_port_fdb_del(dp, switchdev_work->addr,
+				       switchdev_work->vid);
 		if (err) {
-			netdev_dbg(dev, "fdb del failed err=%d\n", err);
-			dev_close(dev);
+			dev_dbg(ds->dev, "port %d fdb del failed err=%d\n",
+				dp->index, err);
+			if (dsa_is_user_port(ds, dp->index))
+				dev_close(dp->slave);
 		}
 		break;
 	}
 	rtnl_unlock();
 
-	kfree(switchdev_work->fdb_info.addr);
 	kfree(switchdev_work);
-	dev_put(dev);
-}
-
-static int
-dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
-				  switchdev_work,
-				  const struct switchdev_notifier_fdb_info *
-				  fdb_info)
-{
-	memcpy(&switchdev_work->fdb_info, fdb_info,
-	       sizeof(switchdev_work->fdb_info));
-	switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
-	if (!switchdev_work->fdb_info.addr)
-		return -ENOMEM;
-	ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
-			fdb_info->addr);
-	return 0;
+	if (dsa_is_user_port(ds, dp->index))
+		dev_put(dp->slave);
 }
 
 /* Called under rcu_read_lock() */
@@ -2135,7 +2125,9 @@  static int dsa_slave_switchdev_event(struct notifier_block *unused,
 				     unsigned long event, void *ptr)
 {
 	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+	const struct switchdev_notifier_fdb_info *fdb_info;
 	struct dsa_switchdev_event_work *switchdev_work;
+	struct dsa_port *dp;
 	int err;
 
 	if (event == SWITCHDEV_PORT_ATTR_SET) {
@@ -2148,20 +2140,32 @@  static int dsa_slave_switchdev_event(struct notifier_block *unused,
 	if (!dsa_slave_dev_check(dev))
 		return NOTIFY_DONE;
 
+	dp = dsa_slave_to_port(dev);
+
 	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
 	if (!switchdev_work)
 		return NOTIFY_BAD;
 
 	INIT_WORK(&switchdev_work->work,
 		  dsa_slave_switchdev_event_work);
-	switchdev_work->dev = dev;
+	switchdev_work->ds = dp->ds;
+	switchdev_work->port = dp->index;
 	switchdev_work->event = event;
 
 	switch (event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
-		if (dsa_slave_switchdev_fdb_work_init(switchdev_work, ptr))
-			goto err_fdb_work_init;
+		fdb_info = ptr;
+
+		if (!fdb_info->added_by_user) {
+			kfree(switchdev_work);
+			return NOTIFY_OK;
+		}
+
+		ether_addr_copy(switchdev_work->addr,
+				fdb_info->addr);
+		switchdev_work->vid = fdb_info->vid;
+
 		dev_hold(dev);
 		break;
 	default:
@@ -2171,10 +2175,6 @@  static int dsa_slave_switchdev_event(struct notifier_block *unused,
 
 	dsa_schedule_work(&switchdev_work->work);
 	return NOTIFY_OK;
-
-err_fdb_work_init:
-	kfree(switchdev_work);
-	return NOTIFY_BAD;
 }
 
 static int dsa_slave_switchdev_blocking_event(struct notifier_block *unused,