Linux Ethernet Bridging - Dec 2016 - [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

On 11/22/2016 09:41 AM, Ido Schimmel wrote:
> Hi Florian,
> 
> On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series allows using the bridge master interface to configure
>> an Ethernet switch port's CPU/management port with different VLAN
attributes than
>> those of the bridge downstream ports/members.
>>
>> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and
mv88e6xxx, I
>> tested this with b53 and a mockup DSA driver.
> 
> We'll need to add a check in mlxsw and ignore any VLAN configuration
for
> the bridge device itself. Otherwise, any configuration done on br0 will
> be propagated to all of its slaves, which is incorrect.
> 
>>
>> Open questions:
>>
>> - if we have more than one bridge on top of a physical switch, the
driver
>>   should keep track of that and verify that we are not going to change
>>   the CPU port VLAN attributes in a way that results in incompatible
settings
>>   to be applied
>>
>> - if the default behavior is to have all VLANs associated with the CPU
port
>>   be ingressing/egressing tagged to the CPU, is this really useful?
> 
> First of all, I want to be sure that when we say "CPU port",
we're
> talking about the same thing. In mlxsw, the CPU port is a pipe between
> the device and the host, through which all packets trapped to the host
> go through. So, when a packet is trapped, the driver reads its Rx
> descriptor, checks through which port it ingressed, resolves its netdev,
> sets skb->dev accordingly and injects it to the Rx path via
> netif_receive_skb(). The CPU port itself isn't represented using a
> netdev.
In the case of DSA, the CPU port is a normal Ethernet MAC driver, but in
premise, this driver plus the DSA tag protocol hook do exactly the same
things as you just describe.
> 
> Given the above, having VLAN filters (or STP) on the CPU port itself
> isn't really helpful (we do have them for physical ports of course...).
> So, mlxsw will not benefit from this patchset and if we've the same
> concept of "CPU port", then I'm not sure why you don't
just enable all
> the VLANs on it?
We do enable all VLANs on the CPU port (at least with b53, but I think
mv88e6xxx does it too), but compared to e.g: mlxsw, we trap all traffic
by default, and actually, quite often (always actually, until we add IP
routing offloads) the CPU is involved in the LAN/WAN routing, so it is
not infrequent to have the following packet flow:

LAN port -> VLAN 1 -> eth0.1 -> NAT/routing -> eth0.2 -> VLAN 2
-> WAN port

In that case, having the ability to define the per-port membership for
VLANs, including the CPU, kind of helps, especially if there are
private/guests VLAN on either the LAN or WAN segments that the CPU does
not necessarily need to play a role in.

NB: this scheme works because in most configurations that we support
today, the CPU port's speed is greater or equal than the speed of the
downstream/front panel ports.
> 
> Also, how are you going to set the VLAN filters for the CPU port when
> you don't offload a bridge, but instead vlan devices between which you
> route packets? You lose your abstraction of CPU port...
As far as I can tell today, this is not particularly helpful with DSA,
where we start with all traffic going to the CPU (each DSA created
network device is segregated from the other) and only then we require
having bridge VLAN filtering enabled in the kernel, and configuring
bridge VLAN membership to have a proper VLAN-based scheme.

If you did configure VLAN membership with e.g: port0.<vid> we could
support that just fine, but that programming interface does not allow
configuring the default VLAN, and in our case, it matters a bit to
support the LAN/WAN routing scenario described. We could agree that all
untagged traffic should go to VLAN 0 or 1 for instance, but that could
then, vary on a per-driver/HW basis.

Hope this clarifies things a bit!
-- 
Florian

Hi Florian,

On Tue, Nov 22, 2016 at 09:56:30AM -0800, Florian Fainelli
wrote:
> On 11/22/2016 09:41 AM, Ido Schimmel wrote:
> > Hi Florian,
> > 
> > On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
> >> Hi all,
> >>
> >> This patch series allows using the bridge master interface to
configure
> >> an Ethernet switch port's CPU/management port with different
VLAN attributes than
> >> those of the bridge downstream ports/members.
> >>
> >> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and
mv88e6xxx, I
> >> tested this with b53 and a mockup DSA driver.
> > 
> > We'll need to add a check in mlxsw and ignore any VLAN
configuration for
> > the bridge device itself. Otherwise, any configuration done on br0
will
> > be propagated to all of its slaves, which is incorrect.
> > 
> >>
> >> Open questions:
> >>
> >> - if we have more than one bridge on top of a physical switch, the
driver
> >>   should keep track of that and verify that we are not going to
change
> >>   the CPU port VLAN attributes in a way that results in
incompatible settings
> >>   to be applied
> >>
> >> - if the default behavior is to have all VLANs associated with the
CPU port
> >>   be ingressing/egressing tagged to the CPU, is this really
useful?
> > 
> > First of all, I want to be sure that when we say "CPU port",
we're
> > talking about the same thing. In mlxsw, the CPU port is a pipe between
> > the device and the host, through which all packets trapped to the host
> > go through. So, when a packet is trapped, the driver reads its Rx
> > descriptor, checks through which port it ingressed, resolves its
netdev,
> > sets skb->dev accordingly and injects it to the Rx path via
> > netif_receive_skb(). The CPU port itself isn't represented using a
> > netdev.
> 
> In the case of DSA, the CPU port is a normal Ethernet MAC driver, but in
> premise, this driver plus the DSA tag protocol hook do exactly the same
> things as you just describe.
Thanks for the detailed explanation! I also took the time to read
dsa.txt, however I still don't quite understand the motivation for
VLAN filtering on the CPU port. In which cases would you like to prevent
packets from going to the host due to their VLAN header? This change
would make sense to me if you only had a limited number of VLANs you
could enable on the CPU port, but from your response I understand that
this isn't the case.

FWIW, I don't have a problem with patches if they are useful for you,
I'm just trying to understand the use case. We can easily patch mlxsw to
ignore calls with orig_dev=br0.

Thanks!

On 11/23/2016 05:48 AM, Ido Schimmel wrote:
> Hi Florian,
> 
> On Tue, Nov 22, 2016 at 09:56:30AM -0800, Florian Fainelli wrote:
>> On 11/22/2016 09:41 AM, Ido Schimmel wrote:
>>> Hi Florian,
>>>
>>> On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
>>>> Hi all,
>>>>
>>>> This patch series allows using the bridge master interface to
configure
>>>> an Ethernet switch port's CPU/management port with
different VLAN attributes than
>>>> those of the bridge downstream ports/members.
>>>>
>>>> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw
and mv88e6xxx, I
>>>> tested this with b53 and a mockup DSA driver.
>>>
>>> We'll need to add a check in mlxsw and ignore any VLAN
configuration for
>>> the bridge device itself. Otherwise, any configuration done on br0
will
>>> be propagated to all of its slaves, which is incorrect.
>>>
>>>>
>>>> Open questions:
>>>>
>>>> - if we have more than one bridge on top of a physical switch,
the driver
>>>>   should keep track of that and verify that we are not going to
change
>>>>   the CPU port VLAN attributes in a way that results in
incompatible settings
>>>>   to be applied
>>>>
>>>> - if the default behavior is to have all VLANs associated with
the CPU port
>>>>   be ingressing/egressing tagged to the CPU, is this really
useful?
>>>
>>> First of all, I want to be sure that when we say "CPU
port", we're
>>> talking about the same thing. In mlxsw, the CPU port is a pipe
between
>>> the device and the host, through which all packets trapped to the
host
>>> go through. So, when a packet is trapped, the driver reads its Rx
>>> descriptor, checks through which port it ingressed, resolves its
netdev,
>>> sets skb->dev accordingly and injects it to the Rx path via
>>> netif_receive_skb(). The CPU port itself isn't represented
using a
>>> netdev.
>>
>> In the case of DSA, the CPU port is a normal Ethernet MAC driver, but
in
>> premise, this driver plus the DSA tag protocol hook do exactly the same
>> things as you just describe.
> 
> Thanks for the detailed explanation! I also took the time to read
> dsa.txt, however I still don't quite understand the motivation for
> VLAN filtering on the CPU port. In which cases would you like to prevent
> packets from going to the host due to their VLAN header? This change
> would make sense to me if you only had a limited number of VLANs you
> could enable on the CPU port, but from your response I understand that
> this isn't the case.
It's not much about VLAN filtering per-se, but more about the default
VLAN membership of the CPU port, in the absence of any explicit
configuration. As an user, I find it a little inconvenient to have to
create one VLAN interface per VLAN that I am adding to the bridge to be
able to terminate that traffic properly towards the host/CPU/management
interface, especially when this VLAN is untagged.

This is really the motivation for these patches: if there is only one
VLAN configured, and it's the default VLAN for all ports, then the
bridge master interface also terminates this VLAN with the same
properties as those added by default (typically with default_pvid: VID 1
untagged, unless changed of course).

If you don't want that as an user, you now have the ability to change
it, and make this VLAN (or any other for that matter) to be terminated
differently at the host/CPU/management port level than how it is
egressing at the downstream ports part of that VLAN too.

Maybe it's a bit overkill...
> 
> FWIW, I don't have a problem with patches if they are useful for you,
> I'm just trying to understand the use case. We can easily patch mlxsw
to
> ignore calls with orig_dev=br0.
OK, if I resubmit, I will try to take care of mlxsw and rocker as well.

Thanks!
-- 
Florian