All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: switchdev: restrict vid range abstraction
@ 2015-07-26 23:45 Vivien Didelot
  2015-07-29  7:31 ` Scott Feldman
  0 siblings, 1 reply; 7+ messages in thread
From: Vivien Didelot @ 2015-07-26 23:45 UTC (permalink / raw)
  To: netdev
  Cc: Vivien Didelot, David S. Miller, Scott Feldman, Jiri Pirko,
	Florian Fainelli, linux-kernel, kernel

This patch replaces the vid_begin and vid_end members of the
switchdev_obj_vlan structure for a single vid member. This way, the VID
range abstraction is restricted to switchdev, not exposed to drivers.

The main benefice to do so is to allow the prepare phase to be called
for each VID, not only once for the whole range.

For example, when adding VLANs 2-5, a switch chip may not be able to add
hardware VLAN 2 due to some bridge restriction (thus must return
-EOPNOTSUPP), while VLAN 3-5 are allowed.

Also, moving the iteration code to switchdev simplifies its
implementation and its drivers code (e.g. Rocker).

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/ethernet/rocker/rocker.c | 74 ++++++++++--------------------------
 include/net/switchdev.h              |  3 +-
 net/bridge/br_vlan.c                 |  6 +--
 net/switchdev/switchdev.c            | 70 +++++++++++++++++-----------------
 4 files changed, 58 insertions(+), 95 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 7b4c347..f0dfd77 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4366,41 +4366,25 @@ static int rocker_port_attr_set(struct net_device *dev,
 }
 
 static int rocker_port_vlan_add(struct rocker_port *rocker_port,
-				enum switchdev_trans trans, u16 vid, u16 flags)
+				enum switchdev_trans trans,
+				const struct switchdev_obj_vlan *vlan)
 {
 	int err;
 
 	/* XXX deal with flags for PVID and untagged */
 
-	err = rocker_port_vlan(rocker_port, trans, 0, vid);
+	err = rocker_port_vlan(rocker_port, trans, 0, vlan->vid);
 	if (err)
 		return err;
 
-	err = rocker_port_router_mac(rocker_port, trans, 0, htons(vid));
+	err = rocker_port_router_mac(rocker_port, trans, 0, htons(vlan->vid));
 	if (err)
 		rocker_port_vlan(rocker_port, trans,
-				 ROCKER_OP_FLAG_REMOVE, vid);
+				 ROCKER_OP_FLAG_REMOVE, vlan->vid);
 
 	return err;
 }
 
-static int rocker_port_vlans_add(struct rocker_port *rocker_port,
-				 enum switchdev_trans trans,
-				 const struct switchdev_obj_vlan *vlan)
-{
-	u16 vid;
-	int err;
-
-	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
-		err = rocker_port_vlan_add(rocker_port, trans,
-					   vid, vlan->flags);
-		if (err)
-			return err;
-	}
-
-	return 0;
-}
-
 static int rocker_port_fdb_add(struct rocker_port *rocker_port,
 			       enum switchdev_trans trans,
 			       const struct switchdev_obj_fdb *fdb)
@@ -4434,8 +4418,8 @@ static int rocker_port_obj_add(struct net_device *dev,
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = rocker_port_vlans_add(rocker_port, obj->trans,
-					    &obj->u.vlan);
+		err = rocker_port_vlan_add(rocker_port, obj->trans,
+					   &obj->u.vlan);
 		break;
 	case SWITCHDEV_OBJ_IPV4_FIB:
 		fib4 = &obj->u.ipv4_fib;
@@ -4455,32 +4439,17 @@ static int rocker_port_obj_add(struct net_device *dev,
 }
 
 static int rocker_port_vlan_del(struct rocker_port *rocker_port,
-				u16 vid, u16 flags)
+				const struct switchdev_obj_vlan *vlan)
 {
 	int err;
 
 	err = rocker_port_router_mac(rocker_port, SWITCHDEV_TRANS_NONE,
-				     ROCKER_OP_FLAG_REMOVE, htons(vid));
+				     ROCKER_OP_FLAG_REMOVE, htons(vlan->vid));
 	if (err)
 		return err;
 
 	return rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE,
-				ROCKER_OP_FLAG_REMOVE, vid);
-}
-
-static int rocker_port_vlans_del(struct rocker_port *rocker_port,
-				 const struct switchdev_obj_vlan *vlan)
-{
-	u16 vid;
-	int err;
-
-	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
-		err = rocker_port_vlan_del(rocker_port, vid, vlan->flags);
-		if (err)
-			return err;
-	}
-
-	return 0;
+				ROCKER_OP_FLAG_REMOVE, vlan->vid);
 }
 
 static int rocker_port_fdb_del(struct rocker_port *rocker_port,
@@ -4505,7 +4474,7 @@ static int rocker_port_obj_del(struct net_device *dev,
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = rocker_port_vlans_del(rocker_port, &obj->u.vlan);
+		err = rocker_port_vlan_del(rocker_port, &obj->u.vlan);
 		break;
 	case SWITCHDEV_OBJ_IPV4_FIB:
 		fib4 = &obj->u.ipv4_fib;
@@ -4565,7 +4534,7 @@ static int rocker_port_vlan_dump(const struct rocker_port *rocker_port,
 		vlan->flags = 0;
 		if (rocker_vlan_id_is_internal(htons(vid)))
 			vlan->flags |= BRIDGE_VLAN_INFO_PVID;
-		vlan->vid_begin = vlan->vid_end = vid;
+		vlan->vid = vid;
 		err = obj->cb(rocker_port->dev, obj);
 		if (err)
 			break;
@@ -4944,9 +4913,9 @@ static void rocker_port_dev_addr_init(struct rocker_port *rocker_port)
 static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
 {
 	const struct pci_dev *pdev = rocker->pdev;
+	const struct switchdev_obj_vlan vlan0 = { 0 };
 	struct rocker_port *rocker_port;
 	struct net_device *dev;
-	u16 untagged_vid = 0;
 	int err;
 
 	dev = alloc_etherdev(sizeof(struct rocker_port));
@@ -4992,8 +4961,7 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
 	rocker_port->internal_vlan_id =
 		rocker_port_internal_vlan_id_get(rocker_port, dev->ifindex);
 
-	err = rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE,
-				   untagged_vid, 0);
+	err = rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE, &vlan0);
 	if (err) {
 		netdev_err(rocker_port->dev, "install untagged VLAN failed\n");
 		goto err_untagged_vlan;
@@ -5241,7 +5209,7 @@ static bool rocker_port_dev_check(const struct net_device *dev)
 static int rocker_port_bridge_join(struct rocker_port *rocker_port,
 				   struct net_device *bridge)
 {
-	u16 untagged_vid = 0;
+	const struct switchdev_obj_vlan vlan0 = { 0 };
 	int err;
 
 	/* Port is joining bridge, so the internal VLAN for the
@@ -5250,7 +5218,7 @@ static int rocker_port_bridge_join(struct rocker_port *rocker_port,
 	 * re-add once internal VLAN has changed.
 	 */
 
-	err = rocker_port_vlan_del(rocker_port, untagged_vid, 0);
+	err = rocker_port_vlan_del(rocker_port, &vlan0);
 	if (err)
 		return err;
 
@@ -5262,16 +5230,15 @@ static int rocker_port_bridge_join(struct rocker_port *rocker_port,
 	rocker_port->bridge_dev = bridge;
 	switchdev_port_fwd_mark_set(rocker_port->dev, bridge, true);
 
-	return rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE,
-				    untagged_vid, 0);
+	return rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE, &vlan0);
 }
 
 static int rocker_port_bridge_leave(struct rocker_port *rocker_port)
 {
-	u16 untagged_vid = 0;
+	const struct switchdev_obj_vlan vlan0 = { 0 };
 	int err;
 
-	err = rocker_port_vlan_del(rocker_port, untagged_vid, 0);
+	err = rocker_port_vlan_del(rocker_port, &vlan0);
 	if (err)
 		return err;
 
@@ -5285,8 +5252,7 @@ static int rocker_port_bridge_leave(struct rocker_port *rocker_port)
 				    false);
 	rocker_port->bridge_dev = NULL;
 
-	err = rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE,
-				   untagged_vid, 0);
+	err = rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE, &vlan0);
 	if (err)
 		return err;
 
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 89da893..337a6d7 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -57,8 +57,7 @@ struct switchdev_obj {
 	union {
 		struct switchdev_obj_vlan {		/* PORT_VLAN */
 			u16 flags;
-			u16 vid_begin;
-			u16 vid_end;
+			u16 vid;
 		} vlan;
 		struct switchdev_obj_ipv4_fib {		/* IPV4_FIB */
 			u32 dst;
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 0d41f81..323bf15 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -54,8 +54,7 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
 			.id = SWITCHDEV_OBJ_PORT_VLAN,
 			.u.vlan = {
 				.flags = flags,
-				.vid_begin = vid,
-				.vid_end = vid,
+				.vid = vid,
 			},
 		};
 
@@ -132,8 +131,7 @@ static void __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
 		struct switchdev_obj vlan_obj = {
 			.id = SWITCHDEV_OBJ_PORT_VLAN,
 			.u.vlan = {
-				.vid_begin = vid,
-				.vid_end = vid,
+				.vid = vid,
 			},
 		};
 
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 33bafa2..81e8d82 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -444,42 +444,32 @@ static int switchdev_port_vlan_dump_cb(struct net_device *dev,
 	struct switchdev_obj_vlan *vlan = &dump->obj.u.vlan;
 	int err = 0;
 
-	if (vlan->vid_begin > vlan->vid_end)
-		return -EINVAL;
-
 	if (dump->filter_mask & RTEXT_FILTER_BRVLAN) {
 		dump->flags = vlan->flags;
-		for (dump->begin = dump->end = vlan->vid_begin;
-		     dump->begin <= vlan->vid_end;
-		     dump->begin++, dump->end++) {
-			err = switchdev_port_vlan_dump_put(dev, dump);
-			if (err)
-				return err;
-		}
+		dump->begin = dump->end = vlan->vid;
+		err = switchdev_port_vlan_dump_put(dev, dump);
+		if (err)
+			return err;
 	} else if (dump->filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED) {
-		if (dump->begin > vlan->vid_begin &&
-		    dump->begin >= vlan->vid_end) {
-			if ((dump->begin - 1) == vlan->vid_end &&
+		if (dump->begin > vlan->vid) {
+			if ((dump->begin - 1) == vlan->vid &&
 			    dump->flags == vlan->flags) {
 				/* prepend */
-				dump->begin = vlan->vid_begin;
+				dump->begin = vlan->vid;
 			} else {
 				err = switchdev_port_vlan_dump_put(dev, dump);
 				dump->flags = vlan->flags;
-				dump->begin = vlan->vid_begin;
-				dump->end = vlan->vid_end;
+				dump->begin = dump->end = vlan->vid;
 			}
-		} else if (dump->end <= vlan->vid_begin &&
-		           dump->end < vlan->vid_end) {
-			if ((dump->end  + 1) == vlan->vid_begin &&
+		} else if (dump->end < vlan->vid) {
+			if ((dump->end  + 1) == vlan->vid &&
 			    dump->flags == vlan->flags) {
 				/* append */
-				dump->end = vlan->vid_end;
+				dump->end = vlan->vid;
 			} else {
 				err = switchdev_port_vlan_dump_put(dev, dump);
 				dump->flags = vlan->flags;
-				dump->begin = vlan->vid_begin;
-				dump->end = vlan->vid_end;
+				dump->begin = dump->end = vlan->vid;
 			}
 		} else {
 			err = -EINVAL;
@@ -625,6 +615,7 @@ static int switchdev_port_br_afspec(struct net_device *dev,
 		.id = SWITCHDEV_OBJ_PORT_VLAN,
 	};
 	struct switchdev_obj_vlan *vlan = &obj.u.vlan;
+	u16 vid_begin = 0, vid_end = 0;
 	int rem;
 	int err;
 
@@ -634,26 +625,35 @@ static int switchdev_port_br_afspec(struct net_device *dev,
 		if (nla_len(attr) != sizeof(struct bridge_vlan_info))
 			return -EINVAL;
 		vinfo = nla_data(attr);
-		vlan->flags = vinfo->flags;
+
 		if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
-			if (vlan->vid_begin)
+			if (vid_begin)
 				return -EINVAL;
-			vlan->vid_begin = vinfo->vid;
+			vid_begin = vinfo->vid;
 		} else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
-			if (!vlan->vid_begin)
+			u16 vid;
+
+			if (!vid_begin)
 				return -EINVAL;
-			vlan->vid_end = vinfo->vid;
-			if (vlan->vid_end <= vlan->vid_begin)
+			vid_end = vinfo->vid;
+			if (vid_end <= vid_begin)
 				return -EINVAL;
-			err = f(dev, &obj);
-			if (err)
-				return err;
-			memset(vlan, 0, sizeof(*vlan));
+
+			for (vid = vid_begin; vid <= vid_end; ++vid) {
+				vlan->flags = vinfo->flags;
+				vlan->vid = vid;
+				err = f(dev, &obj);
+				if (err)
+					return err;
+				memset(vlan, 0, sizeof(*vlan));
+			}
+			vid_begin = vid_end = 0;
 		} else {
-			if (vlan->vid_begin)
+			if (vid_begin)
 				return -EINVAL;
-			vlan->vid_begin = vinfo->vid;
-			vlan->vid_end = vinfo->vid;
+
+			vlan->flags = vinfo->flags;
+			vlan->vid = vinfo->vid;
 			err = f(dev, &obj);
 			if (err)
 				return err;
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: switchdev: restrict vid range abstraction
  2015-07-26 23:45 [PATCH] net: switchdev: restrict vid range abstraction Vivien Didelot
@ 2015-07-29  7:31 ` Scott Feldman
  2015-07-29 18:28   ` David Miller
  0 siblings, 1 reply; 7+ messages in thread
From: Scott Feldman @ 2015-07-29  7:31 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: Netdev, David S. Miller, Jiri Pirko, Florian Fainelli,
	linux-kernel, kernel

On Sun, Jul 26, 2015 at 4:45 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> This patch replaces the vid_begin and vid_end members of the
> switchdev_obj_vlan structure for a single vid member. This way, the VID
> range abstraction is restricted to switchdev, not exposed to drivers.
>
> The main benefice to do so is to allow the prepare phase to be called
> for each VID, not only once for the whole range.
>
> For example, when adding VLANs 2-5, a switch chip may not be able to add
> hardware VLAN 2 due to some bridge restriction (thus must return
> -EOPNOTSUPP), while VLAN 3-5 are allowed.

Hi Vivien,

Since the netlink request (for example vlan add) includes the range,
I'm not seeing how we can response with success for the satisfied
vlans in the range, and also respond with an error for the unsatisfied
vlans in the range.   In other words, from the netlink msgs
perspective, we need to treat a vlan range as all-or-nothing.  So in
your example, if hw can't add vlan 2, we fail the entire request to
add range 2-5.  This is where the prepare phase checks to make sure
the entire request can be satisfied before committing to hw.

-scott

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: switchdev: restrict vid range abstraction
  2015-07-29  7:31 ` Scott Feldman
@ 2015-07-29 18:28   ` David Miller
  2015-07-29 19:14     ` Vivien Didelot
  0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2015-07-29 18:28 UTC (permalink / raw)
  To: sfeldma; +Cc: vivien.didelot, netdev, jiri, f.fainelli, linux-kernel, kernel

From: Scott Feldman <sfeldma@gmail.com>
Date: Wed, 29 Jul 2015 00:31:44 -0700

> Since the netlink request (for example vlan add) includes the range,
> I'm not seeing how we can response with success for the satisfied
> vlans in the range, and also respond with an error for the unsatisfied
> vlans in the range.   In other words, from the netlink msgs
> perspective, we need to treat a vlan range as all-or-nothing.  So in
> your example, if hw can't add vlan 2, we fail the entire request to
> add range 2-5.  This is where the prepare phase checks to make sure
> the entire request can be satisfied before committing to hw.

This was my concern with the change as well.

The user asked for the range to be installed, so if any portion
of it cannot be done we must not make any changes to the HW
configuration and fail the entire request.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: switchdev: restrict vid range abstraction
  2015-07-29 18:28   ` David Miller
@ 2015-07-29 19:14     ` Vivien Didelot
  2015-07-29 19:28       ` David Miller
  2015-07-29 21:17       ` Scott Feldman
  0 siblings, 2 replies; 7+ messages in thread
From: Vivien Didelot @ 2015-07-29 19:14 UTC (permalink / raw)
  To: David; +Cc: sfeldma, netdev, jiri, Florian Fainelli, linux-kernel, kernel

Hi Scott, David,

On Jul 29, 2015, at 2:28 PM, David davem@davemloft.net wrote:

> From: Scott Feldman <sfeldma@gmail.com>
> Date: Wed, 29 Jul 2015 00:31:44 -0700
> 
>> Since the netlink request (for example vlan add) includes the range,
>> I'm not seeing how we can response with success for the satisfied
>> vlans in the range, and also respond with an error for the unsatisfied
>> vlans in the range.   In other words, from the netlink msgs
>> perspective, we need to treat a vlan range as all-or-nothing.  So in
>> your example, if hw can't add vlan 2, we fail the entire request to
>> add range 2-5.  This is where the prepare phase checks to make sure
>> the entire request can be satisfied before committing to hw.

I made this change in order to start restricting the bridge abstraction
to switchdev, since IMHO its info flags do not add much value to the
switch chip drivers perspective.

While a range might be convenient to a user, exposing it to drivers is
likely to end up writing the same vid_begin to vid_end for loop.

> This was my concern with the change as well.
> 
> The user asked for the range to be installed, so if any portion
> of it cannot be done we must not make any changes to the HW
> configuration and fail the entire request.

I understand the concern with the netlink request.

However, this can be confusing to someone. With the previous example:

    bridge vlan add dev port0 vid 2-5 master

must fail for the entire range (due to the single netlink request). But:

    bridge vlan add dev port0 vid 2 master

will silently fallback to software VLAN (assuming that the driver
correctly returned -EOPNOTSUPP in the prepare phase). In other words, no
changes has been committed to the hardware.

Thanks,
-v

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: switchdev: restrict vid range abstraction
  2015-07-29 19:14     ` Vivien Didelot
@ 2015-07-29 19:28       ` David Miller
  2015-07-29 21:17       ` Scott Feldman
  1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2015-07-29 19:28 UTC (permalink / raw)
  To: vivien.didelot; +Cc: sfeldma, netdev, jiri, f.fainelli, linux-kernel, kernel

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Wed, 29 Jul 2015 15:14:05 -0400 (EDT)

> While a range might be convenient to a user, exposing it to drivers is
> likely to end up writing the same vid_begin to vid_end for loop.

It is impossible not to expose this to the driver.

We have to have a prepare/commit sequence where the prepare part
can see the whole of the request.

It must know the full set of changes it must be able to support
in one go in order to signal a failure properly.

So in fact this is the most convenient way for the drivers to be
exposed to this issue.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: switchdev: restrict vid range abstraction
  2015-07-29 19:14     ` Vivien Didelot
  2015-07-29 19:28       ` David Miller
@ 2015-07-29 21:17       ` Scott Feldman
  2015-08-01  1:02         ` Vivien Didelot
  1 sibling, 1 reply; 7+ messages in thread
From: Scott Feldman @ 2015-07-29 21:17 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: David, netdev, Jiří Pírko, Florian Fainelli,
	linux-kernel, kernel

On Wed, Jul 29, 2015 at 12:14 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> Hi Scott, David,
>
> On Jul 29, 2015, at 2:28 PM, David davem@davemloft.net wrote:
>
>> From: Scott Feldman <sfeldma@gmail.com>
>> Date: Wed, 29 Jul 2015 00:31:44 -0700
>>
>>> Since the netlink request (for example vlan add) includes the range,
>>> I'm not seeing how we can response with success for the satisfied
>>> vlans in the range, and also respond with an error for the unsatisfied
>>> vlans in the range.   In other words, from the netlink msgs
>>> perspective, we need to treat a vlan range as all-or-nothing.  So in
>>> your example, if hw can't add vlan 2, we fail the entire request to
>>> add range 2-5.  This is where the prepare phase checks to make sure
>>> the entire request can be satisfied before committing to hw.
>
> I made this change in order to start restricting the bridge abstraction
> to switchdev, since IMHO its info flags do not add much value to the
> switch chip drivers perspective.
>
> While a range might be convenient to a user, exposing it to drivers is
> likely to end up writing the same vid_begin to vid_end for loop.
>
>> This was my concern with the change as well.
>>
>> The user asked for the range to be installed, so if any portion
>> of it cannot be done we must not make any changes to the HW
>> configuration and fail the entire request.
>
> I understand the concern with the netlink request.
>
> However, this can be confusing to someone. With the previous example:
>
>     bridge vlan add dev port0 vid 2-5 master
>
> must fail for the entire range (due to the single netlink request). But:
>
>     bridge vlan add dev port0 vid 2 master
>
> will silently fallback to software VLAN (assuming that the driver
> correctly returned -EOPNOTSUPP in the prepare phase). In other words, no
> changes has been committed to the hardware.

I see your concern now, I think.  net/bridge/br_netlink.c:br_afspec()
does the range loop but doesn't rewind if something goes wrong with
one of the vlans in the range.  The call into switchdev is
one-at-a-time at that point.  If br_afspec() handled the rewind, would
this address your concern?  We can keep the range support in the
switchdev vlan obj, so 'self' can use it.

-scott

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net: switchdev: restrict vid range abstraction
  2015-07-29 21:17       ` Scott Feldman
@ 2015-08-01  1:02         ` Vivien Didelot
  0 siblings, 0 replies; 7+ messages in thread
From: Vivien Didelot @ 2015-08-01  1:02 UTC (permalink / raw)
  To: Scott Feldman
  Cc: David, netdev, Jiří Pírko, Florian Fainelli,
	linux-kernel, kernel

Hi Scott,

On Jul 29, 2015, at 5:17 PM, Scott Feldman sfeldma@gmail.com wrote:

> On Wed, Jul 29, 2015 at 12:14 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
>> Hi Scott, David,
>>
>> On Jul 29, 2015, at 2:28 PM, David davem@davemloft.net wrote:
>>
>>> From: Scott Feldman <sfeldma@gmail.com>
>>> Date: Wed, 29 Jul 2015 00:31:44 -0700
>>>
>>>> Since the netlink request (for example vlan add) includes the range,
>>>> I'm not seeing how we can response with success for the satisfied
>>>> vlans in the range, and also respond with an error for the unsatisfied
>>>> vlans in the range.   In other words, from the netlink msgs
>>>> perspective, we need to treat a vlan range as all-or-nothing.  So in
>>>> your example, if hw can't add vlan 2, we fail the entire request to
>>>> add range 2-5.  This is where the prepare phase checks to make sure
>>>> the entire request can be satisfied before committing to hw.
>>
>> I made this change in order to start restricting the bridge abstraction
>> to switchdev, since IMHO its info flags do not add much value to the
>> switch chip drivers perspective.
>>
>> While a range might be convenient to a user, exposing it to drivers is
>> likely to end up writing the same vid_begin to vid_end for loop.
>>
>>> This was my concern with the change as well.
>>>
>>> The user asked for the range to be installed, so if any portion
>>> of it cannot be done we must not make any changes to the HW
>>> configuration and fail the entire request.
>>
>> I understand the concern with the netlink request.
>>
>> However, this can be confusing to someone. With the previous example:
>>
>>     bridge vlan add dev port0 vid 2-5 master
>>
>> must fail for the entire range (due to the single netlink request). But:
>>
>>     bridge vlan add dev port0 vid 2 master
>>
>> will silently fallback to software VLAN (assuming that the driver
>> correctly returned -EOPNOTSUPP in the prepare phase). In other words, no
>> changes has been committed to the hardware.
> 
> I see your concern now, I think.  net/bridge/br_netlink.c:br_afspec()
> does the range loop but doesn't rewind if something goes wrong with
> one of the vlans in the range.  The call into switchdev is
> one-at-a-time at that point.  If br_afspec() handled the rewind, would
> this address your concern?  We can keep the range support in the
> switchdev vlan obj, so 'self' can use it.

I am not sure is the rewind is needed. My concern was trying to handle
the fallback to software VLAN for a single VID within a range, so that
we can free a switch chip driver for this bridge-specific notion. But
because of the single netlink request, it seems not possible.

At which level does this fallback happen exactly?

Thanks,
-v

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-08-01  1:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-26 23:45 [PATCH] net: switchdev: restrict vid range abstraction Vivien Didelot
2015-07-29  7:31 ` Scott Feldman
2015-07-29 18:28   ` David Miller
2015-07-29 19:14     ` Vivien Didelot
2015-07-29 19:28       ` David Miller
2015-07-29 21:17       ` Scott Feldman
2015-08-01  1:02         ` Vivien Didelot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.