[[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

This patch series provides support for HSR HW offloading in KSZ9477
switch IC.

To test this feature:
ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
ip link set dev lan1 up
ip link set dev lan2 up
ip a add 192.168.0.1/24 dev hsr0
ip link set dev hsr0 up

To remove HSR network device:
ip link del hsr0

It is also possible to create another HSR interface, but it will
only support HSR is software - e.g.
ip link add name hsr1 type hsr slave1 lan3 slave2 lan4 supervision 45 version 1

Test HW:
Two KSZ9477-EVB boards with HSR ports set to "Port1" and "Port2".

Performance SW used:
nuttcp -S --nofork
nuttcp -vv -T 60 -r 192.168.0.2
nuttcp -vv -T 60 -t 192.168.0.2

Code: v6.5-rc7 Linux repository
Tested HSR v0 and v1
Results:
With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps
With no offloading 		       RX: 63 Mbps  TX: 63 Mbps


Lukasz Majewski (2):
  net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication
  net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

 drivers/net/dsa/microchip/ksz9477.c    | 81 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h    |  2 +
 drivers/net/dsa/microchip/ksz_common.c | 76 ++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  3 +
 net/dsa/tag_ksz.c                      |  8 +++
 5 files changed, 170 insertions(+)

-- 
2.20.1

[[RFC PATCH v4 net-next] 1/2] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication

[Lukasz Majewski]

The KSZ9477 has support for HSR (High-Availability Seamless Redundancy).
One of its offloading (i.e. performed in the switch IC hardware) features
is to duplicate received frame to both HSR aware switch ports.

To achieve this goal - the tail TAG needs to be modified. To be more
specific, both ports must be marked as destination (egress) ones.

The NETIF_F_HW_HSR_DUP flag indicates that the device supports HSR and
assures (in HSR core code) that frame is sent only once from HOST to
switch with tail tag indicating both ports.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Use ksz_hsr_get_ports() to obtain the bits values corresponding to
  HSR aware ports

Changes for v3:
- None

Changes for v4:
- Iterate over switch ports to find ones supporting HSR. Comparing to v3,
  where caching of egress tag bits were used, no noticeable performance
  regression has been observed.
---
 net/dsa/tag_ksz.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index ea100bd25939..3632e47dea9e 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -293,6 +293,14 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 	if (is_link_local_ether_addr(hdr->h_dest))
 		val |= KSZ9477_TAIL_TAG_OVERRIDE;
 
+	if (dev->features & NETIF_F_HW_HSR_DUP) {
+		struct net_device *hsr_dev = dp->hsr_dev;
+		struct dsa_port *other_dp;
+
+		dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
+			val |= BIT(other_dp->index);
+	}
+
 	*tag = cpu_to_be16(val);
 
 	return ksz_defer_xmit(dp, skb);
-- 
2.20.1

[[RFC PATCH v4 net-next] 2/2] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Lukasz Majewski]

This patch adds functions for providing in KSZ9477 switch HSR
(High-availability Seamless Redundancy) hardware offloading.

According to AN3474 application note following features are provided:
- TX packet duplication from host to switch (NETIF_F_HW_HSR_DUP)
- RX packet duplication discarding
- Prevention of packet loop

For last two ones - there is a probability that some packets will not
be filtered in HW (in some special cases - described in AN3474).
Hence, the HSR core code shall be used to discard those not caught frames.

Moreover, some switch registers adjustments are required - like setting
MAC address of HSR network interface.

Additionally, the KSZ9477 switch has been configured to forward frames
between HSR ports (e.g. 1,2) members to provide support for
NETIF_F_HW_HSR_FWD flag.

Join and leave functions are written in a way, that are executed with
single port - i.e. configuration is NOT done only when second HSR port
is configured.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Use struct ksz_device to store hsr ports information (not struct dsa)

Changes for v3:
- Enable in-switch forwarding of frames between HSR ports (i.e. enable
  bridging of those two ports)

- The NETIF_F_HW_HSR_FWD flag has been marked as supported by the HSR
  network device

- Remove ETH MAC address validity check as it is done earlier in the net
  driver

- Add comment regarding adding support for NETIF_F_HW_HSR_FWD flag

Changes for v4:
- Merge patches for ksz_common.c and ksz9477.c

- Remove code to set global self-address filtering as this bit has
  already been set at ksz9477_reset_switch() function. Update also
  comment.

- Use already available ksz9477_cfg_port_member() instead of ksz_prmw32()

- Add description about chip limitations

- Allow having only one offloaded hsr interface (e.g. hsr0). Other ones
  (like hsr1) will have only SW HSR support

- Add check if MAC address of HSR device is equal to one from DSA master
- Rewrite the code to support per port join (i.e. not making init only
  when second HSR port is join)
- Add check to allow only one HSR port HW offloading
- Add hsr_ports to ksz_device struct to allow clean removal of network
  interfaces composing hsr device
---
 drivers/net/dsa/microchip/ksz9477.c    | 81 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h    |  2 +
 drivers/net/dsa/microchip/ksz_common.c | 76 ++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  3 +
 4 files changed, 162 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 83b7f2d5c1ea..f36bc427c468 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1141,6 +1141,87 @@ int ksz9477_tc_cbs_set_cinc(struct ksz_device *dev, int port, u32 val)
 	return ksz_pwrite16(dev, port, REG_PORT_MTI_CREDIT_INCREMENT, val);
 }
 
+/* The KSZ9477 provides following HW features to accelerate
+ * HSR frames handling:
+ *
+ * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH
+ * 2. RX PACKET DUPLICATION DISCARDING
+ * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS FILTERING
+ *
+ * Only one from point 1. has the NETIF_F* flag available.
+ *
+ * Ones from point 2 and 3 are "best effort" - i.e. those will
+ * work correctly most of the time, but it may happen that some
+ * frames will not be caught - to be more specific; there is a race
+ * condition in hardware such that, when duplicate packets are received
+ * on member ports very close in time to each other, the hardware fails
+ * to detect that they are duplicates.
+ *
+ * Hence, the SW needs to handle those special cases. However, the speed
+ * up gain is considerable when above features are used.
+ *
+ * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, as HSR frames
+ * can be forwarded in the switch fabric between HSR ports.
+ */
+#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_FWD)
+
+void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr)
+{
+	struct ksz_device *dev = ds->priv;
+	struct net_device *slave;
+	u8 i, data;
+
+	/* Program which port(s) shall support HSR */
+	ksz_rmw32(dev, REG_HSR_PORT_MAP__4, BIT(port), BIT(port));
+
+	/* Forward frames between HSR ports (i.e. bridge together HSR ports) */
+	ksz9477_cfg_port_member(dev, port,
+				BIT(dsa_upstream_port(ds, port)) | BIT(port));
+
+	if (!dev->hsr_ports) {
+		/* Enable discarding of received HSR frames */
+		ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data);
+		data |= HSR_DUPLICATE_DISCARD;
+		data &= ~HSR_NODE_UNICAST;
+		ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data);
+
+		/* Self MAC address filtering for HSR frames to avoid
+		 * traverse of the HSR ring more than once.
+		 *
+		 * The HSR port (i.e. hsr0) MAC address is used.
+		 */
+		for (i = 0; i < ETH_ALEN; i++)
+			ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
+				   hsr->dev_addr[i]);
+	}
+
+	/* Enable per port self-address filtering.
+	 * The global self-address filtering has already been enabled in the
+	 * ksz9477_reset_switch() function.
+	 */
+	ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, true);
+
+	/* Setup HW supported features for lan HSR ports */
+	slave = dsa_to_port(ds, port)->slave;
+	slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES;
+
+	pr_debug("%s: HSR join port: %d\n", __func__,  port);
+}
+
+void ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr)
+{
+	struct ksz_device *dev = ds->priv;
+
+	/* Clear port HSR support */
+	ksz_rmw32(dev, REG_HSR_PORT_MAP__4, BIT(port), 0);
+
+	/* Disable forwarding frames between HSR ports */
+	ksz9477_cfg_port_member(dev, port, BIT(dsa_upstream_port(ds, port)));
+
+	/* Disable per port self-address filtering */
+	ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, false);
+}
+
 int ksz9477_switch_init(struct ksz_device *dev)
 {
 	u8 data8;
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index a6f425866a29..8625bf474764 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -56,5 +56,7 @@ int ksz9477_reset_switch(struct ksz_device *dev);
 int ksz9477_switch_init(struct ksz_device *dev);
 void ksz9477_switch_exit(struct ksz_device *dev);
 void ksz9477_port_queue_split(struct ksz_device *dev, int port);
+void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr);
+void ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr);
 
 #endif
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 42db7679c360..b81c3ac422f9 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -16,6 +16,7 @@
 #include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
 #include <linux/if_vlan.h>
+#include <linux/if_hsr.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/of.h>
@@ -3419,6 +3420,79 @@ static int ksz_setup_tc(struct dsa_switch *ds, int port,
 	}
 }
 
