Re: [RFC net-next] net: dsa: rtl8366rb: support bridge offloading

[Linus Walleij <linus.walleij@linaro.org>]

On Wed, Feb 24, 2021 at 7:12 AM DENG Qingfang <dqfext@gmail.com> wrote:

> Use port isolation registers to configure bridge offloading.
> Remove the VLAN init, as we have proper CPU tag and bridge offloading
> support now.
>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---
> This is not tested, as I don't have a RTL8366RB board. And I think there
> is potential race condition in port_bridge_{join,leave}.

Compilation failed for me like this:

../drivers/net/dsa/rtl8366rb.c:1573:23: error: initialization of 'void
(*)(struct dsa_switch *, int,  struct net_device *)' from incompatible
pointer type 'int (*)(struct dsa_switch *, int,  struct net_device *)'
[-Werror=incompatible-pointer-types]
 1573 |  .port_bridge_leave = rtl8366rb_port_bridge_leave,
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/dsa/rtl8366rb.c:1573:23: note: (near initialization for
'rtl8366rb_switch_ops.port_bridge_leave')

I fixed it like this:

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index e7abf846350d..0719fadadc3d 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -1161,7 +1161,7 @@ rtl8366rb_port_bridge_join(struct dsa_switch
*ds, int port,
                                  0, port_bitmap << 1);
 }

-static int
+static void
 rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
                            struct net_device *bridge)
 {
@@ -1176,14 +1176,17 @@ rtl8366rb_port_bridge_leave(struct dsa_switch
*ds, int port,
                        continue;
                ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
                                         BIT(port + 1), 0);
-               if (ret)
-                       return ret;
+               if (ret) {
+                       dev_err(smi->dev, "failed to leave port %d
from bridge\n",
+                               port);
+                       return;
+               }

                port_bitmap |= BIT(i);
        }

-       return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
-                                 port_bitmap << 1, 0);
+       regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
+                          port_bitmap << 1, 0);
 }

After this it works like a charm.

> -       ret = rtl8366_init_vlan(smi);
> -       if (ret)
> -               return ret;

I suppose we can delete that confused VLAN set-up after this.

> -       ds->configure_vlan_while_not_filtering = false;

This is default true not IIUC so we should be good!

With my minor changes:
Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij