Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180624153339.13572-1-f.fainelli@gmail.com> <wih4lhrgrfd.fsf@dev-r-vrt-156.mtr.labs.mlnx>
 <20180625091713.GA13442@apalos> <9ce291a4-b40d-81d8-1c1a-c4311e5cc113@gmail.com>
 <20180828083257.GA10872@apalos> <da1a9896-94aa-589c-3fd2-023b1586bb15@gmail.com>
 <CAJGZr0JaqBt0BdoCAmjnMT9zN8cjBnJKJJGzWrThTjBKtwp-vA@mail.gmail.com> <25821da3-b923-485f-0991-e1dc943aefec@gmail.com>
In-Reply-To: <25821da3-b923-485f-0991-e1dc943aefec@gmail.com>
From:   Maxim Uvarov <muvarov@gmail.com>
Date:   Wed, 29 Aug 2018 10:14:10 +0300
Message-ID: <CAJGZr0+YJrkjD5jz2cjySZAvgict1f7EeqhVQV6_M0HEB5Ddkw@mail.gmail.com>
Subject: Re: [PATCH RFT] net: dsa: Allow configuring CPU port VLANs
To:     Florian Fainelli <f.fainelli@gmail.com>
Cc:     Ilias Apalodimas <ilias.apalodimas@linaro.org>, petrm@mellanox.com,
        netdev <netdev@vger.kernel.org>, jiri@mellanox.com,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@savoirfairelinux.com>,
        David Miller <davem@davemloft.net>,
        kernel list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=D0=B2=D1=82, 28 =D0=B0=D0=B2=D0=B3. 2018 =D0=B3. =D0=B2 22:17, Florian Fai=
nelli <f.fainelli@gmail.com>:
>
> On 08/28/2018 12:08 PM, Maxim Uvarov wrote:
> > =D0=B2=D1=82, 28 =D0=B0=D0=B2=D0=B3. 2018 =D0=B3. =D0=B2 20:00, Florian=
 Fainelli <f.fainelli@gmail.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@gmail.com> writes:
> >>>>>>
> >>>>>>>   if (netif_is_bridge_master(vlan->obj.orig_dev))
> >>>>>>> -         return -EOPNOTSUPP;
> >>>>>>> +         info.port =3D dp->cpu_dp->index;
> >>>>>>
> >>>>>> The condition above will trigger also when a VLAN is added on a me=
mber
> >>>>>> port, and there's no other port with that VLAN. In that case the V=
LAN
> >>>>>> 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 followin=
g
> >>>> 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 wo=
uld
> >>>> also push the configuration to br0/CPU port since this is the defaul=
t 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, tag=
ged
> >>>> 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, agai=
n,
> >>>> this is expected because we are only programming the bridge master/C=
PU
> >>>> 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 dr=
iver
> >>>> actually need to obtain that at the time the port is enslaved someho=
w?
> >>> As long as ports are attached you get the events (one event per attac=
hed port
> >>> iirc)
> >>> if the event is checked against BRIDGE_VLAN_INFO_BRENTRY, the only wa=
y 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_pv=
id
> >> 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


--=20
Best regards,
Maxim Uvarov