[PATCH net-next v6 0/9] net: dsa: microchip: provide Wake on LAN support

[PATCH net-next v6 0/9] net: dsa: microchip: provide Wake on LAN support

[Oleksij Rempel]

This patch series introduces extensive Wake on LAN (WoL) support for the
Microchip KSZ9477 family of switches, coupled with some code refactoring
and error handling enhancements. The principal aim is to enable and
manage Wake on Magic Packet and other PHY event triggers for waking up
the system, whilst ensuring that the switch isn't reset during a
shutdown if WoL is active.

The Wake on LAN functionality is optional and is particularly beneficial
if the PME pins are connected to the SoC as a wake source or to a PMIC
that can enable or wake the SoC.

changes v6:
- add variables magic_switched_off and magic_switched_on for readability
- EXPORT_SYMBOL(ksz_switch_shutdown); to fix build as module 

changes v5:
- rework Wake on Magic Packet support.
- Make sure we show more or less realistic information on get_wol by
  comparing refcounted mac address against the ports address
- fix mac address refcounting on set_wol()
- rework shutdown sequence by to handle PMIC related issues. Make sure
  PME pin is net frequently toggled.
- use wakeup_source variable instead of reading PME pin register.

changes v4:
- add ksz_switch_shutdown() and do not skip dsa_switch_shutdown() and
  etc.
- try to configure MAC address on WAKE_MAGIC. If not possible, prevent
  WAKE_MAGIC configuration
- use ksz_switch_macaddr_get() for WAKE_MAGIC.
- prevent ksz_port_set_mac_address if WAKE_MAGIC is active
- do some more refactoring and patch reordering

changes v3:
- use ethernet address of DSA master instead from devicetree
- use dev_ops->wol* instead of list of supported switch
- don't shutdown the switch if WoL is enabled
- rework on top of latest HSR changes

changes v2:
- rebase against latest next

Oleksij Rempel (9):
  net: dsa: microchip: Add missing MAC address register offset for
    ksz8863
  dt-bindings: net: dsa: microchip: add wakeup-source property
  net: dsa: microchip: use wakeup-source DT property to enable PME
    output
  net: dsa: microchip: ksz9477: add Wake on LAN support
  net: dsa: microchip: ksz9477: Add Wake on Magic Packet support
  net: dsa: microchip: Refactor comment for ksz_switch_macaddr_get()
    function
  net: dsa: microchip: Add error handling for ksz_switch_macaddr_get()
  net: dsa: microchip: Refactor switch shutdown routine for WoL
    preparation
  net: dsa: microchip: Ensure Stable PME Pin State for Wake-on-LAN

 .../bindings/net/dsa/microchip,ksz.yaml       |   2 +
 drivers/net/dsa/microchip/ksz9477.c           | 198 ++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h           |   5 +
 drivers/net/dsa/microchip/ksz9477_i2c.c       |   5 +-
 drivers/net/dsa/microchip/ksz_common.c        |  97 +++++++--
 drivers/net/dsa/microchip/ksz_common.h        |  10 +
 drivers/net/dsa/microchip/ksz_spi.c           |   5 +-
 7 files changed, 302 insertions(+), 20 deletions(-)

-- 
2.39.2

[PATCH net-next v6 1/9] net: dsa: microchip: Add missing MAC address register offset for ksz8863

[Oleksij Rempel]

Add the missing offset for the global MAC address register
(REG_SW_MAC_ADDR) for the ksz8863 family of switches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b800ace40ce1..02fab1adb27f 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -441,6 +441,7 @@ static const u8 ksz8795_shifts[] = {
 };
 
 static const u16 ksz8863_regs[] = {
+	[REG_SW_MAC_ADDR]		= 0x70,
 	[REG_IND_CTRL_0]		= 0x79,
 	[REG_IND_DATA_8]		= 0x7B,
 	[REG_IND_DATA_CHECK]		= 0x7B,
-- 
2.39.2

[PATCH net-next v6 2/9] dt-bindings: net: dsa: microchip: add wakeup-source property

[Oleksij Rempel]

Add wakeup-source property to enable Wake on Lan functionality in the
switch.

Since PME wake pin is not always attached to the SoC, use wakeup-source
instead of wakeup-gpios

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 41014f5c01c4..5751a729af33 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -72,6 +72,8 @@ properties:
   interrupts:
     maxItems: 1
 
+  wakeup-source: true
+
 required:
   - compatible
   - reg
-- 
2.39.2

[PATCH net-next v6 3/9] net: dsa: microchip: use wakeup-source DT property to enable PME output

[Oleksij Rempel]

KSZ switches with WoL support signals wake event over PME pin. If this
pin is attached to some external PMIC or System Controller can't be
described as GPIO, the only way to describe it in the devicetree is to
use wakeup-source property. So, add support for this property and enable
PME switch output if this property is present.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 3 +++
 drivers/net/dsa/microchip/ksz_common.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 02fab1adb27f..11adae8a2037 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -4159,6 +4159,9 @@ int ksz_switch_register(struct ksz_device *dev)
 			dev_err(dev->dev, "inconsistent synclko settings\n");
 			return -EINVAL;
 		}
+
+		dev->wakeup_source = of_property_read_bool(dev->dev->of_node,
+							   "wakeup-source");
 	}
 
 	ret = dsa_register_switch(dev->ds);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 8842efca0871..f7c471bc040f 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -163,6 +163,7 @@ struct ksz_device {
 	phy_interface_t compat_interface;
 	bool synclko_125;
 	bool synclko_disable;
+	bool wakeup_source;
 
 	struct vlan_table *vlan_cache;
 
-- 
2.39.2

[PATCH net-next v6 4/9] net: dsa: microchip: ksz9477: add Wake on LAN support

[Oleksij Rempel]

Add WoL support for KSZ9477 family of switches. This code was tested on
KSZ8563 chip.

KSZ9477 family of switches supports multiple PHY events:
- wake on Link Up
- wake on Energy Detect.
Since current UAPI can't differentiate between this PHY events, map all of them
to WAKE_PHY.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 100 +++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h    |   4 +
 drivers/net/dsa/microchip/ksz_common.c |  24 ++++++
 drivers/net/dsa/microchip/ksz_common.h |   4 +
 4 files changed, 132 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index cde8ef33d029..b9419d4b5e7b 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -56,6 +56,103 @@ int ksz9477_change_mtu(struct ksz_device *dev, int port, int mtu)
 				  REG_SW_MTU_MASK, frame_size);
 }
 
+/**
+ * ksz9477_handle_wake_reason - Handle wake reason on a specified port.
+ * @dev: The device structure.
+ * @port: The port number.
+ *
+ * This function reads the PME (Power Management Event) status register of a
+ * specified port to determine the wake reason. If there is no wake event, it
+ * returns early. Otherwise, it logs the wake reason which could be due to a
+ * "Magic Packet", "Link Up", or "Energy Detect" event. The PME status register
+ * is then cleared to acknowledge the handling of the wake event.
+ *
+ * Return: 0 on success, or an error code on failure.
+ */
+static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port)
+{
+	u8 pme_status;
+	int ret;
+
+	ret = ksz_pread8(dev, port, REG_PORT_PME_STATUS, &pme_status);
+	if (ret)
+		return ret;
+
+	if (!pme_status)
+		return 0;
+
+	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port,
+		pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "",
+		pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : "");
+
+	return ksz_pwrite8(dev, port, REG_PORT_PME_STATUS, pme_status);
+}
+
+/**
+ * ksz9477_get_wol - Get Wake-on-LAN settings for a specified port.
+ * @dev: The device structure.
+ * @port: The port number.
+ * @wol: Pointer to ethtool Wake-on-LAN settings structure.
+ *
+ * This function checks the PME Pin Control Register to see if  PME Pin Output
+ * Enable is set, indicating PME is enabled. If enabled, it sets the supported
+ * and active WoL flags.
+ */
+void ksz9477_get_wol(struct ksz_device *dev, int port,
+		     struct ethtool_wolinfo *wol)
+{
+	u8 pme_ctrl;
+	int ret;
+
+	if (!dev->wakeup_source)
+		return;
+
+	wol->supported = WAKE_PHY;
+
+	ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl);
+	if (ret)
+		return;
+
+	if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY))
+		wol->wolopts |= WAKE_PHY;
+}
+
+/**
+ * ksz9477_set_wol - Set Wake-on-LAN settings for a specified port.
+ * @dev: The device structure.
+ * @port: The port number.
+ * @wol: Pointer to ethtool Wake-on-LAN settings structure.
+ *
+ * This function configures Wake-on-LAN (WoL) settings for a specified port.
+ * It validates the provided WoL options, checks if PME is enabled via the
+ * switch's PME Pin Control Register, clears any previous wake reasons,
+ * and sets the Magic Packet flag in the port's PME control register if
+ * specified.
+ *
+ * Return: 0 on success, or other error codes on failure.
+ */
+int ksz9477_set_wol(struct ksz_device *dev, int port,
+		    struct ethtool_wolinfo *wol)
+{
+	u8 pme_ctrl = 0;
+	int ret;
+
+	if (wol->wolopts & ~WAKE_PHY)
+		return -EINVAL;
+
+	if (!dev->wakeup_source)
+		return -EOPNOTSUPP;
+
+	ret = ksz9477_handle_wake_reason(dev, port);
+	if (ret)
+		return ret;
+
+	if (wol->wolopts & WAKE_PHY)
+		pme_ctrl |= PME_WOL_LINKUP | PME_WOL_ENERGY;
+
+	return ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, pme_ctrl);
+}
+
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
 {
 	unsigned int val;
@@ -1006,6 +1103,9 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		ksz_pread16(dev, port, REG_PORT_PHY_INT_ENABLE, &data16);
 
 	ksz9477_port_acl_init(dev, port);
+
+	/* clear pending wake flags */
+	ksz9477_handle_wake_reason(dev, port);
 }
 
 void ksz9477_config_cpu_port(struct dsa_switch *ds)
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index f90e2e8ebe80..fa8d0318b437 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -58,6 +58,10 @@ 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);
+void ksz9477_get_wol(struct ksz_device *dev, int port,
+		     struct ethtool_wolinfo *wol);
+int ksz9477_set_wol(struct ksz_device *dev, int port,
+		    struct ethtool_wolinfo *wol);
 
 int ksz9477_port_acl_init(struct ksz_device *dev, int port);
 void ksz9477_port_acl_free(struct ksz_device *dev, int port);
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 11adae8a2037..3f7c86e545a7 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -319,6 +319,8 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.mdb_del = ksz9477_mdb_del,
 	.change_mtu = ksz9477_change_mtu,
 	.phylink_mac_link_up = ksz9477_phylink_mac_link_up,
+	.get_wol = ksz9477_get_wol,
+	.set_wol = ksz9477_set_wol,
 	.config_cpu_port = ksz9477_config_cpu_port,
 	.tc_cbs_set_cinc = ksz9477_tc_cbs_set_cinc,
 	.enable_stp_addr = ksz9477_enable_stp_addr,
@@ -3543,6 +3545,26 @@ static int ksz_setup_tc(struct dsa_switch *ds, int port,
 	}
 }
 
+static void ksz_get_wol(struct dsa_switch *ds, int port,
+			struct ethtool_wolinfo *wol)
+{
+	struct ksz_device *dev = ds->priv;
+
+	if (dev->dev_ops->get_wol)
+		dev->dev_ops->get_wol(dev, port, wol);
+}
+
+static int ksz_set_wol(struct dsa_switch *ds, int port,
+		       struct ethtool_wolinfo *wol)
+{
+	struct ksz_device *dev = ds->priv;
+
+	if (dev->dev_ops->set_wol)
+		return dev->dev_ops->set_wol(dev, port, wol);
+
+	return -EOPNOTSUPP;
+}
+
 static int ksz_port_set_mac_address(struct dsa_switch *ds, int port,
 				    const unsigned char *addr)
 {
@@ -3727,6 +3749,8 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_pause_stats	= ksz_get_pause_stats,
 	.port_change_mtu	= ksz_change_mtu,
 	.port_max_mtu		= ksz_max_mtu,
+	.get_wol		= ksz_get_wol,
+	.set_wol		= ksz_set_wol,
 	.get_ts_info		= ksz_get_ts_info,
 	.port_hwtstamp_get	= ksz_hwtstamp_get,
 	.port_hwtstamp_set	= ksz_hwtstamp_set,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index f7c471bc040f..a7394175fcf6 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -374,6 +374,10 @@ struct ksz_dev_ops {
 				    int duplex, bool tx_pause, bool rx_pause);
 	void (*setup_rgmii_delay)(struct ksz_device *dev, int port);
 	int (*tc_cbs_set_cinc)(struct ksz_device *dev, int port, u32 val);
+	void (*get_wol)(struct ksz_device *dev, int port,
+			struct ethtool_wolinfo *wol);
+	int (*set_wol)(struct ksz_device *dev, int port,
+		       struct ethtool_wolinfo *wol);
 	void (*config_cpu_port)(struct dsa_switch *ds);
 	int (*enable_stp_addr)(struct ksz_device *dev);
 	int (*reset)(struct ksz_device *dev);
-- 
2.39.2

[PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Oleksij Rempel]

Introduce Wake on Magic Packet (WoL) functionality to the ksz9477
driver.

Major changes include:

1. Extending the `ksz9477_handle_wake_reason` function to identify Magic
   Packet wake events alongside existing wake reasons.

2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to
   handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to
   program the switch's MAC address register accordingly when Magic
   Packet wake-up is enabled. This change will prevent WAKE_MAGIC
   activation if the related port has a different MAC address compared
   to a MAC address already used by HSR or an already active WAKE_MAGIC
   on another port.

3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC
   address changes on ports with active Wake on Magic Packet, as the
   switch's MAC address register is utilized for this feature.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz9477.c    | 60 ++++++++++++++++++++++++--
 drivers/net/dsa/microchip/ksz_common.c | 15 +++++--
 drivers/net/dsa/microchip/ksz_common.h |  3 ++
 3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index b9419d4b5e7b..bcc8863951ca 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -81,7 +81,8 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port)
 	if (!pme_status)
 		return 0;
 
-	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port,
+	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port,
+		pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "",
 		pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "",
 		pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : "");
 
@@ -109,10 +110,22 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
 
 	wol->supported = WAKE_PHY;
 
+	/* Check if at this moment we would be able to get the MAC address
+	 * and use it for WAKE_MAGIC support. This result may change dynamically
+	 * depending on configuration of other ports.
+	 */
+	ret = ksz_switch_macaddr_get(dev->ds, port, NULL);
+	if (!ret) {
+		wol->supported |= WAKE_MAGIC;
+		ksz_switch_macaddr_put(dev->ds);
+	}
+
 	ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl);
 	if (ret)
 		return;
 
+	if (pme_ctrl & PME_WOL_MAGICPKT)
+		wol->wolopts |= WAKE_MAGIC;
 	if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY))
 		wol->wolopts |= WAKE_PHY;
 }
@@ -134,10 +147,12 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
 int ksz9477_set_wol(struct ksz_device *dev, int port,
 		    struct ethtool_wolinfo *wol)
 {
-	u8 pme_ctrl = 0;
+	u8 pme_ctrl = 0, pme_ctrl_old = 0;
+	bool magic_switched_off;
+	bool magic_switched_on;
 	int ret;
 
-	if (wol->wolopts & ~WAKE_PHY)
+	if (wol->wolopts & ~(WAKE_PHY | WAKE_MAGIC))
 		return -EINVAL;
 
 	if (!dev->wakeup_source)
@@ -147,10 +162,42 @@ int ksz9477_set_wol(struct ksz_device *dev, int port,
 	if (ret)
 		return ret;
 
+	if (wol->wolopts & WAKE_MAGIC)
+		pme_ctrl |= PME_WOL_MAGICPKT;
 	if (wol->wolopts & WAKE_PHY)
 		pme_ctrl |= PME_WOL_LINKUP | PME_WOL_ENERGY;
 
-	return ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, pme_ctrl);
+	ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl_old);
+	if (ret)
+		return ret;
+
+	if (pme_ctrl_old == pme_ctrl)
+		return 0;
+
+	magic_switched_off = (pme_ctrl_old & PME_WOL_MAGICPKT) &&
+			    !(pme_ctrl & PME_WOL_MAGICPKT);
+	magic_switched_on = !(pme_ctrl_old & PME_WOL_MAGICPKT) &&
+			    (pme_ctrl & PME_WOL_MAGICPKT);
+
+	/* To keep reference count of MAC address, we should do this
+	 * operation only on change of WOL settings.
+	 */
+	if (magic_switched_on) {
+		ret = ksz_switch_macaddr_get(dev->ds, port, NULL);
+		if (ret)
+			return ret;
+	} else if (magic_switched_off) {
+		ksz_switch_macaddr_put(dev->ds);
+	}
+
+	ret = ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, pme_ctrl);
+	if (ret) {
+		if (magic_switched_on)
+			ksz_switch_macaddr_put(dev->ds);
+		return ret;
+	}
+
+	return 0;
 }
 
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
@@ -1106,6 +1153,11 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 
 	/* clear pending wake flags */
 	ksz9477_handle_wake_reason(dev, port);
+
+	/* Disable all WoL options by default. Otherwise
+	 * ksz_switch_macaddr_get/put logic will not work properly.
+	 */
+	ksz_pwrite8(dev, port, REG_PORT_PME_CTRL, 0);
 }
 
 void ksz9477_config_cpu_port(struct dsa_switch *ds)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 3f7c86e545a7..377998966b13 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3569,6 +3569,7 @@ 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);
+	struct ethtool_wolinfo wol;
 
 	if (dp->hsr_dev) {
 		dev_err(ds->dev,
@@ -3577,6 +3578,14 @@ static int ksz_port_set_mac_address(struct dsa_switch *ds, int port,
 		return -EBUSY;
 	}
 
+	ksz_get_wol(ds, dp->index, &wol);
+	if (wol.wolopts & WAKE_MAGIC) {
+		dev_err(ds->dev,
+			"Cannot change MAC address on port %d with active Wake on Magic Packet\n",
+			port);
+		return -EBUSY;
+	}
+
 	return 0;
 }
 
@@ -3587,8 +3596,8 @@ static int ksz_port_set_mac_address(struct dsa_switch *ds, int port,
  * 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)
+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;
@@ -3628,7 +3637,7 @@ static int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static void ksz_switch_macaddr_put(struct dsa_switch *ds)
+void ksz_switch_macaddr_put(struct dsa_switch *ds)
 {
 	struct ksz_switch_macaddr *switch_macaddr;
 	struct ksz_device *dev = ds->priv;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index a7394175fcf6..8fc3210d7a3d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -396,6 +396,9 @@ void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
 bool ksz_get_gbit(struct ksz_device *dev, int port);
 phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
 extern const struct ksz_chip_data ksz_switch_chips[];
+int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
+			   struct netlink_ext_ack *extack);
+void ksz_switch_macaddr_put(struct dsa_switch *ds);
 
 /* Common register access functions */
 static inline struct regmap *ksz_regmap_8(struct ksz_device *dev)
-- 
2.39.2

[PATCH net-next v6 6/9] net: dsa: microchip: Refactor comment for ksz_switch_macaddr_get() function

[Oleksij Rempel]

Update the comment to follow kernel-doc format.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 377998966b13..7b05de6fe987 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3589,12 +3589,20 @@ static int ksz_port_set_mac_address(struct dsa_switch *ds, int port,
 	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.
+/**
+ * ksz_switch_macaddr_get - Program the switch's MAC address register.
+ * @ds: DSA switch instance.
+ * @port: Port number.
+ * @extack: Netlink extended acknowledgment.
+ *
+ * This function programs 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
+ * can share ownership of this address as long as their MAC address is the same.
+ * The MAC addresses of user ports must not change while they have ownership of
+ * the switch MAC address.
+ *
+ * Return: 0 on success, or other error codes on failure.
  */
 int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
 			   struct netlink_ext_ack *extack)
-- 
2.39.2

[PATCH net-next v6 7/9] net: dsa: microchip: Add error handling for ksz_switch_macaddr_get()

[Oleksij Rempel]

Enhance the ksz_switch_macaddr_get() function to handle errors that may
occur during the call to ksz_write8(). Specifically, this update checks
the return value of ksz_write8(), which may fail if regmap ranges
validation is not passed and returns the error code.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7b05de6fe987..79052a54880c 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3612,7 +3612,7 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
 	struct ksz_switch_macaddr *switch_macaddr;
 	struct ksz_device *dev = ds->priv;
 	const u16 *regs = dev->info->regs;
-	int i;
+	int i, ret;
 
 	/* Make sure concurrent MAC address changes are blocked */
 	ASSERT_RTNL();
@@ -3639,8 +3639,11 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
 	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]);
+	for (i = 0; i < ETH_ALEN; i++) {
+		ret = ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.39.2

[PATCH net-next v6 8/9] net: dsa: microchip: Refactor switch shutdown routine for WoL preparation

[Oleksij Rempel]

Centralize the switch shutdown routine in a dedicated function,
ksz_switch_shutdown(), to enhance code maintainability and reduce
redundancy. This change abstracts the common shutdown operations
previously duplicated in ksz9477_i2c_shutdown() and ksz_spi_shutdown().

This refactoring is a preparatory step for an upcoming patch to avoid
reset on shutdown if Wake-on-LAN (WoL) is enabled.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/microchip/ksz9477_i2c.c |  5 +----
 drivers/net/dsa/microchip/ksz_common.c  | 19 +++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h  |  1 +
 drivers/net/dsa/microchip/ksz_spi.c     |  5 +----
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 2710afad4f3a..cac4a607e54a 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -66,10 +66,7 @@ static void ksz9477_i2c_shutdown(struct i2c_client *i2c)
 	if (!dev)
 		return;
 
-	if (dev->dev_ops->reset)
-		dev->dev_ops->reset(dev);
-
-	dsa_switch_shutdown(dev->ds);
+	ksz_switch_shutdown(dev);
 
 	i2c_set_clientdata(i2c, NULL);
 }
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 79052a54880c..08556b1dc452 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3810,6 +3810,25 @@ struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
 }
 EXPORT_SYMBOL(ksz_switch_alloc);
 
+/**
+ * ksz_switch_shutdown - Shutdown routine for the switch device.
+ * @dev: The switch device structure.
+ *
+ * This function is responsible for initiating a shutdown sequence for the
+ * switch device. It invokes the reset operation defined in the device
+ * operations, if available, to reset the switch. Subsequently, it calls the
+ * DSA framework's shutdown function to ensure a proper shutdown of the DSA
+ * switch.
+ */
+void ksz_switch_shutdown(struct ksz_device *dev)
+{
+	if (dev->dev_ops->reset)
+		dev->dev_ops->reset(dev);
+
+	dsa_switch_shutdown(dev->ds);
+}
+EXPORT_SYMBOL(ksz_switch_shutdown);
+
 static void ksz_parse_rgmii_delay(struct ksz_device *dev, int port_num,
 				  struct device_node *port_dn)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 8fc3210d7a3d..34a8e9784cca 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -399,6 +399,7 @@ extern const struct ksz_chip_data ksz_switch_chips[];
 int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
 			   struct netlink_ext_ack *extack);
 void ksz_switch_macaddr_put(struct dsa_switch *ds);
+void ksz_switch_shutdown(struct ksz_device *dev);
 
 /* Common register access functions */
 static inline struct regmap *ksz_regmap_8(struct ksz_device *dev)
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 279338451621..6f6d878e742c 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -114,10 +114,7 @@ static void ksz_spi_shutdown(struct spi_device *spi)
 	if (!dev)
 		return;
 
-	if (dev->dev_ops->reset)
-		dev->dev_ops->reset(dev);
-
-	dsa_switch_shutdown(dev->ds);
+	ksz_switch_shutdown(dev);
 
 	spi_set_drvdata(spi, NULL);
 }
-- 
2.39.2

[PATCH net-next v6 9/9] net: dsa: microchip: Ensure Stable PME Pin State for Wake-on-LAN

[Oleksij Rempel]

Ensures a stable PME (Power Management Event) pin state by disabling PME
on system start and enabling it on shutdown only if WoL (Wake-on-LAN) is
configured. This is needed to avoid issues with some PMICs (Power
Management ICs).

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz9477.c    | 46 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h    |  1 +
 drivers/net/dsa/microchip/ksz_common.c |  8 ++++-
 drivers/net/dsa/microchip/ksz_common.h |  1 +
 4 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index bcc8863951ca..d893dfd68815 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -200,6 +200,46 @@ int ksz9477_set_wol(struct ksz_device *dev, int port,
 	return 0;
 }
 
+/**
+ * ksz9477_wol_pre_shutdown - Prepares the switch device for shutdown while
+ *                            considering Wake-on-LAN (WoL) settings.
+ * @dev: The switch device structure.
+ * @wol_enabled: Pointer to a boolean which will be set to true if WoL is
+ *               enabled on any port.
+ *
+ * This function prepares the switch device for a safe shutdown while taking
+ * into account the Wake-on-LAN (WoL) settings on the user ports. It updates
+ * the wol_enabled flag accordingly to reflect whether WoL is active on any
+ * port.
+ */
+void ksz9477_wol_pre_shutdown(struct ksz_device *dev, bool *wol_enabled)
+{
+	struct dsa_port *dp;
+	int ret;
+
+	*wol_enabled = false;
+
+	if (!dev->wakeup_source)
+		return;
+
+	dsa_switch_for_each_user_port(dp, dev->ds) {
+		u8 pme_ctrl = 0;
+
+		ret = ksz_pread8(dev, dp->index, REG_PORT_PME_CTRL, &pme_ctrl);
+		if (!ret && pme_ctrl)
+			*wol_enabled = true;
+
+		/* make sure there are no pending wake events which would
+		 * prevent the device from going to sleep/shutdown.
+		 */
+		ksz9477_handle_wake_reason(dev, dp->index);
+	}
+
+	/* Now we are save to enable PME pin. */
+	if (*wol_enabled)
+		ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE);
+}
+
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev)
 {
 	unsigned int val;
@@ -1280,6 +1320,12 @@ int ksz9477_setup(struct dsa_switch *ds)
 	/* enable global MIB counter freeze function */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE, true);
 
+	/* Make sure PME (WoL) is not enabled. If requested, it will be
+	 * enabled by ksz9477_wol_pre_shutdown(). Otherwise, some PMICs do not
+	 * like PME events changes before shutdown.
+	 */
+	ksz_write8(dev, REG_SW_PME_CTRL, 0);
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index fa8d0318b437..9e6f1e4b57b7 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -62,6 +62,7 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
 		     struct ethtool_wolinfo *wol);
 int ksz9477_set_wol(struct ksz_device *dev, int port,
 		    struct ethtool_wolinfo *wol);
+void ksz9477_wol_pre_shutdown(struct ksz_device *dev, bool *wol_is_on);
 
 int ksz9477_port_acl_init(struct ksz_device *dev, int port);
 void ksz9477_port_acl_free(struct ksz_device *dev, int port);
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 08556b1dc452..6945ea4febd0 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -321,6 +321,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.phylink_mac_link_up = ksz9477_phylink_mac_link_up,
 	.get_wol = ksz9477_get_wol,
 	.set_wol = ksz9477_set_wol,
+	.wol_pre_shutdown = ksz9477_wol_pre_shutdown,
 	.config_cpu_port = ksz9477_config_cpu_port,
 	.tc_cbs_set_cinc = ksz9477_tc_cbs_set_cinc,
 	.enable_stp_addr = ksz9477_enable_stp_addr,
@@ -3822,7 +3823,12 @@ EXPORT_SYMBOL(ksz_switch_alloc);
  */
 void ksz_switch_shutdown(struct ksz_device *dev)
 {
-	if (dev->dev_ops->reset)
+	bool wol_enabled = false;
+
+	if (dev->dev_ops->wol_pre_shutdown)
+		dev->dev_ops->wol_pre_shutdown(dev, &wol_enabled);
+
+	if (dev->dev_ops->reset && !wol_enabled)
 		dev->dev_ops->reset(dev);
 
 	dsa_switch_shutdown(dev->ds);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 34a8e9784cca..41917de15ba3 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -378,6 +378,7 @@ struct ksz_dev_ops {
 			struct ethtool_wolinfo *wol);
 	int (*set_wol)(struct ksz_device *dev, int port,
 		       struct ethtool_wolinfo *wol);
+	void (*wol_pre_shutdown)(struct ksz_device *dev, bool *wol_enabled);
 	void (*config_cpu_port)(struct dsa_switch *ds);
 	int (*enable_stp_addr)(struct ksz_device *dev);
 	int (*reset)(struct ksz_device *dev);
-- 
2.39.2

Re: [PATCH net-next v6 1/9] net: dsa: microchip: Add missing MAC address register offset for ksz8863

[Russell King (Oracle)]

On Thu, Oct 19, 2023 at 02:28:42PM +0200, Oleksij Rempel wrote:
> Add the missing offset for the global MAC address register
> (REG_SW_MAC_ADDR) for the ksz8863 family of switches.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Florian Fainelli]

