diff mbox series

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

Message ID 20200617083132.1847234-4-linus.walleij@linaro.org
State Superseded
Headers show
Series [net-next,1/5,v2] net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag | expand

Commit Message

Linus Walleij June 17, 2020, 8:31 a.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>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
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

Andrew Lunn June 18, 2020, 1:31 a.m. UTC | #1 
On Wed, Jun 17, 2020 at 10:31:31AM +0200, 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>

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


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


    Andrew
Florian Fainelli June 18, 2020, 3:23 a.m. UTC | #2 
On 6/17/2020 1:31 AM, 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.


Why, you are not really describing what happens if VID = 0 is programmed?

It also sounds like you should be setting
configure_vlan_while_not_filtering if you need the switch to be
configured with VLAN awareness no matter whether there is a bridge
configured or not.
-- 
Florian
Linus Walleij July 5, 2020, 11:04 p.m. UTC | #3 
On Thu, Jun 18, 2020 at 5:24 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 6/17/2020 1:31 AM, Linus Walleij wrote:


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

>

> Why, you are not really describing what happens if VID = 0 is programmed?


OK I'll add some words about it to the commit message.
The packets doesn't go through the switch is the short
answer, I cannot give any better answer I think because,
well, realtek-type documentation (none).

> It also sounds like you should be setting

> configure_vlan_while_not_filtering if you need the switch to be

> configured with VLAN awareness no matter whether there is a bridge

> configured or not.


Oh another thing I didn't know existed, I'll try this,
but it needs to be a separate patch I think, possibly
removing some code for default VLANs.

Thanks!
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 66bd1241204c..7f0691a6da13 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;