[PATCH net-next 0/6] net: dsa: explicit programmation of VLAN on CPU ports

[PATCH net-next 0/6] net: dsa: explicit programmation of VLAN on CPU ports

[Vivien Didelot]

When a VLAN is programmed on a user port, every switch of the fabric also
program the CPU ports and the DSA links as part of the VLAN. To do that,
DSA makes use of bitmaps to prepare all members of a VLAN.

While this is expected for DSA links which are used as conduit between
interconnected switches, only the dedicated CPU port of the slave must be
programmed, not all CPU ports of the fabric. This may also cause problems in
other corners of DSA such as the tag_8021q.c driver, which needs to program
its ports manually, CPU port included.

We need the dsa_port_vlan_{add,del} functions and its dsa_port_vid_{add,del}
variants to simply trigger the VLAN programmation without any logic in them,
but they may currently skip the operation based on the bridge device state.

This patchset gets rid of the bitmap operations, and moves the bridge device
check as well as the explicit programmation of CPU ports where they belong,
in the slave code.

While at it, clear the VLAN flags before programming a CPU port, as it
doesn't make sense to forward the PVID flag for example for such ports.

Vivien Didelot (6):
  net: dsa: remove bitmap operations
  net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add
  net: dsa: add slave VLAN helpers
  net: dsa: check bridge VLAN in slave operations
  net: dsa: program VLAN on CPU port from slave
  net: dsa: clear VLAN flags for CPU port

 include/net/dsa.h |   3 --
 net/dsa/dsa2.c    |  14 -----
 net/dsa/port.c    |  14 ++---
 net/dsa/slave.c   |  79 +++++++++++++++++++++++----
 net/dsa/switch.c  | 135 +++++++++++++++++++++-------------------------
 5 files changed, 136 insertions(+), 109 deletions(-)

-- 
2.23.0

[PATCH net-next 1/6] net: dsa: remove bitmap operations

[Vivien Didelot]

The bitmap operations were introduced to simplify the switch drivers
in the future, since most of them could implement the common VLAN and
MDB operations (add, del, dump) with simple functions taking all target
ports at once, and thus limiting the number of hardware accesses.

Programming an MDB or VLAN this way in a single operation would clearly
simplify the drivers a lot but would require a new get-set interface
in DSA. The usage of such bitmap from the stack also raised concerned
in the past, leading to the dynamic allocation of a new ds->_bitmap
member in the dsa_switch structure. So let's get rid of them for now.

This commit nicely wraps the ds->ops->port_{mdb,vlan}_{prepare,add}
switch operations into new dsa_switch_{mdb,vlan}_{prepare,add}
variants not using any bitmap argument anymore.

New dsa_switch_{mdb,vlan}_match helpers have been introduced to make
clear which local port of a switch must be programmed with the target
object. While the targeted user port is an obvious candidate, the
DSA links must also be programmed, as well as the CPU port for VLANs.

While at it, also remove local variables that are only used once.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 include/net/dsa.h |   3 --
 net/dsa/dsa2.c    |  14 -----
 net/dsa/switch.c  | 132 +++++++++++++++++++++-------------------------
 3 files changed, 59 insertions(+), 90 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 147b757ef8ea..96acb14ec1a8 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -275,9 +275,6 @@ struct dsa_switch {
 	 */
 	bool			vlan_filtering;
 
-	unsigned long		*bitmap;
-	unsigned long		_bitmap;
-
 	/* Dynamically allocated ports, keep last */
 	size_t num_ports;
 	struct dsa_port ports[];
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 8c4eccb0cfe6..f8445fa73448 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -834,20 +834,6 @@ struct dsa_switch *dsa_switch_alloc(struct device *dev, size_t n)
 	if (!ds)
 		return NULL;
 
-	/* We avoid allocating memory outside dsa_switch
-	 * if it is not needed.
-	 */
-	if (n <= sizeof(ds->_bitmap) * 8) {
-		ds->bitmap = &ds->_bitmap;
-	} else {
-		ds->bitmap = devm_kcalloc(dev,
-					  BITS_TO_LONGS(n),
-					  sizeof(unsigned long),
-					  GFP_KERNEL);
-		if (unlikely(!ds->bitmap))
-			return NULL;
-	}
-
 	ds->dev = dev;
 	ds->num_ports = n;
 
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 09d9286b27cc..489eb7b430a4 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -128,57 +128,51 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
 	return ds->ops->port_fdb_del(ds, port, info->addr, info->vid);
 }
 
-static int
-dsa_switch_mdb_prepare_bitmap(struct dsa_switch *ds,
-			      const struct switchdev_obj_port_mdb *mdb,
-			      const unsigned long *bitmap)
+static bool dsa_switch_mdb_match(struct dsa_switch *ds, int port,
+				 struct dsa_notifier_mdb_info *info)
+{
+	if (ds->index == info->sw_index && port == info->port)
+		return true;
+
+	if (dsa_is_dsa_port(ds, port))
+		return true;
+
+	return false;
+}
+
+static int dsa_switch_mdb_prepare(struct dsa_switch *ds,
+				  struct dsa_notifier_mdb_info *info)
 {
 	int port, err;
 
 	if (!ds->ops->port_mdb_prepare || !ds->ops->port_mdb_add)
 		return -EOPNOTSUPP;
 
-	for_each_set_bit(port, bitmap, ds->num_ports) {
-		err = ds->ops->port_mdb_prepare(ds, port, mdb);
-		if (err)
-			return err;
+	for (port = 0; port < ds->num_ports; port++) {
+		if (dsa_switch_mdb_match(ds, port, info)) {
+			err = ds->ops->port_mdb_prepare(ds, port, info->mdb);
+			if (err)
+				return err;
+		}
 	}
 
 	return 0;
 }
 
-static void dsa_switch_mdb_add_bitmap(struct dsa_switch *ds,
-				      const struct switchdev_obj_port_mdb *mdb,
-				      const unsigned long *bitmap)
-{
-	int port;
-
-	if (!ds->ops->port_mdb_add)
-		return;
-
-	for_each_set_bit(port, bitmap, ds->num_ports)
-		ds->ops->port_mdb_add(ds, port, mdb);
-}
-
 static int dsa_switch_mdb_add(struct dsa_switch *ds,
 			      struct dsa_notifier_mdb_info *info)
 {
-	const struct switchdev_obj_port_mdb *mdb = info->mdb;
-	struct switchdev_trans *trans = info->trans;
 	int port;
 
-	/* Build a mask of Multicast group members */
-	bitmap_zero(ds->bitmap, ds->num_ports);
-	if (ds->index == info->sw_index)
-		set_bit(info->port, ds->bitmap);
-	for (port = 0; port < ds->num_ports; port++)
-		if (dsa_is_dsa_port(ds, port))
-			set_bit(port, ds->bitmap);
+	if (switchdev_trans_ph_prepare(info->trans))
+		return dsa_switch_mdb_prepare(ds, info);
 
-	if (switchdev_trans_ph_prepare(trans))
-		return dsa_switch_mdb_prepare_bitmap(ds, mdb, ds->bitmap);
+	if (!ds->ops->port_mdb_add)
+		return 0;
 
-	dsa_switch_mdb_add_bitmap(ds, mdb, ds->bitmap);
+	for (port = 0; port < ds->num_ports; port++)
+		if (dsa_switch_mdb_match(ds, port, info))
+			ds->ops->port_mdb_add(ds, port, info->mdb);
 
 	return 0;
 }
@@ -186,13 +180,11 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 static int dsa_switch_mdb_del(struct dsa_switch *ds,
 			      struct dsa_notifier_mdb_info *info)
 {
-	const struct switchdev_obj_port_mdb *mdb = info->mdb;
-
 	if (!ds->ops->port_mdb_del)
 		return -EOPNOTSUPP;
 
 	if (ds->index == info->sw_index)
-		return ds->ops->port_mdb_del(ds, info->port, mdb);
+		return ds->ops->port_mdb_del(ds, info->port, info->mdb);
 
 	return 0;
 }
@@ -234,59 +226,55 @@ static int dsa_port_vlan_check(struct dsa_switch *ds, int port,
 			     (void *)vlan);
 }
 
-static int
-dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
-			       const struct switchdev_obj_port_vlan *vlan,
-			       const unsigned long *bitmap)
+static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
+				  struct dsa_notifier_vlan_info *info)
+{
+	if (ds->index == info->sw_index && port == info->port)
+		return true;
+
+	if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+		return true;
+
+	return false;
+}
+
+static int dsa_switch_vlan_prepare(struct dsa_switch *ds,
+				   struct dsa_notifier_vlan_info *info)
 {
 	int port, err;
 
 	if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
 		return -EOPNOTSUPP;
 
-	for_each_set_bit(port, bitmap, ds->num_ports) {
-		err = dsa_port_vlan_check(ds, port, vlan);
-		if (err)
-			return err;
+	for (port = 0; port < ds->num_ports; port++) {
+		if (dsa_switch_vlan_match(ds, port, info)) {
+			err = dsa_port_vlan_check(ds, port, info->vlan);
+			if (err)
+				return err;
 
-		err = ds->ops->port_vlan_prepare(ds, port, vlan);
-		if (err)
-			return err;
+			err = ds->ops->port_vlan_prepare(ds, port, info->vlan);
+			if (err)
+				return err;
+		}
 	}
 
 	return 0;
 }
 
-static void
-dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
-			   const struct switchdev_obj_port_vlan *vlan,
-			   const unsigned long *bitmap)
-{
-	int port;
-
-	for_each_set_bit(port, bitmap, ds->num_ports)
-		ds->ops->port_vlan_add(ds, port, vlan);
-}
-
 static int dsa_switch_vlan_add(struct dsa_switch *ds,
 			       struct dsa_notifier_vlan_info *info)
 {
-	const struct switchdev_obj_port_vlan *vlan = info->vlan;
-	struct switchdev_trans *trans = info->trans;
 	int port;
 
-	/* Build a mask of VLAN members */
-	bitmap_zero(ds->bitmap, ds->num_ports);
-	if (ds->index == info->sw_index)
-		set_bit(info->port, ds->bitmap);
-	for (port = 0; port < ds->num_ports; port++)
-		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
-			set_bit(port, ds->bitmap);
+	if (switchdev_trans_ph_prepare(info->trans))
+		return dsa_switch_vlan_prepare(ds, info);
 
-	if (switchdev_trans_ph_prepare(trans))
-		return dsa_switch_vlan_prepare_bitmap(ds, vlan, ds->bitmap);
+	if (!ds->ops->port_vlan_add)
+		return 0;
 
-	dsa_switch_vlan_add_bitmap(ds, vlan, ds->bitmap);
+	for (port = 0; port < ds->num_ports; port++)
+		if (dsa_switch_vlan_match(ds, port, info))
+			ds->ops->port_vlan_add(ds, port, info->vlan);
 
 	return 0;
 }
