Re: [PATCH net] net: dsa: b53: Ensure the default VID is untagged

[Vladimir Oltean]

On Fri, 14 Feb 2020 at 19:08, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
>
>
>
> On 2/14/2020 2:36 AM, Vladimir Oltean wrote:
> > Hi Florian,
> >
> > On Thu, 13 Feb 2020 at 21:10, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
> >>
> >> We need to ensure that the default VID is untagged otherwise the switch
> >> will be sending frames tagged frames and the results can be problematic.
> >> This is especially true with b53 switches that use VID 0 as their
> >> default VLAN since VID 0 has a special meaning.
> >>
> >> Fixes: fea83353177a ("net: dsa: b53: Fix default VLAN ID")
> >> Fixes: 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")
> >> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> >> ---
> >>  drivers/net/dsa/b53/b53_common.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> >> index 449a22172e07..f25c43b300d4 100644
> >> --- a/drivers/net/dsa/b53/b53_common.c
> >> +++ b/drivers/net/dsa/b53/b53_common.c
> >> @@ -1366,6 +1366,9 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
> >>
> >>                 b53_get_vlan_entry(dev, vid, vl);
> >>
> >> +               if (vid == b53_default_pvid(dev))
> >> +                       untagged = true;
> >> +
> >>                 vl->members |= BIT(port);
> >>                 if (untagged && !dsa_is_cpu_port(ds, port))
> >>                         vl->untag |= BIT(port);
> >> --
> >> 2.17.1
> >>
> >
> > Don't you mean to force untagged egress only for the pvid value of 0?
>
> The default VID (0 for most switches, 1 for 5325/65) is configured as
> pvid during b53_configure_vlan() so when we get a call to port_vlan_add
> with VID == 0 this is coming exclusively from
> dsa_slave_vlan_rx_add_vid() since the bridge will never program a VID <
> 1. When dsa_slave_vlan_rx_add_vid() calls us, we do not have any VLAN
> flags set in the object.
> --
> Florian

Exactly. So the only case you need to guard against is when vid == 0
&& vid == b53_default_pvid(dev) - basically the 8021q module messing
with your untagged (pvid-tagged) traffic. So both have to be zero in
order to interfere. If vid is 0 but the b53_default_pvid is 1 - no
problem. If vid is 1 and the b53_default_pvid is 1, again no problem.
At least that's what you described in the previous patch. No?

-Vladimir