Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for bridge_vlan_unwaware

On Tue, 2022-08-02 at 13:59 +0300, Vladimir Oltean wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:

quoted

diff --git a/drivers/net/dsa/microchip/ksz_common.c

b/drivers/net/dsa/microchip/ksz_common.c
index 516fb9d35c87..8a5583b1f2f4 100644

--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c

@@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops =

{
      .vlan_filtering = ksz8_port_vlan_filtering,
      .vlan_add = ksz8_port_vlan_add,
      .vlan_del = ksz8_port_vlan_del,
+     .drop_untagged = ksz8_port_enable_pvid,

You'll have to explain this one. What impact does PVID insertion on
KSZ8
have upon dropping/not dropping untagged packets? This patch is
saying
that when untagged packets should be dropped, PVID insertion should
be
enabled, and when untagged packets should be accepted, PVID insertion
should be disabled. How come?

Its my mistake. I referred KSZ87xx datasheet but I couldn't find the
register for the dropping the untagged packet. If that is the case,
shall I remove the dropping of untagged packet feature from the ksz8
switches?

quoted

      .mirror_add = ksz8_port_mirror_add,
      .mirror_del = ksz8_port_mirror_del,
      .get_caps = ksz8_get_caps,

@@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops

= {
      .vlan_filtering = ksz9477_port_vlan_filtering,
      .vlan_add = ksz9477_port_vlan_add,
      .vlan_del = ksz9477_port_vlan_del,
+     .drop_untagged = ksz9477_port_drop_untagged,
      .mirror_add = ksz9477_port_mirror_add,
      .mirror_del = ksz9477_port_mirror_del,
      .get_caps = ksz9477_get_caps,

@@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops

= {
      .vlan_filtering = ksz9477_port_vlan_filtering,
      .vlan_add = ksz9477_port_vlan_add,
      .vlan_del = ksz9477_port_vlan_del,
+     .drop_untagged = ksz9477_port_drop_untagged,
      .mirror_add = ksz9477_port_mirror_add,
      .mirror_del = ksz9477_port_mirror_del,
      .get_caps = lan937x_phylink_get_caps,

@@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch

*ds, int port,
 {
      struct ksz_device *dev = ds->priv;

+     dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
+                            BRIDGE_VLAN_INFO_UNTAGGED);
+

How many times can this be executed before the VLAN add operation
fails
due to the VLAN already being present on the port? I notice you're
ignoring the return code. Wouldn't it be better to do this at
port_setup() time instead?

(side note, the PVID for standalone mode can be added at port_setup
time. The PVID to use for bridges in VLAN-unaware mode can be
allocated
at port_bridge_join time)

Ok. I will add in port_bridge_join function for bridge vlan_aware
ports.

quoted

      if (!dsa_is_user_port(ds, port))
              return 0;

+static int ksz_commit_pvid(struct dsa_switch *ds, int port)
+{
+     struct dsa_port *dp = dsa_to_port(ds, port);
+     struct net_device *br = dsa_port_bridge_dev_get(dp);
+     struct ksz_device *dev = ds->priv;
+     u16 pvid = KSZ_DEFAULT_VLAN;
+     bool drop_untagged = false;
+     struct ksz_port *p;
+
+     p = &dev->ports[port];
+
+     if (br && br_vlan_enabled(br)) {
+             pvid = p->bridge_pvid.vid;
+             drop_untagged = !p->bridge_pvid.valid;
+     }

This is better in the sense that it resolves the need for the
configure_vlan_while_not_filtering hack. But standalone and VLAN-
unaware
bridge ports still share the same PVID. Even more so, standalone
ports
have address learning enabled, which will poison the address database
of
VLAN-unaware bridge ports (and of other standalone ports):

https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/

Are you going to do further work in this area?

For now, I thought I can fix the issue for bridge vlan unaware port. I
have few other patch series to be submitted like gPTP, tc commands. If
standalone port fix also needed for your patch series I can work on it
otherwise I can take up later stage.

quoted

+
+     ksz_set_pvid(dev, port, pvid);
+
+     if (dev->dev_ops->drop_untagged)
+             dev->dev_ops->drop_untagged(dev, port,
drop_untagged);
+
+     return 0;
+}