+static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr)
+{
+	struct net_device *dm = dsa_port_to_master(dsa_to_port(ds, port));
+	struct ksz_device *dev = ds->priv;
+	enum hsr_version ver;
+	int ret;
+
+	ret = hsr_get_version(hsr, &ver);
+	if (ret)
+		return ret;
+
+	/* Check if HSR net_device's MAC address equals to DSA master.
+	 *
+	 * Only in that way one can assure correct operation between
+	 * different switch features - like WoL, PAUSE and HSR, which
+	 * are using in-switch programmed MAC address.
+	 */
+	if (!ether_addr_equal(dm->dev_addr, hsr->dev_addr)) {
+		dev_err(dev->dev,
+			"DSA master and HSR dev MAC must equal for offloading");
+		return -EOPNOTSUPP;
+	}
+
+	switch (dev->chip_id) {
+	case KSZ9477_CHIP_ID:
+		/* KSZ9477 can support HW offloading of only 1 HSR device */
+		if (dev->hsr_dev && hsr != dev->hsr_dev) {
+			dev_err(dev->dev, "Offload supported for a single HSR");
+			return -EOPNOTSUPP;
+		}
+
+		/* KSZ9477 only supports HSR v0 and v1 */
+		if (!(ver == HSR_V0 || ver == HSR_V1)) {
+			dev_err(dev->dev, "Only HSR v0 and v1 supported");
+			return -EOPNOTSUPP;
+		}
+
+		ksz9477_hsr_join(ds, port, hsr);
+		dev->hsr_dev = hsr;
+		dev->hsr_ports |= BIT(port);
+
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int ksz_hsr_leave(struct dsa_switch *ds, int port,
+			 struct net_device *hsr)
+{
+	struct ksz_device *dev = ds->priv;
+	int ret = 0;
+
+	switch (dev->chip_id) {
+	case KSZ9477_CHIP_ID:
+		if (hsr != dev->hsr_dev)
+			return -EOPNOTSUPP;
+
+		ksz9477_hsr_leave(ds, port, hsr);
+		dev->hsr_ports &= ~BIT(port);
+		if (!dev->hsr_ports)
+			dev->hsr_dev = NULL;
+
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
 static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_tag_protocol	= ksz_get_tag_protocol,
 	.connect_tag_protocol   = ksz_connect_tag_protocol,
@@ -3438,6 +3512,8 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_sset_count		= ksz_sset_count,
 	.port_bridge_join	= ksz_port_bridge_join,
 	.port_bridge_leave	= ksz_port_bridge_leave,
+	.port_hsr_join		= ksz_hsr_join,
+	.port_hsr_leave		= ksz_hsr_leave,
 	.port_stp_state_set	= ksz_port_stp_state_set,
 	.port_pre_bridge_flags	= ksz_port_pre_bridge_flags,
 	.port_bridge_flags	= ksz_port_bridge_flags,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index a4de58847dea..e36d459de5a1 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -169,6 +169,9 @@ struct ksz_device {
 	struct mutex lock_irq;		/* IRQ Access */
 	struct ksz_irq girq;
 	struct ksz_ptp_data ptp_data;
+
+	struct net_device *hsr_dev;     /* HSR */
+	u8 hsr_ports;
 };
 
 /* List of supported models */
-- 
2.20.1

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

Dear Community,

> This patch series provides support for HSR HW offloading in KSZ9477
> switch IC.
> 
> To test this feature:
> ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45
> version 1 ip link set dev lan1 up
> ip link set dev lan2 up
> ip a add 192.168.0.1/24 dev hsr0
> ip link set dev hsr0 up
> 
> To remove HSR network device:
> ip link del hsr0
> 
> It is also possible to create another HSR interface, but it will
> only support HSR is software - e.g.
> ip link add name hsr1 type hsr slave1 lan3 slave2 lan4 supervision 45
> version 1
> 
> Test HW:
> Two KSZ9477-EVB boards with HSR ports set to "Port1" and "Port2".
> 
> Performance SW used:
> nuttcp -S --nofork
> nuttcp -vv -T 60 -r 192.168.0.2
> nuttcp -vv -T 60 -t 192.168.0.2
> 
> Code: v6.5-rc7 Linux repository
> Tested HSR v0 and v1
> Results:
> With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps
> With no offloading 		       RX: 63 Mbps  TX: 63 Mbps
> 
> 
> Lukasz Majewski (2):
>   net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication
>   net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading
> 
>  drivers/net/dsa/microchip/ksz9477.c    | 81
> ++++++++++++++++++++++++++ drivers/net/dsa/microchip/ksz9477.h    |
> 2 + drivers/net/dsa/microchip/ksz_common.c | 76
> ++++++++++++++++++++++++ drivers/net/dsa/microchip/ksz_common.h |  3 +
>  net/dsa/tag_ksz.c                      |  8 +++
>  5 files changed, 170 insertions(+)
> 


Are there any comments regarding this new revision of the HSR support
for KSZ9477 switch?

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Mon, Sep 11, 2023 at 04:58:48PM +0200, Lukasz Majewski wrote:
> Dear Community,
> 
> Are there any comments regarding this new revision of the HSR support
> for KSZ9477 switch?
> 
> Best regards,
> 
> Lukasz Majewski

Yeah, the integration with the DSA master's MAC address is not quite
what I was expecting to see.

See, both the DSA master's MAC address, as well as the HSR device's MAC
address, can be changed at runtime with:

ip link set eth0 address AA:BB:CC:DD:EE:FF # DSA master
ip link set lan1 address AA:BB:CC:DD:EE:FF # indirectly changes the HSR's address too

which is problematic because the hardware does not get updated in that
case, but the address change is not refused either.

Actually, the reason why I haven't yet said anything is because it made
me realize that there is a pre-existing bug in net/dsa/slave.c where we
have this pattern:

	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
		dev_uc_add(master, dev->dev_addr);

but there is no replay of the dev_uc_add() call when the master->dev_addr
changes. This really results in RX packet loss, as I have tested. I don't
know what is the best way to solve it.

Anyway, programming the MAC address of the DSA master or of the HSR
device to hardware seems to require tracking the NETDEV_CHANGEADDR and
NETDEV_PRE_CHANGEADDR events, even if only to reject those changes.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Mon, Sep 11, 2023 at 07:05:01PM +0300, Vladimir Oltean wrote:
> Actually, the reason why I haven't yet said anything is because it made
> me realize that there is a pre-existing bug in net/dsa/slave.c where we
> have this pattern:
> 
> 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
> 		dev_uc_add(master, dev->dev_addr);
> 
> but there is no replay of the dev_uc_add() call when the master->dev_addr
> changes. This really results in RX packet loss, as I have tested. I don't
> know what is the best way to solve it.

Hi @Jakub, I remember you fixed some issues with the dev->dev_addr writes,
after dev_addr_lists.c was changed to a rbtree. Is it easy for you to
tell if the change below is safe from an API perspective?

Is the answer "yes, because dev_uc_add() uses an addr_type of NETDEV_HW_ADDR_T_UNICAST,
and dev->dev_addr uses NETDEV_HW_ADDR_T_LAN, so they never share a struct netdev_hw_addr
for the same MAC address, and thus, they never collide"?

The DSA and 8021q drivers currently have this pattern, from around 2008.
But 8021q also tracks NETDEV_CHANGEADDR events on the real_dev, which is
absent in DSA. If the change below is safe, it would be a simpler solution.

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 48db91b33390..e40474e13660 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -374,11 +374,9 @@ static int dsa_slave_open(struct net_device *dev)
 			goto out;
 	}

-	if (!ether_addr_equal(dev->dev_addr, master->dev_addr)) {
-		err = dev_uc_add(master, dev->dev_addr);
-		if (err < 0)
-			goto del_host_addr;
-	}
+	err = dev_uc_add(master, dev->dev_addr);
+	if (err < 0)
+		goto del_host_addr;

 	err = dsa_port_enable_rt(dp, dev->phydev);
 	if (err)
@@ -387,8 +385,7 @@ static int dsa_slave_open(struct net_device *dev)
 	return 0;

 del_unicast:
-	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
-		dev_uc_del(master, dev->dev_addr);
+	dev_uc_del(master, dev->dev_addr);
 del_host_addr:
 	if (dsa_switch_supports_uc_filtering(ds))
 		dsa_port_standalone_host_fdb_del(dp, dev->dev_addr, 0);
@@ -404,8 +401,7 @@ static int dsa_slave_close(struct net_device *dev)

 	dsa_port_disable_rt(dp);

-	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
-		dev_uc_del(master, dev->dev_addr);
+	dev_uc_del(master, dev->dev_addr);

 	if (dsa_switch_supports_uc_filtering(ds))
 		dsa_port_standalone_host_fdb_del(dp, dev->dev_addr, 0);
@@ -469,14 +465,11 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 			return err;
 	}

-	if (!ether_addr_equal(addr->sa_data, master->dev_addr)) {
-		err = dev_uc_add(master, addr->sa_data);
-		if (err < 0)
-			goto del_unicast;
-	}
+	err = dev_uc_add(master, addr->sa_data);
+	if (err < 0)
+		goto del_unicast;

-	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
-		dev_uc_del(master, dev->dev_addr);
+	dev_uc_del(master, dev->dev_addr);

 	if (dsa_switch_supports_uc_filtering(ds))
 		dsa_port_standalone_host_fdb_del(dp, dev->dev_addr, 0);

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Mon, Sep 11, 2023 at 08:02:22PM +0300, Vladimir Oltean wrote:
> Hi @Jakub, I remember you fixed some issues with the dev->dev_addr writes,

I should have changed To: Jakub.. did that now.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Jakub Kicinski]

On Mon, 11 Sep 2023 20:02:22 +0300 Vladimir Oltean wrote:
> Hi @Jakub, I remember you fixed some issues with the dev->dev_addr writes,
> after dev_addr_lists.c was changed to a rbtree. Is it easy for you to
> tell if the change below is safe from an API perspective?
> 
> Is the answer "yes, because dev_uc_add() uses an addr_type of NETDEV_HW_ADDR_T_UNICAST,
> and dev->dev_addr uses NETDEV_HW_ADDR_T_LAN, so they never share a struct netdev_hw_addr
> for the same MAC address, and thus, they never collide"?
> 
> The DSA and 8021q drivers currently have this pattern, from around 2008.
> But 8021q also tracks NETDEV_CHANGEADDR events on the real_dev, which is
> absent in DSA. If the change below is safe, it would be a simpler solution.

FWIW I think it should be fine from the rbtree perspective, but IDK how
the user space would react to having a duplicate lladdr.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

Hi Vladimir,

> On Mon, Sep 11, 2023 at 04:58:48PM +0200, Lukasz Majewski wrote:
> > Dear Community,
> > 
> > Are there any comments regarding this new revision of the HSR
> > support for KSZ9477 switch?
> > 
> > Best regards,
> > 
> > Lukasz Majewski  
> 
> Yeah, the integration with the DSA master's MAC address is not quite
> what I was expecting to see.
> 
> See, both the DSA master's MAC address, as well as the HSR device's
> MAC address, can be changed at runtime with:
> 
> ip link set eth0 address AA:BB:CC:DD:EE:FF # DSA master
> ip link set lan1 address AA:BB:CC:DD:EE:FF # indirectly changes the
> HSR's address too

IMHO, somebody who will use HSR will not fiddle with mac addresses of
LAN1 and ETH0. It will be setup by savvy user once at boot up.

> 
> which is problematic because the hardware does not get updated in that
> case, but the address change is not refused either.
> 
> Actually, the reason why I haven't yet said anything is because it
> made me realize that there is a pre-existing bug in net/dsa/slave.c
> where we have this pattern:
> 
> 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
> 		dev_uc_add(master, dev->dev_addr);
> 
> but there is no replay of the dev_uc_add() call when the
> master->dev_addr changes. This really results in RX packet loss, as I
> have tested. I don't know what is the best way to solve it.
> 
> Anyway, programming the MAC address of the DSA master or of the HSR
> device to hardware seems to require tracking the NETDEV_CHANGEADDR and
> NETDEV_PRE_CHANGEADDR events, even if only to reject those changes.

Please correct me if I'm wrong, but the above issue (with lack of sync
of mac address change in DSA master and its ports) seems to be
affecting HSR support in a minimal way (when considering the above).

If I may ask - what is your suggestion to have the HSR join feature
merged for KSZ9477 SoC ?

Will the above problem block the HSR offloading support mainlining,
even when the self mac address filtering is one of four HW based
features for this SoC?


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 10:17:48AM +0200, Lukasz Majewski wrote:
> IMHO, somebody who will use HSR will not fiddle with mac addresses of
> LAN1 and ETH0. It will be setup by savvy user once at boot up.

The point is, it has to work in all configurations that are accepted by
the kernel.

> Please correct me if I'm wrong, but the above issue (with lack of sync
> of mac address change in DSA master and its ports) seems to be
> affecting HSR support in a minimal way (when considering the above).

It's a different (and very old) bug for sure. But it has impact upon the
v4 patch set as you've presented it here.

> If I may ask - what is your suggestion to have the HSR join feature
> merged for KSZ9477 SoC ?

Anything that makes sense and works is worth considering.

For example, one can argue that since we already have this pattern in 2
places in net/dsa/slave.c:

	/* If the port doesn't have its own MAC address and relies on the DSA
	 * master's one, inherit it again from the new DSA master.
	 */
	if (is_zero_ether_addr(dp->mac))
		eth_hw_addr_inherit(dev, master);

then the consistent way to react to NETDEV_CHANGEADDR events on the
master is to change the user ports' MAC address yet again, to track the
master.

In any case, as long as it's the DSA master's address that we program to
hardware, then I see it as inevitable to add a new struct dsa_switch_ops ::
master_addr_change() function, similar to master_state_change(). The driver
would always be notified of the current (even initial) MAC address, and
it could update the hardware registers (for WoL, pause frames and HSR
self-address filtering, in this case).

The above 2 changes could be one way to ensure that if a HSR device was
accepted for offload initially, it will remain in a configuration that
will keep working.


Or you can argue that dragging the DSA master into the discussion about
how we should program REG_SW_MAC_ADDR_0 is a complication. An API
internal to the microchip ksz driver could be added, where the user
ports on which the various specialty features are enabled (HSR, WoL)
take a reference on the REG_SW_MAC_ADDR_0 with their MAC address.
If the reference on REG_SW_MAC_ADDR_0 gets bumped from 0 to 1, the
hardware is programmed with the requesting port's MAC address. If the
reference is already elevated, then a request to increase it, coming
from another port, is accepted or denied, depending on whether the MAC
address of that port is equal to what's programmed into REG_SW_MAC_ADDR_0
or not. The refusal gets propagated to the user, together with an
informative extack message. The ports which hold a reference on
REG_SW_MAC_ADDR_0 cannot have their MAC address changed - and for this,
you'd have to add a hook to dsa_switch_ops (and thus to the driver) from
dsa_slave_set_mac_address().


So, there are some options to pick from.

> Will the above problem block the HSR offloading support mainlining,
> even when the self mac address filtering is one of four HW based
> features for this SoC?

I don't know, that depends on you.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4555 bytes --]

Hi Vladimir,

> On Tue, Sep 12, 2023 at 10:17:48AM +0200, Lukasz Majewski wrote:
> > IMHO, somebody who will use HSR will not fiddle with mac addresses
> > of LAN1 and ETH0. It will be setup by savvy user once at boot up.  
> 
> The point is, it has to work in all configurations that are accepted
> by the kernel.
> 
> > Please correct me if I'm wrong, but the above issue (with lack of
> > sync of mac address change in DSA master and its ports) seems to be
> > affecting HSR support in a minimal way (when considering the
> > above).  
> 
> It's a different (and very old) bug for sure. But it has impact upon
> the v4 patch set as you've presented it here.
> 
> > If I may ask - what is your suggestion to have the HSR join feature
> > merged for KSZ9477 SoC ?  
> 
> Anything that makes sense and works is worth considering.
> 
> For example, one can argue that since we already have this pattern in
> 2 places in net/dsa/slave.c:
> 
> 	/* If the port doesn't have its own MAC address and relies on
> the DSA
> 	 * master's one, inherit it again from the new DSA master.
> 	 */
> 	if (is_zero_ether_addr(dp->mac))
> 		eth_hw_addr_inherit(dev, master);
> 
> then the consistent way to react to NETDEV_CHANGEADDR events on the
> master is to change the user ports' MAC address yet again, to track
> the master.
> 
> In any case, as long as it's the DSA master's address that we program
> to hardware, then I see it as inevitable to add a new struct
> dsa_switch_ops :: master_addr_change() function, similar to
> master_state_change(). The driver would always be notified of the
> current (even initial) MAC address, and it could update the hardware
> registers (for WoL, pause frames and HSR self-address filtering, in
> this case).
> 
> The above 2 changes could be one way to ensure that if a HSR device
> was accepted for offload initially, it will remain in a configuration
> that will keep working.
> 

Please correct my understanding. The above change would affect the
whole DSA subsystem. It would require to have the core DSA modified and
would affect its operation?

> 
> Or you can argue that dragging the DSA master into the discussion
> about how we should program REG_SW_MAC_ADDR_0 is a complication.

Yes, it is IMHO the complication.

> An
> API internal to the microchip ksz driver could be added, where the
> user ports on which the various specialty features are enabled (HSR,
> WoL) take a reference on the REG_SW_MAC_ADDR_0 with their MAC address.
> If the reference on REG_SW_MAC_ADDR_0 gets bumped from 0 to 1, the
> hardware is programmed with the requesting port's MAC address. If the
> reference is already elevated, then a request to increase it, coming
> from another port, is accepted or denied, depending on whether the MAC
> address of that port is equal to what's programmed into
> REG_SW_MAC_ADDR_0 or not.
> The refusal gets propagated to the user,
> together with an informative extack message. The ports which hold a
> reference on REG_SW_MAC_ADDR_0 cannot have their MAC address changed
> - and for this, you'd have to add a hook to dsa_switch_ops (and thus
> to the driver) from dsa_slave_set_mac_address().
> 

git grep -n "REG_SW_MAC_ADDR_0"
drivers/net/dsa/microchip/ksz8795_reg.h:326:#define REG_SW_MAC_ADDR_0
        0x68 
drivers/net/dsa/microchip/ksz9477.c:1194:
     ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,

drivers/net/dsa/microchip/ksz9477_reg.h:169:#define REG_SW_MAC_ADDR_0
0x0302

In the current net-next the REG_SW_MAC_ADDR_0 is altered used (the only
usage are now with mine patches on ksz9477).

To sum up:

1. Up till now in (net-next) REG_SW_MAC_ADDR_0 is ONLY declared for
Microchip switches. No risk for access - other than HSR patches.

2. The MAC address alteration of DSA master and propagation to slaves
is a generic DSA bug.

Considering the above - the HSR implementation is safe (to the extend
to the whole DSA subsystem current operation). Am I correct?


> 
> So, there are some options to pick from.
> 
> > Will the above problem block the HSR offloading support mainlining,
> > even when the self mac address filtering is one of four HW based
> > features for this SoC?  
> 
> I don't know, that depends on you.




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 04:03:26PM +0200, Lukasz Majewski wrote:
> > In any case, as long as it's the DSA master's address that we program
> > to hardware, then I see it as inevitable to add a new struct
> > dsa_switch_ops :: master_addr_change() function
> 
> Please correct my understanding. The above change would affect the
> whole DSA subsystem. It would require to have the core DSA modified and
> would affect its operation?

Uhm, yes, that would be the idea. If we were going to track changes to
the DSA master's MAC address, we should do it from the DSA framework
which has the existing netdev notifier listener infrastructure in place.

> > Or you can argue that dragging the DSA master into the discussion
> > about how we should program REG_SW_MAC_ADDR_0 is a complication.
> 
> Yes, it is IMHO the complication.

Ok, it's a point of view.

> git grep -n "REG_SW_MAC_ADDR_0"
> drivers/net/dsa/microchip/ksz8795_reg.h:326:#define REG_SW_MAC_ADDR_0
>         0x68 
> drivers/net/dsa/microchip/ksz9477.c:1194:
>      ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
> 
> drivers/net/dsa/microchip/ksz9477_reg.h:169:#define REG_SW_MAC_ADDR_0
> 0x0302
> 
> In the current net-next the REG_SW_MAC_ADDR_0 is altered used (the only
> usage are now with mine patches on ksz9477).
> 
> To sum up:
> 
> 1. Up till now in (net-next) REG_SW_MAC_ADDR_0 is ONLY declared for
> Microchip switches. No risk for access - other than HSR patches.

I know (except for Oleksij's WoL patches, which will eventually be
resubmitted).

> 2. The MAC address alteration of DSA master and propagation to slaves
> is a generic DSA bug.

Which can be/will be fixed. The diff I've included in the question to
Jakub closes it, in fact.

> Considering the above - the HSR implementation is safe (to the extend
> to the whole DSA subsystem current operation). Am I correct?

If we exclude the aforementioned bug (which won't be a bug forever),
there still exists the case where the MAC address of a DSA user port can
be changed. The HSR driver has a NETDEV_CHANGEADDR handler for this as
well, and updates its own MAC address to follow the port. If that is
allowed to happen after the offload, currently it will break the offload.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3288 bytes --]

Hi Vladimir,

> On Tue, Sep 12, 2023 at 04:03:26PM +0200, Lukasz Majewski wrote:
> > > In any case, as long as it's the DSA master's address that we
> > > program to hardware, then I see it as inevitable to add a new
> > > struct dsa_switch_ops :: master_addr_change() function  
> > 
> > Please correct my understanding. The above change would affect the
> > whole DSA subsystem. It would require to have the core DSA modified
> > and would affect its operation?  
> 
> Uhm, yes, that would be the idea. If we were going to track changes to
> the DSA master's MAC address, we should do it from the DSA framework
> which has the existing netdev notifier listener infrastructure in
> place.
> 
> > > Or you can argue that dragging the DSA master into the discussion
> > > about how we should program REG_SW_MAC_ADDR_0 is a complication.  
> > 
> > Yes, it is IMHO the complication.  
> 
> Ok, it's a point of view.
> 
> > git grep -n "REG_SW_MAC_ADDR_0"
> > drivers/net/dsa/microchip/ksz8795_reg.h:326:#define
> > REG_SW_MAC_ADDR_0 0x68 
> > drivers/net/dsa/microchip/ksz9477.c:1194:
> >      ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
> > 
> > drivers/net/dsa/microchip/ksz9477_reg.h:169:#define
> > REG_SW_MAC_ADDR_0 0x0302
> > 
> > In the current net-next the REG_SW_MAC_ADDR_0 is altered used (the
> > only usage are now with mine patches on ksz9477).
> > 
> > To sum up:
> > 
> > 1. Up till now in (net-next) REG_SW_MAC_ADDR_0 is ONLY declared for
> > Microchip switches. No risk for access - other than HSR patches.  
> 
> I know (except for Oleksij's WoL patches, which will eventually be
> resubmitted).

Are we debating about some possible impact on patches which were posted
and (in a near future?) would be reposted?

> 
> > 2. The MAC address alteration of DSA master and propagation to
> > slaves is a generic DSA bug.  
> 
> Which can be/will be fixed. The diff I've included in the question to
> Jakub closes it, in fact.

Ok.

