| Message ID | 20210929210349.130099-2-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | RTL8366RB enhancements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 84 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Wed, Sep 29, 2021 at 11:03:46PM +0200, Linus Walleij wrote: > The RTL8366RB hardware supports disabling learning per-port > so let's make use of this feature. Rename some unfortunately > named registers in the process. > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > Cc: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Mauri Sandberg <sandberg@mailfence.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: DENG Qingfang <dqfext@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Hi Linus, On 9/29/21 11:03 PM, Linus Walleij wrote: > The RTL8366RB hardware supports disabling learning per-port > so let's make use of this feature. Rename some unfortunately > named registers in the process. Since you have implemented bridge offloading and you are now disabling learning on the CPU port by default, will this mean that all ingress frames on a user port with DA behind the CPU port will be flooded by the switch to all ports in the bridge, as well as the CPU port? It seems that will be the case if now the switch can't learn the SA of frames coming from the CPU. Following your discussion with Vladimir [1], did you come to a conclusion on how you will handle this? Alvin [1] https://lore.kernel.org/netdev/20210908210939.cwwnwgj3p67qvsrh@skbuf/ > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > Cc: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Mauri Sandberg <sandberg@mailfence.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: DENG Qingfang <dqfext@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v3->v4: > - No changes, rebased on other patches. > ChangeLog v2->v3: > - Disable learning by default, learning will be turned > on selectively using the callback. > ChangeLog v1->v2: > - New patch suggested by Vladimir. > --- > drivers/net/dsa/rtl8366rb.c | 50 ++++++++++++++++++++++++++++++++----- > 1 file changed, 44 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c > index bb9d017c2f9f..b3056064b937 100644 > --- a/drivers/net/dsa/rtl8366rb.c > +++ b/drivers/net/dsa/rtl8366rb.c > @@ -14,6 +14,7 @@ > > #include <linux/bitops.h> > #include <linux/etherdevice.h> > +#include <linux/if_bridge.h> > #include <linux/interrupt.h> > #include <linux/irqdomain.h> > #include <linux/irqchip/chained_irq.h> > @@ -42,9 +43,12 @@ > /* Port Enable Control register */ > #define RTL8366RB_PECR 0x0001 > > -/* Switch Security Control registers */ > -#define RTL8366RB_SSCR0 0x0002 > -#define RTL8366RB_SSCR1 0x0003 > +/* Switch per-port learning disablement register */ > +#define RTL8366RB_PORT_LEARNDIS_CTRL 0x0002 > + > +/* Security control, actually aging register */ > +#define RTL8366RB_SECURITY_CTRL 0x0003 > + > #define RTL8366RB_SSCR2 0x0004 > #define RTL8366RB_SSCR2_DROP_UNKNOWN_DA BIT(0) > > @@ -927,13 +931,14 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > /* layer 2 size, see rtl8366rb_change_mtu() */ > rb->max_mtu[i] = 1532; > > - /* Enable learning for all ports */ > - ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0); > + /* Disable learning for all ports */ > + ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, > + RTL8366RB_PORT_ALL); > if (ret) > return ret; > > /* Enable auto ageing for all ports */ > - ret = regmap_write(smi->map, RTL8366RB_SSCR1, 0); > + ret = regmap_write(smi->map, RTL8366RB_SECURITY_CTRL, 0); > if (ret) > return ret; > > @@ -1272,6 +1277,37 @@ static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port, > return ret; > } > > +static int > +rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port, > + struct switchdev_brport_flags flags, > + struct netlink_ext_ack *extack) > +{ > + /* We support enabling/disabling learning */ > + if (flags.mask & ~(BR_LEARNING)) > + return -EINVAL; > + > + return 0; > +} > + > +static int > +rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port, > + struct switchdev_brport_flags flags, > + struct netlink_ext_ack *extack) > +{ > + struct realtek_smi *smi = ds->priv; > + int ret; > + > + if (flags.mask & BR_LEARNING) { > + ret = regmap_update_bits(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, > + BIT(port), > + (flags.val & BR_LEARNING) ? 0 : BIT(port)); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > { > struct realtek_smi *smi = ds->priv; > @@ -1682,6 +1718,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = { > .port_vlan_del = rtl8366_vlan_del, > .port_enable = rtl8366rb_port_enable, > .port_disable = rtl8366rb_port_disable, > + .port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags, > + .port_bridge_flags = rtl8366rb_port_bridge_flags, > .port_change_mtu = rtl8366rb_change_mtu, > .port_max_mtu = rtl8366rb_max_mtu, > }; >
On Thu, Sep 30, 2021 at 12:45 PM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: > Following your discussion with Vladimir [1], did you come to a > conclusion on how you will handle this? I haven't gotten around to running the experiments (short on time), so I intended to play it safe for now. Unless I feel I have to. BTW: all the patches i have left are extensions to RTL8366RB specifically so I think it should be fine for you to submit patches for your switch on top of net-next, maybe we can test this on you chip too, I suspect it works the same on all Realtek switches? Yours, Linus Walleij
Hi Linus, On 10/4/21 10:57 PM, Linus Walleij wrote: > On Thu, Sep 30, 2021 at 12:45 PM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: > >> Following your discussion with Vladimir [1], did you come to a >> conclusion on how you will handle this? > > I haven't gotten around to running the experiments (short on > time), so I intended to play it safe for now. Unless I feel I have > to. Yeah I understand, it takes some time to figure out how these switches really behave... :-) You have Vladimir's Reviewed-by: tag so I guess this change is OK from the maintainer's perspective. > > BTW: all the patches i have left are extensions to RTL8366RB > specifically so I think it should be fine for you to submit patches > for your switch on top of net-next, maybe we can test this > on you chip too, I suspect it works the same on all Realtek > switches? Generally speaking I don't think that the patches you have sent for 66RB are particularly relevant for the 65MB because the register layout and some chip semantics are totally different. Regarding CPU port learning for the RTL8365MB chip: right now I am playing around with the "third way" Vladimir suggested, by enabling learning selectively only for bridge-layer packets (skb->offload_fwd_mark == true). To begin with I'm not even sure if you have this capability with the RTL8366RB. Alvin > > Yours, > Linus Walleij >
On Tue, Oct 5, 2021 at 9:59 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: > On 10/4/21 10:57 PM, Linus Walleij wrote: > > BTW: all the patches i have left are extensions to RTL8366RB > > specifically so I think it should be fine for you to submit patches > > for your switch on top of net-next, maybe we can test this > > on you chip too, I suspect it works the same on all Realtek > > switches? > > Generally speaking I don't think that the patches you have sent for 66RB > are particularly relevant for the 65MB because the register layout and > some chip semantics are totally different. I was mainly thinking about the crazy VLAN set-up that didn't use port isolation and which is now deleted. But maybe you were not using the rtl8366.c file either? Just realtek-smi.c? > Regarding CPU port learning > for the RTL8365MB chip: right now I am playing around with the "third > way" Vladimir suggested, by enabling learning selectively only for > bridge-layer packets (skb->offload_fwd_mark == true). To begin with I'm > not even sure if you have this capability with the RTL8366RB. This will be interesting to see! Yours, Linus Walleij
On 10/5/21 4:07 PM, Linus Walleij wrote: > On Tue, Oct 5, 2021 at 9:59 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: >> On 10/4/21 10:57 PM, Linus Walleij wrote: > >>> BTW: all the patches i have left are extensions to RTL8366RB >>> specifically so I think it should be fine for you to submit patches >>> for your switch on top of net-next, maybe we can test this >>> on you chip too, I suspect it works the same on all Realtek >>> switches? >> >> Generally speaking I don't think that the patches you have sent for 66RB >> are particularly relevant for the 65MB because the register layout and >> some chip semantics are totally different. > > I was mainly thinking about the crazy VLAN set-up that didn't use > port isolation and which is now deleted. But maybe you were not > using the rtl8366.c file either? Just realtek-smi.c? Ah, I was just not using those particularly freaky VLAN functions from the rtl8366 library. I still use vlan_{add,del} and the MIB counter helpers though, as these seem to be OK. I have been keeping up-to-date with your changes to rtl8366.c and tested them locally with my subdriver and they are working like a charm. So we should still benefit from some level of code reuse. Alvin > >> Regarding CPU port learning >> for the RTL8365MB chip: right now I am playing around with the "third >> way" Vladimir suggested, by enabling learning selectively only for >> bridge-layer packets (skb->offload_fwd_mark == true). To begin with I'm >> not even sure if you have this capability with the RTL8366RB. > > This will be interesting to see! > > Yours, > Linus Walleij >
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c index bb9d017c2f9f..b3056064b937 100644 --- a/drivers/net/dsa/rtl8366rb.c +++ b/drivers/net/dsa/rtl8366rb.c @@ -14,6 +14,7 @@ #include <linux/bitops.h> #include <linux/etherdevice.h> +#include <linux/if_bridge.h> #include <linux/interrupt.h> #include <linux/irqdomain.h> #include <linux/irqchip/chained_irq.h> @@ -42,9 +43,12 @@ /* Port Enable Control register */ #define RTL8366RB_PECR 0x0001 -/* Switch Security Control registers */ -#define RTL8366RB_SSCR0 0x0002 -#define RTL8366RB_SSCR1 0x0003 +/* Switch per-port learning disablement register */ +#define RTL8366RB_PORT_LEARNDIS_CTRL 0x0002 + +/* Security control, actually aging register */ +#define RTL8366RB_SECURITY_CTRL 0x0003 + #define RTL8366RB_SSCR2 0x0004 #define RTL8366RB_SSCR2_DROP_UNKNOWN_DA BIT(0) @@ -927,13 +931,14 @@ static int rtl8366rb_setup(struct dsa_switch *ds) /* layer 2 size, see rtl8366rb_change_mtu() */ rb->max_mtu[i] = 1532; - /* Enable learning for all ports */ - ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0); + /* Disable learning for all ports */ + ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, + RTL8366RB_PORT_ALL); if (ret) return ret; /* Enable auto ageing for all ports */ - ret = regmap_write(smi->map, RTL8366RB_SSCR1, 0); + ret = regmap_write(smi->map, RTL8366RB_SECURITY_CTRL, 0); if (ret) return ret; @@ -1272,6 +1277,37 @@ static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port, return ret; } +static int +rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port, + struct switchdev_brport_flags flags, + struct netlink_ext_ack *extack) +{ + /* We support enabling/disabling learning */ + if (flags.mask & ~(BR_LEARNING)) + return -EINVAL; + + return 0; +} + +static int +rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port, + struct switchdev_brport_flags flags, + struct netlink_ext_ack *extack) +{ + struct realtek_smi *smi = ds->priv; + int ret; + + if (flags.mask & BR_LEARNING) { + ret = regmap_update_bits(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, + BIT(port), + (flags.val & BR_LEARNING) ? 0 : BIT(port)); + if (ret) + return ret; + } + + return 0; +} + static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu) { struct realtek_smi *smi = ds->priv; @@ -1682,6 +1718,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = { .port_vlan_del = rtl8366_vlan_del, .port_enable = rtl8366rb_port_enable, .port_disable = rtl8366rb_port_disable, + .port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags, + .port_bridge_flags = rtl8366rb_port_bridge_flags, .port_change_mtu = rtl8366rb_change_mtu, .port_max_mtu = rtl8366rb_max_mtu, };
The RTL8366RB hardware supports disabling learning per-port so let's make use of this feature. Rename some unfortunately named registers in the process. Suggested-by: Vladimir Oltean <olteanv@gmail.com> Cc: Alvin Šipraga <alsi@bang-olufsen.dk> Cc: Mauri Sandberg <sandberg@mailfence.com> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: DENG Qingfang <dqfext@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v3->v4: - No changes, rebased on other patches. ChangeLog v2->v3: - Disable learning by default, learning will be turned on selectively using the callback. ChangeLog v1->v2: - New patch suggested by Vladimir. --- drivers/net/dsa/rtl8366rb.c | 50 ++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 6 deletions(-)