diff mbox series

[net-next,1/4,v4] net: dsa: rtl8366rb: Support disabling learning

Message ID 20210929210349.130099-2-linus.walleij@linaro.org
State Superseded
Headers show
Series RTL8366RB enhancements | expand

Commit Message

Linus Walleij Sept. 29, 2021, 9:03 p.m. UTC 
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(-)

-- 
2.31.1

Comments

Vladimir Oltean Sept. 29, 2021, 9:58 p.m. UTC | #1 
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>
Alvin Šipraga Sept. 30, 2021, 10:45 a.m. UTC | #2 
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,

>   };

>
Linus Walleij Oct. 4, 2021, 8:57 p.m. UTC | #3 
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
Alvin Šipraga Oct. 5, 2021, 7:59 a.m. UTC | #4 
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

>
Linus Walleij Oct. 5, 2021, 2:07 p.m. UTC | #5 
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
Alvin Šipraga Oct. 5, 2021, 2:45 p.m. UTC | #6 
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 mbox series

Patch

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,
 };