> 
> > Considering the above - the HSR implementation is safe (to the
> > extend to the whole DSA subsystem current operation). Am I correct?
> >  
> 
> If we exclude the aforementioned bug (which won't be a bug forever),
> there still exists the case where the MAC address of a DSA user port
> can be changed. The HSR driver has a NETDEV_CHANGEADDR handler for
> this as well, and updates its own MAC address to follow the port. If
> that is allowed to happen after the offload, currently it will break
> the offload.

But then we can have struct ksz_device extended with bitmask -
hw_mac_addr_ports, which could be set to ports (WoL or HSR) when
REG_MAC_ADDR_0 is written.

If WoL would like to alter it after it was written by HSR, then the
error is presented (printed) to the user and we return.

The same would be with HSR altering the WoL's MAC in-device setup.


The HSR or WoL can be added without issues (the first one which is
accepted).

Then the second feature would need to implement this check.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Tue, Sep 12, 2023 at 05:06:41PM +0200, Lukasz Majewski wrote:
> Are we debating about some possible impact on patches which were posted
> and (in a near future?) would be reposted?

We are discussing the ways in which a multi-purpose register should be
programmed. Not "the impact on patches" per se, because Oleksij will
have to adapt no matter what you do, but rather the options that remain
available to him, after the first feature that makes use of the
multi-purpose register makes its way to mainline.

> > > Considering the above - the HSR implementation is safe (to the
> > > extend to the whole DSA subsystem current operation). Am I correct?
> > >  
> > 
> > If we exclude the aforementioned bug (which won't be a bug forever),
> > there still exists the case where the MAC address of a DSA user port
> > can be changed. The HSR driver has a NETDEV_CHANGEADDR handler for
> > this as well, and updates its own MAC address to follow the port. If
> > that is allowed to happen after the offload, currently it will break
> > the offload.
> 
> But then we can have struct ksz_device extended with bitmask -
> hw_mac_addr_ports, which could be set to ports (WoL or HSR) when
> REG_MAC_ADDR_0 is written.
> 
> If WoL would like to alter it after it was written by HSR, then the
> error is presented (printed) to the user and we return.
> 
> The same would be with HSR altering the WoL's MAC in-device setup.
> 
> 
> The HSR or WoL can be added without issues (the first one which is
> accepted).
> 
> Then the second feature would need to implement this check.

This is more or less a rehash of what I proposed as option 2, except for
the fact that you suggest a port mask and I suggest a proper refcount_t.
And the reason why I suggest that is to allow the "WoL+HSR on the same
port" to work. With your proposal, both the HSR and WoL code paths would
set the same bit in hw_mac_addr_ports, which would become problematic
when the time comes to unset it. Not so much when every port calls
refcount_inc() per feature. With WoL+HSR on the same port, the MAC
address would have a refcount of 2, and you could call port_hsr_leave()
and that refcount would just drop to 1 instead of letting go.

There are probably hundreds of implementations of this idea in the
kernel, but the one that comes to my mind is ocelot_mirror_get() +
ocelot_mirror_put(). Again, I need to mention that I know that port
mirroring != HSR - I'm just talking about the technique.

There is one more thing that your reply to my observation fails to
address. Even with this refcount thing, you will still need to add code
to dsa_slave_set_mac_address() which notifies the ksz driver, so that
the driver can refuse MAC address changes, which would break the
offloads. Ack?

In principle it sounds like a plan. It just needs to be implemented.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4317 bytes --]

Hi Vladimir,

> On Tue, Sep 12, 2023 at 05:06:41PM +0200, Lukasz Majewski wrote:
> > Are we debating about some possible impact on patches which were
> > posted and (in a near future?) would be reposted?  
> 
> We are discussing the ways in which a multi-purpose register should be
> programmed. Not "the impact on patches" per se, because Oleksij will
> have to adapt no matter what you do, but rather the options that
> remain available to him, after the first feature that makes use of the
> multi-purpose register makes its way to mainline.
> 
> > > > Considering the above - the HSR implementation is safe (to the
> > > > extend to the whole DSA subsystem current operation). Am I
> > > > correct? 
> > > 
> > > If we exclude the aforementioned bug (which won't be a bug
> > > forever), there still exists the case where the MAC address of a
> > > DSA user port can be changed. The HSR driver has a
> > > NETDEV_CHANGEADDR handler for this as well, and updates its own
> > > MAC address to follow the port. If that is allowed to happen
> > > after the offload, currently it will break the offload.  
> > 
> > But then we can have struct ksz_device extended with bitmask -
> > hw_mac_addr_ports, which could be set to ports (WoL or HSR) when
> > REG_MAC_ADDR_0 is written.
> > 
> > If WoL would like to alter it after it was written by HSR, then the
> > error is presented (printed) to the user and we return.
> > 
> > The same would be with HSR altering the WoL's MAC in-device setup.
> > 
> > 
> > The HSR or WoL can be added without issues (the first one which is
> > accepted).
> > 
> > Then the second feature would need to implement this check.  
> 
> This is more or less a rehash of what I proposed as option 2, except
> for the fact that you suggest a port mask and I suggest a proper
> refcount_t. And the reason why I suggest that is to allow the
> "WoL+HSR on the same port" to work. With your proposal, both the HSR
> and WoL code paths would set the same bit in hw_mac_addr_ports, which
> would become problematic when the time comes to unset it. Not so much
> when every port calls refcount_inc() per feature. With WoL+HSR on the
> same port, the MAC address would have a refcount of 2, and you could
> call port_hsr_leave() and that refcount would just drop to 1 instead
> of letting go.
> 

Why we cannot have even simpler solution - in the HSR/Wol code we read
content of REG_SW_MAC_ADDR_0 (the actually programmed MAC) and:

- If not programmed - use DSA master one (like done in mine patches)

- If already programmed:
	 - check if equal to DSA master - proceed with HSR.
	 - if not equal to DSA master (e.g. WoL altered) - exit HSR join
	   with information that offloading is not possible

Then, the content of REG_SW_MAC_ADDR_X would determine what to do with
it.

> There are probably hundreds of implementations of this idea in the
> kernel, but the one that comes to my mind is ocelot_mirror_get() +
> ocelot_mirror_put(). Again, I need to mention that I know that port
> mirroring != HSR - I'm just talking about the technique.
> 
> There is one more thing that your reply to my observation fails to
> address. Even with this refcount thing, you will still need to add
> code to dsa_slave_set_mac_address() which notifies the ksz driver, so
> that the driver can refuse MAC address changes, which would break the
> offloads. Ack?

And the above problem is not related to the DSA slave address change
discussed earlier?

> 
> In principle it sounds like a plan. It just needs to be implemented.

To clarify:

0. It looks like described above prevention from REG_SW_MAC_ADDR_X
overwriting and DSA slave port MAC address change are needed.

Then questions about time line:

1. The HSR code is accepted without fixes from 0. and then when other
user (WoL) patches are posted problems from 0. needs to be addressed.


or 


2. To accept the HSR code you (and other community members? Russell,
Andrew) require the fixes from 0. first. 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Wed, Sep 13, 2023 at 10:22:19AM +0200, Lukasz Majewski wrote:
> Why we cannot have even simpler solution - in the HSR/Wol code we read
> content of REG_SW_MAC_ADDR_0 (the actually programmed MAC) and:
> 
> - If not programmed - use DSA master one (like done in mine patches)

You said here https://lore.kernel.org/netdev/20230912160326.188e1d13@wsk/
that using the DSA master address is a complication that can be avoided,
no? So why is it now part of the solution that you propose?

I thought we were in agreement to program the actual DSA user ports' MAC
addresses to hardware.

> - If already programmed:
> 	 - check if equal to DSA master - proceed with HSR.
> 	 - if not equal to DSA master (e.g. WoL altered) - exit HSR join
> 	   with information that offloading is not possible

With KSZ switches, a single CPU port is supported, so all ports share
the same DSA master. So if the contents of REG_SW_MAC_ADDR_0 is different
from the DSA master (the same DSA master that was used for an earlier
HSR offload), why do you infer that it was altered by WoL? It makes no
sense.

> Then, the content of REG_SW_MAC_ADDR_X would determine what to do with
> it.
> 
> > There are probably hundreds of implementations of this idea in the
> > kernel, but the one that comes to my mind is ocelot_mirror_get() +
> > ocelot_mirror_put(). Again, I need to mention that I know that port
> > mirroring != HSR - I'm just talking about the technique.
> > 
> > There is one more thing that your reply to my observation fails to
> > address. Even with this refcount thing, you will still need to add
> > code to dsa_slave_set_mac_address() which notifies the ksz driver, so
> > that the driver can refuse MAC address changes, which would break the
> > offloads. Ack?
> 
> And the above problem is not related to the DSA slave address change
> discussed earlier?

"Discussed earlier" is a bit imprecise and I don't know what you're
talking about.

There are 3 netdev kinds at play here: (a) DSA master, (b) DSA user port, (c) HSR device.

- Changing the MAC address of (a) triggers a pre-existing bug. That bug
  can be separated from the HSR offload discussion if the HSR offload
  decides to not program the DSA master's MAC address to hardware, but a
  different MAC address. The pre-existence of the DSA bug is not a strong
  enough argument per se to avoid programming the DSA master's address to
  hardware. But there may be others. Like the fact that DSA user ports may
  inherit the DSA master's MAC address, or they may have their own.
  Limiting HSR offload and WoL to just the "inherit" case may seem a bit
  arbitrary, considering that the self-address filtering from
  hsr_handle_frame() looks at the port_A and port_B MAC addresses.

- Changing the MAC address of (c) does not seem directly possible, but:

- Changing the MAC address of (b) also triggers (c) - see hsr_netdev_notify().
  This is because the HSR driver makes sure that the addresses of
  port_A, port_B and the HSR device are equal at all times.

The simple matter is: if you program the MAC address of a netdev (any
netdev) to hardware, then for a correct and robust implementation, you
need to make sure that the hardware will always be in sync with that
address, keeping in mind that the user may change it. Either you deny
changes, or you update the hardware when the address is updated.

It's not quite clear to me that you're making a distinction between
changing (a) and (b).

> > In principle it sounds like a plan. It just needs to be implemented.
> 
> To clarify:
> 
> 0. It looks like described above prevention from REG_SW_MAC_ADDR_X
> overwriting and DSA slave port MAC address change are needed.
> 
> Then questions about time line:
> 
> 1. The HSR code is accepted without fixes from 0. and then when other
> user (WoL) patches are posted problems from 0. needs to be addressed.
> 
> or 
> 
> 2. To accept the HSR code you (and other community members? Russell,
> Andrew) require the fixes from 0. first. 

If the DSA user port MAC address changes, and REG_SW_MAC_ADDR_0 was
previously programmed with it, and nothing is done in reaction to this,
then this is a problem with the HSR offload. So no, it's not just a
problem with upcoming WoL patches as you imply. You need to integrate a
solution to that problem as part of your HSR patches.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 6452 bytes --]

Hi Vladimir,

> On Wed, Sep 13, 2023 at 10:22:19AM +0200, Lukasz Majewski wrote:
> > Why we cannot have even simpler solution - in the HSR/Wol code we
> > read content of REG_SW_MAC_ADDR_0 (the actually programmed MAC) and:
> > 
> > - If not programmed - use DSA master one (like done in mine
> > patches)  
> 
> You said here
> https://lore.kernel.org/netdev/20230912160326.188e1d13@wsk/ that
> using the DSA master address is a complication that can be avoided,
> no? So why is it now part of the solution that you propose?

The patch v4 uses DSA master (HSR device) MAC address as you advised.

My suggestion was to just use the value (single) if already written
there. If it differs at join time - then just bail out.

> 
> I thought we were in agreement to program the actual DSA user ports'
> MAC addresses to hardware.

No - v4 of this solution uses HSR net device MAC address, which is
supposed to be the same as DSA master.

> 
> > - If already programmed:
> > 	 - check if equal to DSA master - proceed with HSR.
> > 	 - if not equal to DSA master (e.g. WoL altered) - exit HSR
> > join with information that offloading is not possible  
> 
> With KSZ switches, a single CPU port is supported, so all ports share
> the same DSA master. So if the contents of REG_SW_MAC_ADDR_0 is
> different from the DSA master (the same DSA master that was used for
> an earlier HSR offload), why do you infer that it was altered by WoL?
> It makes no sense.

So where is the issue? The only issue is that somebody would change DSA
master (and hence HSR) MAC address, so the REG_SW_MAC_ADDR_0 would not
be changed. Or do I miss something?

> 
> > Then, the content of REG_SW_MAC_ADDR_X would determine what to do
> > with it.
> >   
> > > There are probably hundreds of implementations of this idea in the
> > > kernel, but the one that comes to my mind is ocelot_mirror_get() +
> > > ocelot_mirror_put(). Again, I need to mention that I know that
> > > port mirroring != HSR - I'm just talking about the technique.
> > > 
> > > There is one more thing that your reply to my observation fails to
> > > address. Even with this refcount thing, you will still need to add
> > > code to dsa_slave_set_mac_address() which notifies the ksz
> > > driver, so that the driver can refuse MAC address changes, which
> > > would break the offloads. Ack?  
> > 
> > And the above problem is not related to the DSA slave address change
> > discussed earlier?  
> 
> "Discussed earlier" is a bit imprecise and I don't know what you're
> talking about.
> 
> There are 3 netdev kinds at play here: (a) DSA master, (b) DSA user
> port, (c) HSR device.
> 
> - Changing the MAC address of (a) triggers a pre-existing bug. That
> bug can be separated from the HSR offload discussion if the HSR
> offload decides to not program the DSA master's MAC address to
> hardware, but a different MAC address. The pre-existence of the DSA
> bug is not a strong enough argument per se to avoid programming the
> DSA master's address to hardware.

And this is how v4 is implemented. Or not?

> But there may be others. Like the
> fact that DSA user ports may inherit the DSA master's MAC address, or
> they may have their own. Limiting HSR offload and WoL to just the
> "inherit" case may seem a bit arbitrary, considering that the
> self-address filtering from hsr_handle_frame() looks at the port_A
> and port_B MAC addresses.

As described earlier - the self-address mac filtering to work must have
lan1, lan2, HSR, eth0 MAC address equal. HSR HW offloading will just not
work if they differ.

> 
> - Changing the MAC address of (c) does not seem directly possible,
> but:
> 
> - Changing the MAC address of (b) also triggers (c) - see
> hsr_netdev_notify(). This is because the HSR driver makes sure that
> the addresses of port_A, port_B and the HSR device are equal at all
> times.

Why changing HSR port would affect HSR device MAC address? Shouldn't it
be forbidden, and HSR ports shall inherit MAC address of HSR device
(hsr0) ?

> 
> The simple matter is: if you program the MAC address of a netdev (any
> netdev) to hardware

Which netdev in this case? lan1, lan2, hsr0 or eth0 ?

> , then for a correct and robust implementation, you
> need to make sure that the hardware will always be in sync with that
> address, keeping in mind that the user may change it. Either you deny
> changes, or you update the hardware when the address is updated.
> 

Why I cannot just:

1. Check on hsr_join if lan1, lan2, hsr0 and eth0 MAC is equal and
write it to REG_SW_MAC_ADDR_0?

2. Forbid the change of lan1/lan2/eth0/hsr0 if it may affect HSR HW
offloading? If user want to change it - then one shall delete hsr0 net
device, change MAC and create it again.

How point 2 can be achieved (if possible) ?

> It's not quite clear to me that you're making a distinction between
> changing (a) and (b).
> 
> > > In principle it sounds like a plan. It just needs to be
> > > implemented.  
> > 
> > To clarify:
> > 
> > 0. It looks like described above prevention from REG_SW_MAC_ADDR_X
> > overwriting and DSA slave port MAC address change are needed.
> > 
> > Then questions about time line:
> > 
> > 1. The HSR code is accepted without fixes from 0. and then when
> > other user (WoL) patches are posted problems from 0. needs to be
> > addressed.
> > 
> > or 
> > 
> > 2. To accept the HSR code you (and other community members? Russell,
> > Andrew) require the fixes from 0. first.   
> 
> If the DSA user port MAC address changes,

You mean lan1, lan2, which are joined with hsr0?

> and REG_SW_MAC_ADDR_0 was
> previously programmed with it, and nothing is done in reaction to
> this, then this is a problem with the HSR offload. 

This shall be just forbidden?

> So no, it's not
> just a problem with upcoming WoL patches as you imply. You need to
> integrate a solution to that problem as part of your HSR patches.

I'm really stunned, how much extra work is required to add two
callbacks to DSA subsystem (to have already implemented feature) for a
single chip IC.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Wed, Sep 13, 2023 at 02:15:48PM +0200, Lukasz Majewski wrote:
> > I thought we were in agreement to program the actual DSA user ports'
> > MAC addresses to hardware.
> 
> No - v4 of this solution uses HSR net device MAC address, which is
> supposed to be the same as DSA master.

By "in agreement" I mean "as a result of the discussion on v4 [ this discussion ],
where it became obvious that the DSA master solution is not so robust".
I hope the 12 emails already exchanged on this patch set weren't for nothing.

> > With KSZ switches, a single CPU port is supported, so all ports share
> > the same DSA master. So if the contents of REG_SW_MAC_ADDR_0 is
> > different from the DSA master (the same DSA master that was used for
> > an earlier HSR offload), why do you infer that it was altered by WoL?
> > It makes no sense.
> 
> So where is the issue? The only issue is that somebody would change DSA
> master (and hence HSR) MAC address, so the REG_SW_MAC_ADDR_0 would not
> be changed. Or do I miss something?

Changing the DSA master address and changing the HSR MAC address
(indirectly through the ports' address) are different operations.
The DSA master's address and the user ports' address are not necessarily
in sync.

Each address change is problematic in its own way, and each problem
needs to be tackled in its own way, depending on which MAC address you
desire for REG_SW_MAC_ADDR_0 to track.

But yes, the only issue is that the MAC address can be changed at
runtime, after the initial offload.

> > - Changing the MAC address of (a) triggers a pre-existing bug. That
> > bug can be separated from the HSR offload discussion if the HSR
> > offload decides to not program the DSA master's MAC address to
> > hardware, but a different MAC address. The pre-existence of the DSA
> > bug is not a strong enough argument per se to avoid programming the
> > DSA master's address to hardware.
> 
> And this is how v4 is implemented. Or not?

If A == B initially, then there are 2 ways you can change that condition.
You can change A, or you can change B. Replace "A" with "the DSA
master's address" and "B" with "the HSR device's address".

> > - Changing the MAC address of (c) does not seem directly possible,
> > but:
> > 
> > - Changing the MAC address of (b) also triggers (c) - see
> > hsr_netdev_notify(). This is because the HSR driver makes sure that
> > the addresses of port_A, port_B and the HSR device are equal at all
> > times.
> 
> Why changing HSR port would affect HSR device MAC address?

I don't have a simpler answer than "because that's what the code does".

If you don't have time to experiment to convince yourself that this is
the case, below is a set of commands that should hopefully clarify.

$ ip link
6: eno2: <BROADCAST,MULTICAST> mtu 1508 qdisc noop state DOWN group default qlen 1000
    link/ether 2a:af:42:b7:73:11 brd ff:ff:ff:ff:ff:ff
    altname end0
    altname enp0s0f2
7: swp0@eno2: <BROADCAST,MULTICAST,M-DOWN> mtu 1504 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
8: swp1@eno2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
9: swp2@eno2: <BROADCAST,MULTICAST,M-DOWN> mtu 1504 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
10: sw0p0@swp0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
11: sw0p1@swp0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
12: sw0p2@swp0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
13: sw2p0@swp2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
14: sw2p1@swp2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
15: sw2p2@swp2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
16: sw2p3@swp2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
    link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
# Simplified in a table for brevity. The format will be kept below
$ ip link show ...
     sw0p0              sw0p1              DSA master          hsr0
addr a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73   n/a

$ ip link add hsr0 type hsr slave1 sw0p0 slave2 sw0p1 version 1
[   70.033711] sja1105 spi2.0 sw0p0: entered promiscuous mode
[   70.058066] sja1105 spi2.0 sw0p1: entered promiscuous mode
Warning: dsa_core: Offloading not supported.

$ ip link show ...
     sw0p0              sw0p1              DSA master          hsr0
addr a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73   a6:f4:af:fd:fc:73

$ ip link set swp0 address 00:01:02:03:04:05 # DSA master

$ ip link show ...
     sw0p0              sw0p1              DSA master          hsr0
addr a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73  00:01:02:03:04:05   a6:f4:af:fd:fc:73

$ ip link set sw0p0 address 00:01:02:03:04:06

$ ip link show ...
     sw0p0              sw0p1              DSA master          hsr0
addr 00:01:02:03:04:06  a6:f4:af:fd:fc:73  00:01:02:03:04:05   00:01:02:03:04:06

$ ip link set sw0p1 address 00:01:02:03:04:07

$ ip link show ...
     sw0p0              sw0p1              DSA master          hsr0
addr 00:01:02:03:04:06  00:01:02:03:04:07  00:01:02:03:04:05   00:01:02:03:04:06

> Shouldn't it be forbidden, and HSR ports shall inherit MAC address of
> HSR device (hsr0) ?

Since HSR does _actually_ track the MAC address of port_A (sw0p0 above),
I guess it would be best if the API introduced would always program
REG_SW_MAC_ADDR_0 with the MAC address of the first port that joins the
HSR (and requests a reference to REG_SW_MAC_ADDR_0). That way, the API
should work with no changing for WoL as well. Anyway - minor difference
between first user port and HSR dev_addr.

> > The simple matter is: if you program the MAC address of a netdev (any
> > netdev) to hardware
> 
> Which netdev in this case? lan1, lan2, hsr0 or eth0 ?

Any netdev. It is a general statement which had a continuation, which
you've interrupted.

> > , then for a correct and robust implementation, you
> > need to make sure that the hardware will always be in sync with that
> > address, keeping in mind that the user may change it. Either you deny
> > changes, or you update the hardware when the address is updated.
> > 
> 
> Why I cannot just:
> 
> 1. Check on hsr_join if lan1, lan2, hsr0 and eth0 MAC is equal and
> write it to REG_SW_MAC_ADDR_0?

This is actually unnecessary if you implement the reference scheme on
REG_SW_MAC_ADDR_0 that I've suggested. Checking the MAC address of eth0
is unnecessary in any case, if you don't program it to hardware.

> 2. Forbid the change of lan1/lan2/eth0/hsr0 if it may affect HSR HW
> offloading? If user want to change it - then one shall delete hsr0 net
> device, change MAC and create it again.

I've been saying since yesterday that you should do this.

> How point 2 can be achieved (if possible) ?

Re-read earlier emails.

> > If the DSA user port MAC address changes,
> 
> You mean lan1, lan2, which are joined with hsr0?

Yup. I've been saying this for a number of emails already:
https://lore.kernel.org/netdev/20230912092909.4yj4b2b4xrhzdztu@skbuf/

| The ports which hold a reference on REG_SW_MAC_ADDR_0 cannot have their
| MAC address changed - and for this, you'd have to add a hook to
| dsa_switch_ops (and thus to the driver) from
| dsa_slave_set_mac_address().

> > and REG_SW_MAC_ADDR_0 was
> > previously programmed with it, and nothing is done in reaction to
> > this, then this is a problem with the HSR offload. 
> 
> This shall be just forbidden?

Yup.

> > So no, it's not
> > just a problem with upcoming WoL patches as you imply. You need to
> > integrate a solution to that problem as part of your HSR patches.
> 
> I'm really stunned, how much extra work is required to add two
> callbacks to DSA subsystem (to have already implemented feature) for a
> single chip IC.

Some observations are best kept to yourself. This is only the second HSR
offload in the entire kernel. To complain that the infrastructure needs
some extensions, for something that wasn't even needed for the first
implementation (tracking a MAC address), is unrealistic.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On Wed, Sep 13, 2023 at 04:51:02PM +0300, Vladimir Oltean wrote:
> > I'm really stunned, how much extra work is required to add two
> > callbacks to DSA subsystem (to have already implemented feature) for a
> > single chip IC.
> 
> Some observations are best kept to yourself. This is only the second HSR
> offload in the entire kernel. To complain that the infrastructure needs
> some extensions, for something that wasn't even needed for the first
> implementation (tracking a MAC address), is unrealistic.

Can you please test the attached incremental patch, which applies on top
of your RFC v4 series? It contains an implementation of my own review feedback.

[-- Attachment #2: 0001-net-dsa-microchip-incremental-updates-for-HSR-offloa.patch --]
[-- Type: text/x-diff, Size: 15671 bytes --]

From bc0b8f180ad747267adb52496c1cb353626f6d1c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 13 Sep 2023 21:17:55 +0300
Subject: [PATCH] net: dsa: microchip: incremental updates for HSR offload

This contains the following changes squashed together:

- remove the REG_SW_MAC_ADDR_{0..5} macros from ksz8795_reg.h and
  ksz9477_reg.h, and re-add this register offset to the dev->info->regs[]
  array. Defining macros which have the same name but different values
  is bad practice, because it makes it hard to avoid code duplication.
  The same code does different things, depending on the file it's placed
  in. Case in point, we want to access REG_SW_MAC_ADDR from ksz_common.c,
  but currently we can't, because we don't know which kszXXXX_reg.h to
  include from the common code....

- propagate the extack argument from dsa_slave_changeupper() to
  ds->ops->port_hsr_join(), and modify the xrs700s implementation to add
  it. This patch should be moved *before* the ksz9477 offload, in a
  proper submission.

- add a new ds->ops->port_set_mac_address() DSA callback so that drivers
  can veto MAC address changes on ports. KSZ9477 will need it when it
  offloads HSR.

- new API in ksz_common.c for the management of the global switch MAC address.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/microchip/ksz8795_reg.h |   7 --
 drivers/net/dsa/microchip/ksz9477.c     |  11 +-
 drivers/net/dsa/microchip/ksz9477_reg.h |   7 --
 drivers/net/dsa/microchip/ksz_common.c  | 161 +++++++++++++++++-------
 drivers/net/dsa/microchip/ksz_common.h  |   7 ++
 drivers/net/dsa/xrs700x/xrs700x.c       |   3 +-
 include/net/dsa.h                       |  13 +-
 net/dsa/port.c                          |   5 +-
 net/dsa/port.h                          |   3 +-
 net/dsa/slave.c                         |   9 +-
 10 files changed, 152 insertions(+), 74 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 7a57c6088f80..ee1b673d5f30 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -323,13 +323,6 @@
 	((addr) + REG_PORT_1_CTRL_0 + (port) *	\
 		(REG_PORT_2_CTRL_0 - REG_PORT_1_CTRL_0))
 
-#define REG_SW_MAC_ADDR_0		0x68
-#define REG_SW_MAC_ADDR_1		0x69
-#define REG_SW_MAC_ADDR_2		0x6A
-#define REG_SW_MAC_ADDR_3		0x6B
-#define REG_SW_MAC_ADDR_4		0x6C
-#define REG_SW_MAC_ADDR_5		0x6D
-
 #define TABLE_EXT_SELECT_S		5
 #define TABLE_EEE_V			1
 #define TABLE_ACL_V			2
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index f36bc427c468..3783f2f3332f 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1169,7 +1169,7 @@ void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr)
 {
 	struct ksz_device *dev = ds->priv;
 	struct net_device *slave;
-	u8 i, data;
+	u8 data;
 
 	/* Program which port(s) shall support HSR */
 	ksz_rmw32(dev, REG_HSR_PORT_MAP__4, BIT(port), BIT(port));
@@ -1184,15 +1184,6 @@ void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr)
 		data |= HSR_DUPLICATE_DISCARD;
 		data &= ~HSR_NODE_UNICAST;
 		ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data);
-
-		/* Self MAC address filtering for HSR frames to avoid
-		 * traverse of the HSR ring more than once.
-		 *
-		 * The HSR port (i.e. hsr0) MAC address is used.
-		 */
-		for (i = 0; i < ETH_ALEN; i++)
-			ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
-				   hsr->dev_addr[i]);
 	}
 
 	/* Enable per port self-address filtering.
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index cba3dba58bc3..c8866c180fe5 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -166,13 +166,6 @@
 #define SW_DOUBLE_TAG			BIT(7)
 #define SW_RESET			BIT(1)
 
-#define REG_SW_MAC_ADDR_0		0x0302
-#define REG_SW_MAC_ADDR_1		0x0303
-#define REG_SW_MAC_ADDR_2		0x0304
-#define REG_SW_MAC_ADDR_3		0x0305
-#define REG_SW_MAC_ADDR_4		0x0306
-#define REG_SW_MAC_ADDR_5		0x0307
-
 #define REG_SW_MTU__2			0x0308
 #define REG_SW_MTU_MASK			GENMASK(13, 0)
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b81c3ac422f9..318a8b4e32a0 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -299,6 +299,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
 };
 
 static const u16 ksz8795_regs[] = {
+	[REG_SW_MAC_ADDR]		= 0x68,
 	[REG_IND_CTRL_0]		= 0x6E,
 	[REG_IND_DATA_8]		= 0x70,
 	[REG_IND_DATA_CHECK]		= 0x72,
@@ -427,6 +428,7 @@ static u8 ksz8863_shifts[] = {
 };
 
 static const u16 ksz9477_regs[] = {
+	[REG_SW_MAC_ADDR]		= 0x0302,
 	[P_STP_CTRL]			= 0x0B04,
 	[S_START_CTRL]			= 0x0300,
 	[S_BROADCAST_CTRL]		= 0x0332,
@@ -3420,9 +3422,93 @@ static int ksz_setup_tc(struct dsa_switch *ds, int port,
 	}
 }
 
-static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr)
+static int ksz_port_set_mac_address(struct dsa_switch *ds, int port,
+				    const unsigned char *addr)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+
+	if (dp->hsr_dev) {
+		dev_err(ds->dev,
+			"Cannot change MAC address on port %d with active HSR offload\n",
+			port);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+/* Program the switch's MAC address register with the MAC address of the
+ * requesting user port. This single address is used by the switch for multiple
+ * features, like HSR self-address filtering and WoL. Other user ports are
+ * allowed to share ownership of this address as long as their MAC address is
+ * the same. The user ports' MAC addresses must not change while they have
+ * ownership of the switch MAC address.
+ */
+static int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
+				  struct netlink_ext_ack *extack)
+{
+	struct net_device *slave = dsa_to_port(ds, port)->slave;
+	const unsigned char *addr = slave->dev_addr;
+	struct ksz_switch_macaddr *switch_macaddr;
+	struct ksz_device *dev = ds->priv;
+	const u16 *regs = dev->info->regs;
+	int i;
+
+	/* Make sure concurrent MAC address changes are blocked */
+	ASSERT_RTNL();
+
+	switch_macaddr = dev->switch_macaddr;
+	if (switch_macaddr) {
+		if (!ether_addr_equal(switch_macaddr->addr, addr)) {
+			NL_SET_ERR_MSG_FMT_MOD(extack,
+					       "Switch already configured for MAC address %pM",
+					       switch_macaddr->addr);
+			return -EBUSY;
+		}
+
+		refcount_inc(&switch_macaddr->refcount);
+		return 0;
+	}
+
+	switch_macaddr = kzalloc(sizeof(*switch_macaddr), GFP_KERNEL);
+	if (!switch_macaddr)
+		return -ENOMEM;
+
+	ether_addr_copy(switch_macaddr->addr, addr);
+	refcount_set(&switch_macaddr->refcount, 1);
+	dev->switch_macaddr = switch_macaddr;
+
+	/* Program the switch MAC address to hardware */
+	for (i = 0; i < ETH_ALEN; i++)
+		ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]);
+
+	return 0;
+}
+
+static void ksz_switch_macaddr_put(struct dsa_switch *ds)
+{
+	struct ksz_switch_macaddr *switch_macaddr;
+	struct ksz_device *dev = ds->priv;
+	const u16 *regs = dev->info->regs;
+	int i;
+
+	/* Make sure concurrent MAC address changes are blocked */
+	ASSERT_RTNL();
+
+	switch_macaddr = dev->switch_macaddr;
+	if (!refcount_dec_and_test(&switch_macaddr->refcount))
+		return;
+
+	for (i = 0; i < ETH_ALEN; i++)
+		ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, 0);
+
+	dev->switch_macaddr = NULL;
+	kfree(switch_macaddr);
+}
+
+static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr,
+			struct netlink_ext_ack *extack)
 {
-	struct net_device *dm = dsa_port_to_master(dsa_to_port(ds, port));
 	struct ksz_device *dev = ds->priv;
 	enum hsr_version ver;
 	int ret;
@@ -3431,41 +3517,34 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr)
 	if (ret)
 		return ret;
 
-	/* Check if HSR net_device's MAC address equals to DSA master.
-	 *
-	 * Only in that way one can assure correct operation between
-	 * different switch features - like WoL, PAUSE and HSR, which
-	 * are using in-switch programmed MAC address.
-	 */
-	if (!ether_addr_equal(dm->dev_addr, hsr->dev_addr)) {
-		dev_err(dev->dev,
-			"DSA master and HSR dev MAC must equal for offloading");
+	if (dev->chip_id != KSZ9477_CHIP_ID) {
+		NL_SET_ERR_MSG_MOD(extack, "Chip does not support HSR offload");
 		return -EOPNOTSUPP;
 	}
 
-	switch (dev->chip_id) {
-	case KSZ9477_CHIP_ID:
-		/* KSZ9477 can support HW offloading of only 1 HSR device */
-		if (dev->hsr_dev && hsr != dev->hsr_dev) {
-			dev_err(dev->dev, "Offload supported for a single HSR");
-			return -EOPNOTSUPP;
-		}
-
-		/* KSZ9477 only supports HSR v0 and v1 */
-		if (!(ver == HSR_V0 || ver == HSR_V1)) {
-			dev_err(dev->dev, "Only HSR v0 and v1 supported");
-			return -EOPNOTSUPP;
-		}
-
-		ksz9477_hsr_join(ds, port, hsr);
-		dev->hsr_dev = hsr;
-		dev->hsr_ports |= BIT(port);
+	/* KSZ9477 can support HW offloading of only 1 HSR device */
+	if (dev->hsr_dev && hsr != dev->hsr_dev) {
+		NL_SET_ERR_MSG_MOD(extack, "Offload supported for a single HSR");
+		return -EOPNOTSUPP;
+	}
 
-		break;
-	default:
+	/* KSZ9477 only supports HSR v0 and v1 */
+	if (!(ver == HSR_V0 || ver == HSR_V1)) {
+		NL_SET_ERR_MSG_MOD(extack, "Only HSR v0 and v1 supported");
 		return -EOPNOTSUPP;
 	}
 
+	/* Self MAC address filtering, to avoid frames traversing
+	 * the HSR ring more than once.
+	 */
+	ret = ksz_switch_macaddr_get(ds, port, extack);
+	if (ret)
+		return ret;
+
+	ksz9477_hsr_join(ds, port, hsr);
+	dev->hsr_dev = hsr;
+	dev->hsr_ports |= BIT(port);
+
 	return 0;
 }
 
@@ -3473,24 +3552,17 @@ static int ksz_hsr_leave(struct dsa_switch *ds, int port,
 			 struct net_device *hsr)
 {
 	struct ksz_device *dev = ds->priv;
-	int ret = 0;
 
-	switch (dev->chip_id) {
-	case KSZ9477_CHIP_ID:
-		if (hsr != dev->hsr_dev)
-			return -EOPNOTSUPP;
+	WARN_ON(dev->chip_id != KSZ9477);
 
-		ksz9477_hsr_leave(ds, port, hsr);
-		dev->hsr_ports &= ~BIT(port);
-		if (!dev->hsr_ports)
-			dev->hsr_dev = NULL;
+	ksz9477_hsr_leave(ds, port, hsr);
+	dev->hsr_ports &= ~BIT(port);
+	if (!dev->hsr_ports)
+		dev->hsr_dev = NULL;
 
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
+	ksz_switch_macaddr_put(ds);
 
-	return ret;
+	return 0;
 }
 
 static const struct dsa_switch_ops ksz_switch_ops = {
@@ -3514,6 +3586,7 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.port_bridge_leave	= ksz_port_bridge_leave,
 	.port_hsr_join		= ksz_hsr_join,
 	.port_hsr_leave		= ksz_hsr_leave,
+	.port_set_mac_address	= ksz_port_set_mac_address,
 	.port_stp_state_set	= ksz_port_stp_state_set,
 	.port_pre_bridge_flags	= ksz_port_pre_bridge_flags,
 	.port_bridge_flags	= ksz_port_bridge_flags,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index e36d459de5a1..1f447a34f555 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -101,6 +101,11 @@ struct ksz_ptp_irq {
 	int num;
 };
 
+struct ksz_switch_macaddr {
+	unsigned char addr[ETH_ALEN];
+	refcount_t refcount;
+};
+
 struct ksz_port {
 	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
 	bool learning;
@@ -170,6 +175,7 @@ struct ksz_device {
 	struct ksz_irq girq;
 	struct ksz_ptp_data ptp_data;
 
+	struct ksz_switch_macaddr *switch_macaddr;
 	struct net_device *hsr_dev;     /* HSR */
 	u8 hsr_ports;
 };
@@ -214,6 +220,7 @@ enum ksz_chip_id {
 };
 
 enum ksz_regs {
+	REG_SW_MAC_ADDR,
 	REG_IND_CTRL_0,
 	REG_IND_DATA_8,
 	REG_IND_DATA_CHECK,
diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c
index 753fef757f11..b9d2e3625def 100644
--- a/drivers/net/dsa/xrs700x/xrs700x.c
+++ b/drivers/net/dsa/xrs700x/xrs700x.c
@@ -548,7 +548,8 @@ static void xrs700x_bridge_leave(struct dsa_switch *ds, int port,
 }
 
 static int xrs700x_hsr_join(struct dsa_switch *ds, int port,
-			    struct net_device *hsr)
+			    struct net_device *hsr,
+			    struct netlink_ext_ack *extack)
 {
 	unsigned int val = XRS_HSR_CFG_HSR_PRP;
 	struct dsa_port *partner = NULL, *dp;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 0b9c6aa27047..d98439ea6146 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -969,6 +969,16 @@ struct dsa_switch_ops {
 			       struct phy_device *phy);
 	void	(*port_disable)(struct dsa_switch *ds, int port);
 
+
+	/*
+	 * Notification for MAC address changes on user ports. Drivers can
+	 * currently only veto operations. They should not use the method to
+	 * program the hardware, since the operation is not rolled back in case
+	 * of other errors.
+	 */
+	int	(*port_set_mac_address)(struct dsa_switch *ds, int port,
+					const unsigned char *addr);
+
 	/*
 	 * Compatibility between device trees defining multiple CPU ports and
 	 * drivers which are not OK to use by default the numerically smallest
@@ -1198,7 +1208,8 @@ struct dsa_switch_ops {
 	 * HSR integration
 	 */
 	int	(*port_hsr_join)(struct dsa_switch *ds, int port,
-				 struct net_device *hsr);
+				 struct net_device *hsr,
+				 struct netlink_ext_ack *extack);
 	int	(*port_hsr_leave)(struct dsa_switch *ds, int port,
 				  struct net_device *hsr);
 
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 37ab238e8304..5f01bd4f9dec 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -2024,7 +2024,8 @@ void dsa_shared_port_link_unregister_of(struct dsa_port *dp)
 		dsa_shared_port_setup_phy_of(dp, false);
 }
 
-int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr)
+int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr,
+		      struct netlink_ext_ack *extack)
 {
 	struct dsa_switch *ds = dp->ds;
 	int err;
@@ -2034,7 +2035,7 @@ int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr)
 
 	dp->hsr_dev = hsr;
 
-	err = ds->ops->port_hsr_join(ds, dp->index, hsr);
+	err = ds->ops->port_hsr_join(ds, dp->index, hsr, extack);
 	if (err)
 		dp->hsr_dev = NULL;
 
diff --git a/net/dsa/port.h b/net/dsa/port.h
index dc812512fd0e..334879964e2c 100644
--- a/net/dsa/port.h
+++ b/net/dsa/port.h
@@ -103,7 +103,8 @@ int dsa_port_phylink_create(struct dsa_port *dp);
 void dsa_port_phylink_destroy(struct dsa_port *dp);
 int dsa_shared_port_link_register_of(struct dsa_port *dp);
 void dsa_shared_port_link_unregister_of(struct dsa_port *dp);
-int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
+int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr,
+		      struct netlink_ext_ack *extack);
 void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
 int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast);
 void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid, bool broadcast);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 48db91b33390..4c3e502d7e16 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -457,6 +457,13 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
+	if (ds->ops->port_set_mac_address) {
+		err = ds->ops->port_set_mac_address(ds, dp->index,
+						    addr->sa_data);
+		if (err)
+			return err;
+	}
+
 	/* If the port is down, the address isn't synced yet to hardware or
 	 * to the DSA master, so there is nothing to change.
 	 */
@@ -2862,7 +2869,7 @@ static int dsa_slave_changeupper(struct net_device *dev,
 		}
 	} else if (is_hsr_master(info->upper_dev)) {
 		if (info->linking) {
-			err = dsa_port_hsr_join(dp, info->upper_dev);
+			err = dsa_port_hsr_join(dp, info->upper_dev, extack);
 			if (err == -EOPNOTSUPP) {
 				NL_SET_ERR_MSG_WEAK_MOD(extack,
 							"Offloading not supported");
-- 
2.34.1

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

Hi Vladimir,

> On Wed, Sep 13, 2023 at 04:51:02PM +0300, Vladimir Oltean wrote:
> > > I'm really stunned, how much extra work is required to add two
> > > callbacks to DSA subsystem (to have already implemented feature)
> > > for a single chip IC.  
> > 
> > Some observations are best kept to yourself. This is only the
> > second HSR offload in the entire kernel. To complain that the
> > infrastructure needs some extensions, for something that wasn't
> > even needed for the first implementation (tracking a MAC address),
> > is unrealistic.  
> 
> Can you please test the attached incremental patch, which applies on
> top of your RFC v4 series? It contains an implementation of my own
> review feedback.

Thanks for preparing the patch - it clarified all the point from
previous e-mails... (and shed some light on mine understanding of DSA
internals)

It works when applied on top of v4. No performance regressions (with
nuttcp) observed.

I've also tested the scenario when one tried to alter lan1 after HW
offloading enabled. It was not possible to alter the MAC address.


As fair as I understood from the commit message - some part of this
patch needs to be applied before HSR offloading v4.

Hence I will wait for it to be posted and upstreamed.

Only then some of this patch code would be squashed to v5 of hsr
support.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Vladimir Oltean]

On Thu, Sep 14, 2023 at 11:18:31PM +0200, Lukasz Majewski wrote:
> As fair as I understood from the commit message - some part of this
> patch needs to be applied before HSR offloading v4.
> 
> Hence I will wait for it to be posted and upstreamed.
> 
> Only then some of this patch code would be squashed to v5 of hsr
> support.

No, this isn't how this is going to work. I can't post my patches and
then you post yours, because that would mean some functionality is
introduced without a user (ds->ops->port_set_mac_address), and we don't
accept that, because you may or may not resubmit your HSR patches as a
first user of the new infra.

So, what needs to happen is you need to post all the patches as an
all-or-nothing series. Somewhere in Documentation/process/ it is
probably explained in more detail what to pay attention to, when reposting
what is partly others' work. But the basic idea is that you need to keep
the Author: and Signed-off-by: fields if you aren't making major changes,
but you must also add your own Signed-off-by: at the end. You also have
responsibility for the patches that you post, and have to respond to
review feedback, even if they aren't authored for you. You are obviously
free to make changes to patches until they pass your own criteria.

The most that I can do to help you is to split that squashed patch and
put the result on a branch:
https://github.com/vladimiroltean/linux/commits/lukma-ksz-hsr-rfc-v4

But it's up to you to take it from there, rebase it on net-next, review
the result, test it, make sure that the changes are something that you
can justify when submitting, etc. You won't be alone if you need help,
of course, but the point is that you're not 100% passive to this activity.

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

Hi Vladimir,

> On Thu, Sep 14, 2023 at 11:18:31PM +0200, Lukasz Majewski wrote:
> > As fair as I understood from the commit message - some part of this
> > patch needs to be applied before HSR offloading v4.
> > 
> > Hence I will wait for it to be posted and upstreamed.
> > 
> > Only then some of this patch code would be squashed to v5 of hsr
> > support.  
> 
> No, this isn't how this is going to work. I can't post my patches and
> then you post yours, because that would mean some functionality is
> introduced without a user (ds->ops->port_set_mac_address), and we
> don't accept that, because you may or may not resubmit your HSR
> patches as a first user of the new infra.

Ok. Thanks for the clarification.

> 
> So, what needs to happen is you need to post all the patches as an
> all-or-nothing series. Somewhere in Documentation/process/ it is
> probably explained in more detail what to pay attention to, when
> reposting what is partly others' work. But the basic idea is that you
> need to keep the Author: and Signed-off-by: fields if you aren't
> making major changes, but you must also add your own Signed-off-by:
> at the end. You also have responsibility for the patches that you
> post, and have to respond to review feedback, even if they aren't
> authored for you. You are obviously free to make changes to patches
> until they pass your own criteria.
> 
> The most that I can do to help you is to split that squashed patch and
> put the result on a branch:
> https://github.com/vladimiroltean/linux/commits/lukma-ksz-hsr-rfc-v4
> 
> But it's up to you to take it from there, rebase it on net-next,
> review the result, test it, make sure that the changes are something
> that you can justify when submitting, etc. You won't be alone if you
> need help, of course, but the point is that you're not 100% passive
> to this activity.

No problem. I will test it for a while and then send them for review.

Many thanks for help.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 10084 bytes --]

Hi Vladimir,

> On Wed, Sep 13, 2023 at 02:15:48PM +0200, Lukasz Majewski wrote:
> > > I thought we were in agreement to program the actual DSA user
> > > ports' MAC addresses to hardware.  
> > 
> > No - v4 of this solution uses HSR net device MAC address, which is
> > supposed to be the same as DSA master.  
> 
> By "in agreement" I mean "as a result of the discussion on v4 [ this
> discussion ], where it became obvious that the DSA master solution is
> not so robust". I hope the 12 emails already exchanged on this patch
> set weren't for nothing.
> 
> > > With KSZ switches, a single CPU port is supported, so all ports
> > > share the same DSA master. So if the contents of
> > > REG_SW_MAC_ADDR_0 is different from the DSA master (the same DSA
> > > master that was used for an earlier HSR offload), why do you
> > > infer that it was altered by WoL? It makes no sense.  
> > 
> > So where is the issue? The only issue is that somebody would change
> > DSA master (and hence HSR) MAC address, so the REG_SW_MAC_ADDR_0
> > would not be changed. Or do I miss something?  
> 
> Changing the DSA master address and changing the HSR MAC address
> (indirectly through the ports' address) are different operations.
> The DSA master's address and the user ports' address are not
> necessarily in sync.
> 
> Each address change is problematic in its own way, and each problem
> needs to be tackled in its own way, depending on which MAC address you
> desire for REG_SW_MAC_ADDR_0 to track.
> 
> But yes, the only issue is that the MAC address can be changed at
> runtime, after the initial offload.
> 
> > > - Changing the MAC address of (a) triggers a pre-existing bug.
> > > That bug can be separated from the HSR offload discussion if the
> > > HSR offload decides to not program the DSA master's MAC address to
> > > hardware, but a different MAC address. The pre-existence of the
> > > DSA bug is not a strong enough argument per se to avoid
> > > programming the DSA master's address to hardware.  
> > 
> > And this is how v4 is implemented. Or not?  
> 
> If A == B initially, then there are 2 ways you can change that
> condition. You can change A, or you can change B. Replace "A" with
> "the DSA master's address" and "B" with "the HSR device's address".
> 
> > > - Changing the MAC address of (c) does not seem directly possible,
> > > but:
> > > 
> > > - Changing the MAC address of (b) also triggers (c) - see
> > > hsr_netdev_notify(). This is because the HSR driver makes sure
> > > that the addresses of port_A, port_B and the HSR device are equal
> > > at all times.  
> > 
> > Why changing HSR port would affect HSR device MAC address?  
> 
> I don't have a simpler answer than "because that's what the code
> does".
> 
> If you don't have time to experiment to convince yourself that this is
> the case, below is a set of commands that should hopefully clarify.
> 
> $ ip link
> 6: eno2: <BROADCAST,MULTICAST> mtu 1508 qdisc noop state DOWN group
> default qlen 1000 link/ether 2a:af:42:b7:73:11 brd ff:ff:ff:ff:ff:ff
>     altname end0
>     altname enp0s0f2
> 7: swp0@eno2: <BROADCAST,MULTICAST,M-DOWN> mtu 1504 qdisc noop state
> DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff 8: swp1@eno2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500
> qdisc noop state DOWN group default qlen 1000 link/ether
> a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff 9: swp2@eno2:
> <BROADCAST,MULTICAST,M-DOWN> mtu 1504 qdisc noop state DOWN group
> default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
> 10: sw0p0@swp0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff 11: sw0p1@swp0: <BROADCAST,MULTICAST,M-DOWN> mtu
> 1500 qdisc noop state DOWN group default qlen 1000 link/ether
> a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff 12: sw0p2@swp0:
> <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group
> default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
> 13: sw2p0@swp2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff 14: sw2p1@swp2: <BROADCAST,MULTICAST,M-DOWN> mtu
> 1500 qdisc noop state DOWN group default qlen 1000 link/ether
> a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff 15: sw2p2@swp2:
> <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group
> default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd ff:ff:ff:ff:ff:ff
> 16: sw2p3@swp2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop
> state DOWN group default qlen 1000 link/ether a6:f4:af:fd:fc:73 brd
> ff:ff:ff:ff:ff:ff # Simplified in a table for brevity. The format
> will be kept below $ ip link show ... sw0p0              sw0p1
>       DSA master          hsr0 addr a6:f4:af:fd:fc:73
> a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73   n/a
> 
> $ ip link add hsr0 type hsr slave1 sw0p0 slave2 sw0p1 version 1
> [   70.033711] sja1105 spi2.0 sw0p0: entered promiscuous mode
> [   70.058066] sja1105 spi2.0 sw0p1: entered promiscuous mode
> Warning: dsa_core: Offloading not supported.
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73
> a6:f4:af:fd:fc:73
> 
> $ ip link set swp0 address 00:01:02:03:04:05 # DSA master
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr a6:f4:af:fd:fc:73  a6:f4:af:fd:fc:73  00:01:02:03:04:05
> a6:f4:af:fd:fc:73
> 
> $ ip link set sw0p0 address 00:01:02:03:04:06
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr 00:01:02:03:04:06  a6:f4:af:fd:fc:73  00:01:02:03:04:05
> 00:01:02:03:04:06
> 
> $ ip link set sw0p1 address 00:01:02:03:04:07
> 
> $ ip link show ...
>      sw0p0              sw0p1              DSA master          hsr0
> addr 00:01:02:03:04:06  00:01:02:03:04:07  00:01:02:03:04:05
> 00:01:02:03:04:06
> 
> > Shouldn't it be forbidden, and HSR ports shall inherit MAC address
> > of HSR device (hsr0) ?  
> 
> Since HSR does _actually_ track the MAC address of port_A (sw0p0
> above), I guess it would be best if the API introduced would always
> program REG_SW_MAC_ADDR_0 with the MAC address of the first port that
> joins the HSR (and requests a reference to REG_SW_MAC_ADDR_0). That
> way, the API should work with no changing for WoL as well. Anyway -
> minor difference between first user port and HSR dev_addr.

I've made wrong assumptions - I thought that when we have

hsr0    -> lan1
	-> lan2

it is only possible to adjust hsr0 MAC address as lan1,lan2 are in some
way "slaves" for hsr0.

In other words - I thought that lan1, lan2 "disappear" from "normal"
DSA control as they after "join" are "represented" to DSA by hsr0 (the
below hierarchy).

eth0 	-->  lan3
	-->  lan4
	-->  hsr0	-> lan2
			-> lan1

And then you explained a use case where one can adjust MAC address of
lan1 and it would be propagated to hsr0.

Now it is clear.

> 
> > > The simple matter is: if you program the MAC address of a netdev
> > > (any netdev) to hardware  
> > 
> > Which netdev in this case? lan1, lan2, hsr0 or eth0 ?  
> 
> Any netdev. It is a general statement which had a continuation, which
> you've interrupted.
> 
> > > , then for a correct and robust implementation, you
> > > need to make sure that the hardware will always be in sync with
> > > that address, keeping in mind that the user may change it. Either
> > > you deny changes, or you update the hardware when the address is
> > > updated. 
> > 
> > Why I cannot just:
> > 
> > 1. Check on hsr_join if lan1, lan2, hsr0 and eth0 MAC is equal and
> > write it to REG_SW_MAC_ADDR_0?  
> 
> This is actually unnecessary if you implement the reference scheme on
> REG_SW_MAC_ADDR_0 that I've suggested. Checking the MAC address of
> eth0 is unnecessary in any case, if you don't program it to hardware.
> 
> > 2. Forbid the change of lan1/lan2/eth0/hsr0 if it may affect HSR HW
> > offloading? If user want to change it - then one shall delete hsr0
> > net device, change MAC and create it again.  
> 
> I've been saying since yesterday that you should do this.
> 
> > How point 2 can be achieved (if possible) ?  
> 
> Re-read earlier emails.
> 
> > > If the DSA user port MAC address changes,  
> > 
> > You mean lan1, lan2, which are joined with hsr0?  
> 
> Yup. I've been saying this for a number of emails already:
> https://lore.kernel.org/netdev/20230912092909.4yj4b2b4xrhzdztu@skbuf/
> 
> | The ports which hold a reference on REG_SW_MAC_ADDR_0 cannot have
> their | MAC address changed - and for this, you'd have to add a hook
> to | dsa_switch_ops (and thus to the driver) from
> | dsa_slave_set_mac_address().
> 
> > > and REG_SW_MAC_ADDR_0 was
> > > previously programmed with it, and nothing is done in reaction to
> > > this, then this is a problem with the HSR offload.   
> > 
> > This shall be just forbidden?  
> 
> Yup.
> 
> > > So no, it's not
> > > just a problem with upcoming WoL patches as you imply. You need to
> > > integrate a solution to that problem as part of your HSR patches.
> > >  
> > 
> > I'm really stunned, how much extra work is required to add two
> > callbacks to DSA subsystem (to have already implemented feature)
> > for a single chip IC.  
> 
> Some observations are best kept to yourself. This is only the second
> HSR offload in the entire kernel. To complain that the infrastructure
> needs some extensions, for something that wasn't even needed for the
> first implementation (tracking a MAC address), is unrealistic.




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]