[PATCH v2 net-next 0/2] Dynamic toggling of vlan_filtering for SJA1105 DSA

[PATCH v2 net-next 0/2] Dynamic toggling of vlan_filtering for SJA1105 DSA

[Vladimir Oltean]

This patchset addresses a limitation in dsa_8021q where this sequence of
commands was causing the switch to stop forwarding traffic:

  ip link add name br0 type bridge vlan_filtering 0
  ip link set dev swp2 master br0
  echo 1 > /sys/class/net/br0/bridge/vlan_filtering
  echo 0 > /sys/class/net/br0/bridge/vlan_filtering

The issue has to do with the VLAN table manipulations that dsa_8021q
does without notifying the bridge layer. The solution is to always
restore the VLANs that the bridge knows about, when disabling tagging.

Depends on Vivien Didelot's patchset:
https://patchwork.ozlabs.org/project/netdev/list/?series=127197&state=*

Also see this discussion thread:
https://www.spinics.net/lists/netdev/msg581042.html

Vladimir Oltean (2):
  net: bridge: Populate the pvid flag in br_vlan_get_info
  net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

 net/bridge/br_vlan.c |  2 +
 net/dsa/tag_8021q.c  | 91 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 73 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH v2 net-next 1/2] net: bridge: Populate the pvid flag in br_vlan_get_info

[Vladimir Oltean]

Currently this simplified code snippet fails:

	br_vlan_get_pvid(netdev, &pvid);
	br_vlan_get_info(netdev, pvid, &vinfo);
	ASSERT(!(vinfo.flags & BRIDGE_VLAN_INFO_PVID));

It is intuitive that the pvid of a netdevice should have the
BRIDGE_VLAN_INFO_PVID flag set.

However I can't seem to pinpoint a commit where this behavior was
introduced. It seems like it's been like that since forever.

At a first glance it would make more sense to just handle the
BRIDGE_VLAN_INFO_PVID flag in __vlan_add_flags. However, as Nikolay
explains:

  There are a few reasons why we don't do it, most importantly because
  we need to have only one visible pvid at any single time, even if it's
  stale - it must be just one. Right now that rule will not be violated
  by this change, but people will try using this flag and could see two
  pvids simultaneously. You can see that the pvid code is even using
  memory barriers to propagate the new value faster and everywhere the
  pvid is read only once.  That is the reason the flag is set
  dynamically when dumping entries, too.  A second (weaker) argument
  against would be given the above we don't want another way to do the
  same thing, specifically if it can provide us with two pvids (e.g. if
  walking the vlan list) or if it can provide us with a pvid different
  from the one set in the vg. [Obviously, I'm talking about RCU
  pvid/vlan use cases similar to the dumps.  The locked cases are fine.
  I would like to avoid explaining why this shouldn't be relied upon
  without locking]

So instead of introducing the above change and making sure of the pvid
uniqueness under RCU, simply dynamically populate the pvid flag in
br_vlan_get_info().

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_vlan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index f5b2aeebbfe9..bb98984cd27d 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -1281,6 +1281,8 @@ int br_vlan_get_info(const struct net_device *dev, u16 vid,
 
 	p_vinfo->vid = vid;
 	p_vinfo->flags = v->flags;
+	if (vid == br_get_pvid(vg))
+		p_vinfo->flags |= BRIDGE_VLAN_INFO_PVID;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(br_vlan_get_info);
-- 
2.17.1

[PATCH v2 net-next 2/2] net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

[Vladimir Oltean]

The bridge core assumes that enabling/disabling vlan_filtering will
translate into the simple toggling of a flag for switchdev drivers.

That is clearly not the case for sja1105, which alters the VLAN table
and the pvids in order to obtain port separation in standalone mode.

There are 2 parts to the issue.

First, tag_8021q changes the pvid to a unique per-port rx_vid for frame
identification. But we need to disable tag_8021q when vlan_filtering
kicks in, and at that point, the VLAN configured as pvid will have to be
removed from the filtering table of the ports. With an invalid pvid, the
ports will drop all traffic.  Since the bridge will not call any vlan
operation through switchdev after enabling vlan_filtering, we need to
ensure we're in a functional state ourselves. Hence read the pvid that
the bridge is aware of, and program that into our ports.

Secondly, tag_8021q uses the 1024-3071 range privately in
vlan_filtering=0 mode. Had the user installed one of these VLANs during
a previous vlan_filtering=1 session, then upon the next tag_8021q
cleanup for vlan_filtering to kick in again, VLANs in that range will
get deleted unconditionally, hence breaking user expectation. So when
deleting the VLANs, check if the bridge had knowledge about them, and if
it did, re-apply the settings. Wrap this logic inside a
dsa_8021q_vid_apply helper function to reduce code duplication.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/tag_8021q.c | 91 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 71 insertions(+), 20 deletions(-)

diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 67a1bc635a7b..81f943e365b9 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -93,6 +93,68 @@ int dsa_8021q_rx_source_port(u16 vid)
 }
 EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
 
+static int dsa_8021q_restore_pvid(struct dsa_switch *ds, int port)
+{
+	struct bridge_vlan_info vinfo;
+	struct net_device *slave;
+	u16 pvid;
+	int err;
+
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
+	slave = ds->ports[port].slave;
+
+	err = br_vlan_get_pvid(slave, &pvid);
+	if (err < 0) {
+		dev_err(ds->dev, "Couldn't determine bridge PVID\n");
+		return err;
+	}
+
+	err = br_vlan_get_info(slave, pvid, &vinfo);
+	if (err < 0) {
+		dev_err(ds->dev, "Couldn't determine PVID attributes\n");
+		return err;
+	}
+
+	return dsa_port_vid_add(&ds->ports[port], pvid, vinfo.flags);
+}
+
+/* If @enabled is true, installs @vid with @flags into the switch port's HW
+ * filter.
+ * If @enabled is false, deletes @vid (ignores @flags) from the port. Had the
+ * user explicitly configured this @vid through the bridge core, then the @vid
+ * is installed again, but this time with the flags from the bridge layer.
+ */
+static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid,
+			       u16 flags, bool enabled)
+{
+	struct dsa_port *dp = &ds->ports[port];
+	struct bridge_vlan_info vinfo;
+	int err;
+
+	if (enabled)
+		return dsa_port_vid_add(dp, vid, flags);
+
+	err = dsa_port_vid_del(dp, vid);
+	if (err < 0)
+		return err;
+
+	/* Nothing to restore from the bridge for a non-user port */
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
+	err = br_vlan_get_info(dp->slave, vid, &vinfo);
+	/* Couldn't determine bridge attributes for this vid,
+	 * it means the bridge had not configured it.
+	 */
+	if (err < 0)
+		return 0;
+
+	/* Restore the VID from the bridge */
+	return dsa_port_vid_add(dp, vid, vinfo.flags);
+}
+
 /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
  * front-panel switch port (here swp0).
  *
@@ -148,8 +210,6 @@ EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
 int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 {
 	int upstream = dsa_upstream_port(ds, port);
-	struct dsa_port *dp = &ds->ports[port];
-	struct dsa_port *upstream_dp = &ds->ports[upstream];
 	u16 rx_vid = dsa_8021q_rx_vid(ds, port);
 	u16 tx_vid = dsa_8021q_tx_vid(ds, port);
 	int i, err;
@@ -166,7 +226,6 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 	 * restrictions, so there are no concerns about leaking traffic.
 	 */
 	for (i = 0; i < ds->num_ports; i++) {
-		struct dsa_port *other_dp = &ds->ports[i];
 		u16 flags;
 
 		if (i == upstream)
@@ -179,10 +238,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 			/* The RX VID is a regular VLAN on all others */
 			flags = BRIDGE_VLAN_INFO_UNTAGGED;
 
-		if (enabled)
-			err = dsa_port_vid_add(other_dp, rx_vid, flags);
-		else
-			err = dsa_port_vid_del(other_dp, rx_vid);
+		err = dsa_8021q_vid_apply(ds, i, rx_vid, flags, enabled);
 		if (err) {
 			dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
 				rx_vid, port, err);
@@ -193,10 +249,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 	/* CPU port needs to see this port's RX VID
 	 * as tagged egress.
 	 */
-	if (enabled)
-		err = dsa_port_vid_add(upstream_dp, rx_vid, 0);
-	else
-		err = dsa_port_vid_del(upstream_dp, rx_vid);
+	err = dsa_8021q_vid_apply(ds, upstream, rx_vid, 0, enabled);
 	if (err) {
 		dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
 			rx_vid, port, err);
@@ -204,26 +257,24 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 	}
 
 	/* Finally apply the TX VID on this port and on the CPU port */
-	if (enabled)
-		err = dsa_port_vid_add(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED);
-	else
-		err = dsa_port_vid_del(dp, tx_vid);
+	err = dsa_8021q_vid_apply(ds, port, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
+				  enabled);
 	if (err) {
 		dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
 			tx_vid, port, err);
 		return err;
 	}
-	if (enabled)
-		err = dsa_port_vid_add(upstream_dp, tx_vid, 0);
-	else
-		err = dsa_port_vid_del(upstream_dp, tx_vid);
+	err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
 	if (err) {
 		dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
 			tx_vid, upstream, err);
 		return err;
 	}
 
-	return 0;
+	if (!enabled)
+		err = dsa_8021q_restore_pvid(ds, port);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
 
-- 
2.17.1

Re: [PATCH v2 net-next 2/2] net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

[Vladimir Oltean]

On Sun, 25 Aug 2019 at 21:46, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> The bridge core assumes that enabling/disabling vlan_filtering will
> translate into the simple toggling of a flag for switchdev drivers.
>
> That is clearly not the case for sja1105, which alters the VLAN table
> and the pvids in order to obtain port separation in standalone mode.
>
> There are 2 parts to the issue.
>
> First, tag_8021q changes the pvid to a unique per-port rx_vid for frame
> identification. But we need to disable tag_8021q when vlan_filtering
> kicks in, and at that point, the VLAN configured as pvid will have to be
> removed from the filtering table of the ports. With an invalid pvid, the
> ports will drop all traffic.  Since the bridge will not call any vlan
> operation through switchdev after enabling vlan_filtering, we need to
> ensure we're in a functional state ourselves. Hence read the pvid that
> the bridge is aware of, and program that into our ports.
>
> Secondly, tag_8021q uses the 1024-3071 range privately in
> vlan_filtering=0 mode. Had the user installed one of these VLANs during
> a previous vlan_filtering=1 session, then upon the next tag_8021q
> cleanup for vlan_filtering to kick in again, VLANs in that range will
> get deleted unconditionally, hence breaking user expectation. So when
> deleting the VLANs, check if the bridge had knowledge about them, and if
> it did, re-apply the settings. Wrap this logic inside a
> dsa_8021q_vid_apply helper function to reduce code duplication.
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/tag_8021q.c | 91 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 71 insertions(+), 20 deletions(-)
>
> diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> index 67a1bc635a7b..81f943e365b9 100644
> --- a/net/dsa/tag_8021q.c
> +++ b/net/dsa/tag_8021q.c
> @@ -93,6 +93,68 @@ int dsa_8021q_rx_source_port(u16 vid)
>  }
>  EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
>
> +static int dsa_8021q_restore_pvid(struct dsa_switch *ds, int port)
> +{
> +       struct bridge_vlan_info vinfo;
> +       struct net_device *slave;
> +       u16 pvid;
> +       int err;
> +
> +       if (!dsa_is_user_port(ds, port))
> +               return 0;
> +
> +       slave = ds->ports[port].slave;
> +
> +       err = br_vlan_get_pvid(slave, &pvid);
> +       if (err < 0) {
> +               dev_err(ds->dev, "Couldn't determine bridge PVID\n");
> +               return err;

I now realize that I shouldn't error out here, since it is not an
error to not have a pvid on the port. Will make this change in v3,
along with the other change requests that will arise upon review.

> +       }
> +
> +       err = br_vlan_get_info(slave, pvid, &vinfo);
> +       if (err < 0) {
> +               dev_err(ds->dev, "Couldn't determine PVID attributes\n");
> +               return err;
> +       }
> +
> +       return dsa_port_vid_add(&ds->ports[port], pvid, vinfo.flags);
> +}
> +
> +/* If @enabled is true, installs @vid with @flags into the switch port's HW
> + * filter.
> + * If @enabled is false, deletes @vid (ignores @flags) from the port. Had the
> + * user explicitly configured this @vid through the bridge core, then the @vid
> + * is installed again, but this time with the flags from the bridge layer.
> + */
> +static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid,
> +                              u16 flags, bool enabled)
> +{
> +       struct dsa_port *dp = &ds->ports[port];
> +       struct bridge_vlan_info vinfo;
> +       int err;
> +
> +       if (enabled)
> +               return dsa_port_vid_add(dp, vid, flags);
> +
> +       err = dsa_port_vid_del(dp, vid);
> +       if (err < 0)
> +               return err;
> +
> +       /* Nothing to restore from the bridge for a non-user port */
> +       if (!dsa_is_user_port(ds, port))
> +               return 0;
> +
> +       err = br_vlan_get_info(dp->slave, vid, &vinfo);
> +       /* Couldn't determine bridge attributes for this vid,
> +        * it means the bridge had not configured it.
> +        */
> +       if (err < 0)
> +               return 0;
> +
> +       /* Restore the VID from the bridge */
> +       return dsa_port_vid_add(dp, vid, vinfo.flags);
> +}
> +
>  /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
>   * front-panel switch port (here swp0).
>   *
> @@ -148,8 +210,6 @@ EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
>  int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>  {
>         int upstream = dsa_upstream_port(ds, port);
> -       struct dsa_port *dp = &ds->ports[port];
> -       struct dsa_port *upstream_dp = &ds->ports[upstream];
>         u16 rx_vid = dsa_8021q_rx_vid(ds, port);
>         u16 tx_vid = dsa_8021q_tx_vid(ds, port);
>         int i, err;
> @@ -166,7 +226,6 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>          * restrictions, so there are no concerns about leaking traffic.
>          */
>         for (i = 0; i < ds->num_ports; i++) {
> -               struct dsa_port *other_dp = &ds->ports[i];
>                 u16 flags;
>
>                 if (i == upstream)
> @@ -179,10 +238,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>                         /* The RX VID is a regular VLAN on all others */
>                         flags = BRIDGE_VLAN_INFO_UNTAGGED;
>
> -               if (enabled)
> -                       err = dsa_port_vid_add(other_dp, rx_vid, flags);
> -               else
> -                       err = dsa_port_vid_del(other_dp, rx_vid);
> +               err = dsa_8021q_vid_apply(ds, i, rx_vid, flags, enabled);
>                 if (err) {
>                         dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
>                                 rx_vid, port, err);
> @@ -193,10 +249,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>         /* CPU port needs to see this port's RX VID
>          * as tagged egress.
>          */
> -       if (enabled)
> -               err = dsa_port_vid_add(upstream_dp, rx_vid, 0);
> -       else
> -               err = dsa_port_vid_del(upstream_dp, rx_vid);
> +       err = dsa_8021q_vid_apply(ds, upstream, rx_vid, 0, enabled);
>         if (err) {
>                 dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
>                         rx_vid, port, err);
> @@ -204,26 +257,24 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>         }
>
>         /* Finally apply the TX VID on this port and on the CPU port */
> -       if (enabled)
> -               err = dsa_port_vid_add(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED);
> -       else
> -               err = dsa_port_vid_del(dp, tx_vid);
> +       err = dsa_8021q_vid_apply(ds, port, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
> +                                 enabled);
>         if (err) {
>                 dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
>                         tx_vid, port, err);
>                 return err;
>         }
> -       if (enabled)
> -               err = dsa_port_vid_add(upstream_dp, tx_vid, 0);
> -       else
> -               err = dsa_port_vid_del(upstream_dp, tx_vid);
> +       err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
>         if (err) {
>                 dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
>                         tx_vid, upstream, err);
>                 return err;
>         }
>
> -       return 0;
> +       if (!enabled)
> +               err = dsa_8021q_restore_pvid(ds, port);
> +
> +       return err;
>  }
>  EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
>
> --
> 2.17.1
>

Thanks!
-Vladimir

Re: [PATCH v2 net-next 2/2] net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

[Vivien Didelot]

Hi Vladimir,

On Sun, 25 Aug 2019 21:44:54 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> -	if (enabled)
> -		err = dsa_port_vid_add(upstream_dp, tx_vid, 0);
> -	else
> -		err = dsa_port_vid_del(upstream_dp, tx_vid);
> +	err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
>  	if (err) {
>  		dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
>  			tx_vid, upstream, err);
>  		return err;
>  	}
>  
> -	return 0;
> +	if (!enabled)
> +		err = dsa_8021q_restore_pvid(ds, port);
> +
> +	return err;
>  }

I did not dig that much into tag_8021q.c yet. From seeing this portion,
I'm just wondering if these two helpers couldn't be part of the same logic
as they both act upon the "enabled" condition?

Otherwise I have no complains about the series.


Thanks,

	Vivien

Re: [PATCH v2 net-next 2/2] net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

[Vladimir Oltean]

Hi Vivien,

On Mon, 26 Aug 2019 at 18:20, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Sun, 25 Aug 2019 21:44:54 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > -     if (enabled)
> > -             err = dsa_port_vid_add(upstream_dp, tx_vid, 0);
> > -     else
> > -             err = dsa_port_vid_del(upstream_dp, tx_vid);
> > +     err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
> >       if (err) {
> >               dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
> >                       tx_vid, upstream, err);
> >               return err;
> >       }
> >
> > -     return 0;
> > +     if (!enabled)
> > +             err = dsa_8021q_restore_pvid(ds, port);
> > +
> > +     return err;
> >  }
>
> I did not dig that much into tag_8021q.c yet. From seeing this portion,
> I'm just wondering if these two helpers couldn't be part of the same logic
> as they both act upon the "enabled" condition?
>
> Otherwise I have no complains about the series.
>

