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

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

[Vladimir Oltean]

This patchset addresses a few limitations in DSA and the bridge core
that made it impossible for this sequence of commands to work:

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

Only this sequence was previously working:

  ip link add name br0 type bridge vlan_filtering 1
  ip link set dev swp2 master br0

On SJA1105, the situation is further complicated by the fact that
toggling vlan_filtering is causing a switch reset. However, the hardware
state restoration logic is already there in the driver. It is a matter
of the layers above which need a few fixups.

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

Patch 1/6 is not functionally related but also related to dsa_8021q
handling of VLANs and this is a good opportunity to bring up the subject
for discussion.

Vladimir Oltean (6):
  net: dsa: tag_8021q: Future-proof the reserved fields in the custom
    VID
  net: bridge: Populate the pvid flag in br_vlan_get_info
  net: dsa: Delete the VID from the upstream port as well
  net: dsa: Don't program the VLAN as pvid on the upstream port
  net: dsa: Allow proper internal use of VLANs
  net: dsa: tag_8021q: Restore bridge pvid when enabling vlan_filtering

 net/bridge/br_vlan.c |  2 ++
 net/dsa/port.c       | 10 ++--------
 net/dsa/slave.c      |  8 ++++++++
 net/dsa/switch.c     | 25 +++++++++++++++++++++----
 net/dsa/tag_8021q.c  | 34 +++++++++++++++++++++++++++++++++-
 5 files changed, 66 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH net-next 1/6] net: dsa: tag_8021q: Future-proof the reserved fields in the custom VID

[Vladimir Oltean]

After witnessing the discussion in https://lkml.org/lkml/2019/8/14/151
w.r.t. ioctl extensibility, it became clear that such an issue might
prevent that the 3 RSV bits inside the DSA 802.1Q tag might also suffer
the same fate and be useless for further extension.

So clearly specify that the reserved bits should currently be
transmitted as zero and ignored on receive. The DSA tagger already does
this (and has always did), and is the only known user so far (no
Wireshark dissection plugin, etc). So there should be no incompatibility
to speak of.

Fixes: 0471dd429cea ("net: dsa: tag_8021q: Create a stable binary format")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/tag_8021q.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 6ebbd799c4eb..67a1bc635a7b 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -28,6 +28,7 @@
  *
  * RSV - VID[9]:
  *	To be used for further expansion of SWITCH_ID or for other purposes.
+ *	Must be transmitted as zero and ignored on receive.
  *
  * SWITCH_ID - VID[8:6]:
  *	Index of switch within DSA tree. Must be between 0 and
@@ -35,6 +36,7 @@
  *
  * RSV - VID[5:4]:
  *	To be used for further expansion of PORT or for other purposes.
+ *	Must be transmitted as zero and ignored on receive.
  *
  * PORT - VID[3:0]:
  *	Index of switch port. Must be between 0 and DSA_MAX_PORTS - 1.
-- 
2.17.1

Re: [PATCH net-next 1/6] net: dsa: tag_8021q: Future-proof the reserved fields in the custom VID

[Florian Fainelli]

On 8/19/2019 4:59 PM, Vladimir Oltean wrote:
> After witnessing the discussion in https://lkml.org/lkml/2019/8/14/151
> w.r.t. ioctl extensibility, it became clear that such an issue might
> prevent that the 3 RSV bits inside the DSA 802.1Q tag might also suffer
> the same fate and be useless for further extension.
> 
> So clearly specify that the reserved bits should currently be
> transmitted as zero and ignored on receive. The DSA tagger already does
> this (and has always did), and is the only known user so far (no
> Wireshark dissection plugin, etc). So there should be no incompatibility
> to speak of.
> 
> Fixes: 0471dd429cea ("net: dsa: tag_8021q: Create a stable binary format")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

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

[PATCH net-next 2/6] 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>
---
 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

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

[Nikolay Aleksandrov]

On 8/20/19 2:59 AM, Vladimir Oltean wrote:
> 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>
> ---
>  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);
> 

Looks good, thanks!
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

[PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vladimir Oltean]

Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
is littering a lot. After deleting a VLAN added on a DSA port, it still
remains installed in the hardware filter of the upstream port. Fix this.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/switch.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 09d9286b27cc..84ab2336131e 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -295,11 +295,20 @@ 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;
+	int port;
 
 	if (!ds->ops->port_vlan_del)
 		return -EOPNOTSUPP;
 
+	/* 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);
+
+	for_each_set_bit(port, ds->bitmap, ds->num_ports)
 		return ds->ops->port_vlan_del(ds, info->port, vlan);
 
 	return 0;
-- 
2.17.1

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vivien Didelot]

Vladimir,

On Tue, 20 Aug 2019 02:59:59 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> is littering a lot. After deleting a VLAN added on a DSA port, it still
> remains installed in the hardware filter of the upstream port. Fix this.

Littering a lot, really?

FYI we are not removing the target VLAN from the hardware yet because it would
be too expensive to cache data in DSA core in order to know if the VID is not
used by any other slave port of the fabric anymore, and thus safe to remove.

Keeping the VID programmed for DSA and CPU ports is simpler for the moment,
as an hardware VLAN with only these ports as members is unlikely to harm.

> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/switch.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 09d9286b27cc..84ab2336131e 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -295,11 +295,20 @@ 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;
> +	int port;
>  
>  	if (!ds->ops->port_vlan_del)
>  		return -EOPNOTSUPP;
>  
> +	/* 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);
> +
> +	for_each_set_bit(port, ds->bitmap, ds->num_ports)
>  		return ds->ops->port_vlan_del(ds, info->port, vlan);

You return right away from the loop? You use info->port instead of port?
	
>  
>  	return 0;

Even if you patch wasn't badly broken, "bridge vlan del" targeting a single
switch port would also remove the VLAN from the CPU port and thus breaking
offloaded 802.1q. It would also remove it from the DSA ports interconnecting
multiple switches, thus breaking the 802.1q conduit for the whole fabric.

So you're not fixing anything here, but you're breaking single-chip and
cross-chip hardware VLAN. Seriously wtf is this patch?

NAK!


	Vivien

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vladimir Oltean]

Hi Vivien!

On Tue, 20 Aug 2019 at 08:51, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> Vladimir,
>
> On Tue, 20 Aug 2019 02:59:59 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> > is littering a lot. After deleting a VLAN added on a DSA port, it still
> > remains installed in the hardware filter of the upstream port. Fix this.
>
> Littering a lot, really?
>
> FYI we are not removing the target VLAN from the hardware yet because it would
> be too expensive to cache data in DSA core in order to know if the VID is not
> used by any other slave port of the fabric anymore, and thus safe to remove.
>
> Keeping the VID programmed for DSA and CPU ports is simpler for the moment,
> as an hardware VLAN with only these ports as members is unlikely to harm.
>
> >
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> >  net/dsa/switch.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> > index 09d9286b27cc..84ab2336131e 100644
> > --- a/net/dsa/switch.c
> > +++ b/net/dsa/switch.c
> > @@ -295,11 +295,20 @@ 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;
> > +     int port;
> >
> >       if (!ds->ops->port_vlan_del)
> >               return -EOPNOTSUPP;
> >
> > +     /* 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);
> > +
> > +     for_each_set_bit(port, ds->bitmap, ds->num_ports)
> >               return ds->ops->port_vlan_del(ds, info->port, vlan);
>
> You return right away from the loop? You use info->port instead of port?
>
> >
> >       return 0;
>
> Even if you patch wasn't badly broken, "bridge vlan del" targeting a single
> switch port would also remove the VLAN from the CPU port and thus breaking
> offloaded 802.1q. It would also remove it from the DSA ports interconnecting
> multiple switches, thus breaking the 802.1q conduit for the whole fabric.
>
> So you're not fixing anything here, but you're breaking single-chip and
> cross-chip hardware VLAN. Seriously wtf is this patch?
>
> NAK!
>
>
>         Vivien

I can agree that this isn't one of my brightest moments. But at least
we get to see Cunningham's law in action :)
When dsa_8021q is cleaning up the switch's VLAN table for the bridge
to use it, it is good to really clean it up, i.e. not leave any VLAN
installed on the upstream ports.
But I think this is just an academical concern at this point. In
vlan_filtering mode, the CPU port will accept VLAN frames with the
dsa_8021q ID's, but they will eventually get dropped due to no
destination. The real breaker is the pvid change. If something like
patch 4/6 gets accepted I will drop this one.

Regards,
-Vladimir

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vivien Didelot]

Hi Vladimir,

On Tue, 20 Aug 2019 12:54:44 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> I can agree that this isn't one of my brightest moments. But at least
> we get to see Cunningham's law in action :)
> When dsa_8021q is cleaning up the switch's VLAN table for the bridge
> to use it, it is good to really clean it up, i.e. not leave any VLAN
> installed on the upstream ports.
> But I think this is just an academical concern at this point. In
> vlan_filtering mode, the CPU port will accept VLAN frames with the
> dsa_8021q ID's, but they will eventually get dropped due to no
> destination. The real breaker is the pvid change. If something like
> patch 4/6 gets accepted I will drop this one.

I wish Ward had mentioned to submit such academical concern as RFC :)

Please submit smaller series, targeting a single functional problem each,
with clear and detailed messages.


Thanks,

	Vivien

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Florian Fainelli]

On 8/20/19 10:52 AM, Vivien Didelot wrote:
> Hi Vladimir,
> 
> On Tue, 20 Aug 2019 12:54:44 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
>> I can agree that this isn't one of my brightest moments. But at least
>> we get to see Cunningham's law in action :)
>> When dsa_8021q is cleaning up the switch's VLAN table for the bridge
>> to use it, it is good to really clean it up, i.e. not leave any VLAN
>> installed on the upstream ports.
>> But I think this is just an academical concern at this point. In
>> vlan_filtering mode, the CPU port will accept VLAN frames with the
>> dsa_8021q ID's, but they will eventually get dropped due to no
>> destination. The real breaker is the pvid change. If something like
>> patch 4/6 gets accepted I will drop this one.
> 
> I wish Ward had mentioned to submit such academical concern as RFC :)
> 
> Please submit smaller series, targeting a single functional problem each,
> with clear and detailed messages.

Also, I don't think this change set is useful per-se, if we take care of
removing VLANs on user facing ports, and VLAN filtering is turned on,
then a frame ingressing an user port with a VLAN that is not part of the
VLAN table/entries should simply be discarded on ingress, or on egress
to the CPU port (depending on where the switch performs VID checking),
so the CPU port cannot possibly receive such a frame, and so removing it
from the CPU port is correct from a reference counting perspective, but
useless in practice. Thoughts?
-- 
Florian

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vladimir Oltean]

Hi Florian,

On Tue, 20 Aug 2019 at 22:40, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 8/20/19 10:52 AM, Vivien Didelot wrote:
> > Hi Vladimir,
> >
> > On Tue, 20 Aug 2019 12:54:44 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> >> I can agree that this isn't one of my brightest moments. But at least
> >> we get to see Cunningham's law in action :)
> >> When dsa_8021q is cleaning up the switch's VLAN table for the bridge
> >> to use it, it is good to really clean it up, i.e. not leave any VLAN
> >> installed on the upstream ports.
> >> But I think this is just an academical concern at this point. In
> >> vlan_filtering mode, the CPU port will accept VLAN frames with the
> >> dsa_8021q ID's, but they will eventually get dropped due to no
> >> destination. The real breaker is the pvid change. If something like
> >> patch 4/6 gets accepted I will drop this one.
> >
> > I wish Ward had mentioned to submit such academical concern as RFC :)
> >
> > Please submit smaller series, targeting a single functional problem each,
> > with clear and detailed messages.
>
> Also, I don't think this change set is useful per-se, if we take care of
> removing VLANs on user facing ports, and VLAN filtering is turned on,
> then a frame ingressing an user port with a VLAN that is not part of the
> VLAN table/entries should simply be discarded on ingress, or on egress
> to the CPU port (depending on where the switch performs VID checking),
> so the CPU port cannot possibly receive such a frame, and so removing it
> from the CPU port is correct from a reference counting perspective, but
> useless in practice. Thoughts?

I don't need this patch. I'm not sure what my thought process was at
the time I added it to the patchset.
I'm still interested in getting rid of the vlan bitmap through other
means (picking up your old changeset). Could you take a look at my
questions in that thread? I'm not sure I understand what the user
interaction is supposed to look like for configuring CPU/DSA ports.

> --
> Florian

Thanks,
-Vladimir

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vivien Didelot]

On Tue, 20 Aug 2019 23:40:34 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> I don't need this patch. I'm not sure what my thought process was at
> the time I added it to the patchset.
> I'm still interested in getting rid of the vlan bitmap through other
> means (picking up your old changeset). Could you take a look at my
> questions in that thread? I'm not sure I understand what the user
> interaction is supposed to look like for configuring CPU/DSA ports.

What do you mean by getting rid of the vlan bitmap? What do you need exactly?

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vladimir Oltean]

On Tue, 20 Aug 2019 at 23:58, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> On Tue, 20 Aug 2019 23:40:34 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > I don't need this patch. I'm not sure what my thought process was at
> > the time I added it to the patchset.
> > I'm still interested in getting rid of the vlan bitmap through other
> > means (picking up your old changeset). Could you take a look at my
> > questions in that thread? I'm not sure I understand what the user
> > interaction is supposed to look like for configuring CPU/DSA ports.
>
> What do you mean by getting rid of the vlan bitmap? What do you need exactly?

It would be nice to configure the VLAN attributes of the CPU port in
another way than the implicit way it is done currently. I don't have a
specific use case right now.

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vivien Didelot]

On Wed, 21 Aug 2019 00:02:22 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, 20 Aug 2019 at 23:58, Vivien Didelot <vivien.didelot@gmail.com> wrote:
> >
> > On Tue, 20 Aug 2019 23:40:34 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > > I don't need this patch. I'm not sure what my thought process was at
> > > the time I added it to the patchset.
> > > I'm still interested in getting rid of the vlan bitmap through other
> > > means (picking up your old changeset). Could you take a look at my
> > > questions in that thread? I'm not sure I understand what the user
> > > interaction is supposed to look like for configuring CPU/DSA ports.
> >
> > What do you mean by getting rid of the vlan bitmap? What do you need exactly?
> 
> It would be nice to configure the VLAN attributes of the CPU port in
> another way than the implicit way it is done currently. I don't have a
> specific use case right now.

So you mean you need a lower level API to configure VLANs on a per-port basis,
without any logic, like including CPU and DSA links, etc.

The bitmap operations were introduced to simplify the switch drivers in the
future, since most of them could implement the VLAN operations (add, del)
in simple functions taking all local members at once.

But the Linux interface being exclusively based on a per port (slave) logic,
it is hard to implement right now.

The thing is that CPU ports, as well as DSA links in a multi-chip setup,
need to be programmed transparently when a given user port is configured,
hence the notification sent by a port to all switches of the fabric.

So I'm not against removing the bitmap logic, actually I'm looking into it
as well as moving more bridge checking logic into the slave code itself,
because I'm not a fan of your "Allow proper internal use of VLANs" patch.

But you'll need to provide more than "it would be nice" to push in that
direction, instead of making changes everywhere to make your switch work.


Thanks,

	Vivien

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vladimir Oltean]

On Wed, 21 Aug 2019 at 00:36, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> On Wed, 21 Aug 2019 00:02:22 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Tue, 20 Aug 2019 at 23:58, Vivien Didelot <vivien.didelot@gmail.com> wrote:
> > >
> > > On Tue, 20 Aug 2019 23:40:34 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > I don't need this patch. I'm not sure what my thought process was at
> > > > the time I added it to the patchset.
> > > > I'm still interested in getting rid of the vlan bitmap through other
> > > > means (picking up your old changeset). Could you take a look at my
> > > > questions in that thread? I'm not sure I understand what the user
> > > > interaction is supposed to look like for configuring CPU/DSA ports.
> > >
> > > What do you mean by getting rid of the vlan bitmap? What do you need exactly?
> >
> > It would be nice to configure the VLAN attributes of the CPU port in
> > another way than the implicit way it is done currently. I don't have a
> > specific use case right now.
>
> So you mean you need a lower level API to configure VLANs on a per-port basis,
> without any logic, like including CPU and DSA links, etc.
>
> The bitmap operations were introduced to simplify the switch drivers in the
> future, since most of them could implement the VLAN operations (add, del)
> in simple functions taking all local members at once.
>
> But the Linux interface being exclusively based on a per port (slave) logic,
> it is hard to implement right now.
>
> The thing is that CPU ports, as well as DSA links in a multi-chip setup,
> need to be programmed transparently when a given user port is configured,
> hence the notification sent by a port to all switches of the fabric.
>
> So I'm not against removing the bitmap logic, actually I'm looking into it
> as well as moving more bridge checking logic into the slave code itself,
> because I'm not a fan of your "Allow proper internal use of VLANs" patch.
>
> But you'll need to provide more than "it would be nice" to push in that
> direction, instead of making changes everywhere to make your switch work.
>
>
> Thanks,
>
>         Vivien

I mean I made an argument already for the hack in 4/6 ("Don't program
the VLAN as pvid on the upstream port"). If the hack gets accepted
like that, I have no further need of any change in the implicit VLAN
configuration. But it's still a hack, so in that sense it would be
nicer to not need it and have a better amount of control.

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vivien Didelot]

On Wed, 21 Aug 2019 01:09:39 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> I mean I made an argument already for the hack in 4/6 ("Don't program
> the VLAN as pvid on the upstream port"). If the hack gets accepted
> like that, I have no further need of any change in the implicit VLAN
> configuration. But it's still a hack, so in that sense it would be
> nicer to not need it and have a better amount of control.

How come you simply cannot ignore the PVID flag for the CPU port in the
driver directly, as mv88e6xxx does in preference of the Marvell specific
"unmodified" mode? What PVID are you programming on the CPU port already?


Thanks,

	Vivien

Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vladimir Oltean]

On Wed, 21 Aug 2019 at 06:30, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> On Wed, 21 Aug 2019 01:09:39 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > I mean I made an argument already for the hack in 4/6 ("Don't program
> > the VLAN as pvid on the upstream port"). If the hack gets accepted
> > like that, I have no further need of any change in the implicit VLAN
> > configuration. But it's still a hack, so in that sense it would be
> > nicer to not need it and have a better amount of control.
>
> How come you simply cannot ignore the PVID flag for the CPU port in the
> driver directly, as mv88e6xxx does in preference of the Marvell specific
> "unmodified" mode? What PVID are you programming on the CPU port already?
>
>
> Thanks,
>
>         Vivien

sja1105 has no such thing as an "unmodified" egress policy.
And ignoring the flags in the switch driver for the CPU port is just
as hacky as fixing it up in the DSA core.
I fail to see any reason to change the pvid for the CPU/DSA ports,
maybe you can clarify.

Actually I gave a second thought to the patchset and in a weird,
convoluted way, I can get away with just:
- 2/6: net: bridge: Populate the pvid flag in br_vlan_get_info
- 5/6 net: dsa: Allow proper internal use of VLANs
- 6/6 net: dsa: tag_8021q: Restore bridge pvid when enabling vlan_filtering
A side effect of running dsa_port_restore_pvid on a user port will be
that it is going to also restore the pvid on the CPU port, via the
bitmap operations. I had not thought of this initially when I first
jumped to remove the BRIDGE_VLAN_INFO_PVID flag in 4/6. And the fact
that it would work would just be "programming by coincidence".

I guess my aversion against the VLAN bitmap operations (and, well,
"match" in your new change) stems from the fact that the DSA core sits
in the way of doing custom configuration of the CPU port VLAN
settings. Yes, you can work around it (dsa_8021q first configures the
user ports as pvid and egress untagged, then configures the CPU port
as egress tagged, so that the pvid setting from user ports doesn't
"stick" to the CPU via the bitmap), but it's like pouring water that's
half hot and half cold from a water cooler, when all you want is water
that's at room temperature.

-Vladimir

[PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on the upstream port

[Vladimir Oltean]

Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
programs the VLAN from the bridge into the specified port as well as the
upstream port, with the same set of flags.

Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
user port 2, etc. The upstream port would end up having a pvid equal to
the last user port whose pvid was programmed from the bridge. Less than
useful.

So just don't change the pvid of the upstream port and let it be
whatever the driver set it internally to be.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/switch.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 84ab2336131e..02ccc53f1926 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -239,17 +239,21 @@ dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
 			       const struct switchdev_obj_port_vlan *vlan,
 			       const unsigned long *bitmap)
 {
+	struct switchdev_obj_port_vlan v = *vlan;
 	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 (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+			v.flags &= ~BRIDGE_VLAN_INFO_PVID;
+
+		err = dsa_port_vlan_check(ds, port, &v);
 		if (err)
 			return err;
 
-		err = ds->ops->port_vlan_prepare(ds, port, vlan);
+		err = ds->ops->port_vlan_prepare(ds, port, &v);
 		if (err)
 			return err;
 	}
@@ -262,10 +266,14 @@ dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
 			   const struct switchdev_obj_port_vlan *vlan,
 			   const unsigned long *bitmap)
 {
+	struct switchdev_obj_port_vlan v = *vlan;
 	int port;
 
-	for_each_set_bit(port, bitmap, ds->num_ports)
-		ds->ops->port_vlan_add(ds, port, vlan);
+	for_each_set_bit(port, bitmap, ds->num_ports) {
+		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+			v.flags &= ~BRIDGE_VLAN_INFO_PVID;
+		ds->ops->port_vlan_add(ds, port, &v);
+	}
 }
 
 static int dsa_switch_vlan_add(struct dsa_switch *ds,
-- 
2.17.1

Re: [PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on the upstream port

[Florian Fainelli]

On 8/19/2019 5:00 PM, Vladimir Oltean wrote:
> Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> programs the VLAN from the bridge into the specified port as well as the
> upstream port, with the same set of flags.
> 
> Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
> user port 2, etc. The upstream port would end up having a pvid equal to
> the last user port whose pvid was programmed from the bridge. Less than
> useful.
> 
> So just don't change the pvid of the upstream port and let it be
> whatever the driver set it internally to be.

This patch should allow removing the !dsa_is_cpu_port() checks from
b53_common.c:b53_vlan_add, about time :)

It seems to me that the fundamental issue here is that because we do not
have a user visible network device that 1:1 maps with the CPU (or DSA)
ports for that matter (and for valid reasons, they would represent two
ends of the same pipe), we do not have a good way to control the CPU
port VLAN attributes.

There was a prior attempt at allowing using the bridge master device to
program the CPU port's VLAN attributes, see [1], but I did not follow up
with that until [2] and then life caught me. If you can/want, that would
be great (not asking for TPS reports).

[1]:
https://lists.linuxfoundation.org/pipermail/bridge/2016-November/010112.html
[2]:
https://lore.kernel.org/lkml/20180624153339.13572-1-f.fainelli@gmail.com/T/

> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/switch.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 84ab2336131e..02ccc53f1926 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -239,17 +239,21 @@ dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
>  			       const struct switchdev_obj_port_vlan *vlan,
>  			       const unsigned long *bitmap)
>  {
> +	struct switchdev_obj_port_vlan v = *vlan;
>  	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 (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +			v.flags &= ~BRIDGE_VLAN_INFO_PVID;
> +
> +		err = dsa_port_vlan_check(ds, port, &v);
>  		if (err)
>  			return err;
>  
> -		err = ds->ops->port_vlan_prepare(ds, port, vlan);
> +		err = ds->ops->port_vlan_prepare(ds, port, &v);
>  		if (err)
>  			return err;
>  	}
> @@ -262,10 +266,14 @@ dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
>  			   const struct switchdev_obj_port_vlan *vlan,
>  			   const unsigned long *bitmap)
>  {
> +	struct switchdev_obj_port_vlan v = *vlan;
>  	int port;
>  
> -	for_each_set_bit(port, bitmap, ds->num_ports)
> -		ds->ops->port_vlan_add(ds, port, vlan);
> +	for_each_set_bit(port, bitmap, ds->num_ports) {
> +		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +			v.flags &= ~BRIDGE_VLAN_INFO_PVID;
> +		ds->ops->port_vlan_add(ds, port, &v);
> +	}
>  }
>  
>  static int dsa_switch_vlan_add(struct dsa_switch *ds,
> 

-- 
Florian

Re: [PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on the upstream port

[Vladimir Oltean]

Hi Florian,

On Tue, 20 Aug 2019 at 06:15, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 8/19/2019 5:00 PM, Vladimir Oltean wrote:
> > Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> > programs the VLAN from the bridge into the specified port as well as the
> > upstream port, with the same set of flags.
> >
> > Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
> > user port 2, etc. The upstream port would end up having a pvid equal to
> > the last user port whose pvid was programmed from the bridge. Less than
> > useful.
> >
> > So just don't change the pvid of the upstream port and let it be
> > whatever the driver set it internally to be.
>
> This patch should allow removing the !dsa_is_cpu_port() checks from
> b53_common.c:b53_vlan_add, about time :)
>
> It seems to me that the fundamental issue here is that because we do not
> have a user visible network device that 1:1 maps with the CPU (or DSA)
> ports for that matter (and for valid reasons, they would represent two
> ends of the same pipe), we do not have a good way to control the CPU
> port VLAN attributes.
>
> There was a prior attempt at allowing using the bridge master device to
> program the CPU port's VLAN attributes, see [1], but I did not follow up
> with that until [2] and then life caught me. If you can/want, that would
> be great (not asking for TPS reports).
>
> [1]:
> https://lists.linuxfoundation.org/pipermail/bridge/2016-November/010112.html
> [2]:
> https://lore.kernel.org/lkml/20180624153339.13572-1-f.fainelli@gmail.com/T/
>

So what was the conclusion of that discussion? Should you or should
you not add the check for vlan->flags & BRIDGE_VLAN_INFO_BRENTRY?
I don't exactly handle the meaning of 'master' and 'self' options from
a user perspective.
Right now (no patches applied) I get the following behavior in DSA
(swp2 is already member of br0):

$ echo 1 | sudo tee /sys/class/net/br0/bridge/vlan_filtering
$ sudo bridge vlan add vid 100 dev swp2
$ sudo bridge vlan add vid 101 dev swp2 self
RTNETLINK answers: Operation not supported
$ sudo bridge vlan add vid 102 dev swp2 master
$ sudo bridge vlan add vid 103 dev br0
RTNETLINK answers: Operation not supported
$ sudo bridge vlan add vid 104 dev br0 self
$ sudo bridge vlan add vid 105 dev br0 master
RTNETLINK answers: Operation not supported

$ bridge vlan
port    vlan ids
eth0     1 PVID Egress Untagged

swp5     1 PVID Egress Untagged

swp2     1 PVID Egress Untagged
         100
         102

swp3     1 PVID Egress Untagged

swp4     1 PVID Egress Untagged

br0      1 PVID Egress Untagged
         104

Who returns EOPNOTSUPP for VID 101 and why?
Why is VID 102 not installed in br0? This part I don't understand from
your patchset. Does it mean that the CPU port (br0) will have to be
explicitly configured from now on, even if I run the commands on swp2
with 'master'?


> >
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> >  net/dsa/switch.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> > index 84ab2336131e..02ccc53f1926 100644
> > --- a/net/dsa/switch.c
> > +++ b/net/dsa/switch.c
> > @@ -239,17 +239,21 @@ dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
> >                              const struct switchdev_obj_port_vlan *vlan,
> >                              const unsigned long *bitmap)
> >  {
> > +     struct switchdev_obj_port_vlan v = *vlan;
> >       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 (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> > +                     v.flags &= ~BRIDGE_VLAN_INFO_PVID;
> > +
> > +             err = dsa_port_vlan_check(ds, port, &v);
> >               if (err)
> >                       return err;
> >
> > -             err = ds->ops->port_vlan_prepare(ds, port, vlan);
> > +             err = ds->ops->port_vlan_prepare(ds, port, &v);
> >               if (err)
> >                       return err;
> >       }
> > @@ -262,10 +266,14 @@ dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
> >                          const struct switchdev_obj_port_vlan *vlan,
> >                          const unsigned long *bitmap)
> >  {
> > +     struct switchdev_obj_port_vlan v = *vlan;
> >       int port;
> >
> > -     for_each_set_bit(port, bitmap, ds->num_ports)
> > -             ds->ops->port_vlan_add(ds, port, vlan);
> > +     for_each_set_bit(port, bitmap, ds->num_ports) {
> > +             if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> > +                     v.flags &= ~BRIDGE_VLAN_INFO_PVID;
> > +             ds->ops->port_vlan_add(ds, port, &v);
> > +     }
> >  }
> >
> >  static int dsa_switch_vlan_add(struct dsa_switch *ds,
> >
>
> --
> Florian

Regards,
-Vladimir

Re: [PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on the upstream port

[Florian Fainelli]

On 8/20/19 5:09 AM, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Tue, 20 Aug 2019 at 06:15, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 8/19/2019 5:00 PM, Vladimir Oltean wrote:
>>> Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
>>> programs the VLAN from the bridge into the specified port as well as the
>>> upstream port, with the same set of flags.
>>>
>>> Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
>>> user port 2, etc. The upstream port would end up having a pvid equal to
>>> the last user port whose pvid was programmed from the bridge. Less than
>>> useful.
>>>
>>> So just don't change the pvid of the upstream port and let it be
>>> whatever the driver set it internally to be.
>>
>> This patch should allow removing the !dsa_is_cpu_port() checks from
>> b53_common.c:b53_vlan_add, about time :)
>>
>> It seems to me that the fundamental issue here is that because we do not
>> have a user visible network device that 1:1 maps with the CPU (or DSA)
>> ports for that matter (and for valid reasons, they would represent two
>> ends of the same pipe), we do not have a good way to control the CPU
>> port VLAN attributes.
>>
>> There was a prior attempt at allowing using the bridge master device to
>> program the CPU port's VLAN attributes, see [1], but I did not follow up
>> with that until [2] and then life caught me. If you can/want, that would
>> be great (not asking for TPS reports).
>>
>> [1]:
>> https://lists.linuxfoundation.org/pipermail/bridge/2016-November/010112.html
>> [2]:
>> https://lore.kernel.org/lkml/20180624153339.13572-1-f.fainelli@gmail.com/T/
>>
> 
> So what was the conclusion of that discussion? Should you or should
> you not add the check for vlan->flags & BRIDGE_VLAN_INFO_BRENTRY?

I was not able to test that change, and got distracted for months
(years?) doing "other stuff" that is not DSA related.

> I don't exactly handle the meaning of 'master' and 'self' options from
> a user perspective.
> Right now (no patches applied) I get the following behavior in DSA
> (swp2 is already member of br0):
> 
> $ echo 1 | sudo tee /sys/class/net/br0/bridge/vlan_filtering
> $ sudo bridge vlan add vid 100 dev swp2
> $ sudo bridge vlan add vid 101 dev swp2 self
> RTNETLINK answers: Operation not supported
> $ sudo bridge vlan add vid 102 dev swp2 master
> $ sudo bridge vlan add vid 103 dev br0
> RTNETLINK answers: Operation not supported
> $ sudo bridge vlan add vid 104 dev br0 self
> $ sudo bridge vlan add vid 105 dev br0 master
> RTNETLINK answers: Operation not supported
> 
> $ bridge vlan
> port    vlan ids
> eth0     1 PVID Egress Untagged
> 
> swp5     1 PVID Egress Untagged
> 
> swp2     1 PVID Egress Untagged
>          100
>          102
> 
> swp3     1 PVID Egress Untagged
> 
> swp4     1 PVID Egress Untagged
> 
> br0      1 PVID Egress Untagged
>          104
> 
> Who returns EOPNOTSUPP for VID 101 and why?
> Why is VID 102 not installed in br0? This part I don't understand from
> your patchset. Does it mean that the CPU port (br0) will have to be
> explicitly configured from now on, even if I run the commands on swp2
> with 'master'?

This does not really answer your questions, but maybe let's agree on the
user visible behavior. My expectations would be the following should be
happening with this patch applied:

- when the VLAN is created for the first and is configured on either the
bridge master device (br0) or an user port (swp2), it gets programmed
into the switch for the CPU port and respectively CPU port and swp2 port

- when you change the bridge master device VLAN attributes, or
add/delete a new one, the programming targets only the CPU port with the
proper operation

That way, there would be no change in how the initial VLAN programming
is done, in that the CPU port still gets programmed, but later on, if
you want e.g.: your CPU port not to be tagged, but untagged into a
particular VLAN.

My upcoming weeks don't look great in terms of resuming active or semi
active DSA work, but working with the DSA mock-up driver might be an
option to avoid spending too much time testing on real HW.
-- 
Florian

Re: [PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on the upstream port

[Vivien Didelot]

On Tue, 20 Aug 2019 03:00:00 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> programs the VLAN from the bridge into the specified port as well as the
> upstream port, with the same set of flags.
> 
> Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
> user port 2, etc. The upstream port would end up having a pvid equal to
> the last user port whose pvid was programmed from the bridge. Less than
> useful.
> 
> So just don't change the pvid of the upstream port and let it be
> whatever the driver set it internally to be.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/switch.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 84ab2336131e..02ccc53f1926 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -239,17 +239,21 @@ dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
>  			       const struct switchdev_obj_port_vlan *vlan,
>  			       const unsigned long *bitmap)
>  {
> +	struct switchdev_obj_port_vlan v = *vlan;
>  	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 (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +			v.flags &= ~BRIDGE_VLAN_INFO_PVID;

So you keep the BRIDGE_VLAN_INFO_PVID flag cleared for all other ports that
come after any CPU or DSA port?

> +
> +		err = dsa_port_vlan_check(ds, port, &v);
>  		if (err)
>  			return err;
>  
> -		err = ds->ops->port_vlan_prepare(ds, port, vlan);
> +		err = ds->ops->port_vlan_prepare(ds, port, &v);
>  		if (err)
>  			return err;
>  	}
> @@ -262,10 +266,14 @@ dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
>  			   const struct switchdev_obj_port_vlan *vlan,
>  			   const unsigned long *bitmap)
>  {
> +	struct switchdev_obj_port_vlan v = *vlan;
>  	int port;
>  
> -	for_each_set_bit(port, bitmap, ds->num_ports)
> -		ds->ops->port_vlan_add(ds, port, vlan);
> +	for_each_set_bit(port, bitmap, ds->num_ports) {
> +		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +			v.flags &= ~BRIDGE_VLAN_INFO_PVID;

Same here. Did you intend to initialize your switchdev_obj_port_vlan structure
_within_ the for_each_set_bit loop maybe?

> +		ds->ops->port_vlan_add(ds, port, &v);
> +	}
>  }
>  
>  static int dsa_switch_vlan_add(struct dsa_switch *ds,

Do you even test your patches?

Re: [PATCH net-next 4/6] net: dsa: Don't program the VLAN as pvid on the upstream port

[Vladimir Oltean]

Hi Vivien,

On Tue, 20 Aug 2019 at 09:07, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> On Tue, 20 Aug 2019 03:00:00 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> > programs the VLAN from the bridge into the specified port as well as the
> > upstream port, with the same set of flags.
> >
> > Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
> > user port 2, etc. The upstream port would end up having a pvid equal to
> > the last user port whose pvid was programmed from the bridge. Less than
> > useful.
> >
> > So just don't change the pvid of the upstream port and let it be
> > whatever the driver set it internally to be.
> >
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> >  net/dsa/switch.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> > index 84ab2336131e..02ccc53f1926 100644
> > --- a/net/dsa/switch.c
> > +++ b/net/dsa/switch.c
> > @@ -239,17 +239,21 @@ dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
> >                              const struct switchdev_obj_port_vlan *vlan,
> >                              const unsigned long *bitmap)
> >  {
> > +     struct switchdev_obj_port_vlan v = *vlan;
> >       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 (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> > +                     v.flags &= ~BRIDGE_VLAN_INFO_PVID;
>
> So you keep the BRIDGE_VLAN_INFO_PVID flag cleared for all other ports that
> come after any CPU or DSA port?
>

It looks like the convenient hardware decision of making the CPU port
on my board also be the numerically highest one strikes again :)
I always find bugs when I change the CPU port to another number.
This is another example (also related to the inclusion of upstream
ports in the VLAN bitmap, like they all seem to be):
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=d34d2baa9173f6e0c0f22d005d18e83d1cb54d8d

> > +
> > +             err = dsa_port_vlan_check(ds, port, &v);
> >               if (err)
> >                       return err;
> >
> > -             err = ds->ops->port_vlan_prepare(ds, port, vlan);
> > +             err = ds->ops->port_vlan_prepare(ds, port, &v);
> >               if (err)
> >                       return err;
> >       }
> > @@ -262,10 +266,14 @@ dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
> >                          const struct switchdev_obj_port_vlan *vlan,
> >                          const unsigned long *bitmap)
> >  {
> > +     struct switchdev_obj_port_vlan v = *vlan;
> >       int port;
> >
> > -     for_each_set_bit(port, bitmap, ds->num_ports)
> > -             ds->ops->port_vlan_add(ds, port, vlan);
> > +     for_each_set_bit(port, bitmap, ds->num_ports) {
> > +             if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> > +                     v.flags &= ~BRIDGE_VLAN_INFO_PVID;
>
> Same here. Did you intend to initialize your switchdev_obj_port_vlan structure
> _within_ the for_each_set_bit loop maybe?
>

Thanks for pointing this out.

> > +             ds->ops->port_vlan_add(ds, port, &v);
> > +     }
> >  }
> >
> >  static int dsa_switch_vlan_add(struct dsa_switch *ds,
>
> Do you even test your patches?

No.

Regards,
-Vladimir

[PATCH net-next 5/6] net: dsa: Allow proper internal use of VLANs

[Vladimir Oltean]

Below commit:

commit 2ea7a679ca2abd251c1ec03f20508619707e1749
Author: Andrew Lunn <andrew@lunn.ch>
Date:   Tue Nov 7 00:04:24 2017 +0100

    net: dsa: Don't add vlans when vlan filtering is disabled

    The software bridge can be build with vlan filtering support
    included. However, by default it is turned off. In its turned off
    state, it still passes VLANs via switchev, even though they are not to
    be used. Don't pass these VLANs to the hardware. Only do so when vlan
    filtering is enabled.

    This fixes at least one corner case. There are still issues in other
    corners, such as when vlan_filtering is later enabled.

    Signed-off-by: Andrew Lunn <andrew@lunn.ch>
    Signed-off-by: David S. Miller <davem@davemloft.net>

stubs out SWITCHDEV_OBJ_ID_PORT_VLAN objects notified by the bridge core
to DSA drivers when vlan_filtering is 0.

This is generally a good thing, because it allows dsa_8021q to make
private use of VLANs in that mode.

So it makes sense to move the check for the bridge presence and
vlan_filtering setting one layer above. We don't want calls from
dsa_8021q to be prevented by this, only from the bridge core.

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

diff --git a/net/dsa/port.c b/net/dsa/port.c
index f75301456430..c1cc41c1eeb6 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 33f41178afcc..6a02eb8988d1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -341,6 +341,8 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		if (obj->orig_dev != dev)
 			return -EOPNOTSUPP;
+		if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
+			return 0;
 		err = dsa_port_vlan_add(dp, SWITCHDEV_OBJ_PORT_VLAN(obj),
 					trans);
 		break;
@@ -373,6 +375,8 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		if (obj->orig_dev != dev)
 			return -EOPNOTSUPP;
+		if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
+			return 0;
 		err = dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
 		break;
 	default:
@@ -1073,6 +1077,8 @@ 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.
@@ -1097,6 +1103,8 @@ 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.17.1

Re: [PATCH net-next 5/6] net: dsa: Allow proper internal use of VLANs

[Florian Fainelli]

On 8/19/2019 5:00 PM, Vladimir Oltean wrote:
> Below commit:
> 
> commit 2ea7a679ca2abd251c1ec03f20508619707e1749
> Author: Andrew Lunn <andrew@lunn.ch>
> Date:   Tue Nov 7 00:04:24 2017 +0100
> 
>     net: dsa: Don't add vlans when vlan filtering is disabled
> 
>     The software bridge can be build with vlan filtering support
>     included. However, by default it is turned off. In its turned off
>     state, it still passes VLANs via switchev, even though they are not to
>     be used. Don't pass these VLANs to the hardware. Only do so when vlan
>     filtering is enabled.
> 
>     This fixes at least one corner case. There are still issues in other
>     corners, such as when vlan_filtering is later enabled.
> 
>     Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> stubs out SWITCHDEV_OBJ_ID_PORT_VLAN objects notified by the bridge core
> to DSA drivers when vlan_filtering is 0.
> 
> This is generally a good thing, because it allows dsa_8021q to make
> private use of VLANs in that mode.
> 
> So it makes sense to move the check for the bridge presence and
> vlan_filtering setting one layer above. We don't want calls from
> dsa_8021q to be prevented by this, only from the bridge core.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

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

[PATCH net-next 6/6] net: dsa: tag_8021q: Restore bridge pvid 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.

So, 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.

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

diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 67a1bc635a7b..6423beb1efcd 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -93,6 +93,33 @@ int dsa_8021q_rx_source_port(u16 vid)
 }
 EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
 
+static int dsa_port_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);
+}
+
 /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
  * front-panel switch port (here swp0).
  *
@@ -223,7 +250,10 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 		return err;
 	}
 
-	return 0;
+	if (!enabled)
+		err = dsa_port_restore_pvid(ds, port);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
 
-- 
2.17.1

Re: [PATCH net-next 6/6] net: dsa: tag_8021q: Restore bridge pvid when enabling vlan_filtering

[Florian Fainelli]

On 8/19/2019 5:00 PM, Vladimir Oltean 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.
> 
> So, 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.

That is arguably applicable with DSA at large and not just specifically
for tag_8021q.c no? Is there a reason why you are not seeking to solve
this on a more global scale?

> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/tag_8021q.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> index 67a1bc635a7b..6423beb1efcd 100644
> --- a/net/dsa/tag_8021q.c
> +++ b/net/dsa/tag_8021q.c
> @@ -93,6 +93,33 @@ int dsa_8021q_rx_source_port(u16 vid)
>  }
>  EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
>  
> +static int dsa_port_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);
> +}
> +
>  /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
>   * front-panel switch port (here swp0).
>   *
> @@ -223,7 +250,10 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
>  		return err;
>  	}
>  
> -	return 0;
> +	if (!enabled)
> +		err = dsa_port_restore_pvid(ds, port);
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
>  
> 

-- 
Florian

Re: [PATCH net-next 6/6] net: dsa: tag_8021q: Restore bridge pvid when enabling vlan_filtering

[Vladimir Oltean]

Hi Florian,

On Tue, 20 Aug 2019 at 06:33, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 8/19/2019 5:00 PM, Vladimir Oltean 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.
> >
> > So, 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.
>
> That is arguably applicable with DSA at large and not just specifically
> for tag_8021q.c no? Is there a reason why you are not seeking to solve
> this on a more global scale?
>

Perhaps because I don't have a good feel for what are other DSA
drivers' struggles with restoring the pvid, even after re-reading the
"What to do when a bridge port gets its pvid deleted?" thread.
I understand b53 has a similar need, and for that purpose maybe you
can EXPORT_SYMBOL_GPL(dsa_port_restore_pvid) and use it, but
otherwise, what is the more global scale?

> >
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> > ---
> >  net/dsa/tag_8021q.c | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> > index 67a1bc635a7b..6423beb1efcd 100644
> > --- a/net/dsa/tag_8021q.c
> > +++ b/net/dsa/tag_8021q.c
> > @@ -93,6 +93,33 @@ int dsa_8021q_rx_source_port(u16 vid)
> >  }
> >  EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
> >
> > +static int dsa_port_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);
> > +}
> > +
> >  /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
> >   * front-panel switch port (here swp0).
> >   *
> > @@ -223,7 +250,10 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
> >               return err;
> >       }
> >
> > -     return 0;
> > +     if (!enabled)
> > +             err = dsa_port_restore_pvid(ds, port);
> > +
> > +     return err;
> >  }
> >  EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
> >
> >
>
> --
> Florian

Regards,
-Vladimir

Re: [PATCH net-next 6/6] net: dsa: tag_8021q: Restore bridge pvid when enabling vlan_filtering

[Florian Fainelli]

On 8/20/19 3:28 AM, Vladimir Oltean wrote:
> Hi Florian,
> 
> On Tue, 20 Aug 2019 at 06:33, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 8/19/2019 5:00 PM, Vladimir Oltean 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.
>>>
>>> So, 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.
>>
>> That is arguably applicable with DSA at large and not just specifically
>> for tag_8021q.c no? Is there a reason why you are not seeking to solve
>> this on a more global scale?
>>
> 
> Perhaps because I don't have a good feel for what are other DSA
> drivers' struggles with restoring the pvid, even after re-reading the
> "What to do when a bridge port gets its pvid deleted?" thread.
> I understand b53 has a similar need, and for that purpose maybe you
> can EXPORT_SYMBOL_GPL(dsa_port_restore_pvid) and use it, but
> otherwise, what is the more global scale?

I was just referring to all DSA drivers, not just tag_8021q.c which is,
so far a very narrow user base. I don't have a strong feeling against
calling a helper function versus the core DSA layer doing this for you,
but clearly this is something that should plague all drivers, not just
sja1105, even if the effects of not restoring the PVID there may be more
"catastrophic".
-- 
Florian

Re: [PATCH net-next 6/6] net: dsa: tag_8021q: Restore bridge pvid when enabling vlan_filtering

[Florian Fainelli]

On 8/19/19 5:00 PM, Vladimir Oltean 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.
> 
> So, 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.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

OK, after reading how
drivers/net/dsa/sja1105/sja1105_main.c::sja1105_vlan_filtering makes use
of that functionality, that looks like the correct thing to do.

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

Re: [PATCH net-next 0/6] Dynamic toggling of vlan_filtering for SJA1105 DSA

[Vivien Didelot]

On Tue, 20 Aug 2019 02:59:56 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> This patchset addresses a few limitations in DSA and the bridge core
> that made it impossible for this sequence of commands to work:
> 
>   ip link add name br0 type bridge
>   ip link set dev swp2 master br0
>   echo 1 > /sys/class/net/br0/bridge/vlan_filtering
> 
> Only this sequence was previously working:
> 
>   ip link add name br0 type bridge vlan_filtering 1
>   ip link set dev swp2 master br0

This is not quite true, these sequences of commands do "work". What I see
though is that with the first sequence, the PVID 1 won't be programmed in
the hardware. But the second sequence does program the hardware.

But following bridge members will be correctly programmed with the VLAN
though. The sequence below programs the hardware with VLAN 1 for swp3 as
well as CPU and DSA ports, but not for swp2:

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

This is unfortunately also true for any 802.1Q VLANs. For example, only VID
43 is programmed with the following sequence, but not VID 1 and VID 42:

    ip link add name br0 type bridge
    ip link set dev swp2 master br0
    bridge vlan add dev swp2 vid 42
    echo 1 > /sys/class/net/br0/bridge/vlan_filtering
    bridge vlan add dev swp2 vid 43

So I understand that because VLANs are not propagated by DSA to the hardware
when VLAN filtering is disabled, a port may not be programmed with its
bridge's default PVID, and this is causing a problem for tag_8021q.

Please reword so that we understand better what is the issue being fixed here.

> 
> On SJA1105, the situation is further complicated by the fact that
> toggling vlan_filtering is causing a switch reset. However, the hardware
> state restoration logic is already there in the driver. It is a matter
> of the layers above which need a few fixups.
> 
> Also see this discussion thread:
> https://www.spinics.net/lists/netdev/msg581042.html
> 
> Patch 1/6 is not functionally related but also related to dsa_8021q
> handling of VLANs and this is a good opportunity to bring up the subject
> for discussion.

So please send 1/6 as a separate patch and bring up the discussion there.


Thanks,

	Vivien

Re: [PATCH net-next 0/6] Dynamic toggling of vlan_filtering for SJA1105 DSA

[Vladimir Oltean]

On Tue, 20 Aug 2019 at 11:04, Vivien Didelot <vivien.didelot@gmail.com> wrote:
>
> On Tue, 20 Aug 2019 02:59:56 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > This patchset addresses a few limitations in DSA and the bridge core
> > that made it impossible for this sequence of commands to work:
> >
> >   ip link add name br0 type bridge
> >   ip link set dev swp2 master br0
> >   echo 1 > /sys/class/net/br0/bridge/vlan_filtering
> >
> > Only this sequence was previously working:
> >
> >   ip link add name br0 type bridge vlan_filtering 1
> >   ip link set dev swp2 master br0
>
> This is not quite true, these sequences of commands do "work". What I see
> though is that with the first sequence, the PVID 1 won't be programmed in
> the hardware. But the second sequence does program the hardware.
>
> But following bridge members will be correctly programmed with the VLAN
> though. The sequence below programs the hardware with VLAN 1 for swp3 as
> well as CPU and DSA ports, but not for swp2:
>
>     ip link add name br0 type bridge
>     ip link set dev swp2 master br0
>     echo 1 > /sys/class/net/br0/bridge/vlan_filtering
>     ip link set dev swp3 master br0
>
> This is unfortunately also true for any 802.1Q VLANs. For example, only VID
> 43 is programmed with the following sequence, but not VID 1 and VID 42:
>
>     ip link add name br0 type bridge
>     ip link set dev swp2 master br0
>     bridge vlan add dev swp2 vid 42
>     echo 1 > /sys/class/net/br0/bridge/vlan_filtering
>     bridge vlan add dev swp2 vid 43
>
> So I understand that because VLANs are not propagated by DSA to the hardware
> when VLAN filtering is disabled, a port may not be programmed with its
> bridge's default PVID, and this is causing a problem for tag_8021q.
>
> Please reword so that we understand better what is the issue being fixed here.
>

Not exactly, no.
The fact that a port in DSA is not programmed with the bridge's
default_pvid is just a fact.
Right now, letting the bridge install the default_pvid into the ports
even breaks dsa_8021q when enslaving the ports to a vlan_filtering=0
bridge, but that is perhaps only a timing issue and can be resolved in
other ways.

To understand my issue I need to make a side-note.
The sja1105 needs to be reset at runtime for changing some settings. I
am working on a separate patchset that tries to make those switch
resets as seamless as possible. The end goal is to not disrupt
linuxptp (ptp4l, phc2sys) operation when enabling the tc-taprio
offload. That means:
- saving and restoring the PTP time after reset
- preventing switch reset to happen during TX timestamping
- preventing any clock operation on the PTP timer during switch reset
Since toggling vlan_filtering also needs to reset the switch, I am
simply using that as a handy tool to test how seamless the switch
reset is.
But currently, toggling vlan_filtering breaks traffic, due to nobody
restoring the correct (from the bridge's perspective) pvid on the
ports. So I need to fix this to continue the testing.
On the user ports, I can just query the bridge, and that's exactly
what I'm doing.
On the CPU and DSA ports, I can't query the bridge because the bridge
knows nothing about them. And I also can't query the pvid from the DSA
core - it can change it, but not know what it is. So I need to be
proactive and make DSA avoid changing their pvid needlessly.
With this set of changes, the goal was for traffic to 'just work' when
changing the vlan_filtering setting back and forth. This would also
benefit other future users of dsa_8021q.
There is still work to be done. dsa_8021q uses VID range 1024-2559
privately. If the bridge had been using any VLAN in that range during
vlan_filtering=1, then dsa_8021q should also restore that VLAN.
Currently I'm not doing that because I'm still not clear whether it's
just as simple as calling br_vlan_get_info(slave, {rx,tx}_vid,
&vinfo); and adding that back (I don't completely understand the
interaction with the implicit rules on the CPU ports - I am concerned
that if the switch driver sees the VID is already installed on the CPU
port, it will just skip the command).
Ido Schimmel mentioned that static FDB entries with VLAN != PVID need
to be preserved. I am already doing something in that direction there
- in vlan_filtering=0 mode I am switching to shared VLAN learning
mode. This means the switch is ignoring the VLAN from the FDB entries,
and just uses the DMAC for L2 routing. When going back to the
vlan_filtering=1 mode, the VLAN from the static FDB entries goes back
in use. I think that's ok.
There is also stuff that cannot be reasonably expected to be preserved
across switch resets. Learned FDB entries is one thing. Traffic
(including regular, but not management, traffic originated from the
CPU) will be temporarily dropped.

> >
> > On SJA1105, the situation is further complicated by the fact that
> > toggling vlan_filtering is causing a switch reset. However, the hardware
> > state restoration logic is already there in the driver. It is a matter
> > of the layers above which need a few fixups.
> >
> > Also see this discussion thread:
> > https://www.spinics.net/lists/netdev/msg581042.html
> >
> > Patch 1/6 is not functionally related but also related to dsa_8021q
> > handling of VLANs and this is a good opportunity to bring up the subject
> > for discussion.
>
> So please send 1/6 as a separate patch and bring up the discussion there.
>

Ok.

>
> Thanks,
>
>         Vivien

Regards,
-Vladimir