* [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading @ 2021-08-29 0:26 Linus Walleij 2021-08-29 0:26 ` [PATCH net-next 2/2] net: dsa: rtl8366: Drop custom VLAN set-up Linus Walleij 2021-08-30 8:12 ` [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading Vladimir Oltean 0 siblings, 2 replies; 7+ messages in thread From: Linus Walleij @ 2021-08-29 0:26 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S . Miller, Jakub Kicinski Cc: netdev, DENG Qingfang, Alvin Šipraga, Mauri Sandberg, 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: 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> --- drivers/net/dsa/rtl8366rb.c | 84 +++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c index a89093bc6c6a..14939188c108 100644 --- a/drivers/net/dsa/rtl8366rb.c +++ b/drivers/net/dsa/rtl8366rb.c @@ -300,6 +300,12 @@ #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) + /* bits 0..5 enable force when cleared */ #define RTL8366RB_MAC_FORCE_CTRL_REG 0x0F11 @@ -835,6 +841,21 @@ 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 | + BIT(RTL8366RB_PORT_NUM_CPU + 1)); + if (ret) + return ret; + } + /* CPU port can access all ports */ + ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU), + RTL8366RB_PORT_ISO_PORTS_MASK | + 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 +1148,67 @@ 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), + BIT(port + 1), BIT(port + 1)); + 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, + port_bitmap << 1); +} + +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), + BIT(port + 1), 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), + port_bitmap << 1, 0); +} + static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu) { struct realtek_smi *smi = ds->priv; @@ -1510,6 +1592,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] 7+ messages in thread
* [PATCH net-next 2/2] net: dsa: rtl8366: Drop custom VLAN set-up 2021-08-29 0:26 [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading Linus Walleij @ 2021-08-29 0:26 ` Linus Walleij 2021-08-30 7:33 ` Vladimir Oltean 2021-08-30 8:12 ` [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading Vladimir Oltean 1 sibling, 1 reply; 7+ messages in thread From: Linus Walleij @ 2021-08-29 0:26 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean, David S . Miller, Jakub Kicinski Cc: netdev, Linus Walleij, Alvin Šipraga, Mauri Sandberg, DENG Qingfang 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: Vladimir Oltean <olteanv@gmail.com> Cc: Alvin Šipraga <alsi@bang-olufsen.dk> Cc: Mauri Sandberg <sandberg@mailfence.com> Cc: DENG Qingfang <dqfext@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- 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 14939188c108..17bf8f3ecc7d 100644 --- a/drivers/net/dsa/rtl8366rb.c +++ b/drivers/net/dsa/rtl8366rb.c @@ -984,7 +984,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; @@ -998,8 +998,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] 7+ messages in thread
* Re: [PATCH net-next 2/2] net: dsa: rtl8366: Drop custom VLAN set-up 2021-08-29 0:26 ` [PATCH net-next 2/2] net: dsa: rtl8366: Drop custom VLAN set-up Linus Walleij @ 2021-08-30 7:33 ` Vladimir Oltean 0 siblings, 0 replies; 7+ messages in thread From: Vladimir Oltean @ 2021-08-30 7:33 UTC (permalink / raw) To: Linus Walleij Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, netdev, Alvin Šipraga, Mauri Sandberg, DENG Qingfang On Sun, Aug 29, 2021 at 02:26:01AM +0200, Linus Walleij wrote: > 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: Vladimir Oltean <olteanv@gmail.com> > Cc: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Mauri Sandberg <sandberg@mailfence.com> > Cc: 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] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading 2021-08-29 0:26 [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading Linus Walleij 2021-08-29 0:26 ` [PATCH net-next 2/2] net: dsa: rtl8366: Drop custom VLAN set-up Linus Walleij @ 2021-08-30 8:12 ` Vladimir Oltean 2021-08-30 21:22 ` Linus Walleij 1 sibling, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2021-08-30 8:12 UTC (permalink / raw) To: Linus Walleij Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, netdev, DENG Qingfang, Alvin Šipraga, Mauri Sandberg On Sun, Aug 29, 2021 at 02:26:00AM +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> > --- > drivers/net/dsa/rtl8366rb.c | 84 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > > diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c > index a89093bc6c6a..14939188c108 100644 > --- a/drivers/net/dsa/rtl8366rb.c > +++ b/drivers/net/dsa/rtl8366rb.c > @@ -300,6 +300,12 @@ > #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) If RTL8366RB_NUM_PORTS is 6, then why is RTL8366RB_PORT_ISO_PORTS_MASK a 7-bit field? > + > /* bits 0..5 enable force when cleared */ > #define RTL8366RB_MAC_FORCE_CTRL_REG 0x0F11 > > @@ -835,6 +841,21 @@ 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 | > + BIT(RTL8366RB_PORT_NUM_CPU + 1)); The shifting due to RTL8366RB_PORT_ISO_EN looks weird, I can see it being mishandled in the future, with code moved around, copied and pasted between realtek drivers and such. How about making a macro #define RTL8366RB_PORT_ISO_PORTS(x) ((x) << 1) > + if (ret) > + return ret; > + } > + /* CPU port can access all ports */ Except itself maybe? RTL8366RB_PORT_NUM_CPU is 5, so maybe use something like RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds)) > + ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU), > + RTL8366RB_PORT_ISO_PORTS_MASK | > + 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 +1148,67 @@ 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), > + BIT(port + 1), BIT(port + 1)); > + 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, > + port_bitmap << 1); > +} > + > +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), > + BIT(port + 1), 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), > + port_bitmap << 1, 0); > +} > + > static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > { > struct realtek_smi *smi = ds->priv; > @@ -1510,6 +1592,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 > Looks okay for the most part. It is to be expected for a new driver that introduces bridging offload to also handle .port_pre_bridge_flags, .port_bridge_flags and .port_fast_age, for two reasons: (a) it is expected that a port which does not offload the bridge, and performs forwarding in software, to not perform address learning in hardware (b) it is expected that the addresses learned while the port was under a bridge are not carried over into its life as a standalone port, when it leaves that bridge Also, it would be nice if you could do some minimal isolation at the level of the FDB lookup. Currently, if I am not mistaken, a port will perform FDB lookup even if it is standalone, and it might find an FDB entry for a given {MAC DA, VLAN ID} pair that belongs to a port outside of its isolation mask, so forwarding will be blocked and that packet will be dropped (instead of the expected behavior which is for that packet to be forwarded to the CPU). Normally the expectation is that this FDB-level isolation can be achieved by configuring the VLANs of one bridge to use a filter ID that is different from the VLANs of another bridge, and the port-based default VLAN of standalone ports to use yet another filter ID. This is yet another reason to disable learning on standalone ports, so that their filter ID never contains any FDB entry, and packets are always flooded to their only possible destination, the CPU port. Currently in DSA we do not offer a streamlined way for you to determine what filter ID to use for a certain VLAN belonging to a certain bridge, but at the very least you can test FDB isolation between standalone ports and bridged ports. The simplest way to do that, assuming you already have a forwarding setup with 2 switch ports swp0 and swp1, is to enable CONFIG_BONDING=y, and then: ip link add br0 type bridge ip link set bond0 master br0 ip link set swp1 master bond0 ip link set swp0 master br0 Then ping between station A attached to swp0 and station B attached to swp1. Because swp1 cannot offload bond0, it will fall back to software forwarding and act as standalone, i.e. what you had up till now. With hardware address learning enabled on swp0 (a port that offloads br0), it will learn station A's source MAC address. Then when swp1 needs to send a packet to station A's destination MAC address, it would be tempted to look up the FDB, find that address, and forward to swp0. But swp0 is isolated from swp1. If you use a filter ID for standalone ports and another filter ID for bridged ports you will avoid that problem, and you will also lay the groundwork for the full FDB isolation even between bridges that will be coming during the next development cycle. If you feel that the second part is too much for now, you can just add the extra callbacks for address learning and flushing (although I do have some genuine concerns about how reliable was the software forwarding with this driver, seeing that right now it enables hardware learning unconditionally). Is there something that isolates FDB lookups already? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading 2021-08-30 8:12 ` [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading Vladimir Oltean @ 2021-08-30 21:22 ` Linus Walleij 2021-08-30 22:01 ` Vladimir Oltean 0 siblings, 1 reply; 7+ messages in thread From: Linus Walleij @ 2021-08-30 21:22 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, netdev, DENG Qingfang, Alvin Šipraga, Mauri Sandberg On Mon, Aug 30, 2021 at 10:12 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > +/* 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) > > If RTL8366RB_NUM_PORTS is 6, then why is RTL8366RB_PORT_ISO_PORTS_MASK a > 7-bit field? It's a 6 bit field actually from bit 1 to bit 7 just shifted up one bit because bit 0 is "enable". > > + /* 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 | > > + BIT(RTL8366RB_PORT_NUM_CPU + 1)); > > The shifting due to RTL8366RB_PORT_ISO_EN looks weird, I can see it > being mishandled in the future, with code moved around, copied and > pasted between realtek drivers and such. How about making a macro > > #define RTL8366RB_PORT_ISO_PORTS(x) ((x) << 1) OK > > + /* CPU port can access all ports */ > > Except itself maybe? RTL8366RB_PORT_NUM_CPU is 5, so maybe use something > like > > RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds)) Tested this and it appears to work just fine! > Looks okay for the most part. It is to be expected for a new driver that > introduces bridging offload to also handle .port_pre_bridge_flags, > .port_bridge_flags and .port_fast_age, for two reasons: > (a) it is expected that a port which does not offload the bridge, and > performs forwarding in software, to not perform address learning in > hardware > (b) it is expected that the addresses learned while the port was under a > bridge are not carried over into its life as a standalone port, when > it leaves that bridge I studied the vendor code drop and register file and implemented the BR_LEARNING flag, and I also managed to implement fast aging. Each as a separate patch. Thanks for pointing this out! > Also, it would be nice if you could do some minimal isolation at the > level of the FDB lookup. Currently, if I am not mistaken, a port will > perform FDB lookup even if it is standalone, and it might find an FDB > entry for a given {MAC DA, VLAN ID} pair that belongs to a port outside > of its isolation mask, so forwarding will be blocked and that packet > will be dropped (instead of the expected behavior which is for that > packet to be forwarded to the CPU). > > Normally the expectation is that this FDB-level isolation can be achieved > by configuring the VLANs of one bridge to use a filter ID that is > different from the VLANs of another bridge, and the port-based default > VLAN of standalone ports to use yet another filter ID. This is yet > another reason to disable learning on standalone ports, so that their > filter ID never contains any FDB entry, and packets are always flooded > to their only possible destination, the CPU port. > > Currently in DSA we do not offer a streamlined way for you to determine > what filter ID to use for a certain VLAN belonging to a certain bridge, > but at the very least you can test FDB isolation between standalone > ports and bridged ports. The simplest way to do that, assuming you > already have a forwarding setup with 2 switch ports swp0 and swp1, is to > enable CONFIG_BONDING=y, and then: > > ip link add br0 type bridge > ip link set bond0 master br0 > ip link set swp1 master bond0 > ip link set swp0 master br0 > > Then ping between station A attached to swp0 and station B attached to > swp1. > > Because swp1 cannot offload bond0, it will fall back to software > forwarding and act as standalone, i.e. what you had up till now. > With hardware address learning enabled on swp0 (a port that offloads > br0), it will learn station A's source MAC address. Then when swp1 needs > to send a packet to station A's destination MAC address, it would be > tempted to look up the FDB, find that address, and forward to swp0. But > swp0 is isolated from swp1. If you use a filter ID for standalone ports > and another filter ID for bridged ports you will avoid that problem, and > you will also lay the groundwork for the full FDB isolation even between > bridges that will be coming during the next development cycle. > > If you feel that the second part is too much for now, you can just add > the extra callbacks for address learning and flushing (although I do > have some genuine concerns about how reliable was the software forwarding > with this driver, seeing that right now it enables hardware learning > unconditionally). Is there something that isolates FDB lookups already? Ugh that was massive, I'm not that smart ;) I kinda understand it but have no idea how to achieve this with the current hardware, driver and vendor code mess. I prefer to fix the first part for now. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading 2021-08-30 21:22 ` Linus Walleij @ 2021-08-30 22:01 ` Vladimir Oltean 2021-08-30 22:06 ` Linus Walleij 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2021-08-30 22:01 UTC (permalink / raw) To: Linus Walleij Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, netdev, DENG Qingfang, Alvin Šipraga, Mauri Sandberg On Mon, Aug 30, 2021 at 11:22:11PM +0200, Linus Walleij wrote: > On Mon, Aug 30, 2021 at 10:12 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > +/* 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) > > > > If RTL8366RB_NUM_PORTS is 6, then why is RTL8366RB_PORT_ISO_PORTS_MASK a > > 7-bit field? > > It's a 6 bit field actually from bit 1 to bit 7 just shifted up one > bit because bit 0 is "enable". Understood the part about bit 0 being "ENABLE". But from bit 1 to bit 7, I count 7 bits set.... > > Also, it would be nice if you could do some minimal isolation at the > > level of the FDB lookup. Currently, if I am not mistaken, a port will > > perform FDB lookup even if it is standalone, and it might find an FDB > > entry for a given {MAC DA, VLAN ID} pair that belongs to a port outside > > of its isolation mask, so forwarding will be blocked and that packet > > will be dropped (instead of the expected behavior which is for that > > packet to be forwarded to the CPU). > > > > Normally the expectation is that this FDB-level isolation can be achieved > > by configuring the VLANs of one bridge to use a filter ID that is > > different from the VLANs of another bridge, and the port-based default > > VLAN of standalone ports to use yet another filter ID. This is yet > > another reason to disable learning on standalone ports, so that their > > filter ID never contains any FDB entry, and packets are always flooded > > to their only possible destination, the CPU port. > > > > Currently in DSA we do not offer a streamlined way for you to determine > > what filter ID to use for a certain VLAN belonging to a certain bridge, > > but at the very least you can test FDB isolation between standalone > > ports and bridged ports. The simplest way to do that, assuming you > > already have a forwarding setup with 2 switch ports swp0 and swp1, is to > > enable CONFIG_BONDING=y, and then: > > > > ip link add br0 type bridge > > ip link set bond0 master br0 > > ip link set swp1 master bond0 > > ip link set swp0 master br0 > > > > Then ping between station A attached to swp0 and station B attached to > > swp1. > > > > Because swp1 cannot offload bond0, it will fall back to software > > forwarding and act as standalone, i.e. what you had up till now. > > With hardware address learning enabled on swp0 (a port that offloads > > br0), it will learn station A's source MAC address. Then when swp1 needs > > to send a packet to station A's destination MAC address, it would be > > tempted to look up the FDB, find that address, and forward to swp0. But > > swp0 is isolated from swp1. If you use a filter ID for standalone ports > > and another filter ID for bridged ports you will avoid that problem, and > > you will also lay the groundwork for the full FDB isolation even between > > bridges that will be coming during the next development cycle. > > > > If you feel that the second part is too much for now, you can just add > > the extra callbacks for address learning and flushing (although I do > > have some genuine concerns about how reliable was the software forwarding > > with this driver, seeing that right now it enables hardware learning > > unconditionally). Is there something that isolates FDB lookups already? > > Ugh that was massive, I'm not that smart ;) > > I kinda understand it but have no idea how to achieve this with > the current hardware, driver and vendor code mess. > > I prefer to fix the first part for now. Okay, no problem, I suppose FDB isolation can be revisited as part of the larger rework I've got planned for the next kernel. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading 2021-08-30 22:01 ` Vladimir Oltean @ 2021-08-30 22:06 ` Linus Walleij 0 siblings, 0 replies; 7+ messages in thread From: Linus Walleij @ 2021-08-30 22:06 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Jakub Kicinski, netdev, DENG Qingfang, Alvin Šipraga, Mauri Sandberg On Tue, Aug 31, 2021 at 12:01 AM Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Aug 30, 2021 at 11:22:11PM +0200, Linus Walleij wrote: > > On Mon, Aug 30, 2021 at 10:12 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > > > +/* 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) > > > > > > If RTL8366RB_NUM_PORTS is 6, then why is RTL8366RB_PORT_ISO_PORTS_MASK a > > > 7-bit field? > > > > It's a 6 bit field actually from bit 1 to bit 7 just shifted up one > > bit because bit 0 is "enable". > > Understood the part about bit 0 being "ENABLE". > But from bit 1 to bit 7, I count 7 bits set.... Oh yeah.... something is wrong with my arithmetics. Bit 0: enable Bit 1: port 0 Bit 2: port 1 Bit 3: port 2 Bit 4: port 3 Bit 5: port 4 Bit 6: port 5 - CPU I'll fix with the rest of the comments for v3. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-30 22:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-29 0:26 [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading Linus Walleij 2021-08-29 0:26 ` [PATCH net-next 2/2] net: dsa: rtl8366: Drop custom VLAN set-up Linus Walleij 2021-08-30 7:33 ` Vladimir Oltean 2021-08-30 8:12 ` [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading Vladimir Oltean 2021-08-30 21:22 ` Linus Walleij 2021-08-30 22:01 ` Vladimir Oltean 2021-08-30 22:06 ` Linus Walleij
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.