diff mbox series

[net-next,4/5,v4] net: dsa: rtl8366: VLAN 0 as disable tagging

Message ID 20200706205245.937091-5-linus.walleij@linaro.org
State New
Headers show
Series RTL8366RB DSA tagging and fixes | expand

Commit Message

Linus Walleij July 6, 2020, 8:52 p.m. UTC 
The code in net/8021q/vlan.c, vlan_device_event() sets
VLAN 0 for a VLAN-capable ethernet device when it
comes up.

Since the RTL8366 DSA switches must have a VLAN and
PVID set up for any packets to come through we have
already set up default VLAN for each port as part of
bringing the switch online.

Make sure that setting VLAN 0 has the same effect
and does not try to actually tell the hardware to use
VLAN 0 on the port because that will not work.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v3->v4:
- Resend with the rest
ChangeLog v2->v3:
- Collected Andrew's review tag.
ChangeLog v1->v2:
- Rebased on v5.8-rc1 and other changes.
---
 drivers/net/dsa/rtl8366.c | 65 +++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 13 deletions(-)

-- 
2.26.2

Comments

Florian Fainelli July 6, 2020, 9:23 p.m. UTC | #1 
On 7/6/2020 1:52 PM, Linus Walleij wrote:
> The code in net/8021q/vlan.c, vlan_device_event() sets

> VLAN 0 for a VLAN-capable ethernet device when it

> comes up.

> 

> Since the RTL8366 DSA switches must have a VLAN and

> PVID set up for any packets to come through we have

> already set up default VLAN for each port as part of

> bringing the switch online.

> 

> Make sure that setting VLAN 0 has the same effect

> and does not try to actually tell the hardware to use

> VLAN 0 on the port because that will not work.

> 

> Cc: DENG Qingfang <dqfext@gmail.com>

> Cc: Mauri Sandberg <sandberg@mailfence.com>

> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v3->v4:

> - Resend with the rest

> ChangeLog v2->v3:

> - Collected Andrew's review tag.

> ChangeLog v1->v2:

> - Rebased on v5.8-rc1 and other changes.

> ---

>  drivers/net/dsa/rtl8366.c | 65 +++++++++++++++++++++++++++++++--------

>  1 file changed, 52 insertions(+), 13 deletions(-)

> 

> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c

> index b907c0ed9697..a000d458d121 100644

> --- a/drivers/net/dsa/rtl8366.c

> +++ b/drivers/net/dsa/rtl8366.c

> @@ -355,15 +355,25 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,

>  			 const struct switchdev_obj_port_vlan *vlan)

>  {

>  	struct realtek_smi *smi = ds->priv;

> +	u16 vid_begin = vlan->vid_begin;

> +	u16 vid_end = vlan->vid_end;

>  	u16 vid;

>  	int ret;

>  

> -	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)

> +	if (vid_begin == 0) {

> +		dev_info(smi->dev, "prepare VLAN 0 - ignored\n");

> +		if (vid_end == 0)

> +			return 0;

> +		/* Skip VLAN 0 and start with VLAN 1 */

> +		vid_begin = 1;

> +	}


Humm I still don't understand why you are doing that. Upon DSA network
device creation, VID 0 will be pushed because we advertise support for
NETIF_F_HW_VLAN_CTAG_FILTER, so if nothing else, we will be getting the
"prepare VLAN 0 -ignored" message which is not relevant nor a good idea
to print.

You can force this VLAN to be programmed as untagged, in fact you should
be doing that per the 802.1Q specification.

There are no other cases other than the initial network device creation
that will lead to programming this VLAN ID. The bridge will always
specify a VID range within 1 through 4094 and the VLAN RX filter offload
will not add or remove VID 0 other than at creation/destruction.

As mentioned before, if you need VLAN awareness into the switch from the
get go, you need to set configure_vlan_while_not_filtering and that
would ensure that all ports belong to a VID at startup. Later on, when
the bridge gets set-up, it will be requesting the ports added as bridge
ports to be programmed into VID 1 as PVID untagged. And this should
still be fine.
-- 
Florian
Vladimir Oltean July 7, 2020, 7:20 a.m. UTC | #2 
Hi Linus,

On Mon, Jul 06, 2020 at 02:23:11PM -0700, Florian Fainelli wrote:
> 

> 

> On 7/6/2020 1:52 PM, Linus Walleij wrote:

> > The code in net/8021q/vlan.c, vlan_device_event() sets

> > VLAN 0 for a VLAN-capable ethernet device when it

> > comes up.

> > 

> > Since the RTL8366 DSA switches must have a VLAN and

> > PVID set up for any packets to come through we have

> > already set up default VLAN for each port as part of

> > bringing the switch online.

> > 

> > Make sure that setting VLAN 0 has the same effect

> > and does not try to actually tell the hardware to use

> > VLAN 0 on the port because that will not work.

> > 

> > Cc: DENG Qingfang <dqfext@gmail.com>

> > Cc: Mauri Sandberg <sandberg@mailfence.com>

> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>

> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> > ---

> > ChangeLog v3->v4:

> > - Resend with the rest

> > ChangeLog v2->v3:

> > - Collected Andrew's review tag.

> > ChangeLog v1->v2:

> > - Rebased on v5.8-rc1 and other changes.

> > ---

> >  drivers/net/dsa/rtl8366.c | 65 +++++++++++++++++++++++++++++++--------

> >  1 file changed, 52 insertions(+), 13 deletions(-)

> > 

> > diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c

> > index b907c0ed9697..a000d458d121 100644

> > --- a/drivers/net/dsa/rtl8366.c

> > +++ b/drivers/net/dsa/rtl8366.c

> > @@ -355,15 +355,25 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,

> >  			 const struct switchdev_obj_port_vlan *vlan)

> >  {

> >  	struct realtek_smi *smi = ds->priv;

> > +	u16 vid_begin = vlan->vid_begin;

> > +	u16 vid_end = vlan->vid_end;

> >  	u16 vid;

> >  	int ret;

> >  

> > -	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)

> > +	if (vid_begin == 0) {

> > +		dev_info(smi->dev, "prepare VLAN 0 - ignored\n");

> > +		if (vid_end == 0)

> > +			return 0;

> > +		/* Skip VLAN 0 and start with VLAN 1 */

> > +		vid_begin = 1;

> > +	}

> 

> Humm I still don't understand why you are doing that. Upon DSA network

> device creation, VID 0 will be pushed because we advertise support for

> NETIF_F_HW_VLAN_CTAG_FILTER, so if nothing else, we will be getting the

> "prepare VLAN 0 -ignored" message which is not relevant nor a good idea

> to print.

> 

> You can force this VLAN to be programmed as untagged, in fact you should

> be doing that per the 802.1Q specification.

> 

> There are no other cases other than the initial network device creation

> that will lead to programming this VLAN ID. The bridge will always

> specify a VID range within 1 through 4094 and the VLAN RX filter offload

> will not add or remove VID 0 other than at creation/destruction.

> 

> As mentioned before, if you need VLAN awareness into the switch from the

> get go, you need to set configure_vlan_while_not_filtering and that

> would ensure that all ports belong to a VID at startup. Later on, when

> the bridge gets set-up, it will be requesting the ports added as bridge

> ports to be programmed into VID 1 as PVID untagged. And this should

> still be fine.

> -- 

> Florian


To add to what Florian said, you should basically try to enable and test
"ds->configure_vlan_while_not_filtering = true" regardless, while you're
at it. The whole reason why it's there is because we didn't want to
introduce breakage when changing behavior of the DSA core. But ideally,
all drivers would use this setting, and then it could get deleted and so
would the old behavior of DSA.

Thanks,
-Vladimir
Linus Walleij July 8, 2020, 1:34 p.m. UTC | #3 
On Mon, Jul 6, 2020 at 11:23 PM Florian Fainelli <f.fainelli@gmail.com> wrote:

> As mentioned before, if you need VLAN awareness into the switch from the

> get go, you need to set configure_vlan_while_not_filtering and that

> would ensure that all ports belong to a VID at startup. Later on, when

> the bridge gets set-up, it will be requesting the ports added as bridge

> ports to be programmed into VID 1 as PVID untagged. And this should

> still be fine.


I actually managed to figure this out. There were some confusing bugs in
the code that needed fixing but after that it works smoothly.

