[PATCH net-next] net: dsa: set configure_vlan_while_not_filtering to true by default

[PATCH net-next] net: dsa: set configure_vlan_while_not_filtering to true by default

[Vladimir Oltean]

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.

The Broadcom Starfighter 2 driver calls the common b53_switch_alloc and
therefore also inherits the configure_vlan_while_not_filtering=true
behavior.

Also, print a message through netlink extack 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 the extack should be much more difficult to miss, unlike
kernel logs - 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.

$ ip link add br0 type bridge vlan_filtering 0
$ ip link set sw0p2 master br0
[   60.315148] br0: port 1(sw0p2) entered blocking state
[   60.320350] br0: port 1(sw0p2) entered disabled state
[   60.327839] device sw0p2 entered promiscuous mode
[   60.334905] br0: port 1(sw0p2) entered blocking state
[   60.340142] br0: port 1(sw0p2) entered forwarding state
Warning: dsa_core: skipping configuration of VLAN. # This was the pvid
$ bridge vlan add dev sw0p2 vid 100
Warning: dsa_core: skipping configuration of VLAN.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/b53/b53_common.c       | 3 +--
 drivers/net/dsa/dsa_loop.c             | 1 -
 drivers/net/dsa/hirschmann/hellcreek.c | 5 -----
 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               | 2 --
 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                        | 9 ++++++---
 15 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 5d6d9f91d40a..a238ded354c2 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2602,9 +2602,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;
 	ds->untag_bridge_pvid = true;
-	dev->vlan_enabled = ds->configure_vlan_while_not_filtering;
+	dev->vlan_enabled = true;
 	mutex_init(&dev->reg_mutex);
 	mutex_init(&dev->stats_mutex);
 
diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index be61ce93a377..5f69216376fe 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -323,7 +323,6 @@ static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
 	ds->dev = &mdiodev->dev;
 	ds->ops = &dsa_loop_driver;
 	ds->priv = ps;
-	ds->configure_vlan_while_not_filtering = true;
 	ps->bus = mdiodev->bus;
 
 	dev_set_drvdata(&mdiodev->dev, ds);
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 205249504289..9a1921e653e8 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -1033,11 +1033,6 @@ static int hellcreek_setup(struct dsa_switch *ds)
 	/* Configure PCP <-> TC mapping */
 	hellcreek_setup_tc_identity_mapping(hellcreek);
 
-	/* Allow VLAN configurations while not filtering which is the default
-	 * for new DSA drivers.
-	 */
-	ds->configure_vlan_while_not_filtering = true;
-
 	/* The VLAN awareness is a global switch setting. Therefore, mixed vlan
 	 * filtering setups are not supported.
 	 */
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 244729ee3bdb..9fec97773a15 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -827,6 +827,9 @@ static int gswip_setup(struct dsa_switch *ds)
 	}
 
 	gswip_port_enable(ds, cpu_port, NULL);
+
+	ds->configure_vlan_while_not_filtering = false;
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index d639f9476bd9..37a73421e2cc 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1094,6 +1094,8 @@ static int ksz8795_setup(struct dsa_switch *ds)
 
 	ksz_init_mib_timer(dev);
 
+	ds->configure_vlan_while_not_filtering = false;
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 71cf24f20252..00e38c8e0d01 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1381,6 +1381,8 @@ static int ksz9477_setup(struct dsa_switch *ds)
 
 	ksz_init_mib_timer(dev);
 
+	ds->configure_vlan_while_not_filtering = false;
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 199a135125b2..d2196197d920 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1639,7 +1639,6 @@ mt7530_setup(struct dsa_switch *ds)
 	 * as two netdev instances.
 	 */
 	dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent;
-	ds->configure_vlan_while_not_filtering = true;
 	ds->mtu_enforcement_ingress = true;
 
 	if (priv->id == ID_MT7530) {
@@ -1878,7 +1877,6 @@ mt7531_setup(struct dsa_switch *ds)
 			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
 	}
 
-	ds->configure_vlan_while_not_filtering = true;
 	ds->mtu_enforcement_ingress = true;
 
 	/* Flush the FDB table */
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 4aa7d0a8f197..20ed433b55df 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2858,7 +2858,6 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 
 	chip->ds = ds;
 	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
-	ds->configure_vlan_while_not_filtering = true;
 
 	mv88e6xxx_reg_lock(chip);
 
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 768a74dc462a..8492151f8133 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -599,7 +599,6 @@ static int felix_setup(struct dsa_switch *ds)
 			 ANA_PGID_PGID, PGID_UC);
 
 	ds->mtu_enforcement_ingress = true;
-	ds->configure_vlan_while_not_filtering = true;
 	ds->assisted_learning_on_cpu_port = true;
 
 	return 0;
diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 1e3706054ae1..ca2ad77b71f1 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -402,6 +402,8 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
 	if (ret)
 		goto error;
 
+	ds->configure_vlan_while_not_filtering = false;
+
 	return 0;
 error:
 	dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index f54e8b6c8621..6127823d6c2e 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1431,7 +1431,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
-	priv->ds->configure_vlan_while_not_filtering = true;
 	priv->ds->priv = priv;
 	priv->ops = qca8k_switch_ops;
 	priv->ds->ops = &priv->ops;
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 896978568716..c6cc4938897c 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -972,6 +972,8 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	ds->configure_vlan_while_not_filtering = false;
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 050b1260f358..282253543f3b 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2901,8 +2901,6 @@ static int sja1105_setup(struct dsa_switch *ds)
 
 	ds->mtu_enforcement_ingress = true;
 
-	ds->configure_vlan_while_not_filtering = true;
-
 	rc = sja1105_devlink_setup(ds);
 	if (rc < 0)
 		return rc;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 01f21b0b379a..194e09234648 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -448,6 +448,8 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	if (err)
 		goto unregister_devlink_ports;
 
+	ds->configure_vlan_while_not_filtering = true;
+
 	err = ds->ops->setup(ds);
 	if (err < 0)
 		goto unregister_notifier;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 5a1769602e65..b9fd171e1805 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -323,7 +323,8 @@ dsa_slave_vlan_check_for_8021q_uppers(struct net_device *slave,
 }
 
 static int dsa_slave_vlan_add(struct net_device *dev,
-			      const struct switchdev_obj *obj)
+			      const struct switchdev_obj *obj,
+			      struct netlink_ext_ack *extack)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
@@ -333,8 +334,10 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	if (obj->orig_dev != dev)
 		return -EOPNOTSUPP;
 
-	if (dsa_port_skip_vlan_configuration(dp))
+	if (dsa_port_skip_vlan_configuration(dp)) {
+		NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN");
 		return 0;
+	}
 
 	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
 
@@ -386,7 +389,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 		err = dsa_port_mdb_add(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		err = dsa_slave_vlan_add(dev, obj);
+		err = dsa_slave_vlan_add(dev, obj, extack);
 		break;
 	default:
 		err = -EOPNOTSUPP;
-- 
2.25.1

Re: [PATCH net-next] net: dsa: set configure_vlan_while_not_filtering to true by default

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

On Thu Jan 14 2021, Vladimir Oltean wrote:
> 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.
>
> The Broadcom Starfighter 2 driver calls the common b53_switch_alloc and
> therefore also inherits the configure_vlan_while_not_filtering=true
> behavior.
>
> Also, print a message through netlink extack 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 the extack should be much more difficult to miss, unlike
> kernel logs - 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.
>
> $ ip link add br0 type bridge vlan_filtering 0
> $ ip link set sw0p2 master br0
> [   60.315148] br0: port 1(sw0p2) entered blocking state
> [   60.320350] br0: port 1(sw0p2) entered disabled state
> [   60.327839] device sw0p2 entered promiscuous mode
> [   60.334905] br0: port 1(sw0p2) entered blocking state
> [   60.340142] br0: port 1(sw0p2) entered forwarding state
> Warning: dsa_core: skipping configuration of VLAN. # This was the pvid
> $ bridge vlan add dev sw0p2 vid 100
> Warning: dsa_core: skipping configuration of VLAN.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH net-next] net: dsa: set configure_vlan_while_not_filtering to true by default

[Florian Fainelli]

On 1/14/2021 9:34 AM, Vladimir Oltean wrote:
> 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.
> 
> The Broadcom Starfighter 2 driver calls the common b53_switch_alloc and
> therefore also inherits the configure_vlan_while_not_filtering=true
> behavior.
> 
> Also, print a message through netlink extack 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 the extack should be much more difficult to miss, unlike
> kernel logs - 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.
> 
> $ ip link add br0 type bridge vlan_filtering 0
> $ ip link set sw0p2 master br0
> [   60.315148] br0: port 1(sw0p2) entered blocking state
> [   60.320350] br0: port 1(sw0p2) entered disabled state
> [   60.327839] device sw0p2 entered promiscuous mode
> [   60.334905] br0: port 1(sw0p2) entered blocking state
> [   60.340142] br0: port 1(sw0p2) entered forwarding state
> Warning: dsa_core: skipping configuration of VLAN. # This was the pvid
> $ bridge vlan add dev sw0p2 vid 100
> Warning: dsa_core: skipping configuration of VLAN.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next] net: dsa: set configure_vlan_while_not_filtering to true by default

[Jakub Kicinski]

On Thu, 14 Jan 2021 19:34:26 +0200 Vladimir Oltean wrote:
> 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.

No longer applies :(

Re: [PATCH net-next] net: dsa: set configure_vlan_while_not_filtering to true by default

[Vladimir Oltean]

On Fri, Jan 15, 2021 at 03:03:14PM -0800, Jakub Kicinski wrote:
> On Thu, 14 Jan 2021 19:34:26 +0200 Vladimir Oltean wrote:
> > 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.
> 
> No longer applies :(

What's the error?

Re: [PATCH net-next] net: dsa: set configure_vlan_while_not_filtering to true by default

[Vladimir Oltean]

On Sat, Jan 16, 2021 at 01:12:48AM +0200, Vladimir Oltean wrote:
> On Fri, Jan 15, 2021 at 03:03:14PM -0800, Jakub Kicinski wrote:
> > On Thu, 14 Jan 2021 19:34:26 +0200 Vladimir Oltean wrote:
> > > 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.
> > 
> > No longer applies :(
> 
> What's the error?

Never mind, I know what happened. It rebased cleanly just because git is
smart. I'll resend a v2 with the updated context.