Date:	Wed, 14 Oct 2015 19:58:33 -0700
From:	Scott Feldman <sfeldma@...il.com>
To:	Ido Schimmel <idosch@...lanox.com>
Cc:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Netdev <netdev@...r.kernel.org>,
	Jiří Pírko <jiri@...nulli.us>,
	"David S. Miller" <davem@...emloft.net>,
	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
	Elad Raz <eladr@...lanox.com>
Subject: Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges

On Wed, Oct 14, 2015 at 10:42 AM, Ido Schimmel <idosch@...lanox.com> wrote:
> Wed, Oct 14, 2015 at 08:14:24PM IDT, sfeldma@...il.com wrote:
>>On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
>><vivien.didelot@...oirfairelinux.com> wrote:
>>> On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
>>>> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.didelot@...oirfairelinux.com wrote:
>>>> >On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
>>>> >> Mon, Oct 12, 2015 at 08:36:25PM IDT, vivien.didelot@...oirfairelinux.com wrote:
>>>> >> >Hi guys,
>>>> >> >
>>>> >> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
>>>> >> >> From: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
>>>> >> >>
>>>> >> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>>>> >> >>
>>>> >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists