| Message ID | 1444391651-13775-1-git-send-email-jiri@resnulli.us |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Jiri, On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Some drivers need to implement both switchdev vlan ops and > vid_add/kill ndos. For that to work in bridge code, we need to try > switchdev op first when adding/deleting vlan id. Just curious, when would a driver need to have both operations? I kinda have the same question regarding ndo_fdb_{add,del} and the bridge_{get,set}link equivalent, which is a bit confusing to me. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: Ido Schimmel <idosch@mellanox.com> > --- > net/bridge/br_vlan.c | 58 ++++++++++++++++++++-------------------------------- > 1 file changed, 22 insertions(+), 36 deletions(-) > > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index eae07ee..975deb9 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -72,28 +72,20 @@ static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags) > static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br, > u16 vid, u16 flags) > { > - const struct net_device_ops *ops = dev->netdev_ops; > + struct switchdev_obj_port_vlan v = { > + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, > + .flags = flags, > + .vid_begin = vid, > + .vid_end = vid, > + }; > int err; > > - /* If driver uses VLAN ndo ops, use 8021q to install vid > - * on device, otherwise try switchdev ops to install vid. > + /* Try switchdev op first. In case it is not supported, fallback to > + * 8021q add. > */ > - > - if (ops->ndo_vlan_rx_add_vid) { > - err = vlan_vid_add(dev, br->vlan_proto, vid); > - } else { > - struct switchdev_obj_port_vlan v = { > - .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, > - .flags = flags, > - .vid_begin = vid, > - .vid_end = vid, > - }; > - > - err = switchdev_port_obj_add(dev, &v.obj); > - if (err == -EOPNOTSUPP) > - err = 0; > - } > - > + err = switchdev_port_obj_add(dev, &v.obj); > + if (err == -EOPNOTSUPP) > + return vlan_vid_add(dev, br->vlan_proto, vid); err = vlan_vid_add(dev, br->vlan_proto, vid); Just being picky: the above line would have been preferred to keep a single return path, but this does not justify a v2 though. > return err; > } Thanks, -v -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 9, 2015 at 4:54 AM, Jiri Pirko <jiri@resnulli.us> wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Some drivers need to implement both switchdev vlan ops and > vid_add/kill ndos. For that to work in bridge code, we need to try > switchdev op first when adding/deleting vlan id. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: Ido Schimmel <idosch@mellanox.com> Acked-by: Scott Feldman <sfeldma@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote: > Hi Jiri, > > On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Some drivers need to implement both switchdev vlan ops and >> vid_add/kill ndos. For that to work in bridge code, we need to try >> switchdev op first when adding/deleting vlan id. > > Just curious, when would a driver need to have both operations? Ya, I was kind of curious of that myself. Is this because the driver wants to support standalone VLANs using 802.1q module and vconfig, as well as bridge vlans? With the vlan support built into the bridge, I've been working under the assumption that 802.1q module (and vconfig) aren't needed, and vlans for a bridged and non-bridge port can be managed using the "bridge" iproute2 cmd. > I kinda have the same question regarding ndo_fdb_{add,del} and the > bridge_{get,set}link equivalent, which is a bit confusing to me. I had to look back at my commit 7f109539 to remind myself about the vid_add/kill ndos and bridge_{get,set}link usage. Maybe that write-up helps? I'm not following you on the ndo_fdb_add/del part of your question. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sat, Oct 10, 2015 at 05:20:28AM CEST, sfeldma@gmail.com wrote: >On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot ><vivien.didelot@savoirfairelinux.com> wrote: >> Hi Jiri, >> >> On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote: >>> From: Jiri Pirko <jiri@mellanox.com> >>> >>> Some drivers need to implement both switchdev vlan ops and >>> vid_add/kill ndos. For that to work in bridge code, we need to try >>> switchdev op first when adding/deleting vlan id. >> >> Just curious, when would a driver need to have both operations? > >Ya, I was kind of curious of that myself. Is this because the driver >wants to support standalone VLANs using 802.1q module and vconfig, as >well as bridge vlans? With the vlan support built into the bridge, >I've been working under the assumption that 802.1q module (and >vconfig) aren't needed, and vlans for a bridged and non-bridge port >can be managed using the "bridge" iproute2 cmd. Sure, but this is for standalone port device, without being bridged. In that case if you want to use vlan on top of that, you need these ndos. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct. Friday 09 (41) 08:20 PM, Scott Feldman wrote: > On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot > <vivien.didelot@savoirfairelinux.com> wrote: > > Hi Jiri, > > > > On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote: > >> From: Jiri Pirko <jiri@mellanox.com> > >> > >> Some drivers need to implement both switchdev vlan ops and > >> vid_add/kill ndos. For that to work in bridge code, we need to try > >> switchdev op first when adding/deleting vlan id. > > > > Just curious, when would a driver need to have both operations? > > Ya, I was kind of curious of that myself. Is this because the driver > wants to support standalone VLANs using 802.1q module and vconfig, as > well as bridge vlans? With the vlan support built into the bridge, > I've been working under the assumption that 802.1q module (and > vconfig) aren't needed, and vlans for a bridged and non-bridge port > can be managed using the "bridge" iproute2 cmd. > > > I kinda have the same question regarding ndo_fdb_{add,del} and the > > bridge_{get,set}link equivalent, which is a bit confusing to me. > > I had to look back at my commit 7f109539 to remind myself about the > vid_add/kill ndos and bridge_{get,set}link usage. Maybe that > write-up helps? I'm not following you on the ndo_fdb_add/del part of > your question. Sorry I wasn't clear. What is confusing to me for FDB ops is that we define net_device_ops like: .ndo_fdb_add = switchdev_port_fdb_add, .ndo_fdb_del = switchdev_port_fdb_del, .ndo_fdb_dump = switchdev_port_fdb_dump, But if I'm not mistaken, "bridge fdb" commands use the .ndo_bridge_{get,set,del}link ops, isn't it? Thanks, -v -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-10-10 0:01 GMT-07:00 Jiri Pirko <jiri@resnulli.us>: > Sat, Oct 10, 2015 at 05:20:28AM CEST, sfeldma@gmail.com wrote: >>On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot >><vivien.didelot@savoirfairelinux.com> wrote: >>> Hi Jiri, >>> >>> On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote: >>>> From: Jiri Pirko <jiri@mellanox.com> >>>> >>>> Some drivers need to implement both switchdev vlan ops and >>>> vid_add/kill ndos. For that to work in bridge code, we need to try >>>> switchdev op first when adding/deleting vlan id. >>> >>> Just curious, when would a driver need to have both operations? >> >>Ya, I was kind of curious of that myself. Is this because the driver >>wants to support standalone VLANs using 802.1q module and vconfig, as >>well as bridge vlans? With the vlan support built into the bridge, >>I've been working under the assumption that 802.1q module (and >>vconfig) aren't needed, and vlans for a bridged and non-bridge port >>can be managed using the "bridge" iproute2 cmd. > > Sure, but this is for standalone port device, without being bridged. In > that case if you want to use vlan on top of that, you need these ndos. Standalone VLAN configuration (that is, using vconfig/802.1q module) is fairly limited, and only really makes sense, in its current implementation for a host. By that, I mean, that we cannot configure what is your default VLAN (the untagged one), and it does not matter in the case of a host, because this is something that the other end: the switch, has to have configured for you. For netdevices from a switch device, I think we should really either have the ability to use the same configuration flexibility as the bridge VLAN filtering offers (setting PVID, untagged/default VLAN, tagged), either outside of the bridge layer (which could be of limited use), or just enforce the need of a bridge device to span these specific ports of a switch.
On Sat, Oct 10, 2015 at 9:03 AM, Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote: > On Oct. Friday 09 (41) 08:20 PM, Scott Feldman wrote: >> On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot >> <vivien.didelot@savoirfairelinux.com> wrote: >> > Hi Jiri, >> > >> > On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote: >> >> From: Jiri Pirko <jiri@mellanox.com> >> >> >> >> Some drivers need to implement both switchdev vlan ops and >> >> vid_add/kill ndos. For that to work in bridge code, we need to try >> >> switchdev op first when adding/deleting vlan id. >> > >> > Just curious, when would a driver need to have both operations? >> >> Ya, I was kind of curious of that myself. Is this because the driver >> wants to support standalone VLANs using 802.1q module and vconfig, as >> well as bridge vlans? With the vlan support built into the bridge, >> I've been working under the assumption that 802.1q module (and >> vconfig) aren't needed, and vlans for a bridged and non-bridge port >> can be managed using the "bridge" iproute2 cmd. >> >> > I kinda have the same question regarding ndo_fdb_{add,del} and the >> > bridge_{get,set}link equivalent, which is a bit confusing to me. >> >> I had to look back at my commit 7f109539 to remind myself about the >> vid_add/kill ndos and bridge_{get,set}link usage. Maybe that >> write-up helps? I'm not following you on the ndo_fdb_add/del part of >> your question. > > Sorry I wasn't clear. What is confusing to me for FDB ops is that we > define net_device_ops like: > > .ndo_fdb_add = switchdev_port_fdb_add, > .ndo_fdb_del = switchdev_port_fdb_del, > .ndo_fdb_dump = switchdev_port_fdb_dump, > > But if I'm not mistaken, "bridge fdb" commands use the > .ndo_bridge_{get,set,del}link ops, isn't it? No, the "bridge fdb" cmds use the .ndo_fdb_xxx ops. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jiri Pirko <jiri@resnulli.us> Date: Fri, 9 Oct 2015 13:54:11 +0200 > From: Jiri Pirko <jiri@mellanox.com> > > Some drivers need to implement both switchdev vlan ops and > vid_add/kill ndos. For that to work in bridge code, we need to try > switchdev op first when adding/deleting vlan id. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: Ido Schimmel <idosch@mellanox.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index eae07ee..975deb9 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -72,28 +72,20 @@ static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags) static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br, u16 vid, u16 flags) { - const struct net_device_ops *ops = dev->netdev_ops; + struct switchdev_obj_port_vlan v = { + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, + .flags = flags, + .vid_begin = vid, + .vid_end = vid, + }; int err; - /* If driver uses VLAN ndo ops, use 8021q to install vid - * on device, otherwise try switchdev ops to install vid. + /* Try switchdev op first. In case it is not supported, fallback to + * 8021q add. */ - - if (ops->ndo_vlan_rx_add_vid) { - err = vlan_vid_add(dev, br->vlan_proto, vid); - } else { - struct switchdev_obj_port_vlan v = { - .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, - .flags = flags, - .vid_begin = vid, - .vid_end = vid, - }; - - err = switchdev_port_obj_add(dev, &v.obj); - if (err == -EOPNOTSUPP) - err = 0; - } - + err = switchdev_port_obj_add(dev, &v.obj); + if (err == -EOPNOTSUPP) + return vlan_vid_add(dev, br->vlan_proto, vid); return err; } @@ -122,27 +114,21 @@ static void __vlan_del_list(struct net_bridge_vlan *v) static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br, u16 vid) { - const struct net_device_ops *ops = dev->netdev_ops; - int err = 0; + struct switchdev_obj_port_vlan v = { + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, + .vid_begin = vid, + .vid_end = vid, + }; + int err; - /* If driver uses VLAN ndo ops, use 8021q to delete vid - * on device, otherwise try switchdev ops to delete vid. + /* Try switchdev op first. In case it is not supported, fallback to + * 8021q del. */ - - if (ops->ndo_vlan_rx_kill_vid) { + err = switchdev_port_obj_del(dev, &v.obj); + if (err == -EOPNOTSUPP) { vlan_vid_del(dev, br->vlan_proto, vid); - } else { - struct switchdev_obj_port_vlan v = { - .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, - .vid_begin = vid, - .vid_end = vid, - }; - - err = switchdev_port_obj_del(dev, &v.obj); - if (err == -EOPNOTSUPP) - err = 0; + return 0; } - return err; }