[PATCH net-next v1 0/9] ar9331: mainline some parts of switch functionality

[PATCH net-next v1 0/9] ar9331: mainline some parts of switch functionality

[Oleksij Rempel]

Till now the ar9331 switch was supporting only port multiplexing mode.
With this patch set we should be able to bridging, VLAN and STP

Oleksij Rempel (9):
  net: dsa: add rcv_post call back
  net: dsa: tag_ar9331: detect IGMP and MLD packets
  net: dsa: qca: ar9331: reorder MDIO write sequence
  net: dsa: qca: ar9331: make proper initial port defaults
  net: dsa: qca: ar9331: add forwarding database support
  net: dsa: qca: ar9331: add ageing time support
  net: dsa: qca: ar9331: add bridge support
  net: dsa: qca: ar9331: add STP support
  net: dsa: qca: ar9331: add vlan support

 drivers/net/dsa/qca/ar9331.c | 927 ++++++++++++++++++++++++++++++++++-
 include/net/dsa.h            |   2 +
 net/dsa/dsa.c                |   4 +
 net/dsa/port.c               |   1 +
 net/dsa/tag_ar9331.c         | 130 +++++
 5 files changed, 1059 insertions(+), 5 deletions(-)

-- 
2.29.2

[PATCH net-next v1 1/9] net: dsa: add rcv_post call back

[Oleksij Rempel]

Some switches (for example ar9331) do not provide enough information
about forwarded packets. If the switch decision was made based on IPv4
or IPv6 header, we need to analyze it and set proper flag.

Potentially we can do it in existing rcv path, on other hand we can
avoid part of duplicated work and let the dsa framework set skb header
pointers and then use preprocessed skb one step later withing the rcv_post
call back.

This patch is needed for ar9331 switch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/net/dsa.h | 2 ++
 net/dsa/dsa.c     | 4 ++++
 net/dsa/port.c    | 1 +
 3 files changed, 7 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 57b2c49f72f4..f1a7aa4303a7 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -84,6 +84,7 @@ struct dsa_device_ops {
 	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
 	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev,
 			       struct packet_type *pt);
+	void (*rcv_post)(struct sk_buff *skb);
 	void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
 			     int *offset);
 	/* Used to determine which traffic should match the DSA filter in
@@ -247,6 +248,7 @@ struct dsa_port {
 	struct dsa_switch_tree *dst;
 	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev,
 			       struct packet_type *pt);
+	void (*rcv_post)(struct sk_buff *skb);
 	bool (*filter)(const struct sk_buff *skb, struct net_device *dev);
 
 	enum {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 84cad1be9ce4..fa3e7201e760 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -249,6 +249,10 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, skb->dev);
 
+
+	if (cpu_dp->rcv_post)
+		cpu_dp->rcv_post(skb);
+
 	if (unlikely(!dsa_slave_dev_check(skb->dev))) {
 		/* Packet is to be injected directly on an upper
 		 * device, e.g. a team/bond, so skip all DSA-port
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 01e30264b25b..859957688c62 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -720,6 +720,7 @@ void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp,
 {
 	cpu_dp->filter = tag_ops->filter;
 	cpu_dp->rcv = tag_ops->rcv;
+	cpu_dp->rcv_post = tag_ops->rcv_post;
 	cpu_dp->tag_ops = tag_ops;
 }
 
-- 
2.29.2

Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote:
> Some switches (for example ar9331) do not provide enough information
> about forwarded packets. If the switch decision was made based on IPv4
> or IPv6 header, we need to analyze it and set proper flag.
> 
> Potentially we can do it in existing rcv path, on other hand we can
> avoid part of duplicated work and let the dsa framework set skb header
> pointers and then use preprocessed skb one step later withing the rcv_post
> call back.
> 
> This patch is needed for ar9331 switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

I don't necessarily disagree with this, perhaps we can even move
Florian's dsa_untag_bridge_pvid() call inside a rcv_post() method
implemented by the DSA_TAG_PROTO_BRCM_LEGACY, DSA_TAG_PROTO_BRCM_PREPEND
and DSA_TAG_PROTO_BRCM taggers. Or even better, because Oleksij's
rcv_post is already prototype-compatible with dsa_untag_bridge_pvid, we
can already do:

	.rcv_post = dsa_untag_bridge_pvid,

This should be generally useful for stuff that DSA taggers need to do
which is easiest done after eth_type_trans() was called.

Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 05:05:34PM +0300, Vladimir Oltean wrote:
> On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote:
> > Some switches (for example ar9331) do not provide enough information
> > about forwarded packets. If the switch decision was made based on IPv4
> > or IPv6 header, we need to analyze it and set proper flag.
> > 
> > Potentially we can do it in existing rcv path, on other hand we can
> > avoid part of duplicated work and let the dsa framework set skb header
> > pointers and then use preprocessed skb one step later withing the rcv_post
> > call back.
> > 
> > This patch is needed for ar9331 switch.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> 
> I don't necessarily disagree with this, perhaps we can even move
> Florian's dsa_untag_bridge_pvid() call inside a rcv_post() method
> implemented by the DSA_TAG_PROTO_BRCM_LEGACY, DSA_TAG_PROTO_BRCM_PREPEND
> and DSA_TAG_PROTO_BRCM taggers. Or even better, because Oleksij's
> rcv_post is already prototype-compatible with dsa_untag_bridge_pvid, we
> can already do:
> 
> 	.rcv_post = dsa_untag_bridge_pvid,
> 
> This should be generally useful for stuff that DSA taggers need to do
> which is easiest done after eth_type_trans() was called.

I had some fun with an alternative method of parsing the frame for IGMP
so that you can clear skb->offload_fwd_mark, which doesn't rely on the
introduction of a new method in DSA. It should also have several other
advantages compared to your solution such as the fact that it should
work with VLAN-tagged packets.

Background: we made Receive Packet Steering work on DSA master interfaces
(echo 3 > /sys/class/net/eth0/queues/rx-1/rps_cpus) even when the DSA
tag shifts to the right the IP headers and everything that comes
afterwards. The flow dissector had to be patched for that, just grep for
DSA in net/core/flow_dissector.c.

The problem you're facing is that you can't parse the IP and IGMP
headers in the tagger's rcv() method, since the network header,
transport header offsets and skb->protocol are all messed up, since
eth_type_trans hasn't been called yet.

And that's the trick right there, you're between a rock and a hard
place: too early because eth_type_trans wasn't called yet, and too late
because skb->dev was changed and no longer points to the DSA master, so
the flow dissector adjustment we made doesn't apply.

But if you call the flow dissector _before_ you call "skb->dev =
dsa_master_find_slave" (and yes, while the DSA tag is still there), then
it's virtually as if you had called that while the skb belonged to the
DSA master, so it should work with __skb_flow_dissect.

In fact I prototyped this idea below. I wanted to check whether I can
match something as fine-grained as an IGMPv2 Membership Report message,
and I could.

I prototyped it inside the ocelot tagging protocol driver because that's
what I had handy. I used __skb_flow_dissect with my own flow dissector
which had to be initialized at the tagger module_init time, even though
I think I could have probably just called skb_flow_dissect_flow_keys
with a standard dissector, and that would have removed the need for the
custom module_init in tag_ocelot.c. One thing that is interesting is
that I had to add the bits for IGMP parsing to the flow dissector
myself (based on the existing ICMP code). I was too lazy to do that for
MLD as well, but it is really not hard. Or even better, if you don't
need to look at all inside the IGMP/MLD header, I think you can even
omit adding this parsing code to the flow dissector and just look at
basic.n_proto and basic.ip_proto.

See the snippet below. Hope it helps.

-----------------------------[ cut here ]-----------------------------
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index ffd386ea0dbb..4c25fa47637a 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -190,6 +190,20 @@ struct flow_dissector_key_icmp {
 	u16 id;
 };
 
+/**
+ * flow_dissector_key_igmp:
+ *		type: indicates the message type, see include/uapi/linux/igmp.h
+ *		code: Max Resp Code, the maximum time in 1/10 second
+ *		      increments before sending a responding report
+ *		group: the multicast address being queried when sending a
+ *		       Group-Specific or Group-and-Source-Specific Query.
+ */
+struct flow_dissector_key_igmp {
+	u8 type;
+	u8 code; /* Max Resp Time in IGMPv2 */
+	__be32 group;
+};
+
 /**
  * struct flow_dissector_key_eth_addrs:
  * @src: source Ethernet address
@@ -259,6 +273,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
 	FLOW_DISSECTOR_KEY_PORTS_RANGE, /* struct flow_dissector_key_ports */
 	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
+	FLOW_DISSECTOR_KEY_IGMP, /* struct flow_dissector_key_igmp */
 	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
 	FLOW_DISSECTOR_KEY_TIPC, /* struct flow_dissector_key_tipc */
 	FLOW_DISSECTOR_KEY_ARP, /* struct flow_dissector_key_arp */
@@ -314,6 +329,7 @@ struct flow_keys {
 	struct flow_dissector_key_keyid keyid;
 	struct flow_dissector_key_ports ports;
 	struct flow_dissector_key_icmp icmp;
+	struct flow_dissector_key_igmp igmp;
 	/* 'addrs' must be the last member */
 	struct flow_dissector_key_addrs addrs;
 };
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 5985029e43d4..8cc8c34ea5cd 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -202,6 +202,30 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb,
 	skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen);
 }
 
+static void __skb_flow_dissect_igmp(const struct sk_buff *skb,
+				    struct flow_dissector *flow_dissector,
+				    void *target_container, const void *data,
+				    int thoff, int hlen)
+{
+	struct flow_dissector_key_igmp *key_igmp;
+	struct igmphdr *ih, _ih;
+
+	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IGMP))
+		return;
+
+	ih = __skb_header_pointer(skb, thoff, sizeof(_ih), data, hlen, &_ih);
+	if (!ih)
+		return;
+
+	key_igmp = skb_flow_dissector_target(flow_dissector,
+					     FLOW_DISSECTOR_KEY_IGMP,
+					     target_container);
+
+	key_igmp->type = ih->type;
+	key_igmp->code = ih->code;
+	key_igmp->group = ih->group;
+}
+
 void skb_flow_dissect_meta(const struct sk_buff *skb,
 			   struct flow_dissector *flow_dissector,
 			   void *target_container)
@@ -1398,6 +1422,11 @@ bool __skb_flow_dissect(const struct net *net,
 					data, nhoff, hlen);
 		break;
 
+	case IPPROTO_IGMP:
+		__skb_flow_dissect_igmp(skb, flow_dissector, target_container,
+					data, nhoff, hlen);
+		break;
+
 	default:
 		break;
 	}
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index f9df9cac81c5..a2cc824ddeec 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -2,9 +2,51 @@
 /* Copyright 2019 NXP Semiconductors
  */
 #include <linux/dsa/ocelot.h>
+#include <linux/igmp.h>
 #include <soc/mscc/ocelot.h>
 #include "dsa_priv.h"
 