On 10/19/23 05:28, Oleksij Rempel wrote:
> Introduce Wake on Magic Packet (WoL) functionality to the ksz9477
> driver.
> 
> Major changes include:
> 
> 1. Extending the `ksz9477_handle_wake_reason` function to identify Magic
>     Packet wake events alongside existing wake reasons.
> 
> 2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to
>     handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to
>     program the switch's MAC address register accordingly when Magic
>     Packet wake-up is enabled. This change will prevent WAKE_MAGIC
>     activation if the related port has a different MAC address compared
>     to a MAC address already used by HSR or an already active WAKE_MAGIC
>     on another port.
> 
> 3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC
>     address changes on ports with active Wake on Magic Packet, as the
>     switch's MAC address register is utilized for this feature.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net-next v6 1/9] net: dsa: microchip: Add missing MAC address register offset for ksz8863

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 02:28:42PM +0200, Oleksij Rempel wrote:
> Add the missing offset for the global MAC address register
> (REG_SW_MAC_ADDR) for the ksz8863 family of switches.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next v6 6/9] net: dsa: microchip: Refactor comment for ksz_switch_macaddr_get() function

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 02:28:47PM +0200, Oleksij Rempel wrote:
> Update the comment to follow kernel-doc format.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next v6 7/9] net: dsa: microchip: Add error handling for ksz_switch_macaddr_get()

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 02:28:48PM +0200, Oleksij Rempel wrote:
> Enhance the ksz_switch_macaddr_get() function to handle errors that may
> occur during the call to ksz_write8(). Specifically, this update checks
> the return value of ksz_write8(), which may fail if regmap ranges
> validation is not passed and returns the error code.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 7b05de6fe987..79052a54880c 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -3612,7 +3612,7 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
>  	struct ksz_switch_macaddr *switch_macaddr;
>  	struct ksz_device *dev = ds->priv;
>  	const u16 *regs = dev->info->regs;
> -	int i;
> +	int i, ret;
>  
>  	/* Make sure concurrent MAC address changes are blocked */
>  	ASSERT_RTNL();
> @@ -3639,8 +3639,11 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
>  	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]);
> +	for (i = 0; i < ETH_ALEN; i++) {
> +		ret = ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]);
> +		if (ret)
> +			return ret;
> +	}

I understand that you intend the error to be fatal, but this leaks memory and a refcount.

Re: [PATCH net-next v6 7/9] net: dsa: microchip: Add error handling for ksz_switch_macaddr_get()

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 08:13:08PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 19, 2023 at 02:28:48PM +0200, Oleksij Rempel wrote:
> > Enhance the ksz_switch_macaddr_get() function to handle errors that may
> > occur during the call to ksz_write8(). Specifically, this update checks
> > the return value of ksz_write8(), which may fail if regmap ranges
> > validation is not passed and returns the error code.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > ---
> >  drivers/net/dsa/microchip/ksz_common.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> > index 7b05de6fe987..79052a54880c 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -3612,7 +3612,7 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
> >  	struct ksz_switch_macaddr *switch_macaddr;
> >  	struct ksz_device *dev = ds->priv;
> >  	const u16 *regs = dev->info->regs;
> > -	int i;
> > +	int i, ret;
> >  
> >  	/* Make sure concurrent MAC address changes are blocked */
> >  	ASSERT_RTNL();
> > @@ -3639,8 +3639,11 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port,
> >  	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]);
> > +	for (i = 0; i < ETH_ALEN; i++) {
> > +		ret = ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> I understand that you intend the error to be fatal, but this leaks memory and a refcount.

I didn't estimate correctly what would happen.

If ksz_write8() fails as you say, we will leave behind the reference to
dev->switch_macaddr, which points to valid memory. Which means that the
second time around, the call to ksz_switch_macaddr_get() will succeed,
because the driver thinks that the address has been programmed to
hardware, and it is unaware of the previous failure. But it will not
work. Am I correct?

If so, it needs to tear down properly, because if ksz_switch_macaddr_get()
fails once to write to hardware, it should fail always.

Re: [PATCH net-next v6 9/9] net: dsa: microchip: Ensure Stable PME Pin State for Wake-on-LAN

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 02:28:50PM +0200, Oleksij Rempel wrote:
> Ensures a stable PME (Power Management Event) pin state by disabling PME
> on system start and enabling it on shutdown only if WoL (Wake-on-LAN) is
> configured. This is needed to avoid issues with some PMICs (Power
> Management ICs).
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> index fa8d0318b437..9e6f1e4b57b7 100644
> --- a/drivers/net/dsa/microchip/ksz9477.h
> +++ b/drivers/net/dsa/microchip/ksz9477.h
> @@ -62,6 +62,7 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
>  		     struct ethtool_wolinfo *wol);
>  int ksz9477_set_wol(struct ksz_device *dev, int port,
>  		    struct ethtool_wolinfo *wol);
> +void ksz9477_wol_pre_shutdown(struct ksz_device *dev, bool *wol_is_on);

nitpick: Please synchronize the prototype definition with the
declaration (the name of the "wol_is_on" argument differs).

>  
>  int ksz9477_port_acl_init(struct ksz_device *dev, int port);
>  void ksz9477_port_acl_free(struct ksz_device *dev, int port);

Otherwise:

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next v6 8/9] net: dsa: microchip: Refactor switch shutdown routine for WoL preparation

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 02:28:49PM +0200, Oleksij Rempel wrote:
> Centralize the switch shutdown routine in a dedicated function,
> ksz_switch_shutdown(), to enhance code maintainability and reduce
> redundancy. This change abstracts the common shutdown operations
> previously duplicated in ksz9477_i2c_shutdown() and ksz_spi_shutdown().
> 
> This refactoring is a preparatory step for an upcoming patch to avoid
> reset on shutdown if Wake-on-LAN (WoL) is enabled.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 02:28:46PM +0200, Oleksij Rempel wrote:
> Introduce Wake on Magic Packet (WoL) functionality to the ksz9477
> driver.
> 
> Major changes include:
> 
> 1. Extending the `ksz9477_handle_wake_reason` function to identify Magic
>    Packet wake events alongside existing wake reasons.
> 
> 2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to
>    handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to
>    program the switch's MAC address register accordingly when Magic
>    Packet wake-up is enabled. This change will prevent WAKE_MAGIC
>    activation if the related port has a different MAC address compared
>    to a MAC address already used by HSR or an already active WAKE_MAGIC
>    on another port.
> 
> 3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC
>    address changes on ports with active Wake on Magic Packet, as the
>    switch's MAC address register is utilized for this feature.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz9477.c    | 60 ++++++++++++++++++++++++--
>  drivers/net/dsa/microchip/ksz_common.c | 15 +++++--
>  drivers/net/dsa/microchip/ksz_common.h |  3 ++
>  3 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index b9419d4b5e7b..bcc8863951ca 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -81,7 +81,8 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port)
>  	if (!pme_status)
>  		return 0;
>  
> -	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port,
> +	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port,
> +		pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "",
>  		pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "",
>  		pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : "");

Trivial: if you format the printf string as %s%s%s and the arguments as
"\"Magic Packet\" " : "", then the printed line won't have a trailing
space at the end.

>  
> @@ -109,10 +110,22 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
>  
>  	wol->supported = WAKE_PHY;
>  
> +	/* Check if at this moment we would be able to get the MAC address
> +	 * and use it for WAKE_MAGIC support. This result may change dynamically
> +	 * depending on configuration of other ports.
> +	 */
> +	ret = ksz_switch_macaddr_get(dev->ds, port, NULL);
> +	if (!ret) {
> +		wol->supported |= WAKE_MAGIC;
> +		ksz_switch_macaddr_put(dev->ds);

I don't get it, why do you release the reference on the MAC address as
soon as you successfully get it? Without a reference held, the
programmed address still lingers on, but the HSR offload code, on a
different port with a different MAC address, can change it and break WoL.

> +	}
> +
>  	ret = ksz_pread8(dev, port, REG_PORT_PME_CTRL, &pme_ctrl);
>  	if (ret)
>  		return;
>  
> +	if (pme_ctrl & PME_WOL_MAGICPKT)
> +		wol->wolopts |= WAKE_MAGIC;
>  	if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY))
>  		wol->wolopts |= WAKE_PHY;
>  }

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Russell King (Oracle)]

On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 19, 2023 at 02:28:46PM +0200, Oleksij Rempel wrote:
> > Introduce Wake on Magic Packet (WoL) functionality to the ksz9477
> > driver.
> > 
> > Major changes include:
> > 
> > 1. Extending the `ksz9477_handle_wake_reason` function to identify Magic
> >    Packet wake events alongside existing wake reasons.
> > 
> > 2. Updating the `ksz9477_get_wol` and `ksz9477_set_wol` functions to
> >    handle WAKE_MAGIC alongside the existing WAKE_PHY option, and to
> >    program the switch's MAC address register accordingly when Magic
> >    Packet wake-up is enabled. This change will prevent WAKE_MAGIC
> >    activation if the related port has a different MAC address compared
> >    to a MAC address already used by HSR or an already active WAKE_MAGIC
> >    on another port.
> > 
> > 3. Adding a restriction in `ksz_port_set_mac_address` to prevent MAC
> >    address changes on ports with active Wake on Magic Packet, as the
> >    switch's MAC address register is utilized for this feature.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz9477.c    | 60 ++++++++++++++++++++++++--
> >  drivers/net/dsa/microchip/ksz_common.c | 15 +++++--
> >  drivers/net/dsa/microchip/ksz_common.h |  3 ++
> >  3 files changed, 71 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> > index b9419d4b5e7b..bcc8863951ca 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -81,7 +81,8 @@ static int ksz9477_handle_wake_reason(struct ksz_device *dev, int port)
> >  	if (!pme_status)
> >  		return 0;
> >  
> > -	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port,
> > +	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port,
> > +		pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "",
> >  		pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "",
> >  		pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : "");
> 
> Trivial: if you format the printf string as %s%s%s and the arguments as
> "\"Magic Packet\" " : "", then the printed line won't have a trailing
> space at the end.

Sadly, it still will. The best solution is to prepend the space
character to each entry in the "list" and remove the space characters
after the : in the format string thusly:

	dev_dbg(dev->dev, "Wake event on port %d due to:%s%s%s\n", port,
		pme_status & PME_WOL_MAGICPKT ? " \"Magic Packet\"" : "",
		pme_status & PME_WOL_LINKUP ? " \"Link Up\"" : "",
		pme_status & PME_WOL_ENERGY ? " \"Enery detect\"" : "");

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Oleksij Rempel]

On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 19, 2023 at 02:28:46PM +0200, Oleksij Rempel wrote:
....
> > @@ -109,10 +110,22 @@ void ksz9477_get_wol(struct ksz_device *dev, int port,
> >  
> >  	wol->supported = WAKE_PHY;
> >  
> > +	/* Check if at this moment we would be able to get the MAC address
> > +	 * and use it for WAKE_MAGIC support. This result may change dynamically
> > +	 * depending on configuration of other ports.
> > +	 */
> > +	ret = ksz_switch_macaddr_get(dev->ds, port, NULL);
> > +	if (!ret) {
> > +		wol->supported |= WAKE_MAGIC;
> > +		ksz_switch_macaddr_put(dev->ds);
> 
> I don't get it, why do you release the reference on the MAC address as
> soon as you successfully get it? Without a reference held, the
> programmed address still lingers on, but the HSR offload code, on a
> different port with a different MAC address, can change it and break WoL.

It is ksz9477_get_wol() function. We do not actually need to program
here the MAC address, we only need to test if we would be able to get
it. To show the use more or less correct information on WoL
capabilities. For example, instead showing the user that Wake on Magic
is supported, where we already know that is not the case, we can already
show correct information. May be it will be better to have
extra option for ksz_switch_macaddr_get() to not allocate and do the
refcounting or have a separate function.

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

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 07:17:32PM +0100, Russell King (Oracle) wrote:
> On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote:
> > > -	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s\n", port,
> > > +	dev_dbg(dev->dev, "Wake event on port %d due to: %s %s %s\n", port,
> > > +		pme_status & PME_WOL_MAGICPKT ? "\"Magic Packet\"" : "",
> > >  		pme_status & PME_WOL_LINKUP ? "\"Link Up\"" : "",
> > >  		pme_status & PME_WOL_ENERGY ? "\"Enery detect\"" : "");
> > 
> > Trivial: if you format the printf string as %s%s%s and the arguments as
> > "\"Magic Packet\" " : "", then the printed line won't have a trailing
> > space at the end.
> 
> Sadly, it still will. The best solution is to prepend the space
> character to each entry in the "list" and remove the space characters
> after the : in the format string thusly:
> 
> 	dev_dbg(dev->dev, "Wake event on port %d due to:%s%s%s\n", port,
> 		pme_status & PME_WOL_MAGICPKT ? " \"Magic Packet\"" : "",
> 		pme_status & PME_WOL_LINKUP ? " \"Link Up\"" : "",
> 		pme_status & PME_WOL_ENERGY ? " \"Enery detect\"" : "");

Thanks for correcting me.

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Vladimir Oltean]

On Fri, Oct 20, 2023 at 07:08:56AM +0200, Oleksij Rempel wrote:
> On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote:
> > I don't get it, why do you release the reference on the MAC address as
> > soon as you successfully get it? Without a reference held, the
> > programmed address still lingers on, but the HSR offload code, on a
> > different port with a different MAC address, can change it and break WoL.
> 
> It is ksz9477_get_wol() function. We do not actually need to program
> here the MAC address, we only need to test if we would be able to get
> it. To show the use more or less correct information on WoL
> capabilities. For example, instead showing the user that Wake on Magic
> is supported, where we already know that is not the case, we can already
> show correct information. May be it will be better to have
> extra option for ksz_switch_macaddr_get() to not allocate and do the
> refcounting or have a separate function.

Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port)
which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL
and ether_addr_equal(dev->switch_macaddr->addr, port addr))?

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Vladimir Oltean]

On Fri, Oct 20, 2023 at 11:23:50AM +0300, Vladimir Oltean wrote:
> On Fri, Oct 20, 2023 at 07:08:56AM +0200, Oleksij Rempel wrote:
> > On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote:
> > > I don't get it, why do you release the reference on the MAC address as
> > > soon as you successfully get it? Without a reference held, the
> > > programmed address still lingers on, but the HSR offload code, on a
> > > different port with a different MAC address, can change it and break WoL.
> > 
> > It is ksz9477_get_wol() function. We do not actually need to program
> > here the MAC address, we only need to test if we would be able to get
> > it. To show the use more or less correct information on WoL
> > capabilities. For example, instead showing the user that Wake on Magic
> > is supported, where we already know that is not the case, we can already
> > show correct information. May be it will be better to have
> > extra option for ksz_switch_macaddr_get() to not allocate and do the
> > refcounting or have a separate function.
> 
> Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port)
> which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL
> and ether_addr_equal(dev->switch_macaddr->addr, port addr))?

Hmm, I've checked other uses of the "tryget" name in the kernel and
their semantics all seem to imply that upon success, a reference is
acquired. That would not be the case here, so the naming would be
confusing. How about a bool ksz_switch_macaddr_check(ds, port)?

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Oleksij Rempel]

On Fri, Oct 20, 2023 at 11:23:50AM +0300, Vladimir Oltean wrote:
> On Fri, Oct 20, 2023 at 07:08:56AM +0200, Oleksij Rempel wrote:
> > On Thu, Oct 19, 2023 at 08:29:53PM +0300, Vladimir Oltean wrote:
> > > I don't get it, why do you release the reference on the MAC address as
> > > soon as you successfully get it? Without a reference held, the
> > > programmed address still lingers on, but the HSR offload code, on a
> > > different port with a different MAC address, can change it and break WoL.
> > 
> > It is ksz9477_get_wol() function. We do not actually need to program
> > here the MAC address, we only need to test if we would be able to get
> > it. To show the use more or less correct information on WoL
> > capabilities. For example, instead showing the user that Wake on Magic
> > is supported, where we already know that is not the case, we can already
> > show correct information. May be it will be better to have
> > extra option for ksz_switch_macaddr_get() to not allocate and do the
> > refcounting or have a separate function.
> 
> Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port)
> which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL
> and ether_addr_equal(dev->switch_macaddr->addr, port addr))?

Ack, something like this.
I'll send new version later.

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

Re: [PATCH net-next v6 5/9] net: dsa: microchip: ksz9477: Add Wake on Magic Packet support

[Andrew Lunn]

> > Ah, yes, it is from get_wol(). Maybe a ksz_switch_macaddr_tryget(ds, port)
> > which returns bool (true if dev->switch_macaddr is NULL, or if non-NULL
> > and ether_addr_equal(dev->switch_macaddr->addr, port addr))?
> 
> Ack, something like this.
> I'll send new version later.

And maybe add a comment. Seems like everybody got it wrong what is
going on here. Maybe i should not of suggested it :-)

      Andrew