Am 04.04.21 um 02:16 schrieb Vladimir Oltean:

On Sat, Apr 03, 2021 at 01:48:43PM +0200, Oleksij Rempel wrote:

quoted

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...

quoted

+		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.

quoted

+		/* 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

quoted

+			  | 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