+static const struct flow_dissector_key ocelot_flow_keys[] = {
+	{
+		.key_id = FLOW_DISSECTOR_KEY_CONTROL,
+		.offset = offsetof(struct flow_keys, control),
+	},
+	{
+		.key_id = FLOW_DISSECTOR_KEY_BASIC,
+		.offset = offsetof(struct flow_keys, basic),
+	},
+	{
+		.key_id = FLOW_DISSECTOR_KEY_IGMP,
+		.offset = offsetof(struct flow_keys, igmp),
+	},
+};
+
+static struct flow_dissector ocelot_flow_dissector __read_mostly;
+
+static struct sk_buff *ocelot_drop_igmp(struct sk_buff *skb)
+{
+	struct flow_keys fk;
+
+	memset(&fk, 0, sizeof(fk));
+
+	if (!__skb_flow_dissect(NULL, skb, &ocelot_flow_dissector,
+				&fk, NULL, 0, 0, 0, 0))
+		return skb;
+
+	if (fk.basic.n_proto != htons(ETH_P_IP))
+		return skb;
+
+	if (fk.basic.ip_proto != IPPROTO_IGMP)
+		return skb;
+
+	if (fk.igmp.type != IGMPV2_HOST_MEMBERSHIP_REPORT)
+		return skb;
+
+	skb_dump(KERN_ERR, skb, true);
+
+	return NULL;
+}
+
 static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
 			    struct sk_buff *clone)
 {
@@ -84,6 +126,10 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	u8 *extraction;
 	u16 vlan_tpid;
 
+	skb = ocelot_drop_igmp(skb);
+	if (!skb)
+		return NULL;
+
 	/* Revert skb->data by the amount consumed by the DSA master,
 	 * so it points to the beginning of the frame.
 	 */
@@ -186,6 +232,23 @@ static struct dsa_tag_driver *ocelot_tag_driver_array[] = {
 	&DSA_TAG_DRIVER_NAME(seville_netdev_ops),
 };
 
-module_dsa_tag_drivers(ocelot_tag_driver_array);
+static int __init dsa_tag_driver_module_init(void)
+{
+	skb_flow_dissector_init(&ocelot_flow_dissector, ocelot_flow_keys,
+				ARRAY_SIZE(ocelot_flow_keys));
+
+	dsa_tag_drivers_register(ocelot_tag_driver_array,
+				 ARRAY_SIZE(ocelot_tag_driver_array),
+				 THIS_MODULE);
+	return 0;
+}
+module_init(dsa_tag_driver_module_init);
+
+static void __exit dsa_tag_driver_module_exit(void)
+{
+	dsa_tag_drivers_unregister(ocelot_tag_driver_array,
+				   ARRAY_SIZE(ocelot_tag_driver_array));
+}
+module_exit(dsa_tag_driver_module_exit)
 
 MODULE_LICENSE("GPL v2");
-----------------------------[ cut here ]-----------------------------

Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back

[Florian Fainelli]

On 4/3/2021 16:21, Vladimir Oltean wrote:
> On Sat, Apr 03, 2021 at 05:05:34PM +0300, Vladimir Oltean wrote:
>> On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote:
>>> Some switches (for example ar9331) do not provide enough information
>>> about forwarded packets. If the switch decision was made based on IPv4
>>> or IPv6 header, we need to analyze it and set proper flag.
>>>
>>> Potentially we can do it in existing rcv path, on other hand we can
>>> avoid part of duplicated work and let the dsa framework set skb header
>>> pointers and then use preprocessed skb one step later withing the rcv_post
>>> call back.
>>>
>>> This patch is needed for ar9331 switch.
>>>
>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>> ---
>>
>> I don't necessarily disagree with this, perhaps we can even move
>> Florian's dsa_untag_bridge_pvid() call inside a rcv_post() method
>> implemented by the DSA_TAG_PROTO_BRCM_LEGACY, DSA_TAG_PROTO_BRCM_PREPEND
>> and DSA_TAG_PROTO_BRCM taggers. Or even better, because Oleksij's
>> rcv_post is already prototype-compatible with dsa_untag_bridge_pvid, we
>> can already do:
>>
>> 	.rcv_post = dsa_untag_bridge_pvid,
>>
>> This should be generally useful for stuff that DSA taggers need to do
>> which is easiest done after eth_type_trans() was called.
> 
> I had some fun with an alternative method of parsing the frame for IGMP
> so that you can clear skb->offload_fwd_mark, which doesn't rely on the
> introduction of a new method in DSA. It should also have several other
> advantages compared to your solution such as the fact that it should
> work with VLAN-tagged packets.
> 
> Background: we made Receive Packet Steering work on DSA master interfaces
> (echo 3 > /sys/class/net/eth0/queues/rx-1/rps_cpus) even when the DSA
> tag shifts to the right the IP headers and everything that comes
> afterwards. The flow dissector had to be patched for that, just grep for
> DSA in net/core/flow_dissector.c.
> 
> The problem you're facing is that you can't parse the IP and IGMP
> headers in the tagger's rcv() method, since the network header,
> transport header offsets and skb->protocol are all messed up, since
> eth_type_trans hasn't been called yet.
> 
> And that's the trick right there, you're between a rock and a hard
> place: too early because eth_type_trans wasn't called yet, and too late
> because skb->dev was changed and no longer points to the DSA master, so
> the flow dissector adjustment we made doesn't apply.
> 
> But if you call the flow dissector _before_ you call "skb->dev =
> dsa_master_find_slave" (and yes, while the DSA tag is still there), then
> it's virtually as if you had called that while the skb belonged to the
> DSA master, so it should work with __skb_flow_dissect.
> 
> In fact I prototyped this idea below. I wanted to check whether I can
> match something as fine-grained as an IGMPv2 Membership Report message,
> and I could.
> 
> I prototyped it inside the ocelot tagging protocol driver because that's
> what I had handy. I used __skb_flow_dissect with my own flow dissector
> which had to be initialized at the tagger module_init time, even though
> I think I could have probably just called skb_flow_dissect_flow_keys
> with a standard dissector, and that would have removed the need for the
> custom module_init in tag_ocelot.c. One thing that is interesting is
> that I had to add the bits for IGMP parsing to the flow dissector
> myself (based on the existing ICMP code). I was too lazy to do that for
> MLD as well, but it is really not hard. Or even better, if you don't
> need to look at all inside the IGMP/MLD header, I think you can even
> omit adding this parsing code to the flow dissector and just look at
> basic.n_proto and basic.ip_proto.
> 
> See the snippet below. Hope it helps.

This looks a lot better than introducing hooks at various points in 
dsa_switch_rcv().
-- 
Florian

Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back

[Oleksij Rempel]

Am 04.04.21 um 01:21 schrieb Vladimir Oltean:
> On Sat, Apr 03, 2021 at 05:05:34PM +0300, Vladimir Oltean wrote:
>> On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote:
>>> Some switches (for example ar9331) do not provide enough information
>>> about forwarded packets. If the switch decision was made based on IPv4
>>> or IPv6 header, we need to analyze it and set proper flag.
>>>
>>> Potentially we can do it in existing rcv path, on other hand we can
>>> avoid part of duplicated work and let the dsa framework set skb header
>>> pointers and then use preprocessed skb one step later withing the rcv_post
>>> call back.
>>>
>>> This patch is needed for ar9331 switch.
>>>
>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>> ---
>>
>> I don't necessarily disagree with this, perhaps we can even move
>> Florian's dsa_untag_bridge_pvid() call inside a rcv_post() method
>> implemented by the DSA_TAG_PROTO_BRCM_LEGACY, DSA_TAG_PROTO_BRCM_PREPEND
>> and DSA_TAG_PROTO_BRCM taggers. Or even better, because Oleksij's
>> rcv_post is already prototype-compatible with dsa_untag_bridge_pvid, we
>> can already do:
>>
>> 	.rcv_post = dsa_untag_bridge_pvid,
>>
>> This should be generally useful for stuff that DSA taggers need to do
>> which is easiest done after eth_type_trans() was called.
>
> I had some fun with an alternative method of parsing the frame for IGMP
> so that you can clear skb->offload_fwd_mark, which doesn't rely on the
> introduction of a new method in DSA. It should also have several other
> advantages compared to your solution such as the fact that it should
> work with VLAN-tagged packets.
>
> Background: we made Receive Packet Steering work on DSA master interfaces
> (echo 3 > /sys/class/net/eth0/queues/rx-1/rps_cpus) even when the DSA
> tag shifts to the right the IP headers and everything that comes
> afterwards. The flow dissector had to be patched for that, just grep for
> DSA in net/core/flow_dissector.c.
>
> The problem you're facing is that you can't parse the IP and IGMP
> headers in the tagger's rcv() method, since the network header,
> transport header offsets and skb->protocol are all messed up, since
> eth_type_trans hasn't been called yet.
>
> And that's the trick right there, you're between a rock and a hard
> place: too early because eth_type_trans wasn't called yet, and too late
> because skb->dev was changed and no longer points to the DSA master, so
> the flow dissector adjustment we made doesn't apply.
>
> But if you call the flow dissector _before_ you call "skb->dev =
> dsa_master_find_slave" (and yes, while the DSA tag is still there), then
> it's virtually as if you had called that while the skb belonged to the
> DSA master, so it should work with __skb_flow_dissect.
>
> In fact I prototyped this idea below. I wanted to check whether I can
> match something as fine-grained as an IGMPv2 Membership Report message,
> and I could.
>
> I prototyped it inside the ocelot tagging protocol driver because that's
> what I had handy. I used __skb_flow_dissect with my own flow dissector
> which had to be initialized at the tagger module_init time, even though
> I think I could have probably just called skb_flow_dissect_flow_keys
> with a standard dissector, and that would have removed the need for the
> custom module_init in tag_ocelot.c. One thing that is interesting is
> that I had to add the bits for IGMP parsing to the flow dissector
> myself (based on the existing ICMP code). I was too lazy to do that for
> MLD as well, but it is really not hard. Or even better, if you don't
> need to look at all inside the IGMP/MLD header, I think you can even
> omit adding this parsing code to the flow dissector and just look at
> basic.n_proto and basic.ip_proto.
>
> See the snippet below. Hope it helps.
>
> -----------------------------[ cut here ]-----------------------------
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index ffd386ea0dbb..4c25fa47637a 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -190,6 +190,20 @@ struct flow_dissector_key_icmp {
>  	u16 id;
>  };
>
> +/**
> + * flow_dissector_key_igmp:
> + *		type: indicates the message type, see include/uapi/linux/igmp.h
> + *		code: Max Resp Code, the maximum time in 1/10 second
> + *		      increments before sending a responding report
> + *		group: the multicast address being queried when sending a
> + *		       Group-Specific or Group-and-Source-Specific Query.
> + */
> +struct flow_dissector_key_igmp {
> +	u8 type;
> +	u8 code; /* Max Resp Time in IGMPv2 */
> +	__be32 group;
> +};
> +
>  /**
>   * struct flow_dissector_key_eth_addrs:
>   * @src: source Ethernet address
> @@ -259,6 +273,7 @@ enum flow_dissector_key_id {
>  	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
>  	FLOW_DISSECTOR_KEY_PORTS_RANGE, /* struct flow_dissector_key_ports */
>  	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
> +	FLOW_DISSECTOR_KEY_IGMP, /* struct flow_dissector_key_igmp */
>  	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
>  	FLOW_DISSECTOR_KEY_TIPC, /* struct flow_dissector_key_tipc */
>  	FLOW_DISSECTOR_KEY_ARP, /* struct flow_dissector_key_arp */
> @@ -314,6 +329,7 @@ struct flow_keys {
>  	struct flow_dissector_key_keyid keyid;
>  	struct flow_dissector_key_ports ports;
>  	struct flow_dissector_key_icmp icmp;
> +	struct flow_dissector_key_igmp igmp;
>  	/* 'addrs' must be the last member */
>  	struct flow_dissector_key_addrs addrs;
>  };
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 5985029e43d4..8cc8c34ea5cd 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -202,6 +202,30 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb,
>  	skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen);
>  }
>
> +static void __skb_flow_dissect_igmp(const struct sk_buff *skb,
> +				    struct flow_dissector *flow_dissector,
> +				    void *target_container, const void *data,
> +				    int thoff, int hlen)
> +{
> +	struct flow_dissector_key_igmp *key_igmp;
> +	struct igmphdr *ih, _ih;
> +
> +	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IGMP))
> +		return;
> +
> +	ih = __skb_header_pointer(skb, thoff, sizeof(_ih), data, hlen, &_ih);
> +	if (!ih)
> +		return;
> +
> +	key_igmp = skb_flow_dissector_target(flow_dissector,
> +					     FLOW_DISSECTOR_KEY_IGMP,
> +					     target_container);
> +
> +	key_igmp->type = ih->type;
> +	key_igmp->code = ih->code;
> +	key_igmp->group = ih->group;
> +}
> +
>  void skb_flow_dissect_meta(const struct sk_buff *skb,
>  			   struct flow_dissector *flow_dissector,
>  			   void *target_container)
> @@ -1398,6 +1422,11 @@ bool __skb_flow_dissect(const struct net *net,
>  					data, nhoff, hlen);
>  		break;
>
> +	case IPPROTO_IGMP:
> +		__skb_flow_dissect_igmp(skb, flow_dissector, target_container,
> +					data, nhoff, hlen);
> +		break;
> +
>  	default:
>  		break;
>  	}
> diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
> index f9df9cac81c5..a2cc824ddeec 100644
> --- a/net/dsa/tag_ocelot.c
> +++ b/net/dsa/tag_ocelot.c
> @@ -2,9 +2,51 @@
>  /* Copyright 2019 NXP Semiconductors
>   */
>  #include <linux/dsa/ocelot.h>
> +#include <linux/igmp.h>
>  #include <soc/mscc/ocelot.h>
>  #include "dsa_priv.h"
>
> +static const struct flow_dissector_key ocelot_flow_keys[] = {
> +	{
> +		.key_id = FLOW_DISSECTOR_KEY_CONTROL,
> +		.offset = offsetof(struct flow_keys, control),
> +	},
> +	{
> +		.key_id = FLOW_DISSECTOR_KEY_BASIC,
> +		.offset = offsetof(struct flow_keys, basic),
> +	},
> +	{
> +		.key_id = FLOW_DISSECTOR_KEY_IGMP,
> +		.offset = offsetof(struct flow_keys, igmp),
> +	},
> +};
> +
> +static struct flow_dissector ocelot_flow_dissector __read_mostly;
> +
> +static struct sk_buff *ocelot_drop_igmp(struct sk_buff *skb)
> +{
> +	struct flow_keys fk;
> +
> +	memset(&fk, 0, sizeof(fk));
> +
> +	if (!__skb_flow_dissect(NULL, skb, &ocelot_flow_dissector,
> +				&fk, NULL, 0, 0, 0, 0))
> +		return skb;
> +
> +	if (fk.basic.n_proto != htons(ETH_P_IP))
> +		return skb;
> +
> +	if (fk.basic.ip_proto != IPPROTO_IGMP)
> +		return skb;
> +
> +	if (fk.igmp.type != IGMPV2_HOST_MEMBERSHIP_REPORT)
> +		return skb;
> +
> +	skb_dump(KERN_ERR, skb, true);
> +
> +	return NULL;
> +}
> +
>  static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
>  			    struct sk_buff *clone)
>  {
> @@ -84,6 +126,10 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
>  	u8 *extraction;
>  	u16 vlan_tpid;
>
> +	skb = ocelot_drop_igmp(skb);
> +	if (!skb)
> +		return NULL;

I probably like this idea, but i need more understanding :)

In case of ocelot_drop_igmp() you wont to really drop it? Or it is just
example on how it may potentially work?
I ask, because IGMP should not be dropped.

--
Regards,
Oleksij

Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back

[Vladimir Oltean]

On Sun, Apr 04, 2021 at 07:49:03AM +0200, Oleksij Rempel wrote:
> Am 04.04.21 um 01:21 schrieb Vladimir Oltean:
> > On Sat, Apr 03, 2021 at 05:05:34PM +0300, Vladimir Oltean wrote:
> >> On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote:
> >>> Some switches (for example ar9331) do not provide enough information
> >>> about forwarded packets. If the switch decision was made based on IPv4
> >>> or IPv6 header, we need to analyze it and set proper flag.
> >>>
> >>> Potentially we can do it in existing rcv path, on other hand we can
> >>> avoid part of duplicated work and let the dsa framework set skb header
> >>> pointers and then use preprocessed skb one step later withing the rcv_post
> >>> call back.
> >>>
> >>> This patch is needed for ar9331 switch.
> >>>
> >>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >>> ---
> >>
> >> I don't necessarily disagree with this, perhaps we can even move
> >> Florian's dsa_untag_bridge_pvid() call inside a rcv_post() method
> >> implemented by the DSA_TAG_PROTO_BRCM_LEGACY, DSA_TAG_PROTO_BRCM_PREPEND
> >> and DSA_TAG_PROTO_BRCM taggers. Or even better, because Oleksij's
> >> rcv_post is already prototype-compatible with dsa_untag_bridge_pvid, we
> >> can already do:
> >>
> >> 	.rcv_post = dsa_untag_bridge_pvid,
> >>
> >> This should be generally useful for stuff that DSA taggers need to do
> >> which is easiest done after eth_type_trans() was called.
> >
> > I had some fun with an alternative method of parsing the frame for IGMP
> > so that you can clear skb->offload_fwd_mark, which doesn't rely on the
> > introduction of a new method in DSA. It should also have several other
> > advantages compared to your solution such as the fact that it should
> > work with VLAN-tagged packets.
> >
> > Background: we made Receive Packet Steering work on DSA master interfaces
> > (echo 3 > /sys/class/net/eth0/queues/rx-1/rps_cpus) even when the DSA
> > tag shifts to the right the IP headers and everything that comes
> > afterwards. The flow dissector had to be patched for that, just grep for
> > DSA in net/core/flow_dissector.c.
> >
> > The problem you're facing is that you can't parse the IP and IGMP
> > headers in the tagger's rcv() method, since the network header,
> > transport header offsets and skb->protocol are all messed up, since
> > eth_type_trans hasn't been called yet.
> >
> > And that's the trick right there, you're between a rock and a hard
> > place: too early because eth_type_trans wasn't called yet, and too late
> > because skb->dev was changed and no longer points to the DSA master, so
> > the flow dissector adjustment we made doesn't apply.
> >
> > But if you call the flow dissector _before_ you call "skb->dev =
> > dsa_master_find_slave" (and yes, while the DSA tag is still there), then
> > it's virtually as if you had called that while the skb belonged to the
> > DSA master, so it should work with __skb_flow_dissect.
> >
> > In fact I prototyped this idea below. I wanted to check whether I can
> > match something as fine-grained as an IGMPv2 Membership Report message,
> > and I could.
> >
> > I prototyped it inside the ocelot tagging protocol driver because that's
> > what I had handy. I used __skb_flow_dissect with my own flow dissector
> > which had to be initialized at the tagger module_init time, even though
> > I think I could have probably just called skb_flow_dissect_flow_keys
> > with a standard dissector, and that would have removed the need for the
> > custom module_init in tag_ocelot.c. One thing that is interesting is
> > that I had to add the bits for IGMP parsing to the flow dissector
> > myself (based on the existing ICMP code). I was too lazy to do that for
> > MLD as well, but it is really not hard. Or even better, if you don't
> > need to look at all inside the IGMP/MLD header, I think you can even
> > omit adding this parsing code to the flow dissector and just look at
> > basic.n_proto and basic.ip_proto.
> >
> > See the snippet below. Hope it helps.
> >
> > -----------------------------[ cut here ]-----------------------------
> > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> > index ffd386ea0dbb..4c25fa47637a 100644
> > --- a/include/net/flow_dissector.h
> > +++ b/include/net/flow_dissector.h
> > @@ -190,6 +190,20 @@ struct flow_dissector_key_icmp {
> >  	u16 id;
> >  };
> >
> > +/**
> > + * flow_dissector_key_igmp:
> > + *		type: indicates the message type, see include/uapi/linux/igmp.h
> > + *		code: Max Resp Code, the maximum time in 1/10 second
> > + *		      increments before sending a responding report
> > + *		group: the multicast address being queried when sending a
> > + *		       Group-Specific or Group-and-Source-Specific Query.
> > + */
> > +struct flow_dissector_key_igmp {
> > +	u8 type;
> > +	u8 code; /* Max Resp Time in IGMPv2 */
> > +	__be32 group;
> > +};
> > +
> >  /**
> >   * struct flow_dissector_key_eth_addrs:
> >   * @src: source Ethernet address
> > @@ -259,6 +273,7 @@ enum flow_dissector_key_id {
> >  	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
> >  	FLOW_DISSECTOR_KEY_PORTS_RANGE, /* struct flow_dissector_key_ports */
> >  	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
> > +	FLOW_DISSECTOR_KEY_IGMP, /* struct flow_dissector_key_igmp */
> >  	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
> >  	FLOW_DISSECTOR_KEY_TIPC, /* struct flow_dissector_key_tipc */
> >  	FLOW_DISSECTOR_KEY_ARP, /* struct flow_dissector_key_arp */
> > @@ -314,6 +329,7 @@ struct flow_keys {
> >  	struct flow_dissector_key_keyid keyid;
> >  	struct flow_dissector_key_ports ports;
> >  	struct flow_dissector_key_icmp icmp;
> > +	struct flow_dissector_key_igmp igmp;
> >  	/* 'addrs' must be the last member */
> >  	struct flow_dissector_key_addrs addrs;
> >  };
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 5985029e43d4..8cc8c34ea5cd 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -202,6 +202,30 @@ static void __skb_flow_dissect_icmp(const struct sk_buff *skb,
> >  	skb_flow_get_icmp_tci(skb, key_icmp, data, thoff, hlen);
> >  }
> >
> > +static void __skb_flow_dissect_igmp(const struct sk_buff *skb,
> > +				    struct flow_dissector *flow_dissector,
> > +				    void *target_container, const void *data,
> > +				    int thoff, int hlen)
> > +{
> > +	struct flow_dissector_key_igmp *key_igmp;
> > +	struct igmphdr *ih, _ih;
> > +
> > +	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_IGMP))
> > +		return;
> > +
> > +	ih = __skb_header_pointer(skb, thoff, sizeof(_ih), data, hlen, &_ih);
> > +	if (!ih)
> > +		return;
> > +
> > +	key_igmp = skb_flow_dissector_target(flow_dissector,
> > +					     FLOW_DISSECTOR_KEY_IGMP,
> > +					     target_container);
> > +
> > +	key_igmp->type = ih->type;
> > +	key_igmp->code = ih->code;
> > +	key_igmp->group = ih->group;
> > +}
> > +
> >  void skb_flow_dissect_meta(const struct sk_buff *skb,
> >  			   struct flow_dissector *flow_dissector,
> >  			   void *target_container)
> > @@ -1398,6 +1422,11 @@ bool __skb_flow_dissect(const struct net *net,
> >  					data, nhoff, hlen);
> >  		break;
> >
> > +	case IPPROTO_IGMP:
> > +		__skb_flow_dissect_igmp(skb, flow_dissector, target_container,
> > +					data, nhoff, hlen);
> > +		break;
> > +
> >  	default:
> >  		break;
> >  	}
> > diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
> > index f9df9cac81c5..a2cc824ddeec 100644
> > --- a/net/dsa/tag_ocelot.c
> > +++ b/net/dsa/tag_ocelot.c
> > @@ -2,9 +2,51 @@
> >  /* Copyright 2019 NXP Semiconductors
> >   */
> >  #include <linux/dsa/ocelot.h>
> > +#include <linux/igmp.h>
> >  #include <soc/mscc/ocelot.h>
> >  #include "dsa_priv.h"
> >
> > +static const struct flow_dissector_key ocelot_flow_keys[] = {
> > +	{
> > +		.key_id = FLOW_DISSECTOR_KEY_CONTROL,
> > +		.offset = offsetof(struct flow_keys, control),
> > +	},
> > +	{
> > +		.key_id = FLOW_DISSECTOR_KEY_BASIC,
> > +		.offset = offsetof(struct flow_keys, basic),
> > +	},
> > +	{
> > +		.key_id = FLOW_DISSECTOR_KEY_IGMP,
> > +		.offset = offsetof(struct flow_keys, igmp),
> > +	},
> > +};
> > +
> > +static struct flow_dissector ocelot_flow_dissector __read_mostly;
> > +
> > +static struct sk_buff *ocelot_drop_igmp(struct sk_buff *skb)
> > +{
> > +	struct flow_keys fk;
> > +
> > +	memset(&fk, 0, sizeof(fk));
> > +
> > +	if (!__skb_flow_dissect(NULL, skb, &ocelot_flow_dissector,
> > +				&fk, NULL, 0, 0, 0, 0))
> > +		return skb;
> > +
> > +	if (fk.basic.n_proto != htons(ETH_P_IP))
> > +		return skb;
> > +
> > +	if (fk.basic.ip_proto != IPPROTO_IGMP)
> > +		return skb;
> > +
> > +	if (fk.igmp.type != IGMPV2_HOST_MEMBERSHIP_REPORT)
> > +		return skb;
> > +
> > +	skb_dump(KERN_ERR, skb, true);
> > +
> > +	return NULL;
> > +}
> > +
> >  static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
> >  			    struct sk_buff *clone)
> >  {
> > @@ -84,6 +126,10 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
> >  	u8 *extraction;
> >  	u16 vlan_tpid;
> >
> > +	skb = ocelot_drop_igmp(skb);
> > +	if (!skb)
> > +		return NULL;
> 
> I probably like this idea, but i need more understanding :)
> 
> In case of ocelot_drop_igmp() you wont to really drop it? Or it is just
> example on how it may potentially work?
> I ask, because IGMP should not be dropped.

Uhm, yeah, it is just an example of 'how to do X with IGMP packets from
the DSA receive path'. You don't have to drop them.

[PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Oleksij Rempel]

The ar9331 switch is not forwarding IGMP and MLD packets if IGMP
snooping is enabled. This patch is trying to mimic the HW heuristic to take
same decisions as this switch would do to be able to tell the linux
bridge if some packet was prabably forwarded or not.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/dsa/tag_ar9331.c | 130 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/net/dsa/tag_ar9331.c b/net/dsa/tag_ar9331.c
index 002cf7f952e2..0ba90e0f91bb 100644
--- a/net/dsa/tag_ar9331.c
+++ b/net/dsa/tag_ar9331.c
@@ -6,6 +6,8 @@
 
 #include <linux/bitfield.h>
 #include <linux/etherdevice.h>
+#include <linux/igmp.h>
+#include <net/addrconf.h>
 
 #include "dsa_priv.h"
 
@@ -24,6 +26,69 @@
 #define AR9331_HDR_RESERVED_MASK	GENMASK(5, 4)
 #define AR9331_HDR_PORT_NUM_MASK	GENMASK(3, 0)
 
+/*
+ * This code replicated MLD detection more or less in the same way as the
+ * switch is doing it
+ */
+static int ipv6_mc_check_ip6hdr(struct sk_buff *skb)
+{
+	const struct ipv6hdr *ip6h;
+	unsigned int offset;
+
+	offset = skb_network_offset(skb) + sizeof(*ip6h);
+
+	if (!pskb_may_pull(skb, offset))
+		return -EINVAL;
+
+	ip6h = ipv6_hdr(skb);
+
+	if (ip6h->version != 6)
+		return -EINVAL;
+
+	skb_set_transport_header(skb, offset);
+
+	return 0;
+}
+
+static int ipv6_mc_check_exthdrs(struct sk_buff *skb)
+{
+	const struct ipv6hdr *ip6h;
+	int offset;
+	u8 nexthdr;
+	__be16 frag_off;
+
+	ip6h = ipv6_hdr(skb);
+
+	if (ip6h->nexthdr != IPPROTO_HOPOPTS)
+		return -ENOMSG;
+
+	nexthdr = ip6h->nexthdr;
+	offset = skb_network_offset(skb) + sizeof(*ip6h);
+	offset = ipv6_skip_exthdr(skb, offset, &nexthdr, &frag_off);
+
+	if (offset < 0)
+		return -EINVAL;
+
+	if (nexthdr != IPPROTO_ICMPV6)
+		return -ENOMSG;
+
+	skb_set_transport_header(skb, offset);
+
+	return 0;
+}
+
+static int my_ipv6_mc_check_mld(struct sk_buff *skb)
+{
+	int ret;
+
+	ret = ipv6_mc_check_ip6hdr(skb);
+	if (ret < 0)
+		return ret;
+
+	return ipv6_mc_check_exthdrs(skb);
+}
+
+
 static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
 				       struct net_device *dev)
 {
@@ -31,6 +96,13 @@ static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
 	__le16 *phdr;
 	u16 hdr;
 
+	if (dp->stp_state == BR_STATE_BLOCKING) {
+		/* TODO: should we reflect it in the stats? */
+		netdev_warn_once(dev, "%s:%i dropping blocking packet\n",
+				 __func__, __LINE__);
+		return NULL;
+	}
+
 	phdr = skb_push(skb, AR9331_HDR_LEN);
 
 	hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
@@ -80,11 +152,69 @@ static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
 	return skb;
 }
 
+static void ar9331_tag_rcv_post(struct sk_buff *skb)
+{
+	const struct iphdr *iph;
+	unsigned char *dest;
+	int ret;
+
+	/*
+	 * Since the switch do not tell us which packets was offloaded we assume
+	 * that all of them did. Except:
+	 * - port is not configured for forwarding to any other ports
+	 * - igmp/mld snooping is enabled
+	 * - unicast or multicast flood is disabled on some of bridged ports
+	 * - if we have two port bridge and one is not in forwarding state.
+	 * - packet was dropped on the output port..
+	 * - any other reasons?
+	 */
+	skb->offload_fwd_mark = true;
+
+	dest = eth_hdr(skb)->h_dest;
+	/*
+	 * Complete not multicast traffic seems to be forwarded automatically,
+	 * as long as multicast and unicast flood are enabled
+	 */
+	if (!(is_multicast_ether_addr(dest) && !is_broadcast_ether_addr(dest)))
+		return;
+
+
+	/*
+	 * Documentation do not providing any usable information on how the
+	 * igmp/mld snooping is implemented on this switch. Following
+	 * implementation is based on testing, by sending correct and malformed
+	 * packets to the switch.
+	 * It is not trying to find sane and properly formated packets. Instead
+	 * it is trying to be as close to the switch behavior as possible.
+	 */
+	skb_reset_network_header(skb);
+	switch (ntohs(skb->protocol)) {
+	case ETH_P_IP:
+
+		if (!pskb_network_may_pull(skb, sizeof(*iph)))
+			break;
+
+		iph = ip_hdr(skb);
+		if (iph->protocol == IPPROTO_IGMP)
+			skb->offload_fwd_mark = false;
+
+		break;
+	case ETH_P_IPV6:
+		ret = my_ipv6_mc_check_mld(skb);
+		if (!ret)
+			skb->offload_fwd_mark = false;
+
+		break;
+	}
+}
+
+
 static const struct dsa_device_ops ar9331_netdev_ops = {
 	.name	= "ar9331",
 	.proto	= DSA_TAG_PROTO_AR9331,
 	.xmit	= ar9331_tag_xmit,
 	.rcv	= ar9331_tag_rcv,
+	.rcv_post = ar9331_tag_rcv_post,
 	.overhead = AR9331_HDR_LEN,
 };
 
-- 
2.29.2

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Vladimir Oltean]

Hi Oleksij,

On Sat, Apr 03, 2021 at 01:48:41PM +0200, Oleksij Rempel wrote:
> The ar9331 switch is not forwarding IGMP and MLD packets if IGMP
> snooping is enabled. This patch is trying to mimic the HW heuristic to take
> same decisions as this switch would do to be able to tell the linux
> bridge if some packet was prabably forwarded or not.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

I am not familiar with IGMP/MLD, therefore I don't really understand
what problem you are trying to solve.

Your switch has packet traps for IGMP and MLD, ok. So it doesn't forward
them. Must the IGMP/MLD packets be forwarded by an IGMP/MLD snooping
bridge? Which ones and under what circumstances?

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Oleksij Rempel]

On Sat, Apr 03, 2021 at 04:03:18PM +0300, Vladimir Oltean wrote:
> Hi Oleksij,
> 
> On Sat, Apr 03, 2021 at 01:48:41PM +0200, Oleksij Rempel wrote:
> > The ar9331 switch is not forwarding IGMP and MLD packets if IGMP
> > snooping is enabled. This patch is trying to mimic the HW heuristic to take
> > same decisions as this switch would do to be able to tell the linux
> > bridge if some packet was prabably forwarded or not.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> 
> I am not familiar with IGMP/MLD, therefore I don't really understand
> what problem you are trying to solve.
> 
> Your switch has packet traps for IGMP and MLD, ok. So it doesn't forward
> them. Must the IGMP/MLD packets be forwarded by an IGMP/MLD snooping
> bridge? Which ones and under what circumstances?

I'll better refer to the rfc:
https://tools.ietf.org/html/rfc4541

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 03:26:36PM +0200, Oleksij Rempel wrote:
> On Sat, Apr 03, 2021 at 04:03:18PM +0300, Vladimir Oltean wrote:
> > Hi Oleksij,
> > 
> > On Sat, Apr 03, 2021 at 01:48:41PM +0200, Oleksij Rempel wrote:
> > > The ar9331 switch is not forwarding IGMP and MLD packets if IGMP
> > > snooping is enabled. This patch is trying to mimic the HW heuristic to take
> > > same decisions as this switch would do to be able to tell the linux
> > > bridge if some packet was prabably forwarded or not.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > 
> > I am not familiar with IGMP/MLD, therefore I don't really understand
> > what problem you are trying to solve.
> > 
> > Your switch has packet traps for IGMP and MLD, ok. So it doesn't forward
> > them. Must the IGMP/MLD packets be forwarded by an IGMP/MLD snooping
> > bridge? Which ones and under what circumstances?
> 
> I'll better refer to the rfc:
> https://tools.ietf.org/html/rfc4541

Ok, the question might have been a little bit dumb.
I found this PDF:
https://www.alliedtelesis.com/sites/default/files/documents/how-alliedware/howto_config_igmp1.pdf
and it explains that:
- a snooper floods the Membership Query messages from the network's
  querier towards all ports that are not blocked by STP
- a snooper forwards all Membership Report messages from a client
  towards the All Groups port (which is how it reaches the querier).

I'm asking this because I just want to understand what the bridge code
does. Does the code path for IGMP_HOST_MEMBERSHIP_REPORT (for example)
for a snooper go through should_deliver -> nbp_switchdev_allowed_egress,
which is what you are affecting here?

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Oleksij Rempel]

On Sat, Apr 03, 2021 at 04:46:06PM +0300, Vladimir Oltean wrote:
> On Sat, Apr 03, 2021 at 03:26:36PM +0200, Oleksij Rempel wrote:
> > On Sat, Apr 03, 2021 at 04:03:18PM +0300, Vladimir Oltean wrote:
> > > Hi Oleksij,
> > > 
> > > On Sat, Apr 03, 2021 at 01:48:41PM +0200, Oleksij Rempel wrote:
> > > > The ar9331 switch is not forwarding IGMP and MLD packets if IGMP
> > > > snooping is enabled. This patch is trying to mimic the HW heuristic to take
> > > > same decisions as this switch would do to be able to tell the linux
> > > > bridge if some packet was prabably forwarded or not.
> > > > 
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > ---
> > > 
> > > I am not familiar with IGMP/MLD, therefore I don't really understand
> > > what problem you are trying to solve.
> > > 
> > > Your switch has packet traps for IGMP and MLD, ok. So it doesn't forward
> > > them. Must the IGMP/MLD packets be forwarded by an IGMP/MLD snooping
> > > bridge? Which ones and under what circumstances?
> > 
> > I'll better refer to the rfc:
> > https://tools.ietf.org/html/rfc4541
> 
> Ok, the question might have been a little bit dumb.
> I found this PDF:
> https://www.alliedtelesis.com/sites/default/files/documents/how-alliedware/howto_config_igmp1.pdf
> and it explains that:
> - a snooper floods the Membership Query messages from the network's
>   querier towards all ports that are not blocked by STP
> - a snooper forwards all Membership Report messages from a client
>   towards the All Groups port (which is how it reaches the querier).
> 
> I'm asking this because I just want to understand what the bridge code
> does. Does the code path for IGMP_HOST_MEMBERSHIP_REPORT (for example)
> for a snooper go through should_deliver -> nbp_switchdev_allowed_egress,
> which is what you are affecting here?

yes.

the exact path should depend on this configuration option:
/sys/devices/virtual/net/test/bridge/multicast_snooping

I assume, some optimization can be done by letting DSA know the state
of multicast_snooping.

Off-topic question, this patch set stops to work after rebasing against
latest netdev. I get following warning:
ip l s lan0 master test
RTNETLINK answers: Invalid argumen

Are there some API changes?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 05:22:24PM +0200, Oleksij Rempel wrote:
> Off-topic question, this patch set stops to work after rebasing against
> latest netdev. I get following warning:
> ip l s lan0 master test
> RTNETLINK answers: Invalid argumen
> 
> Are there some API changes?

Yes, it's likely that you are returning -EINVAL to some of the functions
with which DSA calls you at .port_bridge_join time, see dsa_port_switchdev_sync.

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Andrew Lunn]

> @@ -31,6 +96,13 @@ static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
>  	__le16 *phdr;
>  	u16 hdr;
>  
> +	if (dp->stp_state == BR_STATE_BLOCKING) {
> +		/* TODO: should we reflect it in the stats? */
> +		netdev_warn_once(dev, "%s:%i dropping blocking packet\n",
> +				 __func__, __LINE__);
> +		return NULL;
> +	}
> +
>  	phdr = skb_push(skb, AR9331_HDR_LEN);
>  
>  	hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);

Hi Oleksij

This change does not seem to fit with what this patch is doing.

I also think it is wrong. You still need BPDU to pass through a
blocked port, otherwise spanning tree protocol will be unstable.

	Andrew

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Oleksij Rempel]

Am 03.04.21 um 16:49 schrieb Andrew Lunn:
>> @@ -31,6 +96,13 @@ static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
>>  	__le16 *phdr;
>>  	u16 hdr;
>>
>> +	if (dp->stp_state == BR_STATE_BLOCKING) {
>> +		/* TODO: should we reflect it in the stats? */
>> +		netdev_warn_once(dev, "%s:%i dropping blocking packet\n",
>> +				 __func__, __LINE__);
>> +		return NULL;
>> +	}
>> +
>>  	phdr = skb_push(skb, AR9331_HDR_LEN);
>>
>>  	hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
>
> Hi Oleksij
>
> This change does not seem to fit with what this patch is doing.

done

> I also think it is wrong. You still need BPDU to pass through a
> blocked port, otherwise spanning tree protocol will be unstable.

We need a better filter, otherwise, in case of software based STP, we are leaking packages on
blocked port. For example DHCP do trigger lots of spam in the kernel log.

I'll drop STP patch for now, it will be better to make a generic soft STP for all switches without
HW offloading. For example ksz9477 is doing SW based STP in similar way.

--
Regards,
Oleksij

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 07:14:56PM +0200, Oleksij Rempel wrote:
> Am 03.04.21 um 16:49 schrieb Andrew Lunn:
> >> @@ -31,6 +96,13 @@ static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
> >>  	__le16 *phdr;
> >>  	u16 hdr;
> >>
> >> +	if (dp->stp_state == BR_STATE_BLOCKING) {
> >> +		/* TODO: should we reflect it in the stats? */
> >> +		netdev_warn_once(dev, "%s:%i dropping blocking packet\n",
> >> +				 __func__, __LINE__);
> >> +		return NULL;
> >> +	}
> >> +
> >>  	phdr = skb_push(skb, AR9331_HDR_LEN);
> >>
> >>  	hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
> >
> > Hi Oleksij
> >
> > This change does not seem to fit with what this patch is doing.
> 
> done
> 
> > I also think it is wrong. You still need BPDU to pass through a
> > blocked port, otherwise spanning tree protocol will be unstable.
> 
> We need a better filter, otherwise, in case of software based STP, we are leaking packages on
> blocked port. For example DHCP do trigger lots of spam in the kernel log.

I have no idea whatsoever what 'software based STP' is, if you have
hardware-accelerated forwarding.

> I'll drop STP patch for now, it will be better to make a generic soft STP for all switches without
> HW offloading. For example ksz9477 is doing SW based STP in similar way.

How about we discuss first about what your switch is not doing properly?
Have you debugged more than just watching the bridge change port states?
As Andrew said, a port needs to accept and send link-local frames
regardless of the STP state. In the BLOCKING state it must send no other
frames and have address learning disabled. Is this what's happening, is
the switch forwarding frames towards a BLOCKING port?

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Oleksij Rempel]

Am 04.04.21 um 02:02 schrieb Vladimir Oltean:
> On Sat, Apr 03, 2021 at 07:14:56PM +0200, Oleksij Rempel wrote:
>> Am 03.04.21 um 16:49 schrieb Andrew Lunn:
>>>> @@ -31,6 +96,13 @@ static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
>>>>  	__le16 *phdr;
>>>>  	u16 hdr;
>>>>
>>>> +	if (dp->stp_state == BR_STATE_BLOCKING) {
>>>> +		/* TODO: should we reflect it in the stats? */
>>>> +		netdev_warn_once(dev, "%s:%i dropping blocking packet\n",
>>>> +				 __func__, __LINE__);
>>>> +		return NULL;
>>>> +	}
>>>> +
>>>>  	phdr = skb_push(skb, AR9331_HDR_LEN);
>>>>
>>>>  	hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
>>>
>>> Hi Oleksij
>>>
>>> This change does not seem to fit with what this patch is doing.
>>
>> done
>>
>>> I also think it is wrong. You still need BPDU to pass through a
>>> blocked port, otherwise spanning tree protocol will be unstable.
>>
>> We need a better filter, otherwise, in case of software based STP, we are leaking packages on
>> blocked port. For example DHCP do trigger lots of spam in the kernel log.
>
> I have no idea whatsoever what 'software based STP' is, if you have
> hardware-accelerated forwarding.

I do not mean hardware-accelerated forwarding, i mean
hardware-accelerated STP port state helpers.

>> I'll drop STP patch for now, it will be better to make a generic soft STP for all switches without
>> HW offloading. For example ksz9477 is doing SW based STP in similar way.
>
> How about we discuss first about what your switch is not doing properly?
> Have you debugged more than just watching the bridge change port states?
> As Andrew said, a port needs to accept and send link-local frames
> regardless of the STP state. In the BLOCKING state it must send no other
> frames and have address learning disabled. Is this what's happening, is
> the switch forwarding frames towards a BLOCKING port?

The switch is not forwarding BPDU frame to the CPU port. So, the linux
bridge will stack by cycling different state of the port where loop was
detected.

--
Regards,
Oleksij

Re: [PATCH net-next v1 2/9] net: dsa: tag_ar9331: detect IGMP and MLD packets

[Vladimir Oltean]

On Sun, Apr 04, 2021 at 07:35:26AM +0200, Oleksij Rempel wrote:
> Am 04.04.21 um 02:02 schrieb Vladimir Oltean:
> > On Sat, Apr 03, 2021 at 07:14:56PM +0200, Oleksij Rempel wrote:
> >> Am 03.04.21 um 16:49 schrieb Andrew Lunn:
> >>>> @@ -31,6 +96,13 @@ static struct sk_buff *ar9331_tag_xmit(struct sk_buff *skb,
> >>>>  	__le16 *phdr;
> >>>>  	u16 hdr;
> >>>>
> >>>> +	if (dp->stp_state == BR_STATE_BLOCKING) {
> >>>> +		/* TODO: should we reflect it in the stats? */
> >>>> +		netdev_warn_once(dev, "%s:%i dropping blocking packet\n",
> >>>> +				 __func__, __LINE__);
> >>>> +		return NULL;
> >>>> +	}
> >>>> +
> >>>>  	phdr = skb_push(skb, AR9331_HDR_LEN);
> >>>>
> >>>>  	hdr = FIELD_PREP(AR9331_HDR_VERSION_MASK, AR9331_HDR_VERSION);
> >>>
> >>> Hi Oleksij
> >>>
> >>> This change does not seem to fit with what this patch is doing.
> >>
> >> done
> >>
> >>> I also think it is wrong. You still need BPDU to pass through a
> >>> blocked port, otherwise spanning tree protocol will be unstable.
> >>
> >> We need a better filter, otherwise, in case of software based STP, we are leaking packages on
> >> blocked port. For example DHCP do trigger lots of spam in the kernel log.
> >
> > I have no idea whatsoever what 'software based STP' is, if you have
> > hardware-accelerated forwarding.
> 
> I do not mean hardware-accelerated forwarding, i mean
> hardware-accelerated STP port state helpers.

Still no clue what you mean, sorry.

> >> I'll drop STP patch for now, it will be better to make a generic soft STP for all switches without
> >> HW offloading. For example ksz9477 is doing SW based STP in similar way.
> >
> > How about we discuss first about what your switch is not doing properly?
> > Have you debugged more than just watching the bridge change port states?
> > As Andrew said, a port needs to accept and send link-local frames
> > regardless of the STP state. In the BLOCKING state it must send no other
> > frames and have address learning disabled. Is this what's happening, is
> > the switch forwarding frames towards a BLOCKING port?
> 
> The switch is not forwarding BPDU frame to the CPU port. So, the linux
> bridge will stack by cycling different state of the port where loop was
> detected.

The switch should not be 'forwarding' BPDU frames to the CPU port, it
should be 'trapping' them. The difference is subtle but important. Often
times switches have an Access Control List which allows them to steal
packets from the normal FDB-based forwarding path. It is probably the
case that your switch needs to be told to treat STP BPDUs specially and
not just 'forward' them.
To confirm whether I'm right or wrong, if you disable STP and send a
packet with MAC DA 01:80:c2:00:00:00 to the switch, will it flood it
towards all ports or will it only send them to the CPU?

[PATCH net-next v1 3/9] net: dsa: qca: ar9331: reorder MDIO write sequence

[Oleksij Rempel]

In case of this switch we work with 32bit registers on top of 16bit
bus. Some registers (for example access to forwarding database) have
trigger bit on the first 16bit half of request and the result +
configuration of request in the second half. Without this this patch, we would
trigger database operation and overwrite result in one run.

To make it work properly, we should do the second part of transfer
before the first one is done.

So far, this rule seems to work for all registers on this switch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index ca2ad77b71f1..9a5035b2f0ff 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -837,16 +837,17 @@ static int ar9331_mdio_write(void *ctx, u32 reg, u32 val)
 		return 0;
 	}
 
-	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val);
+	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2,
+				  val >> 16);
 	if (ret < 0)
 		goto error;
 
-	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2,
-				  val >> 16);
+	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val);
 	if (ret < 0)
 		goto error;
 
 	return 0;
+
 error:
 	dev_err_ratelimited(&sbus->dev, "Bus error. Failed to write register.\n");
 	return ret;
-- 
2.29.2

Re: [PATCH net-next v1 3/9] net: dsa: qca: ar9331: reorder MDIO write sequence

[Andrew Lunn]

Hi Oleksij

Maybe add a short comment about why the order is important.

> -	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val);
> +	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2,
> +				  val >> 16);
>  	if (ret < 0)
>  		goto error;
>  
> -	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg + 2,
> -				  val >> 16);
> +	ret = __ar9331_mdio_write(sbus, AR9331_SW_MDIO_PHY_MODE_REG, reg, val);
>  	if (ret < 0)
>  		goto error;
>  
>  	return 0;
> +
>  error:
>  	dev_err_ratelimited(&sbus->dev, "Bus error. Failed to write register.\n");
>  	return ret;

With that:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew





> -- 
> 2.29.2
> 

Re: [PATCH net-next v1 3/9] net: dsa: qca: ar9331: reorder MDIO write sequence

[Florian Fainelli]

On 4/3/2021 04:48, Oleksij Rempel wrote:
> In case of this switch we work with 32bit registers on top of 16bit
> bus. Some registers (for example access to forwarding database) have
> trigger bit on the first 16bit half of request and the result +
> configuration of request in the second half. Without this this patch, we would
> trigger database operation and overwrite result in one run.
> 
> To make it work properly, we should do the second part of transfer
> before the first one is done.
> 
> So far, this rule seems to work for all registers on this switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Seems like you could send this as a separate bugfix for the "net" tree 
along with a Fixes tag?
-- 
Florian

[PATCH net-next v1 4/9] net: dsa: qca: ar9331: make proper initial port defaults

[Oleksij Rempel]

Make sure that all external port are actually isolated from each other,
so no packets are leaked.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 145 ++++++++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 9a5035b2f0ff..a3de3598fbf5 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -60,10 +60,19 @@
 
 #define AR9331_SW_REG_FLOOD_MASK		0x2c
 #define AR9331_SW_FLOOD_MASK_BROAD_TO_CPU	BIT(26)
+#define AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP	GENMASK(20, 16)
+#define AR9331_SW_FLOOD_MASK_UNI_FLOOD_DP	GENMASK(4, 0)
 
 #define AR9331_SW_REG_GLOBAL_CTRL		0x30
 #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
 
+#define AR9331_SW_REG_ADDR_TABLE_CTRL		0x5c
+#define AR9331_SW_AT_ARP_EN			BIT(20)
+#define AR9331_SW_AT_LEARN_CHANGE_EN		BIT(18)
+#define AR9331_SW_AT_AGE_EN			BIT(17)
+#define AR9331_SW_AT_AGE_TIME			GENMASK(15, 0)
+#define AR9331_SW_AT_AGE_TIME_COEF		6900 /* Not documented */
+
 #define AR9331_SW_REG_MDIO_CTRL			0x98
 #define AR9331_SW_MDIO_CTRL_BUSY		BIT(31)
 #define AR9331_SW_MDIO_CTRL_MASTER_EN		BIT(30)
@@ -101,6 +110,46 @@
 	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
 	 AR9331_SW_PORT_STATUS_SPEED_M)
 
+#define AR9331_SW_REG_PORT_CTRL(_port)			(0x104 + (_port) * 0x100)
+#define AR9331_SW_PORT_CTRL_ING_MIRROR_EN		BIT(17)
+#define AR9331_SW_PORT_CTRL_EG_MIRROR_EN		BIT(16)
+#define AR9331_SW_PORT_CTRL_DOUBLE_TAG_VLAN		BIT(15)
+#define AR9331_SW_PORT_CTRL_LEARN_EN			BIT(14)
+#define AR9331_SW_PORT_CTRL_SINGLE_VLAN_EN		BIT(13)
+#define AR9331_SW_PORT_CTRL_MAC_LOOP_BACK		BIT(12)
+#define AR9331_SW_PORT_CTRL_HEAD_EN			BIT(11)
+#define AR9331_SW_PORT_CTRL_IGMP_MLD_EN			BIT(10)
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE		GENMASK(9, 8)
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_KEEP		0
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_STRIP		1
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_ADD		2
+#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_DOUBLE		3
+#define AR9331_SW_PORT_CTRL_LEARN_ONE_LOCK		BIT(7)
+#define AR9331_SW_PORT_CTRL_PORT_LOCK_EN		BIT(6)
+#define AR9331_SW_PORT_CTRL_LOCK_DROP_EN		BIT(5)
+#define AR9331_SW_PORT_CTRL_PORT_STATE			GENMASK(2, 0)
+#define AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED		0
+#define AR9331_SW_PORT_CTRL_PORT_STATE_BLOCKING		1
+#define AR9331_SW_PORT_CTRL_PORT_STATE_LISTENING	2
+#define AR9331_SW_PORT_CTRL_PORT_STATE_LEARNING		3
+#define AR9331_SW_PORT_CTRL_PORT_STATE_FORWARD		4
+
+#define AR9331_SW_REG_PORT_VLAN(_port)			(0x108 + (_port) * 0x100)
+#define AR9331_SW_PORT_VLAN_8021Q_MODE			GENMASK(31, 30)
+#define AR9331_SW_8021Q_MODE_SECURE			3
+#define AR9331_SW_8021Q_MODE_CHECK			2
+#define AR9331_SW_8021Q_MODE_FALLBACK			1
+#define AR9331_SW_8021Q_MODE_NONE			0
+#define AR9331_SW_PORT_VLAN_ING_PORT_PRI		GENMASK(29, 27)
+#define AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN		BIT(26)
+#define AR9331_SW_PORT_VLAN_PORT_VID_MEMBER		GENMASK(25, 16)
+#define AR9331_SW_PORT_VLAN_ARP_LEAKY_EN		BIT(15)
+#define AR9331_SW_PORT_VLAN_UNI_LEAKY_EN		BIT(14)
+#define AR9331_SW_PORT_VLAN_MULTI_LEAKY_EN		BIT(13)
+#define AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN	BIT(12)
+#define AR9331_SW_PORT_VLAN_PORT_VID			GENMASK(11, 0)
+#define AR9331_SW_PORT_VLAN_PORT_VID_DEF		1
+
 /* MIB registers */
 #define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
 
@@ -229,6 +278,7 @@ struct ar9331_sw_priv {
 	struct regmap *regmap;
 	struct reset_control *sw_reset;
 	struct ar9331_sw_port port[AR9331_SW_PORTS];
+	int cpu_port;
 };
 
 static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
@@ -371,12 +421,72 @@ static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
 	return 0;
 }
 
-static int ar9331_sw_setup(struct dsa_switch *ds)
+static int ar9331_sw_setup_port(struct dsa_switch *ds, int port)
 {
 	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
 	struct regmap *regmap = priv->regmap;
+	u32 port_mask, port_ctrl, val;
 	int ret;
 
+	/* Generate default port settings */
+	port_ctrl = FIELD_PREP(AR9331_SW_PORT_CTRL_PORT_STATE,
+			       AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED);
+
+	if (dsa_is_cpu_port(ds, port)) {
+		/*
+		 * CPU port should be allowed to communicate with all user
+		 * ports.
+		 */
+		//port_mask = dsa_user_ports(ds);
+		port_mask = 0;
+		/*
+		 * Enable atheros header on CPU port. This will allow us
+		 * communicate with each port separately
+		 */
+		port_ctrl |= AR9331_SW_PORT_CTRL_HEAD_EN;
+		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN;
+	} else if (dsa_is_user_port(ds, port)) {
+		/*
+		 * User ports should communicate only with the CPU port.
+		 */
+		port_mask = BIT(priv->cpu_port);
+		/* Enable unicast address learning by default */
+		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN
+		/* IGMP snooping seems to work correctly, let's use it */
+			  | AR9331_SW_PORT_CTRL_IGMP_MLD_EN
+			  | AR9331_SW_PORT_CTRL_SINGLE_VLAN_EN;
+	} else {
+		/* Other ports do not need to communicate at all */
+		port_mask = 0;
+	}
+
+	val = FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
+			 AR9331_SW_8021Q_MODE_NONE) |
+		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask) |
+		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
+			   AR9331_SW_PORT_VLAN_PORT_VID_DEF);
+
+	ret = regmap_write(regmap, AR9331_SW_REG_PORT_VLAN(port), val);
+	if (ret)
+		goto error;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_PORT_CTRL(port), port_ctrl);
+	if (ret)
+		goto error;
+
+	return 0;
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
+
+	return ret;
+}
+
+static int ar9331_sw_setup(struct dsa_switch *ds)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int ret, i;
+
 	ret = ar9331_sw_reset(priv);
 	if (ret)
 		return ret;
@@ -390,7 +500,8 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
 
 	/* Do not drop broadcast frames */
 	ret = regmap_write_bits(regmap, AR9331_SW_REG_FLOOD_MASK,
-				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU,
+				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU
+				| AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP,
 				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU);
 	if (ret)
 		goto error;
@@ -402,6 +513,36 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
 	if (ret)
 		goto error;
 
+	/*
+	 * Configure the ARL:
+	 * AR9331_SW_AT_ARP_EN - why?
+	 * AR9331_SW_AT_LEARN_CHANGE_EN - why?
+	 */
+	ret = regmap_set_bits(regmap, AR9331_SW_REG_ADDR_TABLE_CTRL,
+			      AR9331_SW_AT_ARP_EN |
+			      AR9331_SW_AT_LEARN_CHANGE_EN);
+	if (ret)
+		goto error;
+
+	/* find the CPU port */
+	priv->cpu_port = -1;
+	for (i = 0; i < ds->num_ports; i++) {
+		if (!dsa_is_cpu_port(ds, i))
+			continue;
+
+		if (priv->cpu_port != -1)
+			dev_err_ratelimited(priv->dev, "%s: more then one CPU port. Already set: %i, trying to add: %i\n",
+					    __func__, priv->cpu_port, i);
+		else
+			priv->cpu_port = i;
+	}
+
+	for (i = 0; i < ds->num_ports; i++) {
+		ret = ar9331_sw_setup_port(ds, i);
+		if (ret)
+			goto error;
+	}
+
 	ds->configure_vlan_while_not_filtering = false;
 
 	return 0;
-- 
2.29.2

Re: [PATCH net-next v1 4/9] net: dsa: qca: ar9331: make proper initial port defaults

[Andrew Lunn]

On Sat, Apr 03, 2021 at 01:48:43PM +0200, Oleksij Rempel wrote:
> Make sure that all external port are actually isolated from each other,
> so no packets are leaked.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 145 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index 9a5035b2f0ff..a3de3598fbf5 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -60,10 +60,19 @@
>  
>  #define AR9331_SW_REG_FLOOD_MASK		0x2c
>  #define AR9331_SW_FLOOD_MASK_BROAD_TO_CPU	BIT(26)
> +#define AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP	GENMASK(20, 16)
> +#define AR9331_SW_FLOOD_MASK_UNI_FLOOD_DP	GENMASK(4, 0)
>  
>  #define AR9331_SW_REG_GLOBAL_CTRL		0x30
>  #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
>  
> +#define AR9331_SW_REG_ADDR_TABLE_CTRL		0x5c
> +#define AR9331_SW_AT_ARP_EN			BIT(20)
> +#define AR9331_SW_AT_LEARN_CHANGE_EN		BIT(18)
> +#define AR9331_SW_AT_AGE_EN			BIT(17)
> +#define AR9331_SW_AT_AGE_TIME			GENMASK(15, 0)
> +#define AR9331_SW_AT_AGE_TIME_COEF		6900 /* Not documented */
> +
>  #define AR9331_SW_REG_MDIO_CTRL			0x98
>  #define AR9331_SW_MDIO_CTRL_BUSY		BIT(31)
>  #define AR9331_SW_MDIO_CTRL_MASTER_EN		BIT(30)
> @@ -101,6 +110,46 @@
>  	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
>  	 AR9331_SW_PORT_STATUS_SPEED_M)
>  
> +#define AR9331_SW_REG_PORT_CTRL(_port)			(0x104 + (_port) * 0x100)
> +#define AR9331_SW_PORT_CTRL_ING_MIRROR_EN		BIT(17)
> +#define AR9331_SW_PORT_CTRL_EG_MIRROR_EN		BIT(16)
> +#define AR9331_SW_PORT_CTRL_DOUBLE_TAG_VLAN		BIT(15)
> +#define AR9331_SW_PORT_CTRL_LEARN_EN			BIT(14)
> +#define AR9331_SW_PORT_CTRL_SINGLE_VLAN_EN		BIT(13)
> +#define AR9331_SW_PORT_CTRL_MAC_LOOP_BACK		BIT(12)
> +#define AR9331_SW_PORT_CTRL_HEAD_EN			BIT(11)
> +#define AR9331_SW_PORT_CTRL_IGMP_MLD_EN			BIT(10)
> +#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE		GENMASK(9, 8)
> +#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_KEEP		0
> +#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_STRIP		1
> +#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_ADD		2
> +#define AR9331_SW_PORT_CTRL_EG_VLAN_MODE_DOUBLE		3
> +#define AR9331_SW_PORT_CTRL_LEARN_ONE_LOCK		BIT(7)
> +#define AR9331_SW_PORT_CTRL_PORT_LOCK_EN		BIT(6)
> +#define AR9331_SW_PORT_CTRL_LOCK_DROP_EN		BIT(5)
> +#define AR9331_SW_PORT_CTRL_PORT_STATE			GENMASK(2, 0)
> +#define AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED		0
> +#define AR9331_SW_PORT_CTRL_PORT_STATE_BLOCKING		1
> +#define AR9331_SW_PORT_CTRL_PORT_STATE_LISTENING	2
> +#define AR9331_SW_PORT_CTRL_PORT_STATE_LEARNING		3
> +#define AR9331_SW_PORT_CTRL_PORT_STATE_FORWARD		4
> +
> +#define AR9331_SW_REG_PORT_VLAN(_port)			(0x108 + (_port) * 0x100)
> +#define AR9331_SW_PORT_VLAN_8021Q_MODE			GENMASK(31, 30)
> +#define AR9331_SW_8021Q_MODE_SECURE			3
> +#define AR9331_SW_8021Q_MODE_CHECK			2
> +#define AR9331_SW_8021Q_MODE_FALLBACK			1
> +#define AR9331_SW_8021Q_MODE_NONE			0
> +#define AR9331_SW_PORT_VLAN_ING_PORT_PRI		GENMASK(29, 27)
> +#define AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN		BIT(26)
> +#define AR9331_SW_PORT_VLAN_PORT_VID_MEMBER		GENMASK(25, 16)
> +#define AR9331_SW_PORT_VLAN_ARP_LEAKY_EN		BIT(15)
> +#define AR9331_SW_PORT_VLAN_UNI_LEAKY_EN		BIT(14)
> +#define AR9331_SW_PORT_VLAN_MULTI_LEAKY_EN		BIT(13)
> +#define AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN	BIT(12)
> +#define AR9331_SW_PORT_VLAN_PORT_VID			GENMASK(11, 0)
> +#define AR9331_SW_PORT_VLAN_PORT_VID_DEF		1
> +
>  /* MIB registers */
>  #define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
>  
> @@ -229,6 +278,7 @@ struct ar9331_sw_priv {
>  	struct regmap *regmap;
>  	struct reset_control *sw_reset;
>  	struct ar9331_sw_port port[AR9331_SW_PORTS];
> +	int cpu_port;
>  };
>  
>  static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
> @@ -371,12 +421,72 @@ static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
>  	return 0;
>  }
>  
> -static int ar9331_sw_setup(struct dsa_switch *ds)
> +static int ar9331_sw_setup_port(struct dsa_switch *ds, int port)
>  {
>  	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
>  	struct regmap *regmap = priv->regmap;
> +	u32 port_mask, port_ctrl, val;
>  	int ret;
>  
> +	/* Generate default port settings */
> +	port_ctrl = FIELD_PREP(AR9331_SW_PORT_CTRL_PORT_STATE,
> +			       AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED);
> +
> +	if (dsa_is_cpu_port(ds, port)) {
> +		/*
> +		 * CPU port should be allowed to communicate with all user
> +		 * ports.
> +		 */
> +		//port_mask = dsa_user_ports(ds);

Please cleanup dead code.

> +		port_mask = 0;

Is 0 the correct value here? It is the same as default, i.e. unused
ports?

> +		/*
> +		 * Enable atheros header on CPU port. This will allow us
> +		 * communicate with each port separately
> +		 */
> +		port_ctrl |= AR9331_SW_PORT_CTRL_HEAD_EN;
> +		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN;
> +	} else if (dsa_is_user_port(ds, port)) {
> +		/*
> +		 * User ports should communicate only with the CPU port.
> +		 */
> +		port_mask = BIT(priv->cpu_port);
> +		/* Enable unicast address learning by default */
> +		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN
> +		/* IGMP snooping seems to work correctly, let's use it */
> +			  | AR9331_SW_PORT_CTRL_IGMP_MLD_EN
> +			  | AR9331_SW_PORT_CTRL_SINGLE_VLAN_EN;

There was a discussion a couple of months ago about if there should be
address learning on the CPU port. Having it enabled allows for devices
which move from behind the CPU onto the switched network. There is a
software workaround in place now, so it might not be needed.


> +	} else {
> +		/* Other ports do not need to communicate at all */
> +		port_mask = 0;
> +	}
> +
> +	val = FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
> +			 AR9331_SW_8021Q_MODE_NONE) |
> +		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask) |
> +		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
> +			   AR9331_SW_PORT_VLAN_PORT_VID_DEF);
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_VLAN(port), val);
> +	if (ret)
> +		goto error;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_CTRL(port), port_ctrl);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);

Typically this function is only called during probe. Do i don't think
it needs rate limiting. 

> +
> +	return ret;
> +}
> +
> +static int ar9331_sw_setup(struct dsa_switch *ds)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int ret, i;
> +
>  	ret = ar9331_sw_reset(priv);
>  	if (ret)
>  		return ret;
> @@ -390,7 +500,8 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
>  
>  	/* Do not drop broadcast frames */
>  	ret = regmap_write_bits(regmap, AR9331_SW_REG_FLOOD_MASK,
> -				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU,
> +				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU
> +				| AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP,
>  				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU);
>  	if (ret)
>  		goto error;
> @@ -402,6 +513,36 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
>  	if (ret)
>  		goto error;
>  
> +	/*
> +	 * Configure the ARL:
> +	 * AR9331_SW_AT_ARP_EN - why?
> +	 * AR9331_SW_AT_LEARN_CHANGE_EN - why?
> +	 */
> +	ret = regmap_set_bits(regmap, AR9331_SW_REG_ADDR_TABLE_CTRL,
> +			      AR9331_SW_AT_ARP_EN |
> +			      AR9331_SW_AT_LEARN_CHANGE_EN);
> +	if (ret)
> +		goto error;
> +
> +	/* find the CPU port */
> +	priv->cpu_port = -1;
> +	for (i = 0; i < ds->num_ports; i++) {
> +		if (!dsa_is_cpu_port(ds, i))
> +			continue;
> +
> +		if (priv->cpu_port != -1)
> +			dev_err_ratelimited(priv->dev, "%s: more then one CPU port. Already set: %i, trying to add: %i\n",
> +					    __func__, priv->cpu_port, i);

Another rate limiting i would not do.

> +		else
> +			priv->cpu_port = i;
> +	}

  Andrew

Re: [PATCH net-next v1 4/9] net: dsa: qca: ar9331: make proper initial port defaults

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 01:48:43PM +0200, Oleksij Rempel wrote:
> Make sure that all external port are actually isolated from each other,
> so no packets are leaked.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 145 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index 9a5035b2f0ff..a3de3598fbf5 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -60,10 +60,19 @@
>  
>  /* MIB registers */
>  #define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
>  
> @@ -229,6 +278,7 @@ struct ar9331_sw_priv {
>  	struct regmap *regmap;
>  	struct reset_control *sw_reset;
>  	struct ar9331_sw_port port[AR9331_SW_PORTS];
> +	int cpu_port;
>  };
>  
>  static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
> @@ -371,12 +421,72 @@ static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
>  	return 0;
>  }
>  
> -static int ar9331_sw_setup(struct dsa_switch *ds)
> +static int ar9331_sw_setup_port(struct dsa_switch *ds, int port)
>  {
>  	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
>  	struct regmap *regmap = priv->regmap;
> +	u32 port_mask, port_ctrl, val;
>  	int ret;
>  
> +	/* Generate default port settings */
> +	port_ctrl = FIELD_PREP(AR9331_SW_PORT_CTRL_PORT_STATE,
> +			       AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED);
> +
> +	if (dsa_is_cpu_port(ds, port)) {
> +		/*
> +		 * CPU port should be allowed to communicate with all user
> +		 * ports.
> +		 */
> +		//port_mask = dsa_user_ports(ds);

Code commented out should ideally not be part of a submitted patch.
And the networking comment style is:

		/* CPU port should be allowed to communicate with all user
		 * ports.
		 */

> +		port_mask = 0;
> +		/*
> +		 * Enable atheros header on CPU port. This will allow us
> +		 * communicate with each port separately
> +		 */
> +		port_ctrl |= AR9331_SW_PORT_CTRL_HEAD_EN;
> +		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN;
> +	} else if (dsa_is_user_port(ds, port)) {
> +		/*
> +		 * User ports should communicate only with the CPU port.
> +		 */
> +		port_mask = BIT(priv->cpu_port);

For all you care, the CPU port here is dsa_to_port(ds, port)->cpu_dp->index,
no need to go to those lengths in order to find it. DSA does not have a
fixed number for the CPU port but a CPU port pointer per port in order
to not close the door for the future support of multiple CPU ports.

> +		/* Enable unicast address learning by default */
> +		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN
> +		/* IGMP snooping seems to work correctly, let's use it */
> +			  | AR9331_SW_PORT_CTRL_IGMP_MLD_EN

I don't really like this ad-hoc enablement of IGMP/MLD snooping from the driver,
please add the pass-through in DSA for SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED
(dsa_slave_port_attr_set, dsa_port_switchdev_sync, dsa_port_switchdev_unsync
should all call a dsa_switch_ops :: port_snoop_igmp_mld function) and then
toggle this bit from there.

> +			  | AR9331_SW_PORT_CTRL_SINGLE_VLAN_EN;
> +	} else {
> +		/* Other ports do not need to communicate at all */
> +		port_mask = 0;
> +	}
> +
> +	val = FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
> +			 AR9331_SW_8021Q_MODE_NONE) |
> +		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask) |
> +		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
> +			   AR9331_SW_PORT_VLAN_PORT_VID_DEF);
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_VLAN(port), val);
> +	if (ret)
> +		goto error;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_CTRL(port), port_ctrl);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static int ar9331_sw_setup(struct dsa_switch *ds)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int ret, i;
> +
>  	ret = ar9331_sw_reset(priv);
>  	if (ret)
>  		return ret;
> @@ -390,7 +500,8 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
>  
>  	/* Do not drop broadcast frames */
>  	ret = regmap_write_bits(regmap, AR9331_SW_REG_FLOOD_MASK,
> -				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU,
> +				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU
> +				| AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP,
>  				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU);
>  	if (ret)
>  		goto error;
> @@ -402,6 +513,36 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
>  	if (ret)
>  		goto error;
>  
> +	/*
> +	 * Configure the ARL:
> +	 * AR9331_SW_AT_ARP_EN - why?
> +	 * AR9331_SW_AT_LEARN_CHANGE_EN - why?
> +	 */

Good question, why?

> +	ret = regmap_set_bits(regmap, AR9331_SW_REG_ADDR_TABLE_CTRL,
> +			      AR9331_SW_AT_ARP_EN |
> +			      AR9331_SW_AT_LEARN_CHANGE_EN);
> +	if (ret)
> +		goto error;
> +
> +	/* find the CPU port */
> +	priv->cpu_port = -1;
> +	for (i = 0; i < ds->num_ports; i++) {
> +		if (!dsa_is_cpu_port(ds, i))
> +			continue;
> +
> +		if (priv->cpu_port != -1)
> +			dev_err_ratelimited(priv->dev, "%s: more then one CPU port. Already set: %i, trying to add: %i\n",

than, not then

> +					    __func__, priv->cpu_port, i);
> +		else
> +			priv->cpu_port = i;
> +	}
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		ret = ar9331_sw_setup_port(ds, i);
> +		if (ret)
> +			goto error;
> +	}
> +
>  	ds->configure_vlan_while_not_filtering = false;
>  
>  	return 0;
> -- 
> 2.29.2
> 

Re: [PATCH net-next v1 4/9] net: dsa: qca: ar9331: make proper initial port defaults

[Oleksij Rempel]

Am 04.04.21 um 02:16 schrieb Vladimir Oltean:
> On Sat, Apr 03, 2021 at 01:48:43PM +0200, Oleksij Rempel wrote:
>> Make sure that all external port are actually isolated from each other,
>> so no packets are leaked.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  drivers/net/dsa/qca/ar9331.c | 145 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 143 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
>> index 9a5035b2f0ff..a3de3598fbf5 100644
>> --- a/drivers/net/dsa/qca/ar9331.c
>> +++ b/drivers/net/dsa/qca/ar9331.c
>> @@ -60,10 +60,19 @@
>>
>>  /* MIB registers */
>>  #define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
>>
>> @@ -229,6 +278,7 @@ struct ar9331_sw_priv {
>>  	struct regmap *regmap;
>>  	struct reset_control *sw_reset;
>>  	struct ar9331_sw_port port[AR9331_SW_PORTS];
>> +	int cpu_port;
>>  };
>>
>>  static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
>> @@ -371,12 +421,72 @@ static int ar9331_sw_mbus_init(struct ar9331_sw_priv *priv)
>>  	return 0;
>>  }
>>
>> -static int ar9331_sw_setup(struct dsa_switch *ds)
>> +static int ar9331_sw_setup_port(struct dsa_switch *ds, int port)
>>  {
>>  	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
>>  	struct regmap *regmap = priv->regmap;
>> +	u32 port_mask, port_ctrl, val;
>>  	int ret;
>>
>> +	/* Generate default port settings */
>> +	port_ctrl = FIELD_PREP(AR9331_SW_PORT_CTRL_PORT_STATE,
>> +			       AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED);
>> +
>> +	if (dsa_is_cpu_port(ds, port)) {
>> +		/*
>> +		 * CPU port should be allowed to communicate with all user
>> +		 * ports.
>> +		 */
>> +		//port_mask = dsa_user_ports(ds);
>
> Code commented out should ideally not be part of a submitted patch.

Sorry I overlooked this one

> And the networking comment style is:
>
> 		/* CPU port should be allowed to communicate with all user
> 		 * ports.
> 		 */

Aaa... networking part of kernel code...

>> +		port_mask = 0;
>> +		/*
>> +		 * Enable atheros header on CPU port. This will allow us
>> +		 * communicate with each port separately
>> +		 */
>> +		port_ctrl |= AR9331_SW_PORT_CTRL_HEAD_EN;
>> +		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN;
>> +	} else if (dsa_is_user_port(ds, port)) {
>> +		/*
>> +		 * User ports should communicate only with the CPU port.
>> +		 */
>> +		port_mask = BIT(priv->cpu_port);
>
> For all you care, the CPU port here is dsa_to_port(ds, port)->cpu_dp->index,
> no need to go to those lengths in order to find it. DSA does not have
> fixed number for the CPU port but a CPU port pointer per port in order
> to not close the door for the future support of multiple CPU ports.

ok.

>> +		/* Enable unicast address learning by default */
>> +		port_ctrl |= AR9331_SW_PORT_CTRL_LEARN_EN
>> +		/* IGMP snooping seems to work correctly, let's use it */
>> +			  | AR9331_SW_PORT_CTRL_IGMP_MLD_EN
>
> I don't really like this ad-hoc enablement of IGMP/MLD snooping from the driver,
> please add the pass-through in DSA for SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED
> (dsa_slave_port_attr_set, dsa_port_switchdev_sync, dsa_port_switchdev_unsync
> should all call a dsa_switch_ops :: port_snoop_igmp_mld function) and then
> toggle this bit from there.

sounds good. Looks like there are few more driver need to be fixed:
drivers/net/dsa/lan9303-core.c
drivers/net/dsa/mv88e6xxx/chip.c


>
>> +			  | AR9331_SW_PORT_CTRL_SINGLE_VLAN_EN;
>> +	} else {
>> +		/* Other ports do not need to communicate at all */
>> +		port_mask = 0;
>> +	}
>> +
>> +	val = FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
>> +			 AR9331_SW_8021Q_MODE_NONE) |
>> +		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask) |
>> +		FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
>> +			   AR9331_SW_PORT_VLAN_PORT_VID_DEF);
>> +
>> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_VLAN(port), val);
>> +	if (ret)
>> +		goto error;
>> +
>> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_CTRL(port), port_ctrl);
>> +	if (ret)
>> +		goto error;
>> +
>> +	return 0;
>> +error:
>> +	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ar9331_sw_setup(struct dsa_switch *ds)
>> +{
>> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
>> +	struct regmap *regmap = priv->regmap;
>> +	int ret, i;
>> +
>>  	ret = ar9331_sw_reset(priv);
>>  	if (ret)
>>  		return ret;
>> @@ -390,7 +500,8 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
>>
>>  	/* Do not drop broadcast frames */
>>  	ret = regmap_write_bits(regmap, AR9331_SW_REG_FLOOD_MASK,
>> -				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU,
>> +				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU
>> +				| AR9331_SW_FLOOD_MASK_MULTI_FLOOD_DP,
>>  				AR9331_SW_FLOOD_MASK_BROAD_TO_CPU);
>>  	if (ret)
>>  		goto error;
>> @@ -402,6 +513,36 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
>>  	if (ret)
>>  		goto error;
>>
>> +	/*
>> +	 * Configure the ARL:
>> +	 * AR9331_SW_AT_ARP_EN - why?
>> +	 * AR9331_SW_AT_LEARN_CHANGE_EN - why?
>> +	 */
>
> Good question, why?

I still do not know if it is a good idea. This bits are enabled by
default. May be you can help me understand it. Datasheet says:
ARP_EN:
ARP frame acknowledge enable. Setting this bit to 1 is an
acknowledgement by the hardware of a received ARP frame and allows it
to be copied to the CPU port.

LEARN_CHANGE_EN:
0 - If a hash violation occurs during learning, no new address will be
learned in the ARL
1 - Enables a new MAC address change if a hash violation occurs
during learning

--
Regards,
Oleksij

[PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support

[Oleksij Rempel]

This switch provides simple address resolution table, without VLAN or
multicast specific information.
With this patch we are able now to read, modify unicast and mulicast
addresses.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 356 +++++++++++++++++++++++++++++++++++
 1 file changed, 356 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index a3de3598fbf5..4a98f14f31f4 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -66,6 +66,47 @@
 #define AR9331_SW_REG_GLOBAL_CTRL		0x30
 #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
 
+/* Size of the address resolution table (ARL) */
+#define AR9331_SW_NUM_ARL_RECORDS		1024
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION0	0x50
+#define AR9331_SW_AT_ADDR_BYTES4		GENMASK(31, 24)
+#define AR9331_SW_AT_ADDR_BYTES5		GENMASK(23, 16)
+#define AR9331_SW_AT_FULL_VIO			BIT(12)
+#define AR9331_SW_AT_PORT_NUM			GENMASK(11, 8)
+#define AR9331_SW_AT_FLUSH_STATIC_EN		BIT(4)
+#define AR9331_SW_AT_BUSY			BIT(3)
+#define AR9331_SW_AT_FUNC			GENMASK(2, 0)
+#define AR9331_SW_AT_FUNC_NOP			0
+#define AR9331_SW_AT_FUNC_FLUSH_ALL		1
+#define AR9331_SW_AT_FUNC_LOAD_ENTRY		2
+#define AR9331_SW_AT_FUNC_PURGE_ENTRY		3
+#define AR9331_SW_AT_FUNC_FLUSH_ALL_UNLOCKED	4
+#define AR9331_SW_AT_FUNC_FLUSH_PORT		5
+#define AR9331_SW_AT_FUNC_GET_NEXT		6
+#define AR9331_SW_AT_FUNC_FIND_MAC		7
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION1	0x54
+#define AR9331_SW_AT_ADDR_BYTES0		GENMASK(31, 24)
+#define AR9331_SW_AT_ADDR_BYTES1		GENMASK(23, 16)
+#define AR9331_SW_AT_ADDR_BYTES2		GENMASK(15, 8)
+#define AR9331_SW_AT_ADDR_BYTES3		GENMASK(7, 0)
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION2	0x58
+#define AR9331_SW_AT_COPY_TO_CPU		BIT(26)
+#define AR9331_SW_AT_REDIRECT_TOCPU		BIT(25)
+#define AR9331_SW_AT_LEAKY_EN			BIT(24)
+#define AR9331_SW_AT_STATUS			GENMASK(19, 16)
+#define AR9331_SW_AT_STATUS_EMPTY		0
+/* STATUS values from 7 to 1 are different aging levels */
+#define AR9331_SW_AT_STATUS_STATIC		0xf
+
+#define AR9331_SW_AT_SA_DROP_EN			BIT(14)
+#define AR9331_SW_AT_MIRROR_EN			BIT(13)
+#define AR9331_SW_AT_PRIORITY_EN		BIT(12)
+#define AR9331_SW_AT_PRIORITY			GENMASK(11, 10)
+#define AR9331_SW_AT_DES_PORT			GENMASK(5, 0)
+
 #define AR9331_SW_REG_ADDR_TABLE_CTRL		0x5c
 #define AR9331_SW_AT_ARP_EN			BIT(20)
 #define AR9331_SW_AT_LEARN_CHANGE_EN		BIT(18)
@@ -266,6 +307,12 @@ struct ar9331_sw_port {
 	struct spinlock stats_lock;
 };
 
+struct ar9331_sw_fdb {
+	u8 port_mask;
+	u8 aging;
+	u8 mac[ETH_ALEN];
+};
+
 struct ar9331_sw_priv {
 	struct device *dev;
 	struct dsa_switch ds;
@@ -765,6 +812,309 @@ static void ar9331_get_stats64(struct dsa_switch *ds, int port,
 	spin_unlock(&p->stats_lock);
 }
 
+static int ar9331_sw_fdb_wait(struct ar9331_sw_priv *priv, u32 *f0)
+{
+	struct regmap *regmap = priv->regmap;
+
+	return regmap_read_poll_timeout(regmap,
+					AR9331_SW_REG_ADDR_TABLE_FUNCTION0,
+					*f0, !(*f0 & AR9331_SW_AT_BUSY),
+					10, 2000);
+}
+
+static int ar9331_sw_port_fdb_write(struct ar9331_sw_priv *priv,
+				    u32 f0, u32 f1, u32 f2)
+{
+	struct regmap *regmap = priv->regmap;
+	int ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION2, f2);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION1, f1);
+	if (ret)
+		return ret;
+
+	return regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0, f0);
+}
+
+static int ar9331_sw_fdb_next(struct ar9331_sw_priv *priv,
+			      struct ar9331_sw_fdb *fdb, int port)
+{
+	struct regmap *regmap = priv->regmap;
+	unsigned int status, ports;
+	u32 f0, f1, f2;
+	int ret;
+
+	/* Keep AT_ADDR_BYTES4/5 to search next entry after current */
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0,
+				 AR9331_SW_AT_FUNC | AR9331_SW_AT_BUSY,
+				 AR9331_SW_AT_BUSY |
+				 FIELD_PREP(AR9331_SW_AT_FUNC,
+					    AR9331_SW_AT_FUNC_GET_NEXT));
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION2, &f2);
+	if (ret)
+		return ret;
+
+	/*
+	 * If the hardware returns an MAC != 0 and the AT_STATUS is zero, there
+	 * is no next valid entry in the address table.
+	 */
+	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
+	fdb->aging = status;
+	if (!status)
+		return 0;
+
+	ret = regmap_read(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION1, &f1);
+	if (ret)
+		return ret;
+
+	fdb->mac[0] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES0, f1);
+	fdb->mac[1] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES1, f1);
+	fdb->mac[2] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES2, f1);
+	fdb->mac[3] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES3, f1);
+	fdb->mac[4] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES4, f0);
+	fdb->mac[5] = FIELD_GET(AR9331_SW_AT_ADDR_BYTES5, f0);
+
+	ports = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
+	if (!(ports & BIT(port)))
+		return -EAGAIN;
+
+	return 0;
+}
+
+static void ar9331_sw_port_fdb_prepare(const unsigned char *mac, u32 *f0,
+				       u32 *f1, unsigned int func)
+{
+	*f1 = FIELD_PREP(AR9331_SW_AT_ADDR_BYTES0, mac[0]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES1, mac[1]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES2, mac[2]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES3, mac[3]);
+	*f0 = FIELD_PREP(AR9331_SW_AT_ADDR_BYTES4, mac[4]) |
+	      FIELD_PREP(AR9331_SW_AT_ADDR_BYTES5, mac[5]) |
+	      FIELD_PREP(AR9331_SW_AT_FUNC, func) | AR9331_SW_AT_BUSY;
+}
+
+static int ar9331_sw_port_fdb_dump(struct dsa_switch *ds, int port,
+				   dsa_fdb_dump_cb_t *cb, void *data)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	int cnt = AR9331_SW_NUM_ARL_RECORDS;
+	struct ar9331_sw_fdb _fdb = { 0 };
+	bool is_static;
+	int ret;
+	u32 f0;
+
+	/*
+	 * Make sure no pending operation is in progress. Since no timeout and
+	 * interval values are documented, we use here "seems to be sane, works
+	 * for me" values.
+	 */
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	/*
+	 * If the address and the AT_STATUS are both zero, the hardware will
+	 * search the first valid entry from entry0.
+	 * If the address is set to zero and the AT_STATUS is not zero, the
+	 * hardware will discover the next valid entry which has an address
+	 * of 0x0.
+	 */
+	ret = ar9331_sw_port_fdb_write(priv, 0, 0, 0);
+	if (ret)
+		return ret;
+
+	while (cnt--) {
+		ret = ar9331_sw_fdb_next(priv, &_fdb, port);
+		if (ret == -EAGAIN)
+			continue;
+		else if (ret)
+			return ret;
+
+		if (!_fdb.aging)
+			break;
+
+		is_static = (_fdb.aging == AR9331_SW_AT_STATUS_STATIC);
+		ret = cb(_fdb.mac, 0, is_static, data);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int ar9331_sw_port_fdb_rmw(struct ar9331_sw_priv *priv,
+				  const unsigned char *mac,
+				  u8 port_mask_set,
+				  u8 port_mask_clr)
+{
+	struct regmap *regmap = priv->regmap;
+	u32 f0, f1, f2;
+	u8 port_mask, port_mask_new, status, func;
+	int ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ar9331_sw_port_fdb_prepare(mac, &f0, &f1, AR9331_SW_AT_FUNC_FIND_MAC);
+
+	ret = ar9331_sw_port_fdb_write(priv, f0, f1, f2);
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION2, &f2);
+	if (ret)
+		return ret;
+
+	port_mask = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
+	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
+	if (status > 0 && status < AR9331_SW_AT_STATUS_STATIC) {
+		dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x\n",
+				    __func__, port_mask);
+
+		if (port_mask_set && port_mask_set != port_mask)
+			dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x, replacing it with static on %x\n",
+					    __func__, port_mask, port_mask_set);
+		port_mask = 0;
+	} else if (!status && !port_mask_set) {
+		return 0;
+	}
+
+	port_mask_new = port_mask & ~port_mask_clr;
+	port_mask_new |= port_mask_set;
+
+	if (port_mask_new == port_mask &&
+	    status == AR9331_SW_AT_STATUS_STATIC) {
+		dev_info(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
+				    __func__, port_mask_new);
+		return 0;
+	}
+
+	if (port_mask_new) {
+		func = AR9331_SW_AT_FUNC_LOAD_ENTRY;
+	} else {
+		func = AR9331_SW_AT_FUNC_PURGE_ENTRY;
+		port_mask_new = port_mask;
+	}
+
+	f2 = FIELD_PREP(AR9331_SW_AT_DES_PORT, port_mask_new) |
+		FIELD_PREP(AR9331_SW_AT_STATUS, AR9331_SW_AT_STATUS_STATIC);
+
+	ar9331_sw_port_fdb_prepare(mac, &f0, &f1, func);
+
+	ret = ar9331_sw_port_fdb_write(priv, f0, f1, f2);
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	if (f0 & AR9331_SW_AT_FULL_VIO) {
+		/* cleanup error status */
+		regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0, 0);
+		dev_err_ratelimited(priv->dev, "%s: can't add new entry, ATU is full\n", __func__);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+
+
+static int ar9331_sw_port_fdb_add(struct dsa_switch *ds, int port,
+				  const unsigned char *mac, u16 vid)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	dev_info(priv->dev, "%s(%pM, %x)\n", __func__, mac, port);
+
+	if (vid)
+		return -EINVAL;
+
+	return ar9331_sw_port_fdb_rmw(priv, mac, port_mask, 0);
+}
+
+static int ar9331_sw_port_fdb_del(struct dsa_switch *ds, int port,
+			       const unsigned char *mac, u16 vid)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (vid)
+		return -EINVAL;
+
+	return ar9331_sw_port_fdb_rmw(priv, mac, 0, port_mask);
+}
+
+static int ar9331_sw_port_mdb_add(struct dsa_switch *ds, int port,
+				  const struct switchdev_obj_port_mdb *mdb)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (mdb->vid)
+		return -EOPNOTSUPP;
+
+	return ar9331_sw_port_fdb_rmw(priv, mdb->addr, port_mask, 0);
+}
+
+static int ar9331_sw_port_mdb_del(struct dsa_switch *ds, int port,
+				  const struct switchdev_obj_port_mdb *mdb)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	u16 port_mask = BIT(port);
+
+	if (mdb->vid)
+		return -EOPNOTSUPP;
+
+	return ar9331_sw_port_fdb_rmw(priv, mdb->addr, 0, port_mask);
+}
+
+static void ar9331_sw_port_fast_age(struct dsa_switch *ds, int port)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int ret;
+	u32 f0;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		goto error;
+
+	/* Flush all non static unicast address on a given port */
+	f0 = FIELD_PREP(AR9331_SW_AT_PORT_NUM, port) |
+		FIELD_PREP(AR9331_SW_AT_FUNC, AR9331_SW_AT_FUNC_FLUSH_PORT) |
+		AR9331_SW_AT_BUSY;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0, f0);
+	if (ret)
+		goto error;
+
+	ret = ar9331_sw_fdb_wait(priv, &f0);
+	if (ret)
+		goto error;
+
+	return;
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -774,6 +1124,12 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.phylink_mac_link_down	= ar9331_sw_phylink_mac_link_down,
 	.phylink_mac_link_up	= ar9331_sw_phylink_mac_link_up,
 	.get_stats64		= ar9331_get_stats64,
+	.port_fast_age          = ar9331_sw_port_fast_age,
+	.port_fdb_del		= ar9331_sw_port_fdb_del,
+	.port_fdb_add		= ar9331_sw_port_fdb_add,
+	.port_fdb_dump		= ar9331_sw_port_fdb_dump,
+	.port_mdb_add           = ar9331_sw_port_mdb_add,
+	.port_mdb_del           = ar9331_sw_port_mdb_del,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.29.2

Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support

[Andrew Lunn]

> +static int ar9331_sw_port_fdb_rmw(struct ar9331_sw_priv *priv,
> +				  const unsigned char *mac,
> +				  u8 port_mask_set,
> +				  u8 port_mask_clr)
> +{
> +	port_mask = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
> +	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
> +	if (status > 0 && status < AR9331_SW_AT_STATUS_STATIC) {
> +		dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x\n",
> +				    __func__, port_mask);
> +
> +		if (port_mask_set && port_mask_set != port_mask)
> +			dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x, replacing it with static on %x\n",
> +					    __func__, port_mask, port_mask_set);
> +		port_mask = 0;
> +	} else if (!status && !port_mask_set) {
> +		return 0;
> +	}

As a generate rule of thumb, use rate limiting where you have no
control of the number of prints, e.g. it is triggered by packet
processing, and there is potentially a lot of them, which could DOS
the box by a remote or unprivileged attacker.

FDB changes should not happen often. Yes, root my be able to DOS the
box by doing bridge fdb add commands in a loop, but only root should
be able to do that.

Plus, i'm not actually sure we should be issuing warnings here. What
does the bridge code do in this case? Is it silent and just does it,
or does it issue a warning?




> +
> +	port_mask_new = port_mask & ~port_mask_clr;
> +	port_mask_new |= port_mask_set;
> +
> +	if (port_mask_new == port_mask &&
> +	    status == AR9331_SW_AT_STATUS_STATIC) {
> +		dev_info(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
> +				    __func__, port_mask_new);

This one should probably be dev_dbg().

     Andrew

Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 05:25:16PM +0200, Andrew Lunn wrote:
> > +static int ar9331_sw_port_fdb_rmw(struct ar9331_sw_priv *priv,
> > +				  const unsigned char *mac,
> > +				  u8 port_mask_set,
> > +				  u8 port_mask_clr)
> > +{
> > +	port_mask = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
> > +	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
> > +	if (status > 0 && status < AR9331_SW_AT_STATUS_STATIC) {
> > +		dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x\n",
> > +				    __func__, port_mask);
> > +
> > +		if (port_mask_set && port_mask_set != port_mask)
> > +			dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x, replacing it with static on %x\n",
> > +					    __func__, port_mask, port_mask_set);
> > +		port_mask = 0;
> > +	} else if (!status && !port_mask_set) {
> > +		return 0;
> > +	}
> 
> As a generate rule of thumb, use rate limiting where you have no
> control of the number of prints, e.g. it is triggered by packet
> processing, and there is potentially a lot of them, which could DOS
> the box by a remote or unprivileged attacker.
> 
> FDB changes should not happen often. Yes, root my be able to DOS the
> box by doing bridge fdb add commands in a loop, but only root should
> be able to do that.

+1
The way I see it, rate limiting should only be done when the user can't
stop the printing from spamming the console, and they just go "argh,
kill it with fire!!!" and throw the box away. As a side note, most of
the time when I can't stop the kernel from printing in a loop, the rate
limit isn't enough to stop me from wanting to throw the box out the
window, but I digress.

> Plus, i'm not actually sure we should be issuing warnings here. What
> does the bridge code do in this case? Is it silent and just does it,
> or does it issue a warning?

:D

What Oleksij doesn't know, I bet, is that he's using the bridge bypass
commands:

bridge fdb add dev lan0 00:01:02:03:04:05

That is the deprecated way of managing FDB entries, and has several
disadvantages such as:
- the bridge software FDB never gets updated with this entry, so other
  drivers which might be subscribed to DSA's FDB (imagine a non-DSA
  driver having the same logic as our ds->assisted_learning_on_cpu_port)
  will never see these FDB entries
- you have to manage duplicates yourself

The correct way to install a static bridge FDB entry is:

bridge fdb add dev lan0 00:01:02:03:04:05 master static

That will error out on duplicates for you.

From my side I would even go as far as deleting the bridge bypass
operations (.ndo_fdb_add and .ndo_fdb_del). The more integration we add
between DSA and the bridge/switchdev, the worse it will be when users
just keep using the bridge bypass. To start that process, I guess we
should start emitting a deprecation warning and then pull the trigger
after a few kernel release cycles.

> > +
> > +	port_mask_new = port_mask & ~port_mask_clr;
> > +	port_mask_new |= port_mask_set;
> > +
> > +	if (port_mask_new == port_mask &&
> > +	    status == AR9331_SW_AT_STATUS_STATIC) {
> > +		dev_info(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
> > +				    __func__, port_mask_new);
> 
> This one should probably be dev_dbg().

Or deleted, along with the overwrite logic.

Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support

[Andrew Lunn]

> > Plus, i'm not actually sure we should be issuing warnings here. What
> > does the bridge code do in this case? Is it silent and just does it,
> > or does it issue a warning?
> 
> :D
> 
> What Oleksij doesn't know, I bet, is that he's using the bridge bypass
> commands:
> 
> bridge fdb add dev lan0 00:01:02:03:04:05
> 
> That is the deprecated way of managing FDB entries, and has several
> disadvantages such as:
> - the bridge software FDB never gets updated with this entry, so other
>   drivers which might be subscribed to DSA's FDB (imagine a non-DSA
>   driver having the same logic as our ds->assisted_learning_on_cpu_port)
>   will never see these FDB entries
> - you have to manage duplicates yourself

I was actually meaning a pure software bridge, with unaccelerated
interfaces. It has a dynamic MAC address in its tables, and the user
adds a static. Ideally, we want the same behaviour.

And i think the answer is:

static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
                  const unsigned char *addr, u16 vid)
{
        struct net_bridge_fdb_entry *fdb;

        if (!is_valid_ether_addr(addr))
                return -EINVAL;

        fdb = br_fdb_find(br, addr, vid);
        if (fdb) {
                /* it is okay to have multiple ports with same
                 * address, just use the first one.
                 */
                if (test_bit(BR_FDB_LOCAL, &fdb->flags))
                        return 0;
                br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
                       source ? source->dev->name : br->dev->name, addr, vid);
                fdb_delete(br, fdb, true);
        }

        fdb = fdb_create(br, source, addr, vid,
                         BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
        if (!fdb)
                return -ENOMEM;

        fdb_add_hw_addr(br, addr);
        fdb_notify(br, fdb, RTM_NEWNEIGH, true);
        return 0;
}

So it looks like it warns and then replaces the dynamic entry.

So having the DSA driver also warn is maybe O.K. Having said that, i
don't think any other DSA driver does.

   Andrew

[PATCH net-next v1 6/9] net: dsa: qca: ar9331: add ageing time support

[Oleksij Rempel]

This switch provides global ageing time configuration, so let DSA use
it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 4a98f14f31f4..b2c22ba924f0 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -1115,6 +1115,25 @@ static void ar9331_sw_port_fast_age(struct dsa_switch *ds, int port)
 	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
 }
 
+static int ar9331_sw_set_ageing_time(struct dsa_switch *ds,
+				     unsigned int ageing_time)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 time, val;
+
+	time = DIV_ROUND_UP(ageing_time, AR9331_SW_AT_AGE_TIME_COEF);
+	if (!time)
+		time = 1;
+	else if (time > U16_MAX)
+		time = U16_MAX;
+
+	val = FIELD_PREP(AR9331_SW_AT_AGE_TIME, time) | AR9331_SW_AT_AGE_EN;
+	return regmap_update_bits(regmap, AR9331_SW_REG_ADDR_TABLE_CTRL,
+				  AR9331_SW_AT_AGE_EN | AR9331_SW_AT_AGE_TIME,
+				  val);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -1130,6 +1149,7 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.port_fdb_dump		= ar9331_sw_port_fdb_dump,
 	.port_mdb_add           = ar9331_sw_port_mdb_add,
 	.port_mdb_del           = ar9331_sw_port_mdb_del,
+	.set_ageing_time	= ar9331_sw_set_ageing_time,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
@@ -1476,6 +1496,8 @@ static int ar9331_sw_probe(struct mdio_device *mdiodev)
 	priv->ops = ar9331_sw_ops;
 	ds->ops = &priv->ops;
 	dev_set_drvdata(&mdiodev->dev, priv);
+	ds->ageing_time_min = AR9331_SW_AT_AGE_TIME_COEF;
+	ds->ageing_time_max = AR9331_SW_AT_AGE_TIME_COEF * U16_MAX;
 
 	for (i = 0; i < ARRAY_SIZE(priv->port); i++) {
 		struct ar9331_sw_port *port = &priv->port[i];
-- 
2.29.2

Re: [PATCH net-next v1 6/9] net: dsa: qca: ar9331: add ageing time support

[Andrew Lunn]

On Sat, Apr 03, 2021 at 01:48:45PM +0200, Oleksij Rempel wrote:
> This switch provides global ageing time configuration, so let DSA use
> it.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v1 6/9] net: dsa: qca: ar9331: add ageing time support

[Florian Fainelli]

On 4/3/2021 04:48, Oleksij Rempel wrote:
> This switch provides global ageing time configuration, so let DSA use
> it.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next v1 7/9] net: dsa: qca: ar9331: add bridge support

[Oleksij Rempel]

This switch is providing forwarding matrix, with it we can configure
individual bridges. Potentially we can configure more then one not VLAN
based bridge on this HW.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 73 ++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index b2c22ba924f0..bf9588574205 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -40,6 +40,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/if_bridge.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/of_mdio.h>
@@ -1134,6 +1135,76 @@ static int ar9331_sw_set_ageing_time(struct dsa_switch *ds,
 				  val);
 }
 
+static int ar9331_sw_port_bridge_join(struct dsa_switch *ds, int port,
+				      struct net_device *br)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int port_mask = BIT(priv->cpu_port);
+	int i, ret;
+	u32 val;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_to_port(ds, i)->bridge_dev != br)
+			continue;
+
+		if (!dsa_is_user_port(ds, port))
+			continue;
+
+		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
+		ret = regmap_set_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
+		if (ret)
+			goto error;
+
+		if (i != port)
+			port_mask |= BIT(i);
+	}
+
+	val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask);
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				 AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, val);
+	if (ret)
+		goto error;
+
+	return 0;
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
+
+	return ret;
+}
+
+static void ar9331_sw_port_bridge_leave(struct dsa_switch *ds, int port,
+					struct net_device *br)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int i, ret;
+	u32 val;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_to_port(ds, i)->bridge_dev != br)
+			continue;
+
+		if (!dsa_is_user_port(ds, port))
+			continue;
+
+		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
+		ret = regmap_clear_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
+		if (ret)
+			goto error;
+	}
+
+	val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(priv->cpu_port));
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				 AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, val);
+	if (ret)
+		goto error;
+
+	return;
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -1150,6 +1221,8 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.port_mdb_add           = ar9331_sw_port_mdb_add,
 	.port_mdb_del           = ar9331_sw_port_mdb_del,
 	.set_ageing_time	= ar9331_sw_set_ageing_time,
+	.port_bridge_join	= ar9331_sw_port_bridge_join,
+	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.29.2

Re: [PATCH net-next v1 7/9] net: dsa: qca: ar9331: add bridge support

[Andrew Lunn]

On Sat, Apr 03, 2021 at 01:48:46PM +0200, Oleksij Rempel wrote:
> This switch is providing forwarding matrix, with it we can configure
> individual bridges. Potentially we can configure more then one not VLAN
> based bridge on this HW.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 73 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index b2c22ba924f0..bf9588574205 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -40,6 +40,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/if_bridge.h>
>  #include <linux/module.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_mdio.h>
> @@ -1134,6 +1135,76 @@ static int ar9331_sw_set_ageing_time(struct dsa_switch *ds,
>  				  val);
>  }
>  
> +static int ar9331_sw_port_bridge_join(struct dsa_switch *ds, int port,
> +				      struct net_device *br)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int port_mask = BIT(priv->cpu_port);
> +	int i, ret;
> +	u32 val;
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		if (dsa_to_port(ds, i)->bridge_dev != br)
> +			continue;
> +
> +		if (!dsa_is_user_port(ds, port))
> +			continue;
> +
> +		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
> +		ret = regmap_set_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
> +		if (ret)
> +			goto error;
> +
> +		if (i != port)
> +			port_mask |= BIT(i);
> +	}
> +
> +	val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask);
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				 AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, val);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static void ar9331_sw_port_bridge_leave(struct dsa_switch *ds, int port,
> +					struct net_device *br)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int i, ret;
> +	u32 val;
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		if (dsa_to_port(ds, i)->bridge_dev != br)
> +			continue;
> +
> +		if (!dsa_is_user_port(ds, port))
> +			continue;
> +
> +		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
> +		ret = regmap_clear_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
> +		if (ret)
> +			goto error;
> +	}

Join and leave only seems to differ by:

> +		if (i != port)
> +			port_mask |= BIT(i);

Maybe refactor the code to add a helper for the identical parts?

      Andrew

Re: [PATCH net-next v1 7/9] net: dsa: qca: ar9331: add bridge support

[Florian Fainelli]

On 4/3/2021 04:48, Oleksij Rempel wrote:
> This switch is providing forwarding matrix, with it we can configure
> individual bridges. Potentially we can configure more then one not VLAN

s/then/than/

> based bridge on this HW.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>   drivers/net/dsa/qca/ar9331.c | 73 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index b2c22ba924f0..bf9588574205 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -40,6 +40,7 @@
>    */
>   
>   #include <linux/bitfield.h>
> +#include <linux/if_bridge.h>
>   #include <linux/module.h>
>   #include <linux/of_irq.h>
>   #include <linux/of_mdio.h>
> @@ -1134,6 +1135,76 @@ static int ar9331_sw_set_ageing_time(struct dsa_switch *ds,
>   				  val);
>   }
>   
> +static int ar9331_sw_port_bridge_join(struct dsa_switch *ds, int port,
> +				      struct net_device *br)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int port_mask = BIT(priv->cpu_port);
> +	int i, ret;
> +	u32 val;
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		if (dsa_to_port(ds, i)->bridge_dev != br)
> +			continue;
> +
> +		if (!dsa_is_user_port(ds, port))
> +			continue;
> +
> +		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
> +		ret = regmap_set_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
> +		if (ret)
> +			goto error;
> +
> +		if (i != port)
> +			port_mask |= BIT(i);
> +	}
> +
> +	val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, port_mask);
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				 AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, val);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);

This is not called more than once per port and per bridge join operation 
so I would drop the rate limiting here. With that fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next v1 8/9] net: dsa: qca: ar9331: add STP support

[Oleksij Rempel]

According to the datasheet, this switch has configurable STP port
states. Suddenly LISTENING and BLOCKING states didn't forwarded packets
to the CPU and linux bridge continuously re enabled ports even if a  loop
was detected. To make it work, I reused bridge functionality to isolate
port in LISTENING and BLOCKING states.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 69 ++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index bf9588574205..83b59e771a5f 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -327,6 +327,7 @@ struct ar9331_sw_priv {
 	struct reset_control *sw_reset;
 	struct ar9331_sw_port port[AR9331_SW_PORTS];
 	int cpu_port;
+	u32 isolated_ports;
 };
 
 static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
@@ -1151,6 +1152,10 @@ static int ar9331_sw_port_bridge_join(struct dsa_switch *ds, int port,
 		if (!dsa_is_user_port(ds, port))
 			continue;
 
+		/* part of the bridge but should be isolated for now */
+		if (priv->isolated_ports & BIT(i))
+			continue;
+
 		val = FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID_MEMBER, BIT(port));
 		ret = regmap_set_bits(regmap, AR9331_SW_REG_PORT_VLAN(i), val);
 		if (ret)
@@ -1205,6 +1210,69 @@ static void ar9331_sw_port_bridge_leave(struct dsa_switch *ds, int port,
 	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
 }
 
+static void ar9331_sw_port_stp_state_set(struct dsa_switch *ds, int port,
+					 u8 state)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct net_device *br = dsa_to_port(ds, port)->bridge_dev;
+	struct regmap *regmap = priv->regmap;
+	u32 port_ctrl = 0, port_state = 0;
+	bool join = false;
+	int ret;
+
+	/*
+	 * STP hw support is buggy or I didn't understood it. So, it seems to
+	 * be easier to make hand crafted implementation by using bridge
+	 * functionality. Similar implementation can be found on ksz9477 switch
+	 * and may be we need some generic code to so for all related devices
+	 */
+	switch (state) {
+	case BR_STATE_FORWARDING:
+		join = true;
+		fallthrough;
+	case BR_STATE_LEARNING:
+		port_ctrl = AR9331_SW_PORT_CTRL_LEARN_EN;
+		fallthrough;
+	case BR_STATE_LISTENING:
+	case BR_STATE_BLOCKING:
+		port_state = AR9331_SW_PORT_CTRL_PORT_STATE_FORWARD;
+		break;
+	case BR_STATE_DISABLED:
+	default:
+		port_state = AR9331_SW_PORT_CTRL_PORT_STATE_DISABLED;
+		break;
+	}
+
+	port_ctrl |= FIELD_PREP(AR9331_SW_PORT_CTRL_PORT_STATE, port_state);
+
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_CTRL(port),
+				 AR9331_SW_PORT_CTRL_LEARN_EN |
+				 AR9331_SW_PORT_CTRL_PORT_STATE, port_ctrl);
+	if (ret)
+		goto error;
+
+	if (!dsa_is_user_port(ds, port))
+		return;
+
+	/*
+	 * here we care only about user ports. CPU port do not need this
+	 * configuration
+	 */
+	if (join) {
+		priv->isolated_ports &= ~BIT(port);
+		if (br)
+			ar9331_sw_port_bridge_join(ds, port, br);
+	} else {
+		priv->isolated_ports |= BIT(port);
+		if (br)
+			ar9331_sw_port_bridge_leave(ds, port, br);
+	}
+
+	return;
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -1223,6 +1291,7 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.set_ageing_time	= ar9331_sw_set_ageing_time,
 	.port_bridge_join	= ar9331_sw_port_bridge_join,
 	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
+	.port_stp_state_set	= ar9331_sw_port_stp_state_set,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.29.2

[PATCH net-next v1 9/9] net: dsa: qca: ar9331: add vlan support

[Oleksij Rempel]

This switch provides simple VLAN resolution database for 16 entries (VLANs).
With this database we can cover typical functionalities as port based
VLANs, untagged and tagged egress. Port based ingress filtering.

The VLAN database is working on top of forwarding database. So,
potentially, we can have multiple VLANs on top of multiple bridges.
Hawing one VLAN on top of multiple bridges will fail on different
levels, most probably DSA framework should warn if some one wont to make
something likes this.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 255 +++++++++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 83b59e771a5f..40062388d4a7 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -67,6 +67,27 @@
 #define AR9331_SW_REG_GLOBAL_CTRL		0x30
 #define AR9331_SW_GLOBAL_CTRL_MFS_M		GENMASK(13, 0)
 
