| Message ID | 20210830214859.403100-2-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RTL8366RB improvements | expand |
On Mon, Aug 30, 2021 at 11:48:55PM +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: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Mauri Sandberg <sandberg@mailfence.com> > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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) Would be nice to enclose pmask between a set of parentheses. > + > /* 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)); > + ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU), > + RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds))| For beauty you can add the missing space here between ) and |. > + 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 */ > + for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) { > + /* 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) > + return ret; > + > + 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_MASK, > + 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++) { > + /* 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) > + return; I don't think it is beneficial here to catch the error and return early? We don't even have a print, it is a rather silent failure. I think if a function that returns void fails, we should limp on and finish... unbridging... the... port... > + > + port_bitmap |= BIT(i); > + } > + > + /* Clear the bits for the ports we can access */ > + 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 +1595,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 >
On 8/30/21 11:48 PM, 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: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Mauri Sandberg <sandberg@mailfence.com> > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > 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)); Maybe dev_dbg? Not all people appreciate chatty drivers... > + 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 */ > + for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) { > + /* 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) > + return ret; > + > + 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_MASK, > + 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++) { > + /* 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) > + return; > + > + port_bitmap |= BIT(i); > + } > + > + /* Clear the bits for the ports we can access */ > + 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 +1595,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, >
On 8/30/2021 2:48 PM, 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: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Mauri Sandberg <sandberg@mailfence.com> > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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. > + 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
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)); + 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 */ + for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) { + /* 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) + return ret; + + 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_MASK, + 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++) { + /* 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) + return; + + port_bitmap |= BIT(i); + } + + /* Clear the bits for the ports we can access */ + 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 +1595,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,