I thought too about trying to merge the 2 into the same function (not
a lot, though).
But consider that they do different things in the "!enabled" case:
- dsa_8021q_vid_apply: check if this specific vid (provided as
argument) was installed in the bridge, and if so, restore it
- dsa_8021q_restore_pvid: search for the bridge port's pvid, and restore that
I don't think that the end result will look cleaner if I merge these 2 things.

>
> Thanks,
>
>         Vivien

Thanks,
-Vladimir

Re: [PATCH v2 net-next 2/2] net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

[Vladimir Oltean]

Vivien,

On Sun, 25 Aug 2019 at 21:46, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> The bridge core assumes that enabling/disabling vlan_filtering will
> translate into the simple toggling of a flag for switchdev drivers.
>
> That is clearly not the case for sja1105, which alters the VLAN table
> and the pvids in order to obtain port separation in standalone mode.
>
> There are 2 parts to the issue.
>
> First, tag_8021q changes the pvid to a unique per-port rx_vid for frame
> identification. But we need to disable tag_8021q when vlan_filtering
> kicks in, and at that point, the VLAN configured as pvid will have to be
> removed from the filtering table of the ports. With an invalid pvid, the
> ports will drop all traffic.  Since the bridge will not call any vlan
> operation through switchdev after enabling vlan_filtering, we need to
> ensure we're in a functional state ourselves. Hence read the pvid that
> the bridge is aware of, and program that into our ports.
>
> Secondly, tag_8021q uses the 1024-3071 range privately in
> vlan_filtering=0 mode. Had the user installed one of these VLANs during
> a previous vlan_filtering=1 session, then upon the next tag_8021q
> cleanup for vlan_filtering to kick in again, VLANs in that range will
> get deleted unconditionally, hence breaking user expectation. So when
> deleting the VLANs, check if the bridge had knowledge about them, and if
> it did, re-apply the settings. Wrap this logic inside a
> dsa_8021q_vid_apply helper function to reduce code duplication.
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/tag_8021q.c | 91 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 71 insertions(+), 20 deletions(-)
>
> diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> index 67a1bc635a7b..81f943e365b9 100644
> --- a/net/dsa/tag_8021q.c
> +++ b/net/dsa/tag_8021q.c
> @@ -93,6 +93,68 @@ int dsa_8021q_rx_source_port(u16 vid)
>  }
>  EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
>
> +static int dsa_8021q_restore_pvid(struct dsa_switch *ds, int port)
> +{
> +       struct bridge_vlan_info vinfo;
> +       struct net_device *slave;
> +       u16 pvid;
> +       int err;
> +
> +       if (!dsa_is_user_port(ds, port))
> +               return 0;
> +
> +       slave = ds->ports[port].slave;
> +
> +       err = br_vlan_get_pvid(slave, &pvid);
> +       if (err < 0) {
> +               dev_err(ds->dev, "Couldn't determine bridge PVID\n");
> +               return err;
> +       }
> +
> +       err = br_vlan_get_info(slave, pvid, &vinfo);
> +       if (err < 0) {
> +               dev_err(ds->dev, "Couldn't determine PVID attributes\n");
> +               return err;
> +       }
> +
> +       return dsa_port_vid_add(&ds->ports[port], pvid, vinfo.flags);

If the bridge had installed a dsa_8021q VLAN here, I need to use the
dsa_slave_vid_add logic to restore it. The dsa_8021q flags on the CPU
port are "ingress tagged", but that may not be the case for the bridge
VLAN.
Should I expose dsa_slave_vlan_add in dsa_priv.h, or should I just
open-code another dsa_port_vid_add for dp->cpu_dp, duplicating a bit
of code from dsa_slave_vlan_add?

> +}
> +
> +/* If @enabled is true, installs @vid with @flags into the switch port's HW
> + * filter.
> + * If @enabled is false, deletes @vid (ignores @flags) from the port. Had the
> + * user explicitly configured this @vid through the bridge core, then the @vid
> + * is installed again, but this time with the flags from the bridge layer.
> + */
> +static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid,
> +                              u16 flags, bool enabled)
> +{
> +       struct dsa_port *dp = &ds->ports[port];
> +       struct bridge_vlan_info vinfo;
> +       int err;
> +
> +       if (enabled)
> +               return dsa_port_vid_add(dp, vid, flags);
> +
> +       err = dsa_port_vid_del(dp, vid);
> +       if (err < 0)
> +               return err;
> +
> +       /* Nothing to restore from the bridge for a non-user port */
> +       if (!dsa_is_user_port(ds, port))
> +               return 0;
> +
> +       err = br_vlan_get_info(dp->slave, vid, &vinfo);
> +       /* Couldn't determine bridge attributes for this vid,
> +        * it means the bridge had not configured it.
> +        */
> +       if (err < 0)
> +               return 0;
> +
> +       /* Restore the VID from the bridge */
> +       return dsa_port_vid_add(dp, vid, vinfo.flags);
> +}
> +
>  /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
>   * front-panel switch port (here swp0).
>   *
> @@ -148,8 +210,6 @@ EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
>  int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>  {
>         int upstream = dsa_upstream_port(ds, port);
> -       struct dsa_port *dp = &ds->ports[port];
> -       struct dsa_port *upstream_dp = &ds->ports[upstream];
>         u16 rx_vid = dsa_8021q_rx_vid(ds, port);
>         u16 tx_vid = dsa_8021q_tx_vid(ds, port);
>         int i, err;
> @@ -166,7 +226,6 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>          * restrictions, so there are no concerns about leaking traffic.
>          */
>         for (i = 0; i < ds->num_ports; i++) {
> -               struct dsa_port *other_dp = &ds->ports[i];
>                 u16 flags;
>
>                 if (i == upstream)
> @@ -179,10 +238,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>                         /* The RX VID is a regular VLAN on all others */
>                         flags = BRIDGE_VLAN_INFO_UNTAGGED;
>
> -               if (enabled)
> -                       err = dsa_port_vid_add(other_dp, rx_vid, flags);
> -               else
> -                       err = dsa_port_vid_del(other_dp, rx_vid);
> +               err = dsa_8021q_vid_apply(ds, i, rx_vid, flags, enabled);
>                 if (err) {
>                         dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
>                                 rx_vid, port, err);
> @@ -193,10 +249,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>         /* CPU port needs to see this port's RX VID
>          * as tagged egress.
>          */
> -       if (enabled)
> -               err = dsa_port_vid_add(upstream_dp, rx_vid, 0);
> -       else
> -               err = dsa_port_vid_del(upstream_dp, rx_vid);
> +       err = dsa_8021q_vid_apply(ds, upstream, rx_vid, 0, enabled);
>         if (err) {
>                 dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
>                         rx_vid, port, err);
> @@ -204,26 +257,24 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>         }
>
>         /* Finally apply the TX VID on this port and on the CPU port */
> -       if (enabled)
> -               err = dsa_port_vid_add(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED);
> -       else
> -               err = dsa_port_vid_del(dp, tx_vid);
> +       err = dsa_8021q_vid_apply(ds, port, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
> +                                 enabled);
>         if (err) {
>                 dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
>                         tx_vid, port, err);
>                 return err;
>         }
> -       if (enabled)
> -               err = dsa_port_vid_add(upstream_dp, tx_vid, 0);
> -       else
> -               err = dsa_port_vid_del(upstream_dp, tx_vid);
> +       err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
>         if (err) {
>                 dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
>                         tx_vid, upstream, err);
>                 return err;
>         }
>
> -       return 0;
> +       if (!enabled)
> +               err = dsa_8021q_restore_pvid(ds, port);
> +
> +       return err;
>  }
>  EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
>
> --
> 2.17.1
>

Thanks,
-Vladimir

Re: [PATCH v2 net-next 2/2] net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

[Vivien Didelot]

Hi Vladimir,

On Thu, 29 Aug 2019 14:50:14 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > +static int dsa_8021q_restore_pvid(struct dsa_switch *ds, int port)
> > +{
> > +       struct bridge_vlan_info vinfo;
> > +       struct net_device *slave;
> > +       u16 pvid;
> > +       int err;
> > +
> > +       if (!dsa_is_user_port(ds, port))
> > +               return 0;
> > +
> > +       slave = ds->ports[port].slave;
> > +
> > +       err = br_vlan_get_pvid(slave, &pvid);
> > +       if (err < 0) {
> > +               dev_err(ds->dev, "Couldn't determine bridge PVID\n");
> > +               return err;
> > +       }
> > +
> > +       err = br_vlan_get_info(slave, pvid, &vinfo);
> > +       if (err < 0) {
> > +               dev_err(ds->dev, "Couldn't determine PVID attributes\n");
> > +               return err;
> > +       }
> > +
> > +       return dsa_port_vid_add(&ds->ports[port], pvid, vinfo.flags);
> 
> If the bridge had installed a dsa_8021q VLAN here, I need to use the
> dsa_slave_vid_add logic to restore it. The dsa_8021q flags on the CPU
> port are "ingress tagged", but that may not be the case for the bridge
> VLAN.
> Should I expose dsa_slave_vlan_add in dsa_priv.h, or should I just
> open-code another dsa_port_vid_add for dp->cpu_dp, duplicating a bit
> of code from dsa_slave_vlan_add?

dsa_slave_* functions are the entry points for operations performed on the
net_device structures exposed to userspace. Using them elsewhere seems wrong.

dsa_port_* functions scope any dsa_port structure regardless its type though,
so I'd suggest duplicating a bit of code in tag_8021q.c to implement this
specific use case, until we figure out something nice to factorize.


Thank you,

	Vivien