* [PATCH net-next 1/6 v6] net: dsa: rtl8366rb: Support bridge offloading
2021-09-25 13:23 [PATCH net-next 0/6 v6] RTL8366(RB) cleanups part 1 Linus Walleij
@ 2021-09-25 13:23 ` Linus Walleij
2021-09-25 18:30 ` Vladimir Oltean
2021-09-25 13:23 ` [PATCH net-next 2/6 v6] net: dsa: rtl8366: Drop custom VLAN set-up Linus Walleij
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 13:23 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, DENG Qingfang, Mauri Sandberg, Alvin Šipraga, Linus Walleij
From: DENG Qingfang <dqfext@gmail.com>
Use port isolation registers to configure bridge offloading.
Tested on the D-Link DIR-685, switching between ports and
sniffing ports to make sure no packets leak.
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v5->v6:
- No changes just resending with the rest of the
patches.
ChangeLog v4->v5:
- No changes just resending with the rest of the
patches.
ChangeLog v3->v4:
- Fix a bug where I managed to mask off the CPU port
from the ports we could access leading to numb
bridge.
- Reword some comments.
ChangeLog v2->v3:
- Parens around the (pmask) in the port isolation macro.
- Do not exit join/leave functions on regmap failures,
print an error and continue.
- Clarify comments around the port in join/leave
functions.
ChangeLog v1->v2:
- introduce RTL8366RB_PORT_ISO_PORTS() to shift the port
mask into place so we are not confused by the enable
bit.
- Use this with dsa_user_ports() to isolate the CPU port
from itself.
---
drivers/net/dsa/rtl8366rb.c | 86 +++++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index a89093bc6c6a..b930050cfd1b 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -300,6 +300,13 @@
#define RTL8366RB_INTERRUPT_STATUS_REG 0x0442
#define RTL8366RB_NUM_INTERRUPT 14 /* 0..13 */
+/* Port isolation registers */
+#define RTL8366RB_PORT_ISO_BASE 0x0F08
+#define RTL8366RB_PORT_ISO(pnum) (RTL8366RB_PORT_ISO_BASE + (pnum))
+#define RTL8366RB_PORT_ISO_EN BIT(0)
+#define RTL8366RB_PORT_ISO_PORTS_MASK GENMASK(7, 1)
+#define RTL8366RB_PORT_ISO_PORTS(pmask) ((pmask) << 1)
+
/* bits 0..5 enable force when cleared */
#define RTL8366RB_MAC_FORCE_CTRL_REG 0x0F11
@@ -835,6 +842,21 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
if (ret)
return ret;
+ /* Isolate all user ports so they can only send packets to itself and the CPU port */
+ for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+ ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i),
+ RTL8366RB_PORT_ISO_PORTS(BIT(RTL8366RB_PORT_NUM_CPU)) |
+ RTL8366RB_PORT_ISO_EN);
+ if (ret)
+ return ret;
+ }
+ /* CPU port can send packets to all ports */
+ ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU),
+ RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds)) |
+ RTL8366RB_PORT_ISO_EN);
+ if (ret)
+ return ret;
+
/* Set up the "green ethernet" feature */
ret = rtl8366rb_jam_table(rtl8366rb_green_jam,
ARRAY_SIZE(rtl8366rb_green_jam), smi, false);
@@ -1127,6 +1149,68 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
rb8366rb_set_port_led(smi, port, false);
}
+static int
+rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
+ struct net_device *bridge)
+{
+ struct realtek_smi *smi = ds->priv;
+ unsigned int port_bitmap = 0;
+ int ret, i;
+
+ /* Loop over all other ports than the current one */
+ for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+ /* Current port handled last */
+ if (i == port)
+ continue;
+ /* Not on this bridge */
+ if (dsa_to_port(ds, i)->bridge_dev != bridge)
+ continue;
+ /* Join this port to each other port on the bridge */
+ ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
+ RTL8366RB_PORT_ISO_PORTS(BIT(port)),
+ RTL8366RB_PORT_ISO_PORTS(BIT(port)));
+ if (ret)
+ dev_err(smi->dev, "failed to join port %d\n", port);
+
+ port_bitmap |= BIT(i);
+ }
+
+ /* Set the bits for the ports we can access */
+ return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
+ RTL8366RB_PORT_ISO_PORTS(port_bitmap),
+ RTL8366RB_PORT_ISO_PORTS(port_bitmap));
+}
+
+static void
+rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
+ struct net_device *bridge)
+{
+ struct realtek_smi *smi = ds->priv;
+ unsigned int port_bitmap = 0;
+ int ret, i;
+
+ /* Loop over all other ports than this one */
+ for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+ /* Current port handled last */
+ if (i == port)
+ continue;
+ /* Not on this bridge */
+ if (dsa_to_port(ds, i)->bridge_dev != bridge)
+ continue;
+ /* Remove this port from any other port on the bridge */
+ ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
+ RTL8366RB_PORT_ISO_PORTS(BIT(port)), 0);
+ if (ret)
+ dev_err(smi->dev, "failed to leave port %d\n", port);
+
+ port_bitmap |= BIT(i);
+ }
+
+ /* Clear the bits for the ports we can not access, leave ourselves */
+ regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
+ RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
+}
+
static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
{
struct realtek_smi *smi = ds->priv;
@@ -1510,6 +1594,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
.get_strings = rtl8366_get_strings,
.get_ethtool_stats = rtl8366_get_ethtool_stats,
.get_sset_count = rtl8366_get_sset_count,
+ .port_bridge_join = rtl8366rb_port_bridge_join,
+ .port_bridge_leave = rtl8366rb_port_bridge_leave,
.port_vlan_filtering = rtl8366_vlan_filtering,
.port_vlan_add = rtl8366_vlan_add,
.port_vlan_del = rtl8366_vlan_del,
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next 1/6 v6] net: dsa: rtl8366rb: Support bridge offloading
2021-09-25 13:23 ` [PATCH net-next 1/6 v6] net: dsa: rtl8366rb: Support bridge offloading Linus Walleij
@ 2021-09-25 18:30 ` Vladimir Oltean
0 siblings, 0 replies; 19+ messages in thread
From: Vladimir Oltean @ 2021-09-25 18:30 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, DENG Qingfang, Mauri Sandberg,
Alvin Šipraga
On Sat, Sep 25, 2021 at 03:23:06PM +0200, Linus Walleij wrote:
> From: DENG Qingfang <dqfext@gmail.com>
>
> Use port isolation registers to configure bridge offloading.
>
> Tested on the D-Link DIR-685, switching between ports and
> sniffing ports to make sure no packets leak.
>
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 2/6 v6] net: dsa: rtl8366: Drop custom VLAN set-up
2021-09-25 13:23 [PATCH net-next 0/6 v6] RTL8366(RB) cleanups part 1 Linus Walleij
2021-09-25 13:23 ` [PATCH net-next 1/6 v6] net: dsa: rtl8366rb: Support bridge offloading Linus Walleij
@ 2021-09-25 13:23 ` Linus Walleij
2021-09-25 13:23 ` [PATCH net-next 3/6 v6] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement Linus Walleij
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 13:23 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Linus Walleij, Mauri Sandberg, DENG Qingfang, Alvin Šipraga
This hacky default VLAN setup was done in order to direct
packets to the right ports and provide port isolation, both
which we now support properly using custom tags and proper
bridge port isolation.
We can drop the custom VLAN code and leave all VLAN handling
alone, as users expect things to be. We can also drop
ds->configure_vlan_while_not_filtering = false; and let
the core deal with any VLANs it wants.
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v5->v6:
- No changes just resending with the rest of the
patches.
ChangeLog v4->v5:
- No changes just resending with the rest of the
patches.
ChangeLog v3->v4:
- No changes
ChangeLog v2->v3:
- Collect a bunch of reviewed-by tags
ChangeLog v1->v2:
- No changes.
---
drivers/net/dsa/realtek-smi-core.h | 1 -
drivers/net/dsa/rtl8366.c | 48 ------------------------------
drivers/net/dsa/rtl8366rb.c | 4 +--
3 files changed, 1 insertion(+), 52 deletions(-)
diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
index fcf465f7f922..c8fbd7b9fd0b 100644
--- a/drivers/net/dsa/realtek-smi-core.h
+++ b/drivers/net/dsa/realtek-smi-core.h
@@ -129,7 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port,
int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable);
int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable);
int rtl8366_reset_vlan(struct realtek_smi *smi);
-int rtl8366_init_vlan(struct realtek_smi *smi);
int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
struct netlink_ext_ack *extack);
int rtl8366_vlan_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 75897a369096..59c5bc4f7b71 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -292,54 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
}
EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
-int rtl8366_init_vlan(struct realtek_smi *smi)
-{
- int port;
- int ret;
-
- ret = rtl8366_reset_vlan(smi);
- if (ret)
- return ret;
-
- /* Loop over the available ports, for each port, associate
- * it with the VLAN (port+1)
- */
- for (port = 0; port < smi->num_ports; port++) {
- u32 mask;
-
- if (port == smi->cpu_port)
- /* For the CPU port, make all ports members of this
- * VLAN.
- */
- mask = GENMASK((int)smi->num_ports - 1, 0);
- else
- /* For all other ports, enable itself plus the
- * CPU port.
- */
- mask = BIT(port) | BIT(smi->cpu_port);
-
- /* For each port, set the port as member of VLAN (port+1)
- * and untagged, except for the CPU port: the CPU port (5) is
- * member of VLAN 6 and so are ALL the other ports as well.
- * Use filter 0 (no filter).
- */
- dev_info(smi->dev, "VLAN%d port mask for port %d, %08x\n",
- (port + 1), port, mask);
- ret = rtl8366_set_vlan(smi, (port + 1), mask, mask, 0);
- if (ret)
- return ret;
-
- dev_info(smi->dev, "VLAN%d port %d, PVID set to %d\n",
- (port + 1), port, (port + 1));
- ret = rtl8366_set_pvid(smi, port, (port + 1));
- if (ret)
- return ret;
- }
-
- return rtl8366_enable_vlan(smi, true);
-}
-EXPORT_SYMBOL_GPL(rtl8366_init_vlan);
-
int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
struct netlink_ext_ack *extack)
{
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index b930050cfd1b..a5b7d7ff8884 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -985,7 +985,7 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
return ret;
}
- ret = rtl8366_init_vlan(smi);
+ ret = rtl8366_reset_vlan(smi);
if (ret)
return ret;
@@ -999,8 +999,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
return -ENODEV;
}
- ds->configure_vlan_while_not_filtering = false;
-
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH net-next 3/6 v6] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement
2021-09-25 13:23 [PATCH net-next 0/6 v6] RTL8366(RB) cleanups part 1 Linus Walleij
2021-09-25 13:23 ` [PATCH net-next 1/6 v6] net: dsa: rtl8366rb: Support bridge offloading Linus Walleij
2021-09-25 13:23 ` [PATCH net-next 2/6 v6] net: dsa: rtl8366: Drop custom VLAN set-up Linus Walleij
@ 2021-09-25 13:23 ` Linus Walleij
2021-09-25 17:12 ` Alvin Šipraga
2021-09-25 13:23 ` [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug Linus Walleij
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 13:23 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Linus Walleij, Mauri Sandberg, Alvin Šipraga, DENG Qingfang
While we were defining one VLAN per port for isolating the ports
the port_vlan_filtering() callback was implemented to enable a
VLAN on the port + 1. This function makes no sense, not only is
it incomplete as it only enables the VLAN, it doesn't do what
the callback is supposed to do, which is to selectively enable
and disable filtering on a certain port.
Implement the correct callback: we have two registers dealing
with filtering on the RTL9366RB, so we implement an ASIC-specific
callback and implement filering using the register bit that makes
the switch drop frames if the port is not in the VLAN member set.
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v5->v6:
- Drop unused leftover variable "ret"
ChangeLog v4->v5:
- Drop the code dropping frames without VID, after Florian
described that this is not expected semantics for this
callback.
ChangeLog v1->v4:
- New patch after discovering that this weirdness of mine is
causing problems.
---
drivers/net/dsa/realtek-smi-core.h | 2 --
drivers/net/dsa/rtl8366.c | 35 ------------------------------
drivers/net/dsa/rtl8366rb.c | 30 ++++++++++++++++++++-----
3 files changed, 24 insertions(+), 43 deletions(-)
diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
index c8fbd7b9fd0b..214f710d7dd5 100644
--- a/drivers/net/dsa/realtek-smi-core.h
+++ b/drivers/net/dsa/realtek-smi-core.h
@@ -129,8 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port,
int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable);
int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable);
int rtl8366_reset_vlan(struct realtek_smi *smi);
-int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
- struct netlink_ext_ack *extack);
int rtl8366_vlan_add(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan,
struct netlink_ext_ack *extack);
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 59c5bc4f7b71..0672dd56c698 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -292,41 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
}
EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
-int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
- struct netlink_ext_ack *extack)
-{
- struct realtek_smi *smi = ds->priv;
- struct rtl8366_vlan_4k vlan4k;
- int ret;
-
- /* Use VLAN nr port + 1 since VLAN0 is not valid */
- if (!smi->ops->is_vlan_valid(smi, port + 1))
- return -EINVAL;
-
- dev_info(smi->dev, "%s filtering on port %d\n",
- vlan_filtering ? "enable" : "disable",
- port);
-
- /* TODO:
- * The hardware support filter ID (FID) 0..7, I have no clue how to
- * support this in the driver when the callback only says on/off.
- */
- ret = smi->ops->get_vlan_4k(smi, port + 1, &vlan4k);
- if (ret)
- return ret;
-
- /* Just set the filter to FID 1 for now then */
- ret = rtl8366_set_vlan(smi, port + 1,
- vlan4k.member,
- vlan4k.untag,
- 1);
- if (ret)
- return ret;
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(rtl8366_vlan_filtering);
-
int rtl8366_vlan_add(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan,
struct netlink_ext_ack *extack)
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index a5b7d7ff8884..2c66a0c2ee50 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -143,6 +143,8 @@
#define RTL8366RB_PHY_NO_OFFSET 9
#define RTL8366RB_PHY_NO_MASK (0x1f << 9)
+/* VLAN Ingress Control Register */
+#define RTL8366RB_VLAN_INGRESS_CTRL1_REG 0x037E
#define RTL8366RB_VLAN_INGRESS_CTRL2_REG 0x037f
/* LED control registers */
@@ -933,11 +935,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
if (ret)
return ret;
- /* Discard VLAN tagged packets if the port is not a member of
- * the VLAN with which the packets is associated.
- */
+ /* Accept all packets by default, we enable filtering on-demand */
+ ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
+ 0);
+ if (ret)
+ return ret;
ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
- RTL8366RB_PORT_ALL);
+ 0);
if (ret)
return ret;
@@ -1209,6 +1213,20 @@ rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
}
+static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port,
+ bool vlan_filtering,
+ struct netlink_ext_ack *extack)
+{
+ struct realtek_smi *smi = ds->priv;
+
+ dev_dbg(smi->dev, "port %d: %s VLAN filtering\n", port,
+ vlan_filtering ? "enable" : "disable");
+
+ /* If the port is not in the member set, the frame will be dropped */
+ return regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
+ BIT(port), vlan_filtering ? BIT(port) : 0);
+}
+
static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
{
struct realtek_smi *smi = ds->priv;
@@ -1437,7 +1455,7 @@ static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
if (smi->vlan4k_enabled)
max = RTL8366RB_NUM_VIDS - 1;
- if (vlan == 0 || vlan > max)
+ if (vlan > max)
return false;
return true;
@@ -1594,7 +1612,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
.get_sset_count = rtl8366_get_sset_count,
.port_bridge_join = rtl8366rb_port_bridge_join,
.port_bridge_leave = rtl8366rb_port_bridge_leave,
- .port_vlan_filtering = rtl8366_vlan_filtering,
+ .port_vlan_filtering = rtl8366rb_vlan_filtering,
.port_vlan_add = rtl8366_vlan_add,
.port_vlan_del = rtl8366_vlan_del,
.port_enable = rtl8366rb_port_enable,
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next 3/6 v6] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement
2021-09-25 13:23 ` [PATCH net-next 3/6 v6] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement Linus Walleij
@ 2021-09-25 17:12 ` Alvin Šipraga
2021-09-25 18:38 ` Vladimir Oltean
0 siblings, 1 reply; 19+ messages in thread
From: Alvin Šipraga @ 2021-09-25 17:12 UTC (permalink / raw)
To: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S . Miller, Jakub Kicinski
Cc: netdev, Mauri Sandberg, DENG Qingfang
On 9/25/21 3:23 PM, Linus Walleij wrote:
> While we were defining one VLAN per port for isolating the ports
> the port_vlan_filtering() callback was implemented to enable a
> VLAN on the port + 1. This function makes no sense, not only is
> it incomplete as it only enables the VLAN, it doesn't do what
> the callback is supposed to do, which is to selectively enable
> and disable filtering on a certain port.
>
> Implement the correct callback: we have two registers dealing
> with filtering on the RTL9366RB, so we implement an ASIC-specific
> callback and implement filering using the register bit that makes
> the switch drop frames if the port is not in the VLAN member set.
>
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v5->v6:
> - Drop unused leftover variable "ret"
> ChangeLog v4->v5:
> - Drop the code dropping frames without VID, after Florian
> described that this is not expected semantics for this
> callback.
> ChangeLog v1->v4:
> - New patch after discovering that this weirdness of mine is
> causing problems.
> ---
> drivers/net/dsa/realtek-smi-core.h | 2 --
> drivers/net/dsa/rtl8366.c | 35 ------------------------------
> drivers/net/dsa/rtl8366rb.c | 30 ++++++++++++++++++++-----
> 3 files changed, 24 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
> index c8fbd7b9fd0b..214f710d7dd5 100644
> --- a/drivers/net/dsa/realtek-smi-core.h
> +++ b/drivers/net/dsa/realtek-smi-core.h
> @@ -129,8 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port,
> int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable);
> int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable);
> int rtl8366_reset_vlan(struct realtek_smi *smi);
> -int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
> - struct netlink_ext_ack *extack);
> int rtl8366_vlan_add(struct dsa_switch *ds, int port,
> const struct switchdev_obj_port_vlan *vlan,
> struct netlink_ext_ack *extack);
> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> index 59c5bc4f7b71..0672dd56c698 100644
> --- a/drivers/net/dsa/rtl8366.c
> +++ b/drivers/net/dsa/rtl8366.c
> @@ -292,41 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
> }
> EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
>
> -int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
> - struct netlink_ext_ack *extack)
> -{
> - struct realtek_smi *smi = ds->priv;
> - struct rtl8366_vlan_4k vlan4k;
> - int ret;
> -
> - /* Use VLAN nr port + 1 since VLAN0 is not valid */
> - if (!smi->ops->is_vlan_valid(smi, port + 1))
> - return -EINVAL;
> -
> - dev_info(smi->dev, "%s filtering on port %d\n",
> - vlan_filtering ? "enable" : "disable",
> - port);
> -
> - /* TODO:
> - * The hardware support filter ID (FID) 0..7, I have no clue how to
> - * support this in the driver when the callback only says on/off.
> - */
> - ret = smi->ops->get_vlan_4k(smi, port + 1, &vlan4k);
> - if (ret)
> - return ret;
> -
> - /* Just set the filter to FID 1 for now then */
> - ret = rtl8366_set_vlan(smi, port + 1,
> - vlan4k.member,
> - vlan4k.untag,
> - 1);
> - if (ret)
> - return ret;
> -
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(rtl8366_vlan_filtering);
> -
> int rtl8366_vlan_add(struct dsa_switch *ds, int port,
> const struct switchdev_obj_port_vlan *vlan,
> struct netlink_ext_ack *extack)
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index a5b7d7ff8884..2c66a0c2ee50 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -143,6 +143,8 @@
> #define RTL8366RB_PHY_NO_OFFSET 9
> #define RTL8366RB_PHY_NO_MASK (0x1f << 9)
>
> +/* VLAN Ingress Control Register */
> +#define RTL8366RB_VLAN_INGRESS_CTRL1_REG 0x037E
> #define RTL8366RB_VLAN_INGRESS_CTRL2_REG 0x037f
>
> /* LED control registers */
> @@ -933,11 +935,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
> if (ret)
> return ret;
>
> - /* Discard VLAN tagged packets if the port is not a member of
> - * the VLAN with which the packets is associated.
> - */
> + /* Accept all packets by default, we enable filtering on-demand */
> + ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
> + 0);
From the previous version of this patch I understood that CTRL1_REG is
for controlling whether or not to accept untagged frames, and CTRL2 for
accepting tagged frames that don't match the port member set. Is that
correct?
Do you know whether DSA _always_ wants ports to accept untagged frames,
or only if the port has PVID set? I'm also asking for my own
understanding. In the latter case I think you might have to set CTRL1 in
rtl8366rb_vlan_filtering() (depending on whether the port has a PVID),
as well as whenever a PVID is set or unset. Of course it depends on the
switch semantics - maybe it ignores CTRL1(port) == 1 if the port has a
PVID, in which case your previous version of the patch would be OK.
> + if (ret)
> + return ret;
> ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
> - RTL8366RB_PORT_ALL);
> + 0);
> if (ret)
> return ret;
>
> @@ -1209,6 +1213,20 @@ rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
> RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
> }
>
> +static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port,
> + bool vlan_filtering,
> + struct netlink_ext_ack *extack)
> +{
> + struct realtek_smi *smi = ds->priv;
> +
> + dev_dbg(smi->dev, "port %d: %s VLAN filtering\n", port,
> + vlan_filtering ? "enable" : "disable");
> +
> + /* If the port is not in the member set, the frame will be dropped */
> + return regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
> + BIT(port), vlan_filtering ? BIT(port) : 0);
> +}
> +
> static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> {
> struct realtek_smi *smi = ds->priv;
> @@ -1437,7 +1455,7 @@ static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
> if (smi->vlan4k_enabled)
> max = RTL8366RB_NUM_VIDS - 1;
>
> - if (vlan == 0 || vlan > max)
> + if (vlan > max)
> return false;
>
> return true;
> @@ -1594,7 +1612,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
> .get_sset_count = rtl8366_get_sset_count,
> .port_bridge_join = rtl8366rb_port_bridge_join,
> .port_bridge_leave = rtl8366rb_port_bridge_leave,
> - .port_vlan_filtering = rtl8366_vlan_filtering,
> + .port_vlan_filtering = rtl8366rb_vlan_filtering,
> .port_vlan_add = rtl8366_vlan_add,
> .port_vlan_del = rtl8366_vlan_del,
> .port_enable = rtl8366rb_port_enable,
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next 3/6 v6] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement
2021-09-25 17:12 ` Alvin Šipraga
@ 2021-09-25 18:38 ` Vladimir Oltean
2021-09-25 22:44 ` Linus Walleij
0 siblings, 1 reply; 19+ messages in thread
From: Vladimir Oltean @ 2021-09-25 18:38 UTC (permalink / raw)
To: Alvin Šipraga
Cc: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli,
David S . Miller, Jakub Kicinski, netdev, Mauri Sandberg,
DENG Qingfang
On Sat, Sep 25, 2021 at 05:12:24PM +0000, Alvin Šipraga wrote:
> From the previous version of this patch I understood that CTRL1_REG is
> for controlling whether or not to accept untagged frames, and CTRL2 for
> accepting tagged frames that don't match the port member set. Is that
> correct?
It might not hurt to add comments which explain what these registers do.
> Do you know whether DSA _always_ wants ports to accept untagged frames,
> or only if the port has PVID set? I'm also asking for my own
> understanding. In the latter case I think you might have to set CTRL1 in
> rtl8366rb_vlan_filtering() (depending on whether the port has a PVID),
> as well as whenever a PVID is set or unset. Of course it depends on the
> switch semantics - maybe it ignores CTRL1(port) == 1 if the port has a
> PVID, in which case your previous version of the patch would be OK.
Documentation/networking/switchdev.rst says:
When the bridge has VLAN filtering enabled and a PVID is not configured on the
ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge
has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
priority-tagged packets must be accepted and forwarded according to the
bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
disabled, the presence/lack of a PVID should not influence the packet
forwarding decision.
Anyway, I suppose Linus can make that adjustment after the fact too, if
everything else is ok in the other patches.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/6 v6] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement
2021-09-25 18:38 ` Vladimir Oltean
@ 2021-09-25 22:44 ` Linus Walleij
0 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 22:44 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Alvin Šipraga, Andrew Lunn, Vivien Didelot,
Florian Fainelli, David S . Miller, Jakub Kicinski, netdev,
Mauri Sandberg, DENG Qingfang
On Sat, Sep 25, 2021 at 8:38 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> Documentation/networking/switchdev.rst says:
Aha now I get this in context, thanks a lot!
> Anyway, I suppose Linus can make that adjustment after the fact too, if
> everything else is ok in the other patches.
I better get this right and explain it to myself while I still have it
in my head, I'm sending a new version of this patch (the whole
set) that fixes it according to spec (I hope).
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug
2021-09-25 13:23 [PATCH net-next 0/6 v6] RTL8366(RB) cleanups part 1 Linus Walleij
` (2 preceding siblings ...)
2021-09-25 13:23 ` [PATCH net-next 3/6 v6] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement Linus Walleij
@ 2021-09-25 13:23 ` Linus Walleij
2021-09-25 17:16 ` Alvin Šipraga
` (2 more replies)
2021-09-25 13:23 ` [PATCH net-next 5/6 v6] net: dsa: rtl8366: Fix a bug in deleting VLANs Linus Walleij
2021-09-25 13:23 ` [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints Linus Walleij
5 siblings, 3 replies; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 13:23 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Linus Walleij, Mauri Sandberg, Alvin Šipraga, DENG Qingfang
The max VLAN number with non-4K VLAN activated is 15, and the
range is 0..15. Not 16.
The impact should be low since we by default have 4K VLAN and
thus have 4095 VLANs to play with in this switch. There will
not be a problem unless the code is rewritten to only use
16 VLANs.
Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v5->v6:
- No changes just resending with the rest of the
patches.
ChangeLog v4->v5:
- Add some more text describing that this is not a critical bug.
- Add Fixes tag
ChangeLog v1->v4:
- New patch for a bug discovered when fixing the other issues.
---
drivers/net/dsa/rtl8366rb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 2c66a0c2ee50..6f25ee57069d 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -1450,7 +1450,7 @@ static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)
static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
{
- unsigned int max = RTL8366RB_NUM_VLANS;
+ unsigned int max = RTL8366RB_NUM_VLANS - 1;
if (smi->vlan4k_enabled)
max = RTL8366RB_NUM_VIDS - 1;
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug
2021-09-25 13:23 ` [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug Linus Walleij
@ 2021-09-25 17:16 ` Alvin Šipraga
2021-09-25 18:44 ` Vladimir Oltean
2021-09-26 2:32 ` Florian Fainelli
2 siblings, 0 replies; 19+ messages in thread
From: Alvin Šipraga @ 2021-09-25 17:16 UTC (permalink / raw)
To: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S . Miller, Jakub Kicinski
Cc: netdev, Mauri Sandberg, DENG Qingfang
On 9/25/21 3:23 PM, Linus Walleij wrote:
> The max VLAN number with non-4K VLAN activated is 15, and the
> range is 0..15. Not 16.
>
> The impact should be low since we by default have 4K VLAN and
> thus have 4095 VLANs to play with in this switch. There will
> not be a problem unless the code is rewritten to only use
> 16 VLANs.
>
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ChangeLog v5->v6:
> - No changes just resending with the rest of the
> patches.
> ChangeLog v4->v5:
> - Add some more text describing that this is not a critical bug.
> - Add Fixes tag
> ChangeLog v1->v4:
> - New patch for a bug discovered when fixing the other issues.
> ---
> drivers/net/dsa/rtl8366rb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 2c66a0c2ee50..6f25ee57069d 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -1450,7 +1450,7 @@ static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)
>
> static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
> {
> - unsigned int max = RTL8366RB_NUM_VLANS;
> + unsigned int max = RTL8366RB_NUM_VLANS - 1;
>
> if (smi->vlan4k_enabled)
> max = RTL8366RB_NUM_VIDS - 1;
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug
2021-09-25 13:23 ` [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug Linus Walleij
2021-09-25 17:16 ` Alvin Šipraga
@ 2021-09-25 18:44 ` Vladimir Oltean
2021-09-26 2:32 ` Florian Fainelli
2 siblings, 0 replies; 19+ messages in thread
From: Vladimir Oltean @ 2021-09-25 18:44 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, Mauri Sandberg, Alvin Šipraga,
DENG Qingfang
On Sat, Sep 25, 2021 at 03:23:09PM +0200, Linus Walleij wrote:
> The max VLAN number with non-4K VLAN activated is 15, and the
> range is 0..15. Not 16.
>
> The impact should be low since we by default have 4K VLAN and
> thus have 4095 VLANs to play with in this switch. There will
> not be a problem unless the code is rewritten to only use
> 16 VLANs.
>
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v5->v6:
> - No changes just resending with the rest of the
> patches.
> ChangeLog v4->v5:
> - Add some more text describing that this is not a critical bug.
> - Add Fixes tag
> ChangeLog v1->v4:
> - New patch for a bug discovered when fixing the other issues.
> ---
> drivers/net/dsa/rtl8366rb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 2c66a0c2ee50..6f25ee57069d 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -1450,7 +1450,7 @@ static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)
>
> static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
> {
> - unsigned int max = RTL8366RB_NUM_VLANS;
> + unsigned int max = RTL8366RB_NUM_VLANS - 1;
>
> if (smi->vlan4k_enabled)
> max = RTL8366RB_NUM_VIDS - 1;
Personally I would have preferred to make this "max" = RTL8366RB_NUM_VIDS,
and then the comparison right below:
if (vlan >= max)
return false;
because it's easier than to carry around "- 1" everywhere.
Anyway, this works just as well.
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug
2021-09-25 13:23 ` [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug Linus Walleij
2021-09-25 17:16 ` Alvin Šipraga
2021-09-25 18:44 ` Vladimir Oltean
@ 2021-09-26 2:32 ` Florian Fainelli
2 siblings, 0 replies; 19+ messages in thread
From: Florian Fainelli @ 2021-09-26 2:32 UTC (permalink / raw)
To: Linus Walleij, Andrew Lunn, Vivien Didelot, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Mauri Sandberg, Alvin Šipraga, DENG Qingfang
On 9/25/2021 6:23 AM, Linus Walleij wrote:
> The max VLAN number with non-4K VLAN activated is 15, and the
> range is 0..15. Not 16.
>
> The impact should be low since we by default have 4K VLAN and
> thus have 4095 VLANs to play with in this switch. There will
> not be a problem unless the code is rewritten to only use
> 16 VLANs.
>
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 5/6 v6] net: dsa: rtl8366: Fix a bug in deleting VLANs
2021-09-25 13:23 [PATCH net-next 0/6 v6] RTL8366(RB) cleanups part 1 Linus Walleij
` (3 preceding siblings ...)
2021-09-25 13:23 ` [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug Linus Walleij
@ 2021-09-25 13:23 ` Linus Walleij
2021-09-25 17:18 ` Alvin Šipraga
2021-09-25 18:50 ` Vladimir Oltean
2021-09-25 13:23 ` [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints Linus Walleij
5 siblings, 2 replies; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 13:23 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Linus Walleij, Mauri Sandberg, Alvin Šipraga, DENG Qingfang
We were checking that the MC (member config) was != 0
for some reason, all we need to check is that the config
has no ports, i.e. no members. Then it can be recycled.
This must be some misunderstanding.
Fixes: 4ddcaf1ebb5e ("net: dsa: rtl8366: Properly clear member config")
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v5->v6:
- No changes just resending with the rest of the
patches.
ChangeLog v4->v5:
- Collect Florians review tag
- Add Fixes tag
ChangeLog v1->v4:
- New patch for a bug found while fixing the other issues.
---
drivers/net/dsa/rtl8366.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 0672dd56c698..f815cd16ad48 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -374,7 +374,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
* anymore then clear the whole member
* config so it can be reused.
*/
- if (!vlanmc.member && vlanmc.untag) {
+ if (!vlanmc.member) {
vlanmc.vid = 0;
vlanmc.priority = 0;
vlanmc.fid = 0;
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next 5/6 v6] net: dsa: rtl8366: Fix a bug in deleting VLANs
2021-09-25 13:23 ` [PATCH net-next 5/6 v6] net: dsa: rtl8366: Fix a bug in deleting VLANs Linus Walleij
@ 2021-09-25 17:18 ` Alvin Šipraga
2021-09-25 18:50 ` Vladimir Oltean
1 sibling, 0 replies; 19+ messages in thread
From: Alvin Šipraga @ 2021-09-25 17:18 UTC (permalink / raw)
To: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S . Miller, Jakub Kicinski
Cc: netdev, Mauri Sandberg, DENG Qingfang
On 9/25/21 3:23 PM, Linus Walleij wrote:
> We were checking that the MC (member config) was != 0
Minor nitpick: actually we were checking the untag mask != 0, right?
The change itself seems fine though:
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> for some reason, all we need to check is that the config
> has no ports, i.e. no members. Then it can be recycled.
> This must be some misunderstanding.
>
> Fixes: 4ddcaf1ebb5e ("net: dsa: rtl8366: Properly clear member config")
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v5->v6:
> - No changes just resending with the rest of the
> patches.
> ChangeLog v4->v5:
> - Collect Florians review tag
> - Add Fixes tag
> ChangeLog v1->v4:
> - New patch for a bug found while fixing the other issues.
> ---
> drivers/net/dsa/rtl8366.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> index 0672dd56c698..f815cd16ad48 100644
> --- a/drivers/net/dsa/rtl8366.c
> +++ b/drivers/net/dsa/rtl8366.c
> @@ -374,7 +374,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
> * anymore then clear the whole member
> * config so it can be reused.
> */
> - if (!vlanmc.member && vlanmc.untag) {
> + if (!vlanmc.member) {
> vlanmc.vid = 0;
> vlanmc.priority = 0;
> vlanmc.fid = 0;
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next 5/6 v6] net: dsa: rtl8366: Fix a bug in deleting VLANs
2021-09-25 13:23 ` [PATCH net-next 5/6 v6] net: dsa: rtl8366: Fix a bug in deleting VLANs Linus Walleij
2021-09-25 17:18 ` Alvin Šipraga
@ 2021-09-25 18:50 ` Vladimir Oltean
1 sibling, 0 replies; 19+ messages in thread
From: Vladimir Oltean @ 2021-09-25 18:50 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, Mauri Sandberg, Alvin Šipraga,
DENG Qingfang
On Sat, Sep 25, 2021 at 03:23:10PM +0200, Linus Walleij wrote:
> We were checking that the MC (member config) was != 0
> for some reason, all we need to check is that the config
> has no ports, i.e. no members. Then it can be recycled.
> This must be some misunderstanding.
>
> Fixes: 4ddcaf1ebb5e ("net: dsa: rtl8366: Properly clear member config")
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
but bug fixes should go to the "net" tree and from there towards "stable".
At the very least they should be the first patches in a series.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints
2021-09-25 13:23 [PATCH net-next 0/6 v6] RTL8366(RB) cleanups part 1 Linus Walleij
` (4 preceding siblings ...)
2021-09-25 13:23 ` [PATCH net-next 5/6 v6] net: dsa: rtl8366: Fix a bug in deleting VLANs Linus Walleij
@ 2021-09-25 13:23 ` Linus Walleij
2021-09-25 17:19 ` Alvin Šipraga
2021-09-25 18:56 ` Vladimir Oltean
5 siblings, 2 replies; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 13:23 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
David S . Miller, Jakub Kicinski
Cc: netdev, Linus Walleij, Mauri Sandberg, Alvin Šipraga, DENG Qingfang
We don't need a message for every VLAN association, dbg
is fine. The message about adding the DSA or CPU
port to a VLAN is directly misleading, this is perfectly
fine.
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: DENG Qingfang <dqfext@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v5->v6:
- No changes just resending with the rest of the
patches.
ChangeLog v4->v5:
- Collect Florians review tag.
ChangeLog v1->v4:
- New patch to deal with confusing messages and too talkative
DSA bridge.
---
drivers/net/dsa/rtl8366.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index f815cd16ad48..bb6189aedcd4 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -318,12 +318,9 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
return ret;
}
- dev_info(smi->dev, "add VLAN %d on port %d, %s, %s\n",
- vlan->vid, port, untagged ? "untagged" : "tagged",
- pvid ? " PVID" : "no PVID");
-
- if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
- dev_err(smi->dev, "port is DSA or CPU port\n");
+ dev_dbg(smi->dev, "add VLAN %d on port %d, %s, %s\n",
+ vlan->vid, port, untagged ? "untagged" : "tagged",
+ pvid ? " PVID" : "no PVID");
member |= BIT(port);
@@ -356,7 +353,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
struct realtek_smi *smi = ds->priv;
int ret, i;
- dev_info(smi->dev, "del VLAN %04x on port %d\n", vlan->vid, port);
+ dev_dbg(smi->dev, "del VLAN %d on port %d\n", vlan->vid, port);
for (i = 0; i < smi->num_vlan_mc; i++) {
struct rtl8366_vlan_mc vlanmc;
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints
2021-09-25 13:23 ` [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints Linus Walleij
@ 2021-09-25 17:19 ` Alvin Šipraga
2021-09-25 18:56 ` Vladimir Oltean
1 sibling, 0 replies; 19+ messages in thread
From: Alvin Šipraga @ 2021-09-25 17:19 UTC (permalink / raw)
To: Linus Walleij, Andrew Lunn, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S . Miller, Jakub Kicinski
Cc: netdev, Mauri Sandberg, DENG Qingfang
On 9/25/21 3:23 PM, Linus Walleij wrote:
> We don't need a message for every VLAN association, dbg
> is fine. The message about adding the DSA or CPU
> port to a VLAN is directly misleading, this is perfectly
> fine.
>
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ChangeLog v5->v6:
> - No changes just resending with the rest of the
> patches.
> ChangeLog v4->v5:
> - Collect Florians review tag.
> ChangeLog v1->v4:
> - New patch to deal with confusing messages and too talkative
> DSA bridge.
> ---
> drivers/net/dsa/rtl8366.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> index f815cd16ad48..bb6189aedcd4 100644
> --- a/drivers/net/dsa/rtl8366.c
> +++ b/drivers/net/dsa/rtl8366.c
> @@ -318,12 +318,9 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
> return ret;
> }
>
> - dev_info(smi->dev, "add VLAN %d on port %d, %s, %s\n",
> - vlan->vid, port, untagged ? "untagged" : "tagged",
> - pvid ? " PVID" : "no PVID");
> -
> - if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> - dev_err(smi->dev, "port is DSA or CPU port\n");
> + dev_dbg(smi->dev, "add VLAN %d on port %d, %s, %s\n",
> + vlan->vid, port, untagged ? "untagged" : "tagged",
> + pvid ? " PVID" : "no PVID");
s/" PVID"/"PVID"/
>
> member |= BIT(port);
>
> @@ -356,7 +353,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
> struct realtek_smi *smi = ds->priv;
> int ret, i;
>
> - dev_info(smi->dev, "del VLAN %04x on port %d\n", vlan->vid, port);
> + dev_dbg(smi->dev, "del VLAN %d on port %d\n", vlan->vid, port);
>
> for (i = 0; i < smi->num_vlan_mc; i++) {
> struct rtl8366_vlan_mc vlanmc;
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints
2021-09-25 13:23 ` [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints Linus Walleij
2021-09-25 17:19 ` Alvin Šipraga
@ 2021-09-25 18:56 ` Vladimir Oltean
2021-09-25 22:54 ` Linus Walleij
1 sibling, 1 reply; 19+ messages in thread
From: Vladimir Oltean @ 2021-09-25 18:56 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, Mauri Sandberg, Alvin Šipraga,
DENG Qingfang
On Sat, Sep 25, 2021 at 03:23:11PM +0200, Linus Walleij wrote:
> We don't need a message for every VLAN association, dbg
> is fine. The message about adding the DSA or CPU
> port to a VLAN is directly misleading, this is perfectly
> fine.
>
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v5->v6:
> - No changes just resending with the rest of the
> patches.
> ChangeLog v4->v5:
> - Collect Florians review tag.
> ChangeLog v1->v4:
> - New patch to deal with confusing messages and too talkative
> DSA bridge.
> ---
> drivers/net/dsa/rtl8366.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> index f815cd16ad48..bb6189aedcd4 100644
> --- a/drivers/net/dsa/rtl8366.c
> +++ b/drivers/net/dsa/rtl8366.c
> @@ -318,12 +318,9 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
> return ret;
> }
>
> - dev_info(smi->dev, "add VLAN %d on port %d, %s, %s\n",
> - vlan->vid, port, untagged ? "untagged" : "tagged",
> - pvid ? " PVID" : "no PVID");
> -
> - if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> - dev_err(smi->dev, "port is DSA or CPU port\n");
> + dev_dbg(smi->dev, "add VLAN %d on port %d, %s, %s\n",
> + vlan->vid, port, untagged ? "untagged" : "tagged",
> + pvid ? " PVID" : "no PVID");
This is better, not going to complain too much, but I mean,
rtl8366_set_vlan and rtl8366_set_pvid already have debugging prints in
them, how can you tolerate so many superfluous prints, what do they
bring useful?
>
> member |= BIT(port);
>
> @@ -356,7 +353,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
> struct realtek_smi *smi = ds->priv;
> int ret, i;
>
> - dev_info(smi->dev, "del VLAN %04x on port %d\n", vlan->vid, port);
> + dev_dbg(smi->dev, "del VLAN %d on port %d\n", vlan->vid, port);
>
> for (i = 0; i < smi->num_vlan_mc; i++) {
> struct rtl8366_vlan_mc vlanmc;
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH net-next 6/6 v6] net: dsa: rtl8366: Drop and depromote pointless prints
2021-09-25 18:56 ` Vladimir Oltean
@ 2021-09-25 22:54 ` Linus Walleij
0 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2021-09-25 22:54 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
Jakub Kicinski, netdev, Mauri Sandberg, Alvin Šipraga,
DENG Qingfang
On Sat, Sep 25, 2021 at 8:56 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > - dev_info(smi->dev, "add VLAN %d on port %d, %s, %s\n",
> > - vlan->vid, port, untagged ? "untagged" : "tagged",
> > - pvid ? " PVID" : "no PVID");
> > -
> > - if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> > - dev_err(smi->dev, "port is DSA or CPU port\n");
> > + dev_dbg(smi->dev, "add VLAN %d on port %d, %s, %s\n",
> > + vlan->vid, port, untagged ? "untagged" : "tagged",
> > + pvid ? " PVID" : "no PVID");
>
> This is better, not going to complain too much, but I mean,
> rtl8366_set_vlan and rtl8366_set_pvid already have debugging prints in
> them, how can you tolerate so many superfluous prints, what do they
> bring useful?
I actually use them ... I suppose one can use ftrace instead
and/or gdb, but I'm one of those die hard printk() debuggers
and I like to follow what happens like that.
When the driver gets mature we can delete most or all of the
dev_dbg() messages I think.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread