[PATCH net-next 0/4] net: dsa: use switchdev attr and obj handlers

[PATCH net-next 0/4] net: dsa: use switchdev attr and obj handlers

[Vivien Didelot]

This series reduces boilerplate in the handling of switchdev attribute and
object operations by using the switchdev_handle_* helpers, which check the
targeted devices and recurse into their lower devices.

This also brings back the ability to inspect operations targeting the bridge
device itself (where .orig_dev is the bridge device and .dev is the slave),
even though that is of no use yet and skipped by this series.

Vivien Didelot (4):
  net: dsa: do not check orig_dev in vlan del
  net: dsa: make cpu_dp non const
  net: dsa: make dsa_slave_dev_check use const
  net: dsa: use switchdev handle helpers

 include/net/dsa.h |  2 +-
 net/dsa/port.c    |  9 ------
 net/dsa/slave.c   | 81 ++++++++++++++++++++---------------------------
 3 files changed, 36 insertions(+), 56 deletions(-)

-- 
2.21.0

[PATCH net-next 1/4] net: dsa: do not check orig_dev in vlan del

[Vivien Didelot]

The current DSA code handling switchdev objects does not recurse into
the lower devices thus is never called with an orig_dev member being
a bridge device, hence remove this useless check.

At the same time, remove the comments about the callers, which is
unlikely to be updated if the code changes and thus confusing.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/port.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 70744fec9717..f83756eaf8a5 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -336,9 +336,6 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
-	/* Can be called from dsa_slave_port_obj_add() or
-	 * dsa_slave_vlan_rx_add_vid()
-	 */
 	if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
 
@@ -354,12 +351,6 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
-	if (vlan->obj.orig_dev && netif_is_bridge_master(vlan->obj.orig_dev))
-		return -EOPNOTSUPP;
-
-	/* Can be called from dsa_slave_port_obj_del() or
-	 * dsa_slave_vlan_rx_kill_vid()
-	 */
 	if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 
-- 
2.21.0

Re: [PATCH net-next 1/4] net: dsa: do not check orig_dev in vlan del

[Florian Fainelli]

On 6/11/19 2:47 PM, Vivien Didelot wrote:
> The current DSA code handling switchdev objects does not recurse into
> the lower devices thus is never called with an orig_dev member being
> a bridge device, hence remove this useless check.
> 
> At the same time, remove the comments about the callers, which is
> unlikely to be updated if the code changes and thus confusing.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 2/4] net: dsa: make cpu_dp non const

[Vivien Didelot]

A port may trigger operations on its dedicated CPU port, so using
cpu_dp as const will raise warnings. Make cpu_dp non const.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/net/dsa.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 82a2baa2dc48..1e8650fa8acc 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -181,7 +181,7 @@ struct dsa_port {
 	struct dsa_switch	*ds;
 	unsigned int		index;
 	const char		*name;
-	const struct dsa_port	*cpu_dp;
+	struct dsa_port		*cpu_dp;
 	const char		*mac;
 	struct device_node	*dn;
 	unsigned int		ageing_time;
-- 
2.21.0

Re: [PATCH net-next 2/4] net: dsa: make cpu_dp non const

[Florian Fainelli]

On 6/11/19 2:47 PM, Vivien Didelot wrote:
> A port may trigger operations on its dedicated CPU port, so using
> cpu_dp as const will raise warnings. Make cpu_dp non const.

Do we need that change now, or could this be part of another series? Anyway:

> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian

[PATCH net-next 3/4] net: dsa: make dsa_slave_dev_check use const

[Vivien Didelot]

The switchdev handle helpers make use of a device checking helper
requiring a const net_device. Make dsa_slave_dev_check compliant
to this.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/slave.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 289a6aa4b51c..cb436a05c9a8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -22,7 +22,7 @@
 
 #include "dsa_priv.h"
 
-static bool dsa_slave_dev_check(struct net_device *dev);
+static bool dsa_slave_dev_check(const struct net_device *dev);
 
 /* slave mii_bus handling ***************************************************/
 static int dsa_slave_phy_read(struct mii_bus *bus, int addr, int reg)
@@ -1408,7 +1408,7 @@ void dsa_slave_destroy(struct net_device *slave_dev)
 	free_netdev(slave_dev);
 }
 
-static bool dsa_slave_dev_check(struct net_device *dev)
+static bool dsa_slave_dev_check(const struct net_device *dev)
 {
 	return dev->netdev_ops == &dsa_slave_netdev_ops;
 }
-- 
2.21.0

Re: [PATCH net-next 3/4] net: dsa: make dsa_slave_dev_check use const

[Florian Fainelli]

On 6/11/19 2:47 PM, Vivien Didelot wrote:
> The switchdev handle helpers make use of a device checking helper
> requiring a const net_device. Make dsa_slave_dev_check compliant
> to this.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 4/4] net: dsa: use switchdev handle helpers

[Vivien Didelot]

Get rid of the dsa_slave_switchdev_port_{attr_set,obj}_event functions
in favor of the switchdev_handle_port_{attr_set,obj_add,obj_del}
helpers which recurse into the lower devices of the target interface.

This has the benefit of being aware of the operations made on the
bridge device itself, where orig_dev is the bridge, and dev is the
slave. This can be used later to configure bridge-wide attributes on
the hardware switches.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/slave.c | 77 +++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index cb436a05c9a8..915dd43873f9 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -283,6 +283,9 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	int ret;
 
+	if (attr->orig_dev != dev)
+		return -EOPNOTSUPP;
+
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
 		ret = dsa_port_set_state(dp, attr->u.stp_state, trans);
@@ -311,11 +314,15 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 
 static int dsa_slave_port_obj_add(struct net_device *dev,
 				  const struct switchdev_obj *obj,
-				  struct switchdev_trans *trans)
+				  struct switchdev_trans *trans,
+				  struct netlink_ext_ack *extack)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	int err;
 
+	if (obj->orig_dev != dev)
+		return -EOPNOTSUPP;
+
 	/* For the prepare phase, ensure the full set of changes is feasable in
 	 * one go in order to signal a failure properly. If an operation is not
 	 * supported, return -EOPNOTSUPP.
@@ -350,6 +357,9 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	int err;
 
+	if (obj->orig_dev != dev)
+		return -EOPNOTSUPP;
+
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
@@ -1479,19 +1489,6 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static int
-dsa_slave_switchdev_port_attr_set_event(struct net_device *netdev,
-		struct switchdev_notifier_port_attr_info *port_attr_info)
-{
-	int err;
-
-	err = dsa_slave_port_attr_set(netdev, port_attr_info->attr,
-				      port_attr_info->trans);
-
-	port_attr_info->handled = true;
-	return notifier_from_errno(err);
-}
-
 struct dsa_switchdev_event_work {
 	struct work_struct work;
 	struct switchdev_notifier_fdb_info fdb_info;
@@ -1566,13 +1563,18 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 {
 	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
 	struct dsa_switchdev_event_work *switchdev_work;
+	int err;
+
+	if (event == SWITCHDEV_PORT_ATTR_SET) {
+		err = switchdev_handle_port_attr_set(dev, ptr,
+						     dsa_slave_dev_check,
+						     dsa_slave_port_attr_set);
+		return notifier_from_errno(err);
+	}
 
 	if (!dsa_slave_dev_check(dev))
 		return NOTIFY_DONE;
 
-	if (event == SWITCHDEV_PORT_ATTR_SET)
-		return dsa_slave_switchdev_port_attr_set_event(dev, ptr);
-
 	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
 	if (!switchdev_work)
 		return NOTIFY_BAD;
@@ -1602,41 +1604,28 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 	return NOTIFY_BAD;
 }
 
-static int
-dsa_slave_switchdev_port_obj_event(unsigned long event,
-			struct net_device *netdev,
-			struct switchdev_notifier_port_obj_info *port_obj_info)
-{
-	int err = -EOPNOTSUPP;
-
-	switch (event) {
-	case SWITCHDEV_PORT_OBJ_ADD:
-		err = dsa_slave_port_obj_add(netdev, port_obj_info->obj,
-					     port_obj_info->trans);
-		break;
-	case SWITCHDEV_PORT_OBJ_DEL:
-		err = dsa_slave_port_obj_del(netdev, port_obj_info->obj);
-		break;
-	}
-
-	port_obj_info->handled = true;
-	return notifier_from_errno(err);
-}
-
 static int dsa_slave_switchdev_blocking_event(struct notifier_block *unused,
 					      unsigned long event, void *ptr)
 {
 	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
-
-	if (!dsa_slave_dev_check(dev))
-		return NOTIFY_DONE;
+	int err;
 
 	switch (event) {
-	case SWITCHDEV_PORT_OBJ_ADD: /* fall through */
+	case SWITCHDEV_PORT_OBJ_ADD:
+		err = switchdev_handle_port_obj_add(dev, ptr,
+						    dsa_slave_dev_check,
+						    dsa_slave_port_obj_add);
+		return notifier_from_errno(err);
 	case SWITCHDEV_PORT_OBJ_DEL:
-		return dsa_slave_switchdev_port_obj_event(event, dev, ptr);
+		err = switchdev_handle_port_obj_del(dev, ptr,
+						    dsa_slave_dev_check,
+						    dsa_slave_port_obj_del);
+		return notifier_from_errno(err);
 	case SWITCHDEV_PORT_ATTR_SET:
-		return dsa_slave_switchdev_port_attr_set_event(dev, ptr);
+		err = switchdev_handle_port_attr_set(dev, ptr,
+						     dsa_slave_dev_check,
+						     dsa_slave_port_attr_set);
+		return notifier_from_errno(err);
 	}
 
 	return NOTIFY_DONE;
-- 
2.21.0

Re: [PATCH net-next 4/4] net: dsa: use switchdev handle helpers

[Florian Fainelli]

On 6/11/19 2:47 PM, Vivien Didelot wrote:
> Get rid of the dsa_slave_switchdev_port_{attr_set,obj}_event functions
> in favor of the switchdev_handle_port_{attr_set,obj_add,obj_del}
> helpers which recurse into the lower devices of the target interface.
> 
> This has the benefit of being aware of the operations made on the
> bridge device itself, where orig_dev is the bridge, and dev is the
> slave. This can be used later to configure bridge-wide attributes on
> the hardware switches.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Nice cleanup, thanks!
-- 
Florian

Re: [PATCH net-next 0/4] net: dsa: use switchdev attr and obj handlers

[Vivien Didelot]

Hi David,

On Tue, 11 Jun 2019 17:47:43 -0400, Vivien Didelot <vivien.didelot@gmail.com> wrote:
> This series reduces boilerplate in the handling of switchdev attribute and
> object operations by using the switchdev_handle_* helpers, which check the
> targeted devices and recurse into their lower devices.
> 
> This also brings back the ability to inspect operations targeting the bridge
> device itself (where .orig_dev is the bridge device and .dev is the slave),
> even though that is of no use yet and skipped by this series.
> 
> Vivien Didelot (4):
>   net: dsa: do not check orig_dev in vlan del
>   net: dsa: make cpu_dp non const
>   net: dsa: make dsa_slave_dev_check use const
>   net: dsa: use switchdev handle helpers
> 
>  include/net/dsa.h |  2 +-
>  net/dsa/port.c    |  9 ------
>  net/dsa/slave.c   | 81 ++++++++++++++++++++---------------------------
>  3 files changed, 36 insertions(+), 56 deletions(-)

Please do not merge. The orig_dev != dev test in patch 4 is not correct,
because it skips the programming of the HOST_MDB object. I'll respin in a few.


Thanks,
Vivien