[net-next,4/4] net: dsa: set configure_vlan_while_not_filtering to true by default

From: Vladimir Oltean <vladimir.oltean@nxp.com>

As explained in commit 54a0ed0df496 ("net: dsa: provide an option for
drivers to always receive bridge VLANs"), DSA has historically been
skipping VLAN switchdev operations when the bridge wasn't in
vlan_filtering mode, but the reason why it was doing that has never been
clear. So the configure_vlan_while_not_filtering option is there merely
to preserve functionality for existing drivers. It isn't some behavior
that drivers should opt into. Ideally, when all drivers leave this flag
set, we can delete the dsa_port_skip_vlan_configuration() function.

New drivers always seem to omit setting this flag, for some reason. So
let's reverse the logic: the DSA core sets it by default to true before
the .setup() callback, and legacy drivers can turn it off. This way, new
drivers get the new behavior by default, unless they explicitly set the
flag to false, which is more obvious during review.

Remove the assignment from drivers which were setting it to true, and
add the assignment to false for the drivers that didn't previously have
it. This way, it should be easier to see how many we have left.

The following drivers: lan9303, mv88e6060 were skipped from setting this
flag to false, because they didn't have any VLAN offload ops in the
first place.

Also, print a message to the console every time a VLAN has been skipped.
This is mildly annoying on purpose, so that (a) it is at least clear
that VLANs are being skipped - the legacy behavior in itself is
confusing - and (b) people have one more incentive to convert to the new
behavior.

No behavior change except for the added prints is intended at this time.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/b53/b53_common.c       |  1 +
 drivers/net/dsa/bcm_sf2.c              |  1 +
 drivers/net/dsa/dsa_loop.c             |  1 -
 drivers/net/dsa/lantiq_gswip.c         |  3 +++
 drivers/net/dsa/microchip/ksz8795.c    |  2 ++
 drivers/net/dsa/microchip/ksz9477.c    |  2 ++
 drivers/net/dsa/mt7530.c               |  1 -
 drivers/net/dsa/mv88e6xxx/chip.c       |  1 +
 drivers/net/dsa/ocelot/felix.c         |  1 -
 drivers/net/dsa/qca/ar9331.c           |  2 ++
 drivers/net/dsa/qca8k.c                |  1 -
 drivers/net/dsa/rtl8366rb.c            |  2 ++
 drivers/net/dsa/sja1105/sja1105_main.c |  2 --
 net/dsa/dsa2.c                         |  2 ++
 net/dsa/slave.c                        | 29 +++++++++++++++++++-------
 15 files changed, 38 insertions(+), 13 deletions(-)

On Mon, Sep 07, 2020 at 09:07:34PM -0700, Florian Fainelli wrote:
> You should be able to make b53 and bcm_sf2 also use
> configure_vlan_while_not_filtering to true (or rather not specify it), give
> me until tomorrow to run various tests if you don't mind.

Ok, but I would prefer not doing that in this patch.

Note that, given a choice, I try to avoid introducing functional changes
in drivers where I don't have the hardware to test, or somebody to
confirm that it works, at the very least.

For that reason, mv88e6xxx doesn't even have this flag set, even though
Russell had sent a patch for it with the old name:
https://lore.kernel.org/netdev/E1ij6pq-00084C-47@rmk-PC.armlinux.org.uk/

Somebody with a Marvell switch should probably pick that up and resend.

Thanks,
-Vladimir

On Tue, Sep 08, 2020 at 05:02:06PM -0700, Florian Fainelli wrote:
> Found the problem, we do not allow the CPU port to be configured as
> untagged, and when we toggle vlan_filtering we actually incorrectly "move"
> the PVID from 1 to 0,

pvid 1 must be coming from the default_pvid of the bridge, I assume.
Where is pvid 0 (aka dev->ports[port].pvid) coming from? Is it simply
the cached value from B53_VLAN_PORT_DEF_TAG, from a previous
b53_vlan_filtering() call? Strange.

> which is incorrect, but since the CPU is also untagged in VID 0 this
> is why it "works" or rather two mistakes canceling it each other.

How does the CPU end up untagged in VLAN 0?

> I still need to confirm this, but the bridge in VLAN filtering mode seems to
> support receiving frames with the default_pvid as tagged, and it will untag
> it for the bridge master device transparently.

So it seems.

> The reason for not allowing the CPU port to be untagged
> (ca8931948344c485569b04821d1f6bcebccd376b) was because the CPU port could be
> added as untagged in several VLANs, e.g.: when port0-3 are PVID 1 untagged,
> and port 4 is PVID 2 untagged. Back then there was no support for Broadcom
> tags, so the only way to differentiate traffic properly was to also add a
> pair of tagged VIDs to the DSA master.
> I am still trying to remember whether there were other concerns that
> prompted me to make that change and would appreciate some thoughts on that.

I think it makes some sense to always configure the VLANs on the CPU
port as tagged either way. I did the same in Felix and it's ok. But that
was due to a hardware limitation. On sja1105 I'm keeping the same flags
as on the user port, and that is ok too.

> Tangentially, maybe we should finally add support for programming the CPU
> port's VLAN membership independently from the other ports.

How?

> The following appears to work nicely now and allows us to get rid of the
> b53_vlan_filtering() logic, which would no longer work now because it
> assumed that toggling vlan_filtering implied that there would be no VLAN
> configuration when filtering was off.
>
> diff --git a/drivers/net/dsa/b53/b53_common.c
> b/drivers/net/dsa/b53/b53_common.c
> index 26fcff85d881..fac033730f4a 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1322,23 +1322,6 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up);
>  int b53_vlan_filtering(struct dsa_switch *ds, int port, bool
> vlan_filtering)
>  {
>         struct b53_device *dev = ds->priv;
> -       u16 pvid, new_pvid;
> -
> -       b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid);
> -       if (!vlan_filtering) {
> -               /* Filtering is currently enabled, use the default PVID
> since
> -                * the bridge does not expect tagging anymore
> -                */
> -               dev->ports[port].pvid = pvid;
> -               new_pvid = b53_default_pvid(dev);
> -       } else {
> -               /* Filtering is currently disabled, restore the previous
> PVID */
> -               new_pvid = dev->ports[port].pvid;
> -       }
> -
> -       if (pvid != new_pvid)
> -               b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port),
> -                           new_pvid);

Yes, much simpler.

> 
>         b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering);
> 
> @@ -1389,7 +1372,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
>                         untagged = true;
> 
>                 vl->members |= BIT(port);
> -               if (untagged && !dsa_is_cpu_port(ds, port))
> +               if (untagged)
>                         vl->untag |= BIT(port);
>                 else
>                         vl->untag &= ~BIT(port);
> @@ -1427,7 +1410,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
>                 if (pvid == vid)
>                         pvid = b53_default_pvid(dev);
> 
> -               if (untagged && !dsa_is_cpu_port(ds, port))
> +               if (untagged)

Ok, so you're removing this workaround now. A welcome simplification.

>                         vl->untag &= ~(BIT(port));
> 
>                 b53_set_vlan_entry(dev, vid, vl);
> @@ -2563,6 +2546,8 @@ struct b53_device *b53_switch_alloc(struct device
> *base,
>         dev->priv = priv;
>         dev->ops = ops;
>         ds->ops = &b53_switch_ops;
> +       ds->configure_vlan_while_not_filtering = true;
> +       dev->vlan_enabled = ds->configure_vlan_while_not_filtering;
>         mutex_init(&dev->reg_mutex);
>         mutex_init(&dev->stats_mutex);
> 
> 
> -- 
> Florian

Looks good!

I'm going to hold off with my configure_vlan_while_not_filtering patch.
You can send this one before me.

Thanks,
-Vladimir

On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote:
> How do you make sure that the CPU port sees the frame untagged which would
> be necessary for a VLAN-unaware bridge? Do you have a special remapping
> rule?

No, I don't have any remapping rules that would be relevant here.
Why would the frames need to be necessarily untagged for a VLAN-unaware
bridge, why is it a problem if they aren't?

bool br_allowed_ingress(const struct net_bridge *br,
			struct net_bridge_vlan_group *vg, struct sk_buff *skb,
			u16 *vid, u8 *state)
{
	/* If VLAN filtering is disabled on the bridge, all packets are
	 * permitted.
	 */
	if (!br_opt_get(br, BROPT_VLAN_ENABLED)) {
		BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
		return true;
	}

	return __allowed_ingress(br, vg, skb, vid, state);
}

If I have a VLAN on a bridged switch port where the bridge is not
filtering, I have an 8021q upper of the bridge with that VLAN ID.

> Initially the concern I had was with the use case described above which was
> a 802.1Q separation, but in hindsight MAC address learning would result in
> the frames going to the appropriate ports/VLANs anyway.

If by "separation" you mean "limiting the forwarding domain", the switch
keeps the same VLAN associated with the frame internally, regardless of
whether it's egress-tagged or not.

> > 
> > > Tangentially, maybe we should finally add support for programming the CPU
> > > port's VLAN membership independently from the other ports.
> > 
> > How?
> 
> Something like this:
> 
> https://lore.kernel.org/lkml/20180625091713.GA13442@apalos/T/

I need to take some time to understand what's going on there.

Hi Vladimir,

On Tue Sep 08 2020, Vladimir Oltean wrote:
> On Tue, Sep 08, 2020 at 12:14:12PM +0200, Kurt Kanzenbach wrote:
>> On Mon Sep 07 2020, Vladimir Oltean wrote:
>> > New drivers always seem to omit setting this flag, for some reason.
>>
>> Yes, because it's not well documented, or is it? Before writing the
>> hellcreek DSA driver, I've read dsa.rst documentation to find out what
>> callback function should to what. Did I miss something?
>
> Honestly, Documentation/networking/dsa/dsa.rst is out of date by quite a
> bit. And this trend of having boolean flags in struct dsa_switch started
> after the documentation stopped being updated.

Maybe it would be good to document new flags when they're introduced :).

>
> But I didn't say it's your fault for not setting the flag, it is easy to
> miss, and that's what this patch is trying to improve.
>
>> > So let's reverse the logic: the DSA core sets it by default to true
>> > before the .setup() callback, and legacy drivers can turn it off. This
>> > way, new drivers get the new behavior by default, unless they
>> > explicitly set the flag to false, which is more obvious during review.
>>
>> Yeah, that behavior makes more sense to me. Thank you.
>
> Ok, thanks.

Is this merged? I don't see it. Do I have to set
`configure_vlan_while_not_filtering' explicitly to true for the next
hellcreek version?

Thanks,
Kurt

On Fri, Oct 02, 2020 at 10:06:28AM +0200, Kurt Kanzenbach wrote:
> Hi Vladimir,
> 
> On Tue Sep 08 2020, Vladimir Oltean wrote:
> > On Tue, Sep 08, 2020 at 12:14:12PM +0200, Kurt Kanzenbach wrote:
> >> On Mon Sep 07 2020, Vladimir Oltean wrote:
> >> > New drivers always seem to omit setting this flag, for some reason.
> >>
> >> Yes, because it's not well documented, or is it? Before writing the
> >> hellcreek DSA driver, I've read dsa.rst documentation to find out what
> >> callback function should to what. Did I miss something?
> >
> > Honestly, Documentation/networking/dsa/dsa.rst is out of date by quite a
> > bit. And this trend of having boolean flags in struct dsa_switch started
> > after the documentation stopped being updated.
> 
> Maybe it would be good to document new flags when they're introduced :).

Yup, will definitely do that when I resend.

> >
> > But I didn't say it's your fault for not setting the flag, it is easy to
> > miss, and that's what this patch is trying to improve.
> >
> >> > So let's reverse the logic: the DSA core sets it by default to true
> >> > before the .setup() callback, and legacy drivers can turn it off. This
> >> > way, new drivers get the new behavior by default, unless they
> >> > explicitly set the flag to false, which is more obvious during review.
> >>
> >> Yeah, that behavior makes more sense to me. Thank you.
> >
> > Ok, thanks.
> 
> Is this merged? I don't see it. Do I have to set
> `configure_vlan_while_not_filtering' explicitly to true for the next
> hellcreek version?

Yes, please set it to true. The refactoring change didn't get merged in
time, I don't want it to interfere with your series.

Thanks,
-Vladimir

Hi Kurt,

On Fri, Oct 02, 2020 at 11:15:27AM +0300, Vladimir Oltean wrote:
> > Is this merged? I don't see it. Do I have to set
> > `configure_vlan_while_not_filtering' explicitly to true for the next
> > hellcreek version?
> 
> Yes, please set it to true. The refactoring change didn't get merged in
> time, I don't want it to interfere with your series.

Do you plan on resending hellcreek for 5.10?

Thanks,
-Vladimir

Hi Vladimir,

On Sat Oct 03 2020, Vladimir Oltean wrote:
> Hi Kurt,
>
> On Fri, Oct 02, 2020 at 11:15:27AM +0300, Vladimir Oltean wrote:
>> > Is this merged? I don't see it. Do I have to set
>> > `configure_vlan_while_not_filtering' explicitly to true for the next
>> > hellcreek version?
>> 
>> Yes, please set it to true. The refactoring change didn't get merged in
>> time, I don't want it to interfere with your series.
>
> Do you plan on resending hellcreek for 5.10?

I've implemented the configure_vlan_while_not_filtering behaviour
yesterday with caching and lists like the sja driver does. It needs some
testing and then I'll post it. Maybe next week.

Thanks,
Kurt

On Sat, Oct 03, 2020 at 11:45:27AM +0200, Kurt Kanzenbach wrote:
> Maybe next week.

The merge window opens next week. This means you can still resend as
RFC, but it can only be accepted in net-next after ~2 weeks.

Thanks,
-Vladimir

On Sun, Oct 04, 2020 at 01:56:17PM +0300, Vladimir Oltean wrote:
> On Sat, Oct 03, 2020 at 11:45:27AM +0200, Kurt Kanzenbach wrote:
> > Maybe next week.
> 
> The merge window opens next week. This means you can still resend as
> RFC, but it can only be accepted in net-next after ~2 weeks.

False alarm, looks like we have an rc8. I guess that means you can
resend some time this week.

Thanks,
-Vladimir