It is maybe not the most optimal setup - using one VLAN 1 for the whole
bridge and all ports denies the bridge to optimize the packet flow
per-port, but that is something we should fix in generic code and I
will describe it in detail in some patches I'm cooking.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index b907c0ed9697..a000d458d121 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -355,15 +355,25 @@  int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
 			 const struct switchdev_obj_port_vlan *vlan)
 {
 	struct realtek_smi *smi = ds->priv;
+	u16 vid_begin = vlan->vid_begin;
+	u16 vid_end = vlan->vid_end;
 	u16 vid;
 	int ret;
 
-	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
+	if (vid_begin == 0) {
+		dev_info(smi->dev, "prepare VLAN 0 - ignored\n");
+		if (vid_end == 0)
+			return 0;
+		/* Skip VLAN 0 and start with VLAN 1 */
+		vid_begin = 1;
+	}
+
+	for (vid = vid_begin; vid < vid_end; vid++)
 		if (!smi->ops->is_vlan_valid(smi, vid))
 			return -EINVAL;
 
 	dev_info(smi->dev, "prepare VLANs %04x..%04x\n",
-		 vlan->vid_begin, vlan->vid_end);
+		 vid_begin, vid_end);
 
 	/* Enable VLAN in the hardware
 	 * FIXME: what's with this 4k business?
@@ -383,27 +393,46 @@  void rtl8366_vlan_add(struct dsa_switch *ds, int port,
 	bool untagged = !!(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
 	bool pvid = !!(vlan->flags & BRIDGE_VLAN_INFO_PVID);
 	struct realtek_smi *smi = ds->priv;
+	u16 vid_begin = vlan->vid_begin;
+	u16 vid_end = vlan->vid_end;
 	u32 member = 0;
 	u32 untag = 0;
 	u16 vid;
 	int ret;
 
-	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
-		if (!smi->ops->is_vlan_valid(smi, vid))
+	if (vid_begin == 0) {
+		dev_info(smi->dev, "set VLAN 0 on port %d = default VLAN\n",
+			 port);
+		/* Set up default tagging */
+		ret = rtl8366_set_default_vlan_and_pvid(smi, port);
+		if (ret) {
+			dev_err(smi->dev,
+				"error setting default VLAN on port %d\n",
+				port);
 			return;
+		}
+		if (vid_end == 0)
+			return;
+		/* Skip VLAN 0 and start with VLAN 1 */
+		vid_begin = 1;
+	}
 
-	dev_info(smi->dev, "add VLAN on port %d, %s, %s\n",
-		 port,
-		 untagged ? "untagged" : "tagged",
-		 pvid ? " PVID" : "no PVID");
+	for (vid = vid_begin; vid < vid_end; vid++)
+		if (!smi->ops->is_vlan_valid(smi, vid))
+			return;
 
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
 		dev_err(smi->dev, "port is DSA or CPU port\n");
 
-	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+	for (vid = vid_begin; vid <= vid_end; ++vid) {
 		int pvid_val = 0;
 
-		dev_info(smi->dev, "add VLAN %04x\n", vid);
+		dev_info(smi->dev, "add VLAN %04x to port %d, %s, %s\n",
+			 vid,
+			 port,
+			 untagged ? "untagged" : "tagged",
+			 pvid ? " PVID" : "no PVID");
+
 		member |= BIT(port);
 
 		if (untagged)
@@ -437,15 +466,25 @@  int rtl8366_vlan_del(struct dsa_switch *ds, int port,
 		     const struct switchdev_obj_port_vlan *vlan)
 {
 	struct realtek_smi *smi = ds->priv;
+	u16 vid_begin = vlan->vid_begin;
+	u16 vid_end = vlan->vid_end;
 	u16 vid;
 	int ret;
 
-	dev_info(smi->dev, "del VLAN on port %d\n", port);
+	if (vid_begin == 0) {
+		dev_info(smi->dev, "remove port %d from VLAN 0 (no-op)\n",
+			 port);
+		if (vid_end == 0)
+			return 0;
+		/* Skip VLAN 0 and start with VLAN 1 */
+		vid_begin = 1;
+	}
 
-	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+	for (vid = vid_begin; vid <= vid_end; ++vid) {
 		int i;
 
-		dev_info(smi->dev, "del VLAN %04x\n", vid);
+		dev_info(smi->dev, "remove VLAN %04x from port %d\n",
+			 vid, port);
 
 		for (i = 0; i < smi->num_vlan_mc; i++) {
 			struct rtl8366_vlan_mc vlanmc;