Re: [PATCH net-next 1/5 v2] net: dsa: rtl8366rb: support bridge offloading
flat view
From: Florian Fainelli <f.fainelli@gmail.com>
Date: 2021-08-31 01:01:33
On 8/30/2021 2:48 PM, Linus Walleij wrote:
quoted hunk ↗ jump to hunk
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: Alvin Šipraga <redacted> Cc: Mauri Sandberg <redacted> Signed-off-by: DENG Qingfang <dqfext@gmail.com> Signed-off-by: Linus Walleij <redacted> --- 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 | 87 +++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c index a89093bc6c6a..50ee7cd62484 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,22 @@ static int rtl8366rb_setup(struct dsa_switch *ds) if (ret) return ret; + /* Isolate all user ports so only the CPU port can access them */ + for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) { + ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i), + RTL8366RB_PORT_ISO_EN | + RTL8366RB_PORT_ISO_PORTS(BIT(RTL8366RB_PORT_NUM_CPU))); + if (ret) + return ret; + } + /* CPU port can access all ports */ + dev_info(smi->dev, "DSA user port mask: %08x\n", dsa_user_ports(ds));
As mentioned by Alvin, this should be a dev_dbg() or completely eliminated.
quoted hunk ↗ jump to hunk
+ 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 +1150,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 this one */
This sentence is a bit weird, how about: Loop over all ports but this one?
+ for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+ /* Handled last */This comment is a bit misleading as it would suggest that the loop does act on 'port' when really it does that outside of the loop. With those nitpicks fixed: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian