Thu, Oct 15, 2015 at 05:58:33AM IDT, sfel...@gmail.com wrote: >On Wed, Oct 14, 2015 at 10:42 AM, Ido Schimmel <ido...@mellanox.com> wrote: >> Wed, Oct 14, 2015 at 08:14:24PM IDT, sfel...@gmail.com wrote: >>>On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot >>><vivien.dide...@savoirfairelinux.com> wrote: >>>> On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote: >>>>> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com >>>>> wrote: >>>>> >On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote: >>>>> >> Mon, Oct 12, 2015 at 08:36:25PM IDT, >>>>> >> vivien.dide...@savoirfairelinux.com wrote: >>>>> >> >Hi guys, >>>>> >> > >>>>> >> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote: >>>>> >> >> From: Nikolay Aleksandrov <niko...@cumulusnetworks.com> >>>>> >> >> >>>>> >> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges. >>>>> >> >> >>>>> >> >> Signed-off-by: Nikolay Aleksandrov <niko...@cumulusnetworks.com> >>>>> >> >> --- >>>>> >> >> net/switchdev/switchdev.c | 3 +++ >>>>> >> >> 1 file changed, 3 insertions(+) >>>>> >> >> >>>>> >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c >>>>> >> >> index 6e4a4f9ad927..256c596de896 100644 >>>>> >> >> --- a/net/switchdev/switchdev.c >>>>> >> >> +++ b/net/switchdev/switchdev.c >>>>> >> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct >>>>> >> >> net_device *dev, >>>>> >> >> if (vlan.vid_begin) >>>>> >> >> return -EINVAL; >>>>> >> >> vlan.vid_begin = vinfo->vid; >>>>> >> >> + /* don't allow range of pvids */ >>>>> >> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID) >>>>> >> >> + return -EINVAL; >>>>> >> >> } else if (vinfo->flags & >>>>> >> >> BRIDGE_VLAN_INFO_RANGE_END) { >>>>> >> >> if (!vlan.vid_begin) >>>>> >> >> return -EINVAL; >>>>> >> >> -- >>>>> >> >> 2.4.3 >>>>> >> >> >>>>> >> > >>>>> >> >Yes the patch looks good, but it is a minor check though. I hope the >>>>> >> >subject of this thread is making sense. >>>>> >> > >>>>> >> >VLAN ranges seem to have been included for an UX purpose (so commands >>>>> >> >look like Cisco IOS). We don't want to change any existing interface, >>>>> >> >so >>>>> >> >we pushed that down to drivers, with the only valid reason that, maybe >>>>> >> >one day, an hardware can be capable of programming a range on a >>>>> >> >per-port >>>>> >> >basis. >>>>> >> Hi, >>>>> >> >>>>> >> That's actually what we are doing in mlxsw. We can do up to 256 >>>>> >> entries in >>>>> >> one go. We've yet to submit this part. >>>>> > >>>>> >Perfect Ido, thanks for pointing this out! I'm OK with the range then. >>>>> > >>>>> >So there is now a very last question in my head for this, which is more >>>>> >a matter of kernel design. Should the user be aware of such underlying >>>>> >support? In other words, would it make sense to do this in a driver: >>>>> > >>>>> > foo_port_vlan_add(struct net_device *dev, >>>>> > struct switchdev_obj_port_vlan *vlan) >>>>> > { >>>>> > if (vlan->vid_begin != vlan->vid_end) >>>>> > return -ENOTSUPP; /* or something more relevant for user */ >>>>> > >>>>> > return foo_port_single_vlan_add(dev, vlan->vid_begin); >>>>> > } >>>>> > >>>>> >So drivers keep being simple, and we can easily propagate the fact that >>>>> >one-or-all VLAN is not supportable, vs. the VLAN feature itself is not >>>>> >implemented and must be done in software. >>>>> I think that if you want to keep it simple, then Scott's advice from the >>>>> previous thread is the most appropriate one. I believe the hardware you >>>>> are using is simply not meant to support multiple 802.1Q bridges. >>>> >>>> You mean allowing only one Linux bridge over an hardware switch? >>>> >>>> It would for sure simplify how, as developers and users, we represent a >>>> physical switch. But I am not sure how to achieve that and I don't have >>>> strong opinions on this TBH. >>> >>>Hi Vivien, I think it's possible to keep switch ports on just one >>>bridge if we do a little bit of work on the NETDEV_CHANGEUPPER >>>notifier. This will give you the driver-level control you want. Do >>>you have time to investigate? The idea is: >>> >>>1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is >>>being added to a second bridge,then return NOTIFY_BAD. Your driver >>>needs to track the bridge count. >>> >>>2) In __netdev_upper_dev_link(), check the return code from the >>>call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if >>>NOTIFY_BAD, abort the linking operation (goto rollback_xxx). >>> >> Hi, >> >> We are doing something similar in mlxsw (not upstream yet). Jiri >> introduced PRE_CHANGEUPPER, which is called from the function you >> mentioned, but before the linking operation (so that you don't need to >> rollback). > >Oh, cool. > >> If the notification is about a linking operation and the master is a >> bridge different than the current one, then NOTIFY_BAD is returned. > >So you're wanting to restrict to just one bridge also? Or is >NOTIFY_BAD returned for some other reason? I guess I should be >patient and wait for the patch. Yes, currently that's what we are doing. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html