Message-ID: <CAJGZr0+YJrkjD5jz2cjySZAvgict1f7EeqhVQV6_M0HEB5Ddkw@mail.gmail.com>
Date:   Wed, 29 Aug 2018 10:14:10 +0300
From:   Maxim Uvarov <muvarov@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Ilias Apalodimas <ilias.apalodimas@...aro.org>, petrm@...lanox.com,
        netdev <netdev@...r.kernel.org>, jiri@...lanox.com,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        David Miller <davem@...emloft.net>,
        kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFT] net: dsa: Allow configuring CPU port VLANs

вт, 28 авг. 2018 г. в 22:17, Florian Fainelli <f.fainelli@...il.com>:
>
> On 08/28/2018 12:08 PM, Maxim Uvarov wrote:
> > вт, 28 авг. 2018 г. в 20:00, Florian Fainelli <f.fainelli@...il.com>:
> >>
> >> On 08/28/2018 01:32 AM, Ilias Apalodimas wrote:
> >>> On Fri, Aug 10, 2018 at 04:58:10PM -0700, Florian Fainelli wrote:
> >>>> On 06/25/2018 02:17 AM, Ilias Apalodimas wrote:
> >>>>> On Mon, Jun 25, 2018 at 12:13:10PM +0300, Petr Machata wrote:
> >>>>>> Florian Fainelli <f.fainelli@...il.com> writes:
> >>>>>>
> >>>>>>>   if (netif_is_bridge_master(vlan->obj.orig_dev))
> >>>>>>> -         return -EOPNOTSUPP;
> >>>>>>> +         info.port = dp->cpu_dp->index;
> >>>>>>
> >>>>>> The condition above will trigger also when a VLAN is added on a member
> >>>>>> port, and there's no other port with that VLAN. In that case the VLAN
> >>>>>> comes without the BRIDGE_VLAN_INFO_BRENTRY flag. In mlxsw we have this
> >>>>>> to get the bridge VLANs:
> >>>>>>
> >>>>>>    if (netif_is_bridge_master(orig_dev)) {
> >>>>>>            [...]
> >>>>>>            if ((vlan->flags & BRIDGE_VLAN_INFO_BRENTRY) &&
> >>>>>>            [...]
> >>>>>>
> >>>>>> This doesn't appear to be done in DSA unless I'm missing something.
> >>>>> Petr's right. This will trigger for VLANs added on 'not cpu ports' if the VLAN
> >>>>> is not already a member.
> >>>>>
> >>>>> This command has BRIDGE_VLAN_INFO_BRENTRY set:
> >>>>> bridge vlan add dev br0 vid 100 pvid untagged self
> >>>>> I had the same issue on my CPSW RFC and solved it
> >>>>> exactly the same was as Petr suggested.
> >>>>
> >>>> Humm, there must be something obvious I am missing, but the following
> >>>> don't exactly result in what I would expect after adding a check for
> >>>> vlan->flags & BRIDGE_VLAN_INFO_BRENTRY:
> >>>>
> >>>> brctl addbr br0
> >>>> echo 1 > /sys/class/net/br0/bridge/vlan_filtering
> >>>> brctl addif br0 lan1
> >>>>
> >>>> #1 results in lan1 being programmed with VID 1, PVID, untagged, but not
> >>>> the CPU port. I would have sort of expected that the bridge layer would
> >>>> also push the configuration to br0/CPU port since this is the default VLAN:
> >>>>
> >>>> bridge vlan show dev br0
> >>>> port vlan ids
> >>>> br0  1 PVID Egress Untagged
> >>>>
> >>>> But it does not.
> >>>>
> >>>> bridge vlan add vid 2 dev lan1
> >>>>
> >>>> #2 same thing, results in only lan1 being programmed with VID 2, tagged
> >>>> but that is expected because we are creating the VLAN only for the
> >>>> user-facing port.
> >>>>
> >>>> bridge vlan add vid 3 dev br0 self
> >>>>
> >>>> #3 results in the CPU port being programmed with VID 3, tagged, again,
> >>>> this is expected because we are only programming the bridge master/CPU
> >>>> port here.
> >>>>
> >>>> Does #1 also happen for cpsw and mlxsw or do you actually get events
> >>>> about the bridge's default VLAN configuration? Or does the switch driver
> >>>> actually need to obtain that at the time the port is enslaved somehow?
> >>> As long as ports are attached you get the events (one event per attached port
> >>> iirc)
> >>> if the event is checked against BRIDGE_VLAN_INFO_BRENTRY, the only way to add a
> >>> VLAN to the cpu port is via 'bridge vlan add vid 3 dev br0 self'
> >>
> >> Do we have a guarantee that upon port enslavement, whatever default_pvid
> >> is configured on the bridge master device also happens to be the port's
> >> default_pvid settings as well?
> >
> > I think default pvid is per port thing. I.e. each port can have it's
> > own pvid (i.e. it will tag with vlan id not tagged incoming packet to
> > that port),
>
> We are talking about the bridge master device's default_pvid which can
> be set prior to any port being enslaved into the bridge. As of today, if
> you enslave a port of a switch into a bridge, you need to properly
> configure the CPU/management port as well otherwise things just wont' be
> working. At the time we enslave the first port into the bridge, there is
> no notification AFAICT that is generated to tell us about what the
> bridge master device's default_pvid is.
>
> > I did not exactly understand use case. With adding vlan filtering to
> > cpu port you filter out packets from other vlan groups to cpu port.
> > This might be useful
> > only for multicast packes or missing fbd entry on some dsa port. Is
> > filtering multicast a main problem to solve here?
> > Linux is missing vlan ingress policy. I.e. filtering (echo 1 >
> > /sys/br0/vlan_filter) has to be case of 3 policies: secure (default
> > now), check and fallback. With current secure mode it
> > might work, but with check mode it will be needed to add all vlans to
> > cpu port. Btw, on some hardware vlan ingress policies are also per
> > port, not per bridge.
>
> The general use case is that the CPU port on switches that have such a
> thing is just a normal port on which you should be able to configure
> exactly the VLAN membership and attributes.
>

that has to be good feature to add.

> With DSA switches today, we cannot do that, because there is no network
> interface exposed for the CPU port (and there should not be one), so
> when you target the bridge master device, e.g: br0, we can generate
> events towards the switch driver that map to the CPU port.
>
> There are many reasons for trying to do that, if we don't support such a
> thing, then we need to have the CPU port be part of all VLAN IDs that
> get added to ports, as a tagged member (because if untagged, you can't
> differentiate traffic anymore).
>
> Regarding your suggestion, we could certainly change vlan_filtering to
> take several values:
>
> 0: disabled
> 1: secure
> 2: check
>
> Or something like that.

I think that will work.

Maxim.

> --
> Florian



-- 
Best regards,
Maxim Uvarov

Powered by blists - more mailing lists