+#define AR9331_SW_NUM_VLAN_RECORDS		16
+
+#define AR9331_SW_REG_VLAN_TABLE_FUNCTION0	0x40
+#define AR9331_SW_VT0_PRI_EN			BIT(31)
+#define AR9331_SW_VT0_PRI			GENMASK(30, 28)
+#define AR9331_SW_VT0_VID			GENMASK(27, 16)
+#define AR9331_SW_VT0_PORT_NUM			GENMASK(11, 8)
+#define AR9331_SW_VT0_FULL_VIO			BIT(4)
+#define AR9331_SW_VT0_BUSY			BIT(3)
+#define AR9331_SW_VT0_FUNC			GENMASK(2, 0)
+#define AR9331_SW_VT0_FUNC_NOP			0
+#define AR9331_SW_VT0_FUNC_FLUSH_ALL		1
+#define AR9331_SW_VT0_FUNC_LOAD_ENTRY		2
+#define AR9331_SW_VT0_FUNC_PURGE_ENTRY		3
+#define AR9331_SW_VT0_FUNC_DEL_PORT		4
+#define AR9331_SW_VT0_FUNC_GET_NEXT		5
+
+#define AR9331_SW_REG_VLAN_TABLE_FUNCTION1	0x44
+#define AR9331_SW_VT1_VALID			BIT(11)
+#define AR9331_SW_VT1_VID_MEM			GENMASK(9, 0)
+
 /* Size of the address resolution table (ARL) */
 #define AR9331_SW_NUM_ARL_RECORDS		1024
 
@@ -308,6 +329,11 @@ struct ar9331_sw_port {
 	struct spinlock stats_lock;
 };
 
+struct ar9331_sw_vlan_db {
+	u16 vid;
+	u8 port_mask;
+};
+
 struct ar9331_sw_fdb {
 	u8 port_mask;
 	u8 aging;
@@ -328,6 +354,7 @@ struct ar9331_sw_priv {
 	struct ar9331_sw_port port[AR9331_SW_PORTS];
 	int cpu_port;
 	u32 isolated_ports;
+	struct ar9331_sw_vlan_db vdb[AR9331_SW_NUM_VLAN_RECORDS];
 };
 
 static struct ar9331_sw_priv *ar9331_sw_port_to_priv(struct ar9331_sw_port *port)
@@ -1273,6 +1300,231 @@ static void ar9331_sw_port_stp_state_set(struct dsa_switch *ds, int port,
 	dev_err_ratelimited(priv->dev, "%s: error: %i\n", __func__, ret);
 }
 
+static int ar9331_port_vlan_filtering(struct dsa_switch *ds, int port,
+				      bool vlan_filtering,
+				      struct netlink_ext_ack *extack)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 mode;
+	int ret;
+
+	if (vlan_filtering)
+		mode = AR9331_SW_8021Q_MODE_SECURE;
+	else
+		mode = AR9331_SW_8021Q_MODE_NONE;
+
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				 AR9331_SW_PORT_VLAN_8021Q_MODE,
+				 FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
+					    mode));
+	if (ret)
+		dev_err_ratelimited(priv->dev, "%s: error: %pe\n",
+				    __func__, ERR_PTR(ret));
+
+	return ret;
+}
+
+static int ar9331_sw_vt_wait(struct ar9331_sw_priv *priv, u32 *f0)
+{
+	struct regmap *regmap = priv->regmap;
+
+	return regmap_read_poll_timeout(regmap,
+				        AR9331_SW_REG_VLAN_TABLE_FUNCTION0,
+				        *f0, !(*f0 & AR9331_SW_VT0_BUSY),
+				        100, 2000);
+}
+
+static int ar9331_sw_port_vt_rmw(struct ar9331_sw_priv *priv, u16 vid,
+				 u8 port_mask_set, u8 port_mask_clr)
+{
+	struct regmap *regmap = priv->regmap;
+	u32 f0, f1, port_mask = 0, port_mask_new, func;
+	struct ar9331_sw_vlan_db *vdb = NULL;
+	int ret, i;
+
+	if (!vid)
+		return 0;
+
+	ret = ar9331_sw_vt_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
+	if (ret)
+		goto error;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, 0);
+	if (ret)
+		goto error;
+
+	for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
+		if (priv->vdb[i].vid == vid) {
+			vdb = &priv->vdb[i];
+			break;
+		}
+	}
+
+	ret = regmap_read(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, &f1);
+	if (ret)
+		return ret;
+
+	if (vdb) {
+		port_mask = vdb->port_mask;
+	}
+
+	port_mask_new = port_mask & ~port_mask_clr;
+	port_mask_new |= port_mask_set;
+
+	if (port_mask_new && port_mask_new == port_mask) {
+		dev_info(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
+				    __func__, port_mask_new);
+		return 0;
+	}
+
+	if (port_mask_new) {
+		func = AR9331_SW_VT0_FUNC_LOAD_ENTRY;
+	} else {
+		func = AR9331_SW_VT0_FUNC_PURGE_ENTRY;
+		port_mask_new = port_mask;
+	}
+
+	if (vdb) {
+		vdb->port_mask = port_mask_new;
+
+		if (!port_mask_new)
+			vdb->vid = 0;
+	} else {
+		for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
+			if (!priv->vdb[i].vid) {
+				vdb = &priv->vdb[i];
+				break;
+			}
+		}
+
+		if (!vdb) {
+			dev_err_ratelimited(priv->dev, "Local VDB is full\n");
+			return -ENOMEM;
+		}
+		vdb->vid = vid;
+		vdb->port_mask = port_mask_new;
+	}
+
+	f0 = FIELD_PREP(AR9331_SW_VT0_VID, vid) |
+	     FIELD_PREP(AR9331_SW_VT0_FUNC, func) |
+	     AR9331_SW_VT0_BUSY;
+	f1 = FIELD_PREP(AR9331_SW_VT1_VID_MEM, port_mask_new) |
+		AR9331_SW_VT1_VALID;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, f1);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, f0);
+	if (ret)
+		return ret;
+
+	ret = ar9331_sw_vt_wait(priv, &f0);
+	if (ret)
+		return ret;
+
+	if (f0 & AR9331_SW_VT0_FULL_VIO) {
+		/* cleanup error status */
+		regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
+		dev_err_ratelimited(priv->dev, "%s: can't add new entry, VT is full\n", __func__);
+		return -ENOMEM;
+	}
+
+	return 0;
+
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %pe\n", __func__,
+			    ERR_PTR(ret));
+
+	return ret;
+}
+
+static int ar9331_port_vlan_set_pvid(struct ar9331_sw_priv *priv, int port,
+				     u16 pvid)
+{
+	struct regmap *regmap = priv->regmap;
+	int ret;
+	u32 mask, val;
+
+	mask = AR9331_SW_PORT_VLAN_8021Q_MODE |
+		AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
+		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN;
+	val = AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
+		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN |
+		FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
+			   AR9331_SW_8021Q_MODE_FALLBACK);
+
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				 mask, val);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
+				  AR9331_SW_PORT_VLAN_PORT_VID,
+				  FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
+					     pvid));
+}
+
+static int ar9331_port_vlan_add(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_vlan *vlan,
+				struct netlink_ext_ack *extack)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct regmap *regmap = priv->regmap;
+	int ret, mode;
+
+	ret = ar9331_sw_port_vt_rmw(priv, vlan->vid, BIT(port), 0);
+	if (ret)
+		goto error;
+
+	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
+		ret = ar9331_port_vlan_set_pvid(priv, port, vlan->vid);
+
+	if (ret)
+		goto error;
+
+	if (vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED)
+		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_STRIP;
+	else
+		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_ADD;
+
+	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_CTRL(port),
+				 AR9331_SW_PORT_CTRL_EG_VLAN_MODE, mode);
+	if (ret)
+		goto error;
+
+	return 0;
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %pe\n", __func__,
+			    ERR_PTR(ret));
+
+	return ret;
+}
+
+static int ar9331_port_vlan_del(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_vlan *vlan)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	int ret;
+
+	ret = ar9331_sw_port_vt_rmw(priv, vlan->vid, 0, BIT(port));
+	if (ret)
+		goto error;
+
+	return 0;
+
+error:
+	dev_err_ratelimited(priv->dev, "%s: error: %pe\n", __func__,
+			    ERR_PTR(ret));
+
+	return ret;
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -1292,6 +1544,9 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.port_bridge_join	= ar9331_sw_port_bridge_join,
 	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
 	.port_stp_state_set	= ar9331_sw_port_stp_state_set,
+	.port_vlan_filtering	= ar9331_port_vlan_filtering,
+	.port_vlan_add		= ar9331_port_vlan_add,
+	.port_vlan_del		= ar9331_port_vlan_del,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
-- 
2.29.2

Re: [PATCH net-next v1 9/9] net: dsa: qca: ar9331: add vlan support

[Vladimir Oltean]

On Sat, Apr 03, 2021 at 01:48:48PM +0200, Oleksij Rempel wrote:
> This switch provides simple VLAN resolution database for 16 entries (VLANs).
> With this database we can cover typical functionalities as port based
> VLANs, untagged and tagged egress. Port based ingress filtering.
> 
> The VLAN database is working on top of forwarding database. So,

Define 'on top'.

> potentially, we can have multiple VLANs on top of multiple bridges.
> Hawing one VLAN on top of multiple bridges will fail on different

s/Hawing/Having/

> levels, most probably DSA framework should warn if some one wont to make

s/wont/wants/
s/some one/someone/

> something likes this.

Finally, why should the DSA framework warn?
Even in the default configuration of two bridges, the default_pvid (1)
will be the same. What problems do you have with that?

In commit 0ee2af4ebbe3 ("net: dsa: set configure_vlan_while_not_filtering
to true by default"), I did not notice that ar9331 does not have VLAN
operations, and I mistakenly set ds->configure_vlan_while_not_filtering
= false for your driver. Could you please delete that line and ensure the
following works?

ip link add br0 type bridge
ip link set lan0 master br0
bridge vlan add dev lan0 vid 100
ip link set br0 type bridge vlan_filtering 1
# make sure you can receive traffic with VLAN 100

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 255 +++++++++++++++++++++++++++++++++++
>  1 file changed, 255 insertions(+)
> 
> +static int ar9331_sw_vt_wait(struct ar9331_sw_priv *priv, u32 *f0)
> +{
> +	struct regmap *regmap = priv->regmap;
> +
> +	return regmap_read_poll_timeout(regmap,
> +				        AR9331_SW_REG_VLAN_TABLE_FUNCTION0,
> +				        *f0, !(*f0 & AR9331_SW_VT0_BUSY),
> +				        100, 2000);
> +}
> +
> +static int ar9331_sw_port_vt_rmw(struct ar9331_sw_priv *priv, u16 vid,
> +				 u8 port_mask_set, u8 port_mask_clr)
> +{
> +	struct regmap *regmap = priv->regmap;
> +	u32 f0, f1, port_mask = 0, port_mask_new, func;
> +	struct ar9331_sw_vlan_db *vdb = NULL;
> +	int ret, i;
> +
> +	if (!vid)
> +		return 0;
> +
> +	ret = ar9331_sw_vt_wait(priv, &f0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
> +	if (ret)
> +		goto error;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, 0);
> +	if (ret)
> +		goto error;
> +
> +	for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
> +		if (priv->vdb[i].vid == vid) {
> +			vdb = &priv->vdb[i];
> +			break;
> +		}
> +	}
> +
> +	ret = regmap_read(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, &f1);
> +	if (ret)
> +		return ret;
> +
> +	if (vdb) {
> +		port_mask = vdb->port_mask;
> +	}
> +
> +	port_mask_new = port_mask & ~port_mask_clr;
> +	port_mask_new |= port_mask_set;
> +
> +	if (port_mask_new && port_mask_new == port_mask) {
> +		dev_info(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
> +				    __func__, port_mask_new);

With VLANs, the bridge is indeed much less strict compared to FDBs, due
to the old API having ranges baked in (which were never used).

That being said, is there actually any value in this message? Would you
mind deleting it (I see how it could annoy a user)?

You might want to look at devlink regions if you want to debug the VLAN
table of the hardware.

> +		return 0;
> +	}
> +
> +	if (port_mask_new) {
> +		func = AR9331_SW_VT0_FUNC_LOAD_ENTRY;
> +	} else {
> +		func = AR9331_SW_VT0_FUNC_PURGE_ENTRY;
> +		port_mask_new = port_mask;
> +	}
> +
> +	if (vdb) {
> +		vdb->port_mask = port_mask_new;
> +
> +		if (!port_mask_new)
> +			vdb->vid = 0;
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(priv->vdb); i++) {
> +			if (!priv->vdb[i].vid) {
> +				vdb = &priv->vdb[i];
> +				break;
> +			}
> +		}
> +
> +		if (!vdb) {
> +			dev_err_ratelimited(priv->dev, "Local VDB is full\n");

You have a netlink extack at your disposal, use it.

> +			return -ENOMEM;
> +		}
> +		vdb->vid = vid;
> +		vdb->port_mask = port_mask_new;
> +	}
> +
> +	f0 = FIELD_PREP(AR9331_SW_VT0_VID, vid) |
> +	     FIELD_PREP(AR9331_SW_VT0_FUNC, func) |
> +	     AR9331_SW_VT0_BUSY;
> +	f1 = FIELD_PREP(AR9331_SW_VT1_VID_MEM, port_mask_new) |
> +		AR9331_SW_VT1_VALID;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION1, f1);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, f0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ar9331_sw_vt_wait(priv, &f0);
> +	if (ret)
> +		return ret;
> +
> +	if (f0 & AR9331_SW_VT0_FULL_VIO) {
> +		/* cleanup error status */
> +		regmap_write(regmap, AR9331_SW_REG_VLAN_TABLE_FUNCTION0, 0);
> +		dev_err_ratelimited(priv->dev, "%s: can't add new entry, VT is full\n", __func__);

Similar comment as above.

> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +
> +error:
> +	dev_err_ratelimited(priv->dev, "%s: error: %pe\n", __func__,
> +			    ERR_PTR(ret));
> +
> +	return ret;
> +}
> +
> +static int ar9331_port_vlan_set_pvid(struct ar9331_sw_priv *priv, int port,
> +				     u16 pvid)
> +{
> +	struct regmap *regmap = priv->regmap;
> +	int ret;
> +	u32 mask, val;
> +
> +	mask = AR9331_SW_PORT_VLAN_8021Q_MODE |
> +		AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
> +		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN;
> +	val = AR9331_SW_PORT_VLAN_FORCE_DEFALUT_VID_EN |
> +		AR9331_SW_PORT_VLAN_FORCE_PORT_VLAN_EN |
> +		FIELD_PREP(AR9331_SW_PORT_VLAN_8021Q_MODE,
> +			   AR9331_SW_8021Q_MODE_FALLBACK);
> +
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				 mask, val);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(regmap, AR9331_SW_REG_PORT_VLAN(port),
> +				  AR9331_SW_PORT_VLAN_PORT_VID,
> +				  FIELD_PREP(AR9331_SW_PORT_VLAN_PORT_VID,
> +					     pvid));
> +}
> +
> +static int ar9331_port_vlan_add(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_port_vlan *vlan,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;

You don't need to cast a void pointer in the C language.

> +	struct regmap *regmap = priv->regmap;
> +	int ret, mode;
> +
> +	ret = ar9331_sw_port_vt_rmw(priv, vlan->vid, BIT(port), 0);
> +	if (ret)
> +		goto error;
> +
> +	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
> +		ret = ar9331_port_vlan_set_pvid(priv, port, vlan->vid);
> +
> +	if (ret)
> +		goto error;
> +
> +	if (vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED)
> +		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_STRIP;
> +	else
> +		mode = AR9331_SW_PORT_CTRL_EG_VLAN_MODE_ADD;
> +
> +	ret = regmap_update_bits(regmap, AR9331_SW_REG_PORT_CTRL(port),
> +				 AR9331_SW_PORT_CTRL_EG_VLAN_MODE, mode);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	dev_err_ratelimited(priv->dev, "%s: error: %pe\n", __func__,
> +			    ERR_PTR(ret));
> +
> +	return ret;
> +}
> +
> +static int ar9331_port_vlan_del(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	int ret;
> +
> +	ret = ar9331_sw_port_vt_rmw(priv, vlan->vid, 0, BIT(port));
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +
> +error:
> +	dev_err_ratelimited(priv->dev, "%s: error: %pe\n", __func__,
> +			    ERR_PTR(ret));
> +
> +	return ret;
> +}

This looks like a whole lot of boilerplate compared to:

	ret = vlan_table_read_modify_write
	if (ret)
		print_error

	return ret

> +
>  static const struct dsa_switch_ops ar9331_sw_ops = {
>  	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
>  	.setup			= ar9331_sw_setup,
> @@ -1292,6 +1544,9 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
>  	.port_bridge_join	= ar9331_sw_port_bridge_join,
>  	.port_bridge_leave	= ar9331_sw_port_bridge_leave,
>  	.port_stp_state_set	= ar9331_sw_port_stp_state_set,
> +	.port_vlan_filtering	= ar9331_port_vlan_filtering,
> +	.port_vlan_add		= ar9331_port_vlan_add,
> +	.port_vlan_del		= ar9331_port_vlan_del,
>  };
>  
>  static irqreturn_t ar9331_sw_irq(int irq, void *data)
> -- 
> 2.29.2
>