On Oct. Wednesday 14 (42) 08:42 PM, Ido Schimmel 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).
>
> If the notification is about a linking operation and the master is a
> bridge different than the current one, then NOTIFY_BAD is returned.
Great, I'll wait for this then.
Scott, this is another good reason why we definitely need a simple
struct device per switch chip. In addition to the port net_device
registration, the netdev notifier is another exact same piece of code
that both Rocker and DSA implement.
> Vivien, regarding your WAN interface question, this is something we
> currently don't do. We don't even flood traffic from bridged ports
> to CPU (although we can), as it can saturate the bus. Only control
> traffic is supposed to go there.
I kinda answered it myself: a Linux bridge needs to remain a user
abstraction of a logical group of net_device. In other words, we must
allow physical distinct ports under the same bridge.
Below is an example of a custom router with 2 chained switch chips sw0
and sw1, and what usage I believe we expect:
[ Linux soft bridge "br0" which can accelerate VLAN, STP, etc. ]
(CPU) (WAN)
[ sw0p0 sw0p1 sw0p2 ] [ sw1p0 sw1p1 sw1p2 sw1p3 ] [ eth0 ] [ eth1 ]
`--DSA--' `-------'
Thanks,
-v
--
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