@@ -294,13 +282,11 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 static int dsa_switch_vlan_del(struct dsa_switch *ds,
 			       struct dsa_notifier_vlan_info *info)
 {
-	const struct switchdev_obj_port_vlan *vlan = info->vlan;
-
 	if (!ds->ops->port_vlan_del)
 		return -EOPNOTSUPP;
 
 	if (ds->index == info->sw_index)
-		return ds->ops->port_vlan_del(ds, info->port, vlan);
+		return ds->ops->port_vlan_del(ds, info->port, info->vlan);
 
 	return 0;
 }
-- 
2.23.0

[PATCH net-next 2/6] net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add

[Vivien Didelot]

Currently dsa_port_vid_add returns 0 if the switch returns -EOPNOTSUPP.

This function is used in the tag_8021q.c code to offload the PVID of
ports, which would simply not work if .port_vlan_add is not supported
by the underlying switch.

Do not skip -EOPNOTSUPP in dsa_port_vid_add but only when necessary,
that is to say in dsa_slave_vlan_rx_add_vid.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/port.c  | 4 ++--
 net/dsa/slave.c | 7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index f75301456430..ef28df7ecbde 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -382,8 +382,8 @@ int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags)
 
 	trans.ph_prepare = true;
 	err = dsa_port_vlan_add(dp, &vlan, &trans);
-	if (err == -EOPNOTSUPP)
-		return 0;
+	if (err)
+		return err;
 
 	trans.ph_prepare = false;
 	return dsa_port_vlan_add(dp, &vlan, &trans);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 33f41178afcc..9d61d9dbf001 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1082,8 +1082,11 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 			return -EBUSY;
 	}
 
-	/* This API only allows programming tagged, non-PVID VIDs */
-	return dsa_port_vid_add(dp, vid, 0);
+	ret = dsa_port_vid_add(dp, vid, 0);
+	if (ret && ret != -EOPNOTSUPP)
+		return ret;
+
+	return 0;
 }
 
 static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
-- 
2.23.0

Re: [PATCH net-next 2/6] net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add

[Vladimir Oltean]

Hi Vivien,

On 8/22/19 11:13 PM, Vivien Didelot wrote:
> Currently dsa_port_vid_add returns 0 if the switch returns -EOPNOTSUPP.
> 
> This function is used in the tag_8021q.c code to offload the PVID of
> ports, which would simply not work if .port_vlan_add is not supported
> by the underlying switch.
> 
> Do not skip -EOPNOTSUPP in dsa_port_vid_add but only when necessary,
> that is to say in dsa_slave_vlan_rx_add_vid.
> 

Do you know why Florian suppressed -EOPNOTSUPP in 061f6a505ac3 ("net: 
dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")?
I forced a return value of -EOPNOTSUPP here and when I create a VLAN 
sub-interface nothing breaks, it just says:
RTNETLINK answers: Operation not supported
which IMO is expected.

> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---
>   net/dsa/port.c  | 4 ++--
>   net/dsa/slave.c | 7 +++++--
>   2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index f75301456430..ef28df7ecbde 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -382,8 +382,8 @@ int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags)
>   
>   	trans.ph_prepare = true;
>   	err = dsa_port_vlan_add(dp, &vlan, &trans);
> -	if (err == -EOPNOTSUPP)
> -		return 0;
> +	if (err)
> +		return err;
>   
>   	trans.ph_prepare = false;
>   	return dsa_port_vlan_add(dp, &vlan, &trans);
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 33f41178afcc..9d61d9dbf001 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1082,8 +1082,11 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>   			return -EBUSY;
>   	}
>   
> -	/* This API only allows programming tagged, non-PVID VIDs */
> -	return dsa_port_vid_add(dp, vid, 0);
> +	ret = dsa_port_vid_add(dp, vid, 0);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
> +
> +	return 0;
>   }
>   
>   static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
> 

Regards,
-Vladimir

Re: [PATCH net-next 2/6] net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add

[Vivien Didelot]

Hi Vladimir,

On Fri, 23 Aug 2019 01:06:58 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> Hi Vivien,
> 
> On 8/22/19 11:13 PM, Vivien Didelot wrote:
> > Currently dsa_port_vid_add returns 0 if the switch returns -EOPNOTSUPP.
> > 
> > This function is used in the tag_8021q.c code to offload the PVID of
> > ports, which would simply not work if .port_vlan_add is not supported
> > by the underlying switch.
> > 
> > Do not skip -EOPNOTSUPP in dsa_port_vid_add but only when necessary,
> > that is to say in dsa_slave_vlan_rx_add_vid.
> > 
> 
> Do you know why Florian suppressed -EOPNOTSUPP in 061f6a505ac3 ("net: 
> dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")?
> I forced a return value of -EOPNOTSUPP here and when I create a VLAN 
> sub-interface nothing breaks, it just says:
> RTNETLINK answers: Operation not supported
> which IMO is expected.

I do not know what you mean. This patch does not change the behavior of
dsa_slave_vlan_rx_add_vid, which returns 0 if -EOPNOTSUPP is caught.


Thanks,

	Vivien

Re: [PATCH net-next 2/6] net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add

[Vladimir Oltean]

On Fri, 23 Aug 2019 at 02:43, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Fri, 23 Aug 2019 01:06:58 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > Hi Vivien,
> >
> > On 8/22/19 11:13 PM, Vivien Didelot wrote:
> > > Currently dsa_port_vid_add returns 0 if the switch returns -EOPNOTSUPP.
> > >
> > > This function is used in the tag_8021q.c code to offload the PVID of
> > > ports, which would simply not work if .port_vlan_add is not supported
> > > by the underlying switch.
> > >
> > > Do not skip -EOPNOTSUPP in dsa_port_vid_add but only when necessary,
> > > that is to say in dsa_slave_vlan_rx_add_vid.
> > >
> >
> > Do you know why Florian suppressed -EOPNOTSUPP in 061f6a505ac3 ("net:
> > dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")?
> > I forced a return value of -EOPNOTSUPP here and when I create a VLAN
> > sub-interface nothing breaks, it just says:
> > RTNETLINK answers: Operation not supported
> > which IMO is expected.
>
> I do not know what you mean. This patch does not change the behavior of
> dsa_slave_vlan_rx_add_vid, which returns 0 if -EOPNOTSUPP is caught.
>

Yes, but what's wrong with just forwarding -EOPNOTSUPP?

>
> Thanks,
>
>         Vivien

Re: [PATCH net-next 2/6] net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add

[Florian Fainelli]

On 8/22/19 4:44 PM, Vladimir Oltean wrote:
> On Fri, 23 Aug 2019 at 02:43, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>>
>> Hi Vladimir,
>>
>> On Fri, 23 Aug 2019 01:06:58 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
>>> Hi Vivien,
>>>
>>> On 8/22/19 11:13 PM, Vivien Didelot wrote:
>>>> Currently dsa_port_vid_add returns 0 if the switch returns -EOPNOTSUPP.
>>>>
>>>> This function is used in the tag_8021q.c code to offload the PVID of
>>>> ports, which would simply not work if .port_vlan_add is not supported
>>>> by the underlying switch.
>>>>
>>>> Do not skip -EOPNOTSUPP in dsa_port_vid_add but only when necessary,
>>>> that is to say in dsa_slave_vlan_rx_add_vid.
>>>>
>>>
>>> Do you know why Florian suppressed -EOPNOTSUPP in 061f6a505ac3 ("net:
>>> dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")?
>>> I forced a return value of -EOPNOTSUPP here and when I create a VLAN
>>> sub-interface nothing breaks, it just says:
>>> RTNETLINK answers: Operation not supported
>>> which IMO is expected.
>>
>> I do not know what you mean. This patch does not change the behavior of
>> dsa_slave_vlan_rx_add_vid, which returns 0 if -EOPNOTSUPP is caught.
>>
> 
> Yes, but what's wrong with just forwarding -EOPNOTSUPP?

It makes us fail adding the VLAN to the slave network device, which
sounds silly, if we can't offload it in HW, that's fine, we can still do
a SW VLAN instead, see net/8021q/vlan_core.c::vlan_add_rx_filter_info().

Maybe a more correct solution is to set the NETIF_F_HW_VLAN_CTAG_FILTER
feature bit only if we have the ability to offload, now that I think
about it. Would you want me to cook a patch doing that?
-- 
Florian

Re: [PATCH net-next 2/6] net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add

[Vladimir Oltean]

Hi Florian,

On Fri, 23 Aug 2019 at 19:23, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 8/22/19 4:44 PM, Vladimir Oltean wrote:
> > On Fri, 23 Aug 2019 at 02:43, Vivien Didelot <vivien.didelot@gmail.com> wrote:
> >>
> >> Hi Vladimir,
> >>
> >> On Fri, 23 Aug 2019 01:06:58 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> >>> Hi Vivien,
> >>>
> >>> On 8/22/19 11:13 PM, Vivien Didelot wrote:
> >>>> Currently dsa_port_vid_add returns 0 if the switch returns -EOPNOTSUPP.
> >>>>
> >>>> This function is used in the tag_8021q.c code to offload the PVID of
> >>>> ports, which would simply not work if .port_vlan_add is not supported
> >>>> by the underlying switch.
> >>>>
> >>>> Do not skip -EOPNOTSUPP in dsa_port_vid_add but only when necessary,
> >>>> that is to say in dsa_slave_vlan_rx_add_vid.
> >>>>
> >>>
> >>> Do you know why Florian suppressed -EOPNOTSUPP in 061f6a505ac3 ("net:
> >>> dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")?
> >>> I forced a return value of -EOPNOTSUPP here and when I create a VLAN
> >>> sub-interface nothing breaks, it just says:
> >>> RTNETLINK answers: Operation not supported
> >>> which IMO is expected.
> >>
> >> I do not know what you mean. This patch does not change the behavior of
> >> dsa_slave_vlan_rx_add_vid, which returns 0 if -EOPNOTSUPP is caught.
> >>
> >
> > Yes, but what's wrong with just forwarding -EOPNOTSUPP?
>
> It makes us fail adding the VLAN to the slave network device, which
> sounds silly, if we can't offload it in HW, that's fine, we can still do
> a SW VLAN instead, see net/8021q/vlan_core.c::vlan_add_rx_filter_info().
>
> Maybe a more correct solution is to set the NETIF_F_HW_VLAN_CTAG_FILTER
> feature bit only if we have the ability to offload, now that I think
> about it. Would you want me to cook a patch doing that?

sja1105 doesn't support offloading NETIF_F_HW_VLAN_CTAG_FILTER even
though it does support programming VLANs.
Adding an offloaded VLAN sub-interface on a standalone switch port
(vlan_filtering=0, uses dsa_8021q) would make the driver insert a VLAN
entry whilst the TPID is ETH_P_DSA_8021Q.
Maybe just let the driver set the netdev features, similar to how it
does for the number of TX queues?

> --
> Florian

Regards,
-Vladimir

Re: [PATCH net-next 2/6] net: dsa: do not skip -EOPNOTSUPP in dsa_port_vid_add

[Florian Fainelli]

On 8/23/19 9:32 AM, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Fri, 23 Aug 2019 at 19:23, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 8/22/19 4:44 PM, Vladimir Oltean wrote:
>>> On Fri, 23 Aug 2019 at 02:43, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>>>>
>>>> Hi Vladimir,
>>>>
>>>> On Fri, 23 Aug 2019 01:06:58 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
>>>>> Hi Vivien,
>>>>>
>>>>> On 8/22/19 11:13 PM, Vivien Didelot wrote:
>>>>>> Currently dsa_port_vid_add returns 0 if the switch returns -EOPNOTSUPP.
>>>>>>
>>>>>> This function is used in the tag_8021q.c code to offload the PVID of
>>>>>> ports, which would simply not work if .port_vlan_add is not supported
>>>>>> by the underlying switch.
>>>>>>
>>>>>> Do not skip -EOPNOTSUPP in dsa_port_vid_add but only when necessary,
>>>>>> that is to say in dsa_slave_vlan_rx_add_vid.
>>>>>>
>>>>>
>>>>> Do you know why Florian suppressed -EOPNOTSUPP in 061f6a505ac3 ("net:
>>>>> dsa: Add ndo_vlan_rx_{add, kill}_vid implementation")?
>>>>> I forced a return value of -EOPNOTSUPP here and when I create a VLAN
>>>>> sub-interface nothing breaks, it just says:
>>>>> RTNETLINK answers: Operation not supported
>>>>> which IMO is expected.
>>>>
>>>> I do not know what you mean. This patch does not change the behavior of
>>>> dsa_slave_vlan_rx_add_vid, which returns 0 if -EOPNOTSUPP is caught.
>>>>
>>>
>>> Yes, but what's wrong with just forwarding -EOPNOTSUPP?
>>
>> It makes us fail adding the VLAN to the slave network device, which
>> sounds silly, if we can't offload it in HW, that's fine, we can still do
>> a SW VLAN instead, see net/8021q/vlan_core.c::vlan_add_rx_filter_info().
>>
>> Maybe a more correct solution is to set the NETIF_F_HW_VLAN_CTAG_FILTER
>> feature bit only if we have the ability to offload, now that I think
>> about it. Would you want me to cook a patch doing that?
> 
> sja1105 doesn't support offloading NETIF_F_HW_VLAN_CTAG_FILTER even
> though it does support programming VLANs.

The additional of the ndo_vlan_rx_{add,kill}_vid() is such that
standalone DSA ports continue to work while there is a bridge with
vlan_filtering=1 spanning other ports. In order for that ndo operation
to be called, we need to advertise the NETIF_F_HW_VLAN_CTAG_FILTER feature.

> Adding an offloaded VLAN sub-interface on a standalone switch port
> (vlan_filtering=0, uses dsa_8021q) would make the driver insert a VLAN
> entry whilst the TPID is ETH_P_DSA_8021Q.
> Maybe just let the driver set the netdev features, similar to how it
> does for the number of TX queues?

Why should we bend the framework because sja1105 and dsa_8021q are
special? Let me counter the argument: if the tagging is DSA_8021Q, why
not clear the feature then?
-- 
Florian

[PATCH net-next 3/6] net: dsa: add slave VLAN helpers

[Vivien Didelot]

Add dsa_slave_vlan_add and dsa_slave_vlan_del helpers to handle
SWITCHDEV_OBJ_ID_PORT_VLAN switchdev objects. Also copy the
switchdev_obj_port_vlan structure on add since we will modify it in
future patches.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/slave.c | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9d61d9dbf001..8f5126c41282 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -312,6 +312,26 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }
 
+static int dsa_slave_vlan_add(struct net_device *dev,
+			      const struct switchdev_obj *obj,
+			      struct switchdev_trans *trans)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan;
+	int err;
+
+	if (obj->orig_dev != dev)
+		return -EOPNOTSUPP;
+
+	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
+
+	err = dsa_port_vlan_add(dp, &vlan, trans);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int dsa_slave_port_obj_add(struct net_device *dev,
 				  const struct switchdev_obj *obj,
 				  struct switchdev_trans *trans,
@@ -339,10 +359,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 				       trans);
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		if (obj->orig_dev != dev)
-			return -EOPNOTSUPP;
-		err = dsa_port_vlan_add(dp, SWITCHDEV_OBJ_PORT_VLAN(obj),
-					trans);
+		err = dsa_slave_vlan_add(dev, obj, trans);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -352,6 +369,17 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	return err;
 }
 
+static int dsa_slave_vlan_del(struct net_device *dev,
+			      const struct switchdev_obj *obj)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+
+	if (obj->orig_dev != dev)
+		return -EOPNOTSUPP;
+
+	return dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
+}
+
 static int dsa_slave_port_obj_del(struct net_device *dev,
 				  const struct switchdev_obj *obj)
 {
@@ -371,9 +399,7 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 		err = dsa_port_mdb_del(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		if (obj->orig_dev != dev)
-			return -EOPNOTSUPP;
-		err = dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
+		err = dsa_slave_vlan_del(dev, obj);
 		break;
 	default:
 		err = -EOPNOTSUPP;
-- 
2.23.0

[PATCH net-next 4/6] net: dsa: check bridge VLAN in slave operations

[Vivien Didelot]

The bridge VLANs are not offloaded by dsa_port_vlan_* if the port is
not bridged or if its bridge is not VLAN aware.

This is a good thing but other corners of DSA, such as the tag_8021q
driver, may need to program VLANs regardless the bridge state.

And also because bridge_dev is specific to user ports anyway, move
these checks were it belongs, one layer up in the slave code.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
Suggested-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/port.c  | 10 ++--------
 net/dsa/slave.c | 12 ++++++++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index ef28df7ecbde..9b54e5a76297 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -348,10 +348,7 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
-	if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
-		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
-
-	return 0;
+	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
 }
 
 int dsa_port_vlan_del(struct dsa_port *dp,
@@ -363,10 +360,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
-	if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
-		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
-
-	return 0;
+	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
 
 int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8f5126c41282..82e48d247b81 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -323,6 +323,9 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	if (obj->orig_dev != dev)
 		return -EOPNOTSUPP;
 
+	if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
+		return 0;
+
 	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	err = dsa_port_vlan_add(dp, &vlan, trans);
@@ -377,6 +380,9 @@ static int dsa_slave_vlan_del(struct net_device *dev,
 	if (obj->orig_dev != dev)
 		return -EOPNOTSUPP;
 
+	if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
+		return 0;
+
 	return dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
 }
 
@@ -1099,6 +1105,9 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 	 * need to emulate the switchdev prepare + commit phase.
 	 */
 	if (dp->bridge_dev) {
+		if (!br_vlan_enabled(dp->bridge_dev))
+			return 0;
+
 		/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
 		 * device, respectively the VID is not found, returning
 		 * 0 means success, which is a failure for us here.
@@ -1126,6 +1135,9 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 	 * need to emulate the switchdev prepare + commit phase.
 	 */
 	if (dp->bridge_dev) {
+		if (!br_vlan_enabled(dp->bridge_dev))
+			return 0;
+
 		/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
 		 * device, respectively the VID is not found, returning
 		 * 0 means success, which is a failure for us here.
-- 
2.23.0

[PATCH net-next 5/6] net: dsa: program VLAN on CPU port from slave

[Vivien Didelot]

DSA currently programs a VLAN on the CPU port implicitly after the
related notifier is received by a switch.

While we still need to do this transparent programmation of the DSA
links in the fabric, programming the CPU port this way may cause
problems in some corners such as the tag_8021q driver.

Because the dedicated CPU port is specific to a slave, make their
programmation explicit a few layers up, in the slave code.

Note that technically, DSA links have a dedicated CPU port as well,
but since they are only used as conduit between interconnected switches
of a fabric, programming them transparently this way is fine.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/slave.c  | 14 ++++++++++++++
 net/dsa/switch.c |  5 ++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 82e48d247b81..8267c156a51a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -332,6 +332,10 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	if (err)
 		return err;
 
+	err = dsa_port_vlan_add(dp->cpu_dp, &vlan, trans);
+	if (err)
+		return err;
+
 	return 0;
 }
 
@@ -383,6 +387,9 @@ static int dsa_slave_vlan_del(struct net_device *dev,
 	if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
 		return 0;
 
+	/* Do not deprogram the CPU port as it may be shared with other user
+	 * ports which can be members of this VLAN as well.
+	 */
 	return dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
 }
 
@@ -1121,6 +1128,10 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 	if (ret && ret != -EOPNOTSUPP)
 		return ret;
 
+	ret = dsa_port_vid_add(dp->cpu_dp, vid, 0);
+	if (ret && ret != -EOPNOTSUPP)
+		return ret;
+
 	return 0;
 }
 
@@ -1151,6 +1162,9 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 	if (ret == -EOPNOTSUPP)
 		ret = 0;
 
+	/* Do not deprogram the CPU port as it may be shared with other user
+	 * ports which can be members of this VLAN as well.
+	 */
 	return ret;
 }
 
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 489eb7b430a4..6a9607518823 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -232,7 +232,7 @@ static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
 	if (ds->index == info->sw_index && port == info->port)
 		return true;
 
-	if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+	if (dsa_is_dsa_port(ds, port))
 		return true;
 
 	return false;
@@ -288,6 +288,9 @@ static int dsa_switch_vlan_del(struct dsa_switch *ds,
 	if (ds->index == info->sw_index)
 		return ds->ops->port_vlan_del(ds, info->port, info->vlan);
 
+	/* Do not deprogram the DSA links as they may be used as conduit
+	 * for other VLAN members in the fabric.
+	 */
 	return 0;
 }
 
-- 
2.23.0

Re: [PATCH net-next 5/6] net: dsa: program VLAN on CPU port from slave

[Vladimir Oltean]

On Thu, 22 Aug 2019 at 23:13, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> DSA currently programs a VLAN on the CPU port implicitly after the
> related notifier is received by a switch.
>
> While we still need to do this transparent programmation of the DSA
> links in the fabric, programming the CPU port this way may cause
> problems in some corners such as the tag_8021q driver.
>
> Because the dedicated CPU port is specific to a slave, make their
> programmation explicit a few layers up, in the slave code.
>
> Note that technically, DSA links have a dedicated CPU port as well,
> but since they are only used as conduit between interconnected switches
> of a fabric, programming them transparently this way is fine.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---
>  net/dsa/slave.c  | 14 ++++++++++++++
>  net/dsa/switch.c |  5 ++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 82e48d247b81..8267c156a51a 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -332,6 +332,10 @@ static int dsa_slave_vlan_add(struct net_device *dev,
>         if (err)
>                 return err;
>
> +       err = dsa_port_vlan_add(dp->cpu_dp, &vlan, trans);
> +       if (err)
> +               return err;
> +
>         return 0;
>  }
>
> @@ -383,6 +387,9 @@ static int dsa_slave_vlan_del(struct net_device *dev,
>         if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
>                 return 0;
>
> +       /* Do not deprogram the CPU port as it may be shared with other user
> +        * ports which can be members of this VLAN as well.
> +        */

+1 for the comments, the deletion of dp->cpu_dp is less likely to get
patched into the code in the future now.

>         return dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
>  }
>
> @@ -1121,6 +1128,10 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>         if (ret && ret != -EOPNOTSUPP)
>                 return ret;
>
> +       ret = dsa_port_vid_add(dp->cpu_dp, vid, 0);
> +       if (ret && ret != -EOPNOTSUPP)
> +               return ret;
> +

I think it's worth understanding what the EOPNOTSUPP -> 0 is avoiding.

>         return 0;
>  }
>
> @@ -1151,6 +1162,9 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
>         if (ret == -EOPNOTSUPP)
>                 ret = 0;
>
> +       /* Do not deprogram the CPU port as it may be shared with other user
> +        * ports which can be members of this VLAN as well.
> +        */
>         return ret;
>  }
>
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 489eb7b430a4..6a9607518823 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -232,7 +232,7 @@ static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
>         if (ds->index == info->sw_index && port == info->port)
>                 return true;
>
> -       if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +       if (dsa_is_dsa_port(ds, port))

Much better, thank you.



>                 return true;
>
>         return false;
> @@ -288,6 +288,9 @@ static int dsa_switch_vlan_del(struct dsa_switch *ds,
>         if (ds->index == info->sw_index)
>                 return ds->ops->port_vlan_del(ds, info->port, info->vlan);
>
> +       /* Do not deprogram the DSA links as they may be used as conduit
> +        * for other VLAN members in the fabric.
> +        */
>         return 0;
>  }
>
> --
> 2.23.0
>

[PATCH net-next 6/6] net: dsa: clear VLAN flags for CPU port

[Vivien Didelot]

When the bridge offloads a VLAN on a slave port, we also need to
program its dedicated CPU port as a member of the VLAN.

Drivers may handle the CPU port's membership as they want. For example,
Marvell as a special "Unmodified" mode to pass frames as is through
such ports.

Even though DSA expects the drivers to handle the CPU port membership,
they are unlikely to program such VLANs untagged, and certainly not as
PVID. This patch clears the VLAN flags before programming the CPU port.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
Suggested-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/slave.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8267c156a51a..48df48f76c67 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -332,6 +332,12 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	if (err)
 		return err;
 
+	/* We need the dedicated CPU port to be a member of the VLAN as well.
+	 * Even though drivers often handle CPU membership in special ways,
+	 * CPU ports are likely to be tagged, so clear the VLAN flags.
+	 */
+	vlan.flags = 0;
+
 	err = dsa_port_vlan_add(dp->cpu_dp, &vlan, trans);
 	if (err)
 		return err;
-- 
2.23.0

Re: [PATCH net-next 6/6] net: dsa: clear VLAN flags for CPU port

[Vladimir Oltean]

On 8/22/19 11:13 PM, Vivien Didelot wrote:
> When the bridge offloads a VLAN on a slave port, we also need to
> program its dedicated CPU port as a member of the VLAN.
> 
> Drivers may handle the CPU port's membership as they want. For example,
> Marvell as a special "Unmodified" mode to pass frames as is through
> such ports.
> 
> Even though DSA expects the drivers to handle the CPU port membership,
> they are unlikely to program such VLANs untagged, and certainly not as
> PVID. This patch clears the VLAN flags before programming the CPU port.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>   net/dsa/slave.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 8267c156a51a..48df48f76c67 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -332,6 +332,12 @@ static int dsa_slave_vlan_add(struct net_device *dev,
>   	if (err)
>   		return err;
>   
> +	/* We need the dedicated CPU port to be a member of the VLAN as well.
> +	 * Even though drivers often handle CPU membership in special ways,
> +	 * CPU ports are likely to be tagged, so clear the VLAN flags.
> +	 */
> +	vlan.flags = 0;
> +

How does this work exactly?
If I run 'sudo bridge vlan add vid 1 dev swp4 pvid untagged', then the 
CPU port starts sending VLAN-tagged traffic. I see this in tcpdump on 
the DSA master port, but if I tcpdump on swp4, the VLAN tag is removed. 
Who is doing that?

>   	err = dsa_port_vlan_add(dp->cpu_dp, &vlan, trans);
>   	if (err)
>   		return err;
> 

Re: [PATCH net-next 6/6] net: dsa: clear VLAN flags for CPU port

[Florian Fainelli]

On 8/22/19 4:51 PM, Vladimir Oltean wrote:
> On 8/22/19 11:13 PM, Vivien Didelot wrote:
>> When the bridge offloads a VLAN on a slave port, we also need to
>> program its dedicated CPU port as a member of the VLAN.
>>
>> Drivers may handle the CPU port's membership as they want. For example,
>> Marvell as a special "Unmodified" mode to pass frames as is through
>> such ports.
>>
>> Even though DSA expects the drivers to handle the CPU port membership,
>> they are unlikely to program such VLANs untagged, and certainly not as
>> PVID. This patch clears the VLAN flags before programming the CPU port.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
>> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
>> ---
>>   net/dsa/slave.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 8267c156a51a..48df48f76c67 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -332,6 +332,12 @@ static int dsa_slave_vlan_add(struct net_device
>> *dev,
>>       if (err)
>>           return err;
>>   +    /* We need the dedicated CPU port to be a member of the VLAN as
>> well.
>> +     * Even though drivers often handle CPU membership in special ways,
>> +     * CPU ports are likely to be tagged, so clear the VLAN flags.
>> +     */
>> +    vlan.flags = 0;
>> +
> 
> How does this work exactly?
> If I run 'sudo bridge vlan add vid 1 dev swp4 pvid untagged', then the
> CPU port starts sending VLAN-tagged traffic. I see this in tcpdump on
> the DSA master port, but if I tcpdump on swp4, the VLAN tag is removed.
> Who is doing that?

If vlan.flags = 0, then it does not have either BRIDGE_VLAN_INFO_PVID or
BRIDGE_VLAN_INFO_UNTAGGED which means the VLAN should be programmed
tagged on the CPU.

Since swp4 is part of the same VLAN, but has it configured PVID
untagged, the tag is removed, that sounds about what I would expect to
see...
-- 
Florian

Re: [PATCH net-next 6/6] net: dsa: clear VLAN flags for CPU port

[Vladimir Oltean]

Hi Florian,

On Fri, 23 Aug 2019 at 20:00, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 8/22/19 4:51 PM, Vladimir Oltean wrote:
> > On 8/22/19 11:13 PM, Vivien Didelot wrote:
> >> When the bridge offloads a VLAN on a slave port, we also need to
> >> program its dedicated CPU port as a member of the VLAN.
> >>
> >> Drivers may handle the CPU port's membership as they want. For example,
> >> Marvell as a special "Unmodified" mode to pass frames as is through
> >> such ports.
> >>
> >> Even though DSA expects the drivers to handle the CPU port membership,
> >> they are unlikely to program such VLANs untagged, and certainly not as
> >> PVID. This patch clears the VLAN flags before programming the CPU port.
> >>
> >> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> >> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> >> ---
> >>   net/dsa/slave.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> >> index 8267c156a51a..48df48f76c67 100644
> >> --- a/net/dsa/slave.c
> >> +++ b/net/dsa/slave.c
> >> @@ -332,6 +332,12 @@ static int dsa_slave_vlan_add(struct net_device
> >> *dev,
> >>       if (err)
> >>           return err;
> >>   +    /* We need the dedicated CPU port to be a member of the VLAN as
> >> well.
> >> +     * Even though drivers often handle CPU membership in special ways,
> >> +     * CPU ports are likely to be tagged, so clear the VLAN flags.
> >> +     */
> >> +    vlan.flags = 0;
> >> +
> >
> > How does this work exactly?
> > If I run 'sudo bridge vlan add vid 1 dev swp4 pvid untagged', then the
> > CPU port starts sending VLAN-tagged traffic. I see this in tcpdump on
> > the DSA master port, but if I tcpdump on swp4, the VLAN tag is removed.
> > Who is doing that?
>
> If vlan.flags = 0, then it does not have either BRIDGE_VLAN_INFO_PVID or
> BRIDGE_VLAN_INFO_UNTAGGED which means the VLAN should be programmed
> tagged on the CPU.
>
> Since swp4 is part of the same VLAN, but has it configured PVID
> untagged, the tag is removed, that sounds about what I would expect to
> see...
> --
> Florian

The VLAN is "egress untagged", and "ingress tagged" (at least so it
becomes with this patch). Of course in tcpdump I was looking for
ingress traffic.
This patch is relying now on __netif_receive_skb_core[1] to remove the
VLAN header from frames as soon as they exit the DSA master and before
they enter the DSA packet_type handler. My point is that even untagged
traffic gets pvid-tagged on ingress, and the net core has to remove
the tag when it previously didn't have to. I'm not sure of other
implications.
Vivien, can't you just unset the PVID flag? Keeping the same
tagged/untagged setting on ingress as on egress does make more sense.

Regards,
-Vladimir

[1]: https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L4898

Re: [PATCH net-next 6/6] net: dsa: clear VLAN flags for CPU port

[Vivien Didelot]

On Sat, 24 Aug 2019 22:53:08 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> Vivien, can't you just unset the PVID flag? Keeping the same
> tagged/untagged setting on ingress as on egress does make more sense.

Why not. I've sent a v2 with this single change.


Thanks,

	Vivien