[PATCH net-next v3 0/7] net: Add generic support for netdev LEDs

[PATCH net-next v3 0/7] net: Add generic support for netdev LEDs

[Andrew Lunn]

For some devices, the MAC controls the LEDs in the RJ45 connector, not
the PHY. This patchset provides generic support for such LEDs, and
adds the first user, mv88e6xxx.

The common code netdev_leds_setup() is passed a DT node containing the
LEDs and a structure of operations to act on the LEDs. The core will
then create an cdev LED for each LED found in the device tree node.

The callbacks are passed the netdev, and the index of the LED. In
order to make use of this within DSA, helpers are added to convert a
netdev to a ds and port.

The mv88e6xxx has been extended to add basic support for the 6352
LEDs. Only software control is added, but the API supports hardware
offload which can be added to the mv88e6xxx driver later.

For testing and demonstration, the Linksys Mamba aka. wrt1900ac has
the needed DT nodes added to describe its LEDs.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
Changes in v3:
- Change Internet port LED from LED_FUNCTION_LAN to LED_FUNCTION_WAN
- Another attempt to get Kconfig correct
- Link to v2: https://lore.kernel.org/r/20240330-v6-8-0-net-next-mv88e6xxx-leds-v4-v2-0-fc5beb9febc5@lunn.ch

Changes in v2:
- Validate maximum number of LEDs in core code
- Change Kconfig due to 0-day reports
- Link to v1: https://lore.kernel.org/r/20240317-v6-8-0-net-next-mv88e6xxx-leds-v4-v1-0-80a4e6c6293e@lunn.ch

---
Andrew Lunn (7):
      dsa: move call to driver port_setup after creation of netdev.
      net: Add helpers for netdev LEDs
      net: dsa: mv88e6xxx: Add helpers for 6352 LED blink and brightness
      net: dsa: mv88e6xxx: Tie the low level LED functions to device ops
      net: dsa: Add helpers to convert netdev to ds or port index
      dsa: mv88e6xxx: Create port/netdev LEDs
      arm: boot: dts: mvebu: linksys-mamba: Add Ethernet LEDs

 .../boot/dts/marvell/armada-xp-linksys-mamba.dts   |  66 +++++++
 drivers/net/dsa/mv88e6xxx/Kconfig                  |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c                   | 125 ++++++++++++-
 drivers/net/dsa/mv88e6xxx/chip.h                   |  19 ++
 drivers/net/dsa/mv88e6xxx/port.c                   |  93 ++++++++++
 drivers/net/dsa/mv88e6xxx/port.h                   |  76 +++++++-
 include/net/dsa.h                                  |  17 ++
 include/net/netdev_leds.h                          |  45 +++++
 net/Kconfig                                        |  11 ++
 net/core/Makefile                                  |   1 +
 net/core/netdev-leds.c                             | 201 +++++++++++++++++++++
 net/dsa/devlink.c                                  |  17 +-
 net/dsa/dsa.c                                      |   3 +
 net/dsa/user.c                                     |   8 +
 net/dsa/user.h                                     |   7 -
 15 files changed, 665 insertions(+), 25 deletions(-)
---
base-commit: 3b4cf29bdab08328dfab5bb7b41a62937ea5b379
change-id: 20240316-v6-8-0-net-next-mv88e6xxx-leds-v4-ab77d73d52a4

Best regards,
-- 
Andrew Lunn <andrew@lunn.ch>

[PATCH net-next v3 1/7] dsa: move call to driver port_setup after creation of netdev.

[Andrew Lunn]

The drivers call port_setup() is a good place to add the LEDs of a
port to the netdev representing the port. However, when port_setup()
is called in dsa_port_devlink_setup() the netdev does not exist
yet. That only happens in dsa_user_create() which is latter in
dsa_port_setup().

Move the call to port_setup() out of dsa_port_devlink_setup() and to
the end of dsa_port_setup() where the netdev will exist.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/devlink.c | 17 +----------------
 net/dsa/dsa.c     |  3 +++
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
index 431bf52290a1..9c3dc6319269 100644
--- a/net/dsa/devlink.c
+++ b/net/dsa/devlink.c
@@ -294,20 +294,12 @@ int dsa_port_devlink_setup(struct dsa_port *dp)
 	struct dsa_switch_tree *dst = dp->ds->dst;
 	struct devlink_port_attrs attrs = {};
 	struct devlink *dl = dp->ds->devlink;
-	struct dsa_switch *ds = dp->ds;
 	const unsigned char *id;
 	unsigned char len;
-	int err;
 
 	memset(dlp, 0, sizeof(*dlp));
 	devlink_port_init(dl, dlp);
 
-	if (ds->ops->port_setup) {
-		err = ds->ops->port_setup(ds, dp->index);
-		if (err)
-			return err;
-	}
-
 	id = (const unsigned char *)&dst->index;
 	len = sizeof(dst->index);
 
@@ -331,14 +323,7 @@ int dsa_port_devlink_setup(struct dsa_port *dp)
 	}
 
 	devlink_port_attrs_set(dlp, &attrs);
-	err = devlink_port_register(dl, dlp, dp->index);
-	if (err) {
-		if (ds->ops->port_teardown)
-			ds->ops->port_teardown(ds, dp->index);
-		return err;
-	}
-
-	return 0;
+	return devlink_port_register(dl, dlp, dp->index);
 }
 
 void dsa_port_devlink_teardown(struct dsa_port *dp)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 09d2f5d4b3dd..6ffee2a7de94 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -520,6 +520,9 @@ static int dsa_port_setup(struct dsa_port *dp)
 		break;
 	}
 
+	if (ds->ops->port_setup)
+		err = ds->ops->port_setup(ds, dp->index);
+
 	if (err && dsa_port_enabled)
 		dsa_port_disable(dp);
 	if (err && dsa_port_link_registered)

-- 
2.43.0

[PATCH net-next v3 2/7] net: Add helpers for netdev LEDs

[Andrew Lunn]

Add a set of helpers for parsing the standard device tree properties
for LEDs as part of an ethernet device, and registering them with the
LED subsystem. This code can be used by any sort of netdev driver,
including plain MAC, DSA switches or pure switchdev switch driver.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/netdev_leds.h |  45 +++++++++++
 net/Kconfig               |  11 +++
 net/core/Makefile         |   1 +
 net/core/netdev-leds.c    | 201 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 258 insertions(+)

diff --git a/include/net/netdev_leds.h b/include/net/netdev_leds.h
new file mode 100644
index 000000000000..239f492f29f5
--- /dev/null
+++ b/include/net/netdev_leds.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Helpers used for creating and managing LEDs on a netdev MAC
+ * driver.
+ */
+
+#ifndef _NET_NETDEV_LEDS_H
+#define _NET_NETDEV_LEDS_H
+
+struct netdev_leds_ops {
+	int (*brightness_set)(struct net_device *ndev, u8 led,
+			      enum led_brightness brightness);
+	int (*blink_set)(struct net_device *ndev, u8 led,
+			 unsigned long *delay_on,  unsigned long *delay_off);
+	int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
+				       unsigned long flags);
+	int (*hw_control_set)(struct net_device *ndev, u8 led,
+			      unsigned long flags);
+	int (*hw_control_get)(struct net_device *ndev, u8 led,
+			      unsigned long *flags);
+};
+
+#ifdef CONFIG_NETDEV_LEDS
+int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
+		      struct list_head *list, struct netdev_leds_ops *ops,
+		      int max_leds);
+
+void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);
+
+#else
+static inline int netdev_leds_setup(struct net_device *ndev,
+				    struct device_node *np,
+				    struct list_head *list,
+				    struct netdev_leds_ops *ops)
+{
+	return 0;
+}
+
+static inline void netdev_leds_teardown(struct list_head *list,
+					struct net_device *ndev)
+{
+}
+#endif /* CONFIG_NETDEV_LEDS */
+
+#endif /* _NET_PORT_LEDS_H */
diff --git a/net/Kconfig b/net/Kconfig
index 3e57ccf0da27..753dfd11f014 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -516,4 +516,15 @@ config NET_TEST
 
 	  If unsure, say N.
 
+config NETDEV_LEDS
+	bool "NETDEV helper code for MAC LEDs"
+	select LEDS_CLASS
+	select LEDS_TRIGGERS
+	select LEDS_TRIGGER_NETDEV
+	help
+	  NICs and Switches often contain LED controllers. When the LEDs
+	  are part of the MAC, the MAC driver, aka netdev driver, should
+	  make the LEDs available. NETDEV_LEDS offers a small library
+	  of code to help MAC drivers do this.
+
 endif   # if NET
diff --git a/net/core/Makefile b/net/core/Makefile
index 21d6fbc7e884..d04ce07541b5 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -42,3 +42,4 @@ obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
 obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
 obj-$(CONFIG_OF)	+= of_net.o
 obj-$(CONFIG_NET_TEST) += net_test.o
+obj-$(CONFIG_NETDEV_LEDS) += netdev-leds.o
diff --git a/net/core/netdev-leds.c b/net/core/netdev-leds.c
new file mode 100644
index 000000000000..802dd819a991
--- /dev/null
+++ b/net/core/netdev-leds.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/leds.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <net/netdev_leds.h>
+
+struct netdev_led {
+	struct list_head led_list;
+	struct led_classdev led_cdev;
+	struct netdev_leds_ops *ops;
+	struct net_device *ndev;
+	u8 index;
+};
+
+#define to_netdev_led(d) container_of(d, struct netdev_led, led_cdev)
+
+static int netdev_brightness_set(struct led_classdev *led_cdev,
+				 enum led_brightness value)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->brightness_set(netdev_led->ndev,
+					       netdev_led->index,
+					       value);
+}
+
+static int netdev_blink_set(struct led_classdev *led_cdev,
+			    unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->blink_set(netdev_led->ndev,
+					  netdev_led->index,
+					  delay_on, delay_off);
+}
+
+static __maybe_unused int
+netdev_hw_control_is_supported(struct led_classdev *led_cdev,
+			       unsigned long flags)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->hw_control_is_supported(netdev_led->ndev,
+							netdev_led->index,
+							flags);
+}
+
+static __maybe_unused int netdev_hw_control_set(struct led_classdev *led_cdev,
+						unsigned long flags)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->hw_control_set(netdev_led->ndev,
+					       netdev_led->index,
+					       flags);
+}
+
+static __maybe_unused int netdev_hw_control_get(struct led_classdev *led_cdev,
+						unsigned long *flags)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return netdev_led->ops->hw_control_get(netdev_led->ndev,
+					       netdev_led->index,
+					       flags);
+}
+
+static struct device *
+netdev_hw_control_get_device(struct led_classdev *led_cdev)
+{
+	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
+
+	return &netdev_led->ndev->dev;
+}
+
+static int netdev_led_setup(struct net_device *ndev, struct device_node *led,
+			    struct list_head *list, struct netdev_leds_ops *ops,
+			    int max_leds)
+{
+	struct led_init_data init_data = {};
+	struct device *dev = &ndev->dev;
+	struct netdev_led *netdev_led;
+	struct led_classdev *cdev;
+	u32 index;
+	int err;
+
+	netdev_led = devm_kzalloc(dev, sizeof(*netdev_led), GFP_KERNEL);
+	if (!netdev_led)
+		return -ENOMEM;
+
+	netdev_led->ndev = ndev;
+	netdev_led->ops = ops;
+	cdev = &netdev_led->led_cdev;
+
+	err = of_property_read_u32(led, "reg", &index);
+	if (err)
+		return err;
+
+	if (index >= max_leds)
+		return -EINVAL;
+
+	netdev_led->index = index;
+
+	if (ops->brightness_set)
+		cdev->brightness_set_blocking = netdev_brightness_set;
+	if (ops->blink_set)
+		cdev->blink_set = netdev_blink_set;
+#ifdef CONFIG_LEDS_TRIGGERS
+	if (ops->hw_control_is_supported)
+		cdev->hw_control_is_supported = netdev_hw_control_is_supported;
+	if (ops->hw_control_set)
+		cdev->hw_control_set = netdev_hw_control_set;
+	if (ops->hw_control_get)
+		cdev->hw_control_get = netdev_hw_control_get;
+	cdev->hw_control_trigger = "netdev";
+#endif
+	cdev->hw_control_get_device = netdev_hw_control_get_device;
+	cdev->max_brightness = 1;
+	init_data.fwnode = of_fwnode_handle(led);
+	init_data.devname_mandatory = true;
+
+	init_data.devicename = dev_name(dev);
+	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
+	if (err)
+		return err;
+
+	INIT_LIST_HEAD(&netdev_led->led_list);
+	list_add(&netdev_led->led_list, list);
+
+	return 0;
+}
+
+/**
+ * netdev_leds_setup - Parse DT node and create LEDs for netdev
+ *
+ * @ndev: struct netdev for the MAC
+ * @np: ethernet-node in device tree
+ * @list: list to add LEDs to
+ * @ops: structure of ops to manipulate the LED.
+ * @max_leds: maximum number of LEDs support by netdev.
+ *
+ * Parse the device tree node, as described in
+ * ethernet-controller.yaml, and find any LEDs. For each LED found,
+ * ensure the reg value is less than max_leds, create an LED and
+ * register it with the LED subsystem. The LED will be added to the
+ * list, which can be shared by all netdevs of the device. The ops
+ * structure contains the callbacks needed to control the LEDs.
+ *
+ * Return 0 in success, otherwise an negative error code.
+ */
+int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
+		      struct list_head *list, struct netdev_leds_ops *ops,
+		      int max_leds)
+{
+	struct device_node *leds, *led;
+	int err;
+
+	leds = of_get_child_by_name(np, "leds");
+	if (!leds)
+		return 0;
+
+	for_each_available_child_of_node(leds, led) {
+		err = netdev_led_setup(ndev, led, list, ops, max_leds);
+		if (err) {
+			of_node_put(led);
+			return err;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(netdev_leds_setup);
+
+/**
+ * netdev_leds_teardown - Remove LEDs for a netdev
+ *
+ * @list: list to add LEDs to teardown
+ * @ndev: The netdev for which LEDs should be removed
+ *
+ * Unregister all LEDs for a given netdev, freeing up any allocated
+ * memory.
+ */
+void netdev_leds_teardown(struct list_head *list, struct net_device *ndev)
+{
+	struct netdev_led *netdev_led;
+	struct led_classdev *cdev;
+	struct device *dev;
+
+	list_for_each_entry(netdev_led, list, led_list) {
+		if (netdev_led->ndev != ndev)
+			continue;
+		dev = &netdev_led->ndev->dev;
+		cdev = &netdev_led->led_cdev;
+		devm_led_classdev_unregister(dev, cdev);
+	}
+}
+EXPORT_SYMBOL_GPL(netdev_leds_teardown);

-- 
2.43.0

[PATCH net-next v3 3/7] net: dsa: mv88e6xxx: Add helpers for 6352 LED blink and brightness

[Andrew Lunn]

The 6352 family has two LEDs per port for ports 0-4. Ports 5 and 6
share a couple of LEDs. Add support functions to set the brightness,
i.e. on or off, and to make the LEDs blink at a fixed rate.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/port.c | 93 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/port.h | 76 +++++++++++++++++++++++++++++++-
 2 files changed, 168 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 5394a8cf7bf1..37315a4aa9cf 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1723,3 +1723,96 @@ int mv88e6393x_port_set_policy(struct mv88e6xxx_chip *chip, int port,
 
 	return mv88e6393x_port_policy_write(chip, port, ptr, reg);
 }
+
+/* Offset 0x16: LED Control Register */
+
+static int mv88e6352_port_led_write(struct mv88e6xxx_chip *chip, int port,
+				    u16 pointer, u16 data)
+{
+	u16 reg = MV88E6352_PORT_LED_CTL_UPDATE | pointer | data;
+
+	return mv88e6xxx_port_write(chip, port, MV88E6352_PORT_LED_CTL, reg);
+}
+
+static int mv88e6352_port_led_read(struct mv88e6xxx_chip *chip, int port,
+				   u16 pointer, u16 *data)
+{
+	int err;
+	u16 val;
+
+	err = mv88e6xxx_port_write(chip, port, MV88E6352_PORT_LED_CTL, pointer);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6352_PORT_LED_CTL, &val);
+	if (err)
+		return err;
+
+	*data = val & MV88E6352_PORT_LED_CTL_DATA_MASK;
+
+	return 0;
+}
+
+int mv88e6352_port_led_brightness_set(struct mv88e6xxx_chip *chip, int port,
+				      u8 led, enum led_brightness value)
+{
+	int err;
+	u16 val;
+
+	if (port > 5)
+		return -EOPNOTSUPP;
+
+	err = mv88e6352_port_led_read(chip, port,
+				      MV88E6352_PORT_LED_CTL_PTR_LED01,
+				      &val);
+	if (err)
+		return err;
+
+	if (led == 0) {
+		val &= ~MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_MASK;
+		if (value)
+			val |= MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_ON;
+		else
+			val |= MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_OFF;
+	} else {
+		val &= ~MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_MASK;
+		if (value)
+			val |= MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_ON;
+		else
+			val |= MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_OFF;
+	}
+	return mv88e6352_port_led_write(chip, port,
+					MV88E6352_PORT_LED_CTL_PTR_LED01,
+					val);
+}
+
+int mv88e6352_port_led_blink_set(struct mv88e6xxx_chip *chip, int port, u8 led,
+				 unsigned long *delay_on,
+				 unsigned long *delay_off)
+{
+	int err;
+	u16 val;
+
+	if (port > 5)
+		return -EOPNOTSUPP;
+
+	/* Reset default is 84ms */
+	*delay_on = 84 / 2;
+	*delay_off = 84 / 2;
+	err = mv88e6352_port_led_read(chip, port,
+				      MV88E6352_PORT_LED_CTL_PTR_LED01,
+				      &val);
+	if (err)
+		return err;
+
+	if (led == 0) {
+		val &= ~MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_MASK;
+		val |= MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_BLINK;
+	} else {
+		val &= ~MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_MASK;
+		val |= MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_BLINK;
+	}
+	return mv88e6352_port_led_write(chip, port,
+					MV88E6352_PORT_LED_CTL_PTR_LED01,
+					val);
+}
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 86deeb347cbc..72556e4d154c 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -294,6 +294,76 @@
 /* Offset 0x13: OutFiltered Counter */
 #define MV88E6XXX_PORT_OUT_FILTERED	0x13
 
+/* Offset 0x16: LED Control */
+#define MV88E6352_PORT_LED_CTL					0x16
+#define MV88E6352_PORT_LED_CTL_UPDATE				0x8000
+#define MV88E6352_PORT_LED_CTL_PTR_LED01			0x0000
+#define MV88E6352_PORT_LED_CTL_PTR_STRETCH_BLINK		0x6000
+#define MV88E6352_PORT_LED_CTL_PTR_SPECIAL			0x7000
+#define MV88E6352_PORT_LED_CTL_DATA_MASK			0x03ff
+/* Ports 0-4 */
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_P2_SPECIAL	0x0000
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_10_100_ACT	0x0010
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_1000		0x0030
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_P1_SPECIAL	0x0040
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_10_1000_ACT	0x0060
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_10_1000		0x0070
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_ACT		0x0080
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_100		0x0090
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_100_ACT		0x00A0
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_10_100		0x00B0
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_PTP_ACT		0x00C0
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_BLINK		0x00D0
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_OFF		0x00E0
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_ON		0x00F0
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_MASK		0x00F0
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED1_LINK_ACT		0x0000
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_100_1000_ACT	0x0001
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_1000_ACT		0x0002
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_LINK_ACT		0x0003
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_P0_SPECIAL	0x0004
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_DUPLEX_COL	0x0006
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_10_1000_ACT	0x0007
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_LINK		0x0008
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_10		0x0009
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_10_ACT		0x000A
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_100_1000		0x000B
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_PTP_ACT		0x000C
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_BLINK		0x000D
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_OFF		0x000E
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_ON		0x000F
+#define MV88E6352_PORT_LED_CTL_DATA_LED01_LED0_MASK		0x000F
+
+/* Port 5 */
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P6_ACT		0x0000
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_FIBER_1000_ACT	0x0010
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_FIBER_100_ACT   0x0020
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_FIBER		0x0030
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P5_ACT		0x0040
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P6_LINK		0x0050
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P6_DUPLEX_COL	0x0060
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P6_LINK_ACT	0x0070
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P0_SPECIAL	0x0080
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P1_SPECIAL	0x0090
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P2_SPECIAL	0x00A0
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P6_PTP_ACT	0x00C0
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_BLINK		0x00D0
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_OFF		0x00E0
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_ON		0x00F0
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED1_P5_LINK_ACT	0x0000
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_FIBER_100_ACT	0x0001
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_FIBER_1000_ACT	0x0002
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_P0_SPECIAL	0x0003
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_P1_SPECIAL	0x0004
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_P2_SPECIAL	0x0005
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_P5_DUPLEX_COL	0x0006
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_P5_LINK_ACT	0x0007
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_P6_LINK_ACT	0x0008
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_BLINK		0x000D
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_OFF		0x000E
+#define MV88E6352_PORT_LED_CTL_DATA5_LED01_LED0_ON		0x000F
+/* Port 6 does not have any LEDs */
+
 /* Offset 0x18: IEEE Priority Mapping Table */
 #define MV88E6390_PORT_IEEE_PRIO_MAP_TABLE			0x18
 #define MV88E6390_PORT_IEEE_PRIO_MAP_TABLE_UPDATE		0x8000
@@ -459,5 +529,9 @@ int mv88e6xxx_port_hidden_write(struct mv88e6xxx_chip *chip, int block,
 int mv88e6xxx_port_hidden_wait(struct mv88e6xxx_chip *chip);
 int mv88e6xxx_port_hidden_read(struct mv88e6xxx_chip *chip, int block, int port,
 			       int reg, u16 *val);
-
+int mv88e6352_port_led_brightness_set(struct mv88e6xxx_chip *chip, int port,
+				      u8 led, enum led_brightness value);
+int mv88e6352_port_led_blink_set(struct mv88e6xxx_chip *chip, int port, u8 led,
+				 unsigned long *delay_on,
+				 unsigned long *delay_off);
 #endif /* _MV88E6XXX_PORT_H */

-- 
2.43.0

[PATCH net-next v3 4/7] net: dsa: mv88e6xxx: Tie the low level LED functions to device ops

[Andrew Lunn]

Make the LED brightness and blink helpers available for the 6352
family via their ops structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
 drivers/net/dsa/mv88e6xxx/chip.h | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9ed1821184ec..3d7e4aa9293a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4526,6 +4526,8 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.phylink_get_caps = mv88e6352_phylink_get_caps,
 	.pcs_ops = &mv88e6352_pcs_ops,
+	.led_brightness_set = mv88e6352_port_led_brightness_set,
+	.led_blink_set = mv88e6352_port_led_blink_set,
 };
 
 static const struct mv88e6xxx_ops mv88e6175_ops = {
@@ -4628,6 +4630,8 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.gpio_ops = &mv88e6352_gpio_ops,
 	.phylink_get_caps = mv88e6352_phylink_get_caps,
 	.pcs_ops = &mv88e6352_pcs_ops,
+	.led_brightness_set = mv88e6352_port_led_brightness_set,
+	.led_blink_set = mv88e6352_port_led_blink_set,
 };
 
 static const struct mv88e6xxx_ops mv88e6185_ops = {
@@ -4897,6 +4901,8 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.ptp_ops = &mv88e6352_ptp_ops,
 	.phylink_get_caps = mv88e6352_phylink_get_caps,
 	.pcs_ops = &mv88e6352_pcs_ops,
+	.led_brightness_set = mv88e6352_port_led_brightness_set,
+	.led_blink_set = mv88e6352_port_led_blink_set,
 };
 
 static const struct mv88e6xxx_ops mv88e6250_ops = {
@@ -5310,6 +5316,8 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.serdes_set_tx_amplitude = mv88e6352_serdes_set_tx_amplitude,
 	.phylink_get_caps = mv88e6352_phylink_get_caps,
 	.pcs_ops = &mv88e6352_pcs_ops,
+	.led_brightness_set = mv88e6352_port_led_brightness_set,
+	.led_blink_set = mv88e6352_port_led_blink_set,
 };
 
 static const struct mv88e6xxx_ops mv88e6390_ops = {
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 85eb293381a7..64f8bde68ccf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -661,6 +661,13 @@ struct mv88e6xxx_ops {
 
 	/* Max Frame Size */
 	int (*set_max_frame_size)(struct mv88e6xxx_chip *chip, int mtu);
+
+	/* LEDs */
+	int (*led_brightness_set)(struct mv88e6xxx_chip *chip, int port,
+				  u8 led, enum led_brightness value);
+	int (*led_blink_set)(struct mv88e6xxx_chip *chip, int port, u8 led,
+			     unsigned long *delay_on,
+			     unsigned long *delay_off);
 };
 
 struct mv88e6xxx_irq_ops {

-- 
2.43.0

[PATCH net-next v3 5/7] net: dsa: Add helpers to convert netdev to ds or port index

[Andrew Lunn]

The LED helpers make use of a struct netdev. Add helpers a DSA driver
can use to convert a netdev to a struct dsa_switch and the port index.

To do this, dsa_user_to_port() has to be made available out side of
net/dev, to convert the inline function in net/dsa/user.h into a
normal function, and export it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/dsa.h | 17 +++++++++++++++++
 net/dsa/user.c    |  8 ++++++++
 net/dsa/user.h    |  7 -------
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 7c0da9effe4e..1fbfada6678d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1359,6 +1359,23 @@ int dsa_register_switch(struct dsa_switch *ds);
 void dsa_switch_shutdown(struct dsa_switch *ds);
 struct dsa_switch *dsa_switch_find(int tree_index, int sw_index);
 void dsa_flush_workqueue(void);
+
+struct dsa_port *dsa_user_to_port(const struct net_device *dev);
+
+static inline struct dsa_switch *dsa_user_to_ds(const struct net_device *ndev)
+{
+	struct dsa_port *dp = dsa_user_to_port(ndev);
+
+	return dp->ds;
+}
+
+static inline unsigned int dsa_user_to_index(const struct net_device *ndev)
+{
+	struct dsa_port *dp = dsa_user_to_port(ndev);
+
+	return dp->index;
+}
+
 #ifdef CONFIG_PM_SLEEP
 int dsa_switch_suspend(struct dsa_switch *ds);
 int dsa_switch_resume(struct dsa_switch *ds);
diff --git a/net/dsa/user.c b/net/dsa/user.c
index 16d395bb1a1f..bbee3f63e2c7 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -3699,3 +3699,11 @@ void dsa_user_unregister_notifier(void)
 	if (err)
 		pr_err("DSA: failed to unregister user notifier (%d)\n", err);
 }
+
+struct dsa_port *dsa_user_to_port(const struct net_device *dev)
+{
+	struct dsa_user_priv *p = netdev_priv(dev);
+
+	return p->dp;
+}
+EXPORT_SYMBOL_GPL(dsa_user_to_port);
diff --git a/net/dsa/user.h b/net/dsa/user.h
index 996069130bea..b6bcf027643e 100644
--- a/net/dsa/user.h
+++ b/net/dsa/user.h
@@ -51,13 +51,6 @@ int dsa_user_change_conduit(struct net_device *dev, struct net_device *conduit,
 int dsa_user_manage_vlan_filtering(struct net_device *dev,
 				   bool vlan_filtering);
 
-static inline struct dsa_port *dsa_user_to_port(const struct net_device *dev)
-{
-	struct dsa_user_priv *p = netdev_priv(dev);
-
-	return p->dp;
-}
-
 static inline struct net_device *
 dsa_user_to_conduit(const struct net_device *dev)
 {

-- 
2.43.0

[PATCH net-next v3 6/7] dsa: mv88e6xxx: Create port/netdev LEDs

[Andrew Lunn]

Make use of the helpers to add LEDs to the user ports when the port is
setup.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/Kconfig |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c  | 117 +++++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/mv88e6xxx/chip.h  |  12 ++++
 3 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig b/drivers/net/dsa/mv88e6xxx/Kconfig
index e3181d5471df..ded5c6b9132b 100644
--- a/drivers/net/dsa/mv88e6xxx/Kconfig
+++ b/drivers/net/dsa/mv88e6xxx/Kconfig
@@ -5,6 +5,7 @@ config NET_DSA_MV88E6XXX
 	select IRQ_DOMAIN
 	select NET_DSA_TAG_EDSA
 	select NET_DSA_TAG_DSA
+	select NETDEV_LEDS
 	help
 	  This driver adds support for most of the Marvell 88E6xxx models of
 	  Ethernet switch chips, except 88E6060.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 3d7e4aa9293a..b8e39dcad2da 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -31,6 +31,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/phylink.h>
 #include <net/dsa.h>
+#include <net/netdev_leds.h>
 
 #include "chip.h"
 #include "devlink.h"
@@ -3129,6 +3130,105 @@ static int mv88e6xxx_switch_reset(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_software_reset(chip);
 }
 
+static int mv88e6xxx_led_brightness_set(struct net_device *ndev,
+					u8 led, enum led_brightness value)
+{
+	struct dsa_switch *ds = dsa_user_to_ds(ndev);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int port = dsa_user_to_index(ndev);
+	int err;
+
+	if (chip->info->ops->led_brightness_set) {
+		mv88e6xxx_reg_lock(chip);
+		err = chip->info->ops->led_brightness_set(chip, port, led,
+							  value);
+		mv88e6xxx_reg_unlock(chip);
+		return err;
+	}
+	return -EOPNOTSUPP;
+}
+
+static int mv88e6xxx_led_blink_set(struct net_device *ndev, u8 led,
+				   unsigned long *delay_on,
+				   unsigned long *delay_off)
+{
+	struct dsa_switch *ds = dsa_user_to_ds(ndev);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int port = dsa_user_to_index(ndev);
+	int err;
+
+	if (chip->info->ops->led_blink_set) {
+		mv88e6xxx_reg_lock(chip);
+		err = chip->info->ops->led_blink_set(chip, port, led,
+						     delay_on, delay_off);
+		mv88e6xxx_reg_unlock(chip);
+		return err;
+	}
+	return -EOPNOTSUPP;
+}
+
+static int mv88e6xxx_led_hw_control_is_supported(struct net_device *ndev,
+						 u8 led, unsigned long flags)
+{
+	struct dsa_switch *ds = dsa_user_to_ds(ndev);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int port = dsa_user_to_index(ndev);
+	int err;
+
+	if (chip->info->ops->led_hw_control_is_supported) {
+		mv88e6xxx_reg_lock(chip);
+		err = chip->info->ops->led_hw_control_is_supported(chip, port,
+								   led, flags);
+		mv88e6xxx_reg_unlock(chip);
+		return err;
+	}
+	return -EOPNOTSUPP;
+}
+
+static int mv88e6xxx_led_hw_control_set(struct net_device *ndev, u8 led,
+					unsigned long flags)
+{
+	struct dsa_switch *ds = dsa_user_to_ds(ndev);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int port = dsa_user_to_index(ndev);
+	int err;
+
+	if (chip->info->ops->led_hw_control_set) {
+		mv88e6xxx_reg_lock(chip);
+		err = chip->info->ops->led_hw_control_set(chip, port,
+							  led, flags);
+		mv88e6xxx_reg_unlock(chip);
+		return err;
+	}
+	return -EOPNOTSUPP;
+}
+
+static int mv88e6xxx_led_hw_control_get(struct net_device *ndev,
+					u8 led, unsigned long *flags)
+{
+	struct dsa_switch *ds = dsa_user_to_ds(ndev);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int port = dsa_user_to_index(ndev);
+	int err;
+
+	if (chip->info->ops->led_hw_control_get) {
+		mv88e6xxx_reg_lock(chip);
+		err = chip->info->ops->led_hw_control_get(chip, port,
+							  led, flags);
+		mv88e6xxx_reg_unlock(chip);
+		return err;
+	}
+	return -EOPNOTSUPP;
+}
+
+static struct netdev_leds_ops mv88e6xxx_netdev_leds_ops = {
+	.brightness_set = mv88e6xxx_led_brightness_set,
+	.blink_set = mv88e6xxx_led_blink_set,
+	.hw_control_is_supported = mv88e6xxx_led_hw_control_is_supported,
+	.hw_control_set = mv88e6xxx_led_hw_control_set,
+	.hw_control_get = mv88e6xxx_led_hw_control_get,
+};
+
 static int mv88e6xxx_set_port_mode(struct mv88e6xxx_chip *chip, int port,
 				   enum mv88e6xxx_frame_mode frame,
 				   enum mv88e6xxx_egress_mode egress, u16 etype)
@@ -4006,6 +4106,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 
 static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
 {
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
@@ -4016,13 +4117,26 @@ static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
 			return err;
 	}
 
-	return mv88e6xxx_setup_devlink_regions_port(ds, port);
+	err  = mv88e6xxx_setup_devlink_regions_port(ds, port);
+	if (err)
+		return err;
+
+	if (dp->dn) {
+		err = netdev_leds_setup(dp->user, dp->dn, &chip->leds,
+					&mv88e6xxx_netdev_leds_ops, 2);
+		if (err)
+			mv88e6xxx_teardown_devlink_regions_port(ds, port);
+	}
+	return err;
 }
 
 static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
 {
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct mv88e6xxx_chip *chip = ds->priv;
 
+	netdev_leds_teardown(&chip->leds, dp->user);
+
 	mv88e6xxx_teardown_devlink_regions_port(ds, port);
 
 	if (chip->info->ops->pcs_ops &&
@@ -6397,6 +6511,7 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
 	INIT_LIST_HEAD(&chip->mdios);
 	idr_init(&chip->policies);
 	INIT_LIST_HEAD(&chip->msts);
+	INIT_LIST_HEAD(&chip->leds);
 
 	return chip;
 }
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 64f8bde68ccf..b70e74203b31 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -432,6 +432,9 @@ struct mv88e6xxx_chip {
 
 	/* Bridge MST to SID mappings */
 	struct list_head msts;
+
+	/* LEDs associated to the ports */
+	struct list_head leds;
 };
 
 struct mv88e6xxx_bus_ops {
@@ -668,6 +671,15 @@ struct mv88e6xxx_ops {
 	int (*led_blink_set)(struct mv88e6xxx_chip *chip, int port, u8 led,
 			     unsigned long *delay_on,
 			     unsigned long *delay_off);
+	int (*led_hw_control_is_supported)(struct mv88e6xxx_chip *chip,
+					   int port, u8 led,
+					   unsigned long flags);
+	int (*led_hw_control_set)(struct mv88e6xxx_chip *chip,
+				  int port, u8 led,
+				  unsigned long flags);
+	int (*led_hw_control_get)(struct mv88e6xxx_chip *chip,
+				  int port, u8 led,
+				  unsigned long *flags);
 };
 
 struct mv88e6xxx_irq_ops {

-- 
2.43.0

[PATCH net-next v3 7/7] arm: boot: dts: mvebu: linksys-mamba: Add Ethernet LEDs

[Andrew Lunn]

List the front panel Ethernet LEDs in the switch section of the
device tree. They can then be controlled via /sys/class/led/

The node contains a label property to influence the name of the LED.
Without it, all the LEDs get the name lan:white, which classes, and so
some get a number appended. lan:white_1, lan:white_2, etc. Using the
label the LEDs are named lan1:front, lan2:front, lan3:front, where
lanX indicates the interface name, and front indicates they are on the
front of the box.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../boot/dts/marvell/armada-xp-linksys-mamba.dts   | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/arch/arm/boot/dts/marvell/armada-xp-linksys-mamba.dts b/arch/arm/boot/dts/marvell/armada-xp-linksys-mamba.dts
index ea859f7ea042..0784fd0355b2 100644
--- a/arch/arm/boot/dts/marvell/armada-xp-linksys-mamba.dts
+++ b/arch/arm/boot/dts/marvell/armada-xp-linksys-mamba.dts
@@ -19,6 +19,7 @@
 /dts-v1/;
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
 #include "armada-xp-mv78230.dtsi"
 
 / {
@@ -276,26 +277,91 @@ ethernet-ports {
 			ethernet-port@0 {
 				reg = <0>;
 				label = "lan4";
+
+				leds {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					led@0 {
+						reg = <0>;
+						color = <LED_COLOR_ID_WHITE>;
+						function = LED_FUNCTION_LAN;
+						label = "front";
+						default-state = "keep";
+					};
+				};
 			};
 
 			ethernet-port@1 {
 				reg = <1>;
 				label = "lan3";
+
+				leds {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					led@0 {
+						reg = <0>;
+						color = <LED_COLOR_ID_WHITE>;
+						function = LED_FUNCTION_LAN;
+						label = "front";
+						default-state = "keep";
+					};
+				};
 			};
 
 			ethernet-port@2 {
 				reg = <2>;
 				label = "lan2";
+
+				leds {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					led@0 {
+						reg = <0>;
+						color = <LED_COLOR_ID_WHITE>;
+						function = LED_FUNCTION_LAN;
+						label = "front";
+						default-state = "keep";
+					};
+				};
 			};
 
 			ethernet-port@3 {
 				reg = <3>;
 				label = "lan1";
+
+				leds {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					led@0 {
+						reg = <0>;
+						color = <LED_COLOR_ID_WHITE>;
+						function = LED_FUNCTION_LAN;
+						label = "front";
+						default-state = "keep";
+					};
+				};
 			};
 
 			ethernet-port@4 {
 				reg = <4>;
 				label = "internet";
+
+				leds {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					led@0 {
+						reg = <0>;
+						color = <LED_COLOR_ID_WHITE>;
+						function = LED_FUNCTION_WAN;
+						label = "front";
+						default-state = "keep";
+					};
+				};
 			};
 
 			ethernet-port@5 {

-- 
2.43.0

Re: [PATCH net-next v3 1/7] dsa: move call to driver port_setup after creation of netdev.

[Vladimir Oltean]

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

Title: "net: dsa:" prefix, no "."

On Mon, Apr 01, 2024 at 08:35:46AM -0500, Andrew Lunn wrote:
> The drivers call port_setup() is a good place to add the LEDs of a
> port to the netdev representing the port. However, when port_setup()
> is called in dsa_port_devlink_setup() the netdev does not exist
> yet. That only happens in dsa_user_create() which is latter in

later

> dsa_port_setup().
> 
> Move the call to port_setup() out of dsa_port_devlink_setup() and to
> the end of dsa_port_setup() where the netdev will exist.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/dsa/devlink.c | 17 +----------------
>  net/dsa/dsa.c     |  3 +++
>  2 files changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
> index 431bf52290a1..9c3dc6319269 100644
> --- a/net/dsa/devlink.c
> +++ b/net/dsa/devlink.c
> @@ -294,20 +294,12 @@ int dsa_port_devlink_setup(struct dsa_port *dp)
>  	struct dsa_switch_tree *dst = dp->ds->dst;
>  	struct devlink_port_attrs attrs = {};
>  	struct devlink *dl = dp->ds->devlink;
> -	struct dsa_switch *ds = dp->ds;
>  	const unsigned char *id;
>  	unsigned char len;
> -	int err;
>  
>  	memset(dlp, 0, sizeof(*dlp));
>  	devlink_port_init(dl, dlp);
>  
> -	if (ds->ops->port_setup) {
> -		err = ds->ops->port_setup(ds, dp->index);
> -		if (err)
> -			return err;
> -	}
> -
>  	id = (const unsigned char *)&dst->index;
>  	len = sizeof(dst->index);
>  
> @@ -331,14 +323,7 @@ int dsa_port_devlink_setup(struct dsa_port *dp)
>  	}
>  
>  	devlink_port_attrs_set(dlp, &attrs);
> -	err = devlink_port_register(dl, dlp, dp->index);
> -	if (err) {
> -		if (ds->ops->port_teardown)
> -			ds->ops->port_teardown(ds, dp->index);
> -		return err;
> -	}
> -
> -	return 0;
> +	return devlink_port_register(dl, dlp, dp->index);
>  }
>  
>  void dsa_port_devlink_teardown(struct dsa_port *dp)
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 09d2f5d4b3dd..6ffee2a7de94 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -520,6 +520,9 @@ static int dsa_port_setup(struct dsa_port *dp)
>  		break;
>  	}
>  
> +	if (ds->ops->port_setup)
> +		err = ds->ops->port_setup(ds, dp->index);
> +

This overwrites the not-yet-checked "err", masking the dsa_user_create()
return code, and breaking the error handling logic below. Not to
mention, if ds->ops->port_setup() fails for a user port, we should call
dsa_user_destroy().

>  	if (err && dsa_port_enabled)
>  		dsa_port_disable(dp);
>  	if (err && dsa_port_link_registered)
> 
> -- 
> 2.43.0
> 

It would have been good for the API, if we want the netdev to be
available for user ports at port_setup() time, for it to be available at
port_teardown() time as well. So dsa_port_devlink_teardown() needs
changing too.

Additionally, for CPU and DSA ports, this change will make
ds->ops->port_enable() be visible from the driver API earlier than
ds->ops->port_setup(), which isn't exactly intuitive or great or better
than before.

In fact, I think it's very difficult not to make mistakes changing the
code in its current form. These 3 patches I've prepared - which replace
this patch - should help (see attached).

[-- Attachment #2: 0001-net-dsa-consolidate-setup-and-teardown-for-shared-po.patch --]
[-- Type: text/x-diff, Size: 1841 bytes --]

From 205b3503ca15c93ba8e90d42bb2855fa0c8497e3 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 1 Apr 2024 17:43:29 +0300
Subject: [PATCH 1/3] net: dsa: consolidate setup and teardown for shared ports

CPU and DSA ports have the same port setup and teardown logic, only the
string that gets printed on error differs. Consolidate the code paths.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 09d2f5d4b3dd..64369fa5fd07 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -479,23 +479,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 		dsa_port_disable(dp);
 		break;
 	case DSA_PORT_TYPE_CPU:
-		if (dp->dn) {
-			err = dsa_shared_port_link_register_of(dp);
-			if (err)
-				break;
-			dsa_port_link_registered = true;
-		} else {
-			dev_warn(ds->dev,
-				 "skipping link registration for CPU port %d\n",
-				 dp->index);
-		}
-
-		err = dsa_port_enable(dp, NULL);
-		if (err)
-			break;
-		dsa_port_enabled = true;
-
-		break;
 	case DSA_PORT_TYPE_DSA:
 		if (dp->dn) {
 			err = dsa_shared_port_link_register_of(dp);
@@ -504,7 +487,8 @@ static int dsa_port_setup(struct dsa_port *dp)
 			dsa_port_link_registered = true;
 		} else {
 			dev_warn(ds->dev,
-				 "skipping link registration for DSA port %d\n",
+				 "skipping link registration for %s port %d\n",
+				 dsa_port_is_cpu(dp) ? "CPU" : "DSA",
 				 dp->index);
 		}
 
@@ -543,10 +527,6 @@ static void dsa_port_teardown(struct dsa_port *dp)
 	case DSA_PORT_TYPE_UNUSED:
 		break;
 	case DSA_PORT_TYPE_CPU:
-		dsa_port_disable(dp);
-		if (dp->dn)
-			dsa_shared_port_link_unregister_of(dp);
-		break;
 	case DSA_PORT_TYPE_DSA:
 		dsa_port_disable(dp);
 		if (dp->dn)
-- 
2.34.1


[-- Attachment #3: 0002-net-dsa-break-out-port-setup-and-teardown-code-per-p.patch --]
[-- Type: text/x-diff, Size: 3823 bytes --]

From b3876b65634004dc16648016820077b21a6270dd Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 1 Apr 2024 18:01:28 +0300
Subject: [PATCH 2/3] net: dsa: break out port setup and teardown code per port
 type

It is very hard to make changes to the control flow of dsa_port_setup(),
and this is because the different port types need a different setup
procedure.

By breaking these out into separate functions, it becomes clearer what
needs what, and how the teardown should look like.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa.c | 102 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 67 insertions(+), 35 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 64369fa5fd07..5d65da9a1971 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -460,12 +460,69 @@ static void dsa_tree_teardown_cpu_ports(struct dsa_switch_tree *dst)
 			dp->cpu_dp = NULL;
 }
 
-static int dsa_port_setup(struct dsa_port *dp)
+static int dsa_unused_port_setup(struct dsa_port *dp)
+{
+	dsa_port_disable(dp);
+
+	return 0;
+}
+
+static void dsa_unused_port_teardown(struct dsa_port *dp)
+{
+}
+
+static int dsa_shared_port_setup(struct dsa_port *dp)
 {
-	bool dsa_port_link_registered = false;
 	struct dsa_switch *ds = dp->ds;
-	bool dsa_port_enabled = false;
-	int err = 0;
+	bool link_registered = false;
+	int err;
+
+	if (dp->dn) {
+		err = dsa_shared_port_link_register_of(dp);
+		if (err)
+			return err;
+
+		link_registered = true;
+	} else {
+		dev_warn(ds->dev,
+			 "skipping link registration for %s port %d\n",
+			 dsa_port_is_cpu(dp) ? "CPU" : "DSA",
+			 dp->index);
+	}
+
+	err = dsa_port_enable(dp, NULL);
+	if (err && link_registered)
+		dsa_shared_port_link_unregister_of(dp);
+
+	return err;
+}
+
+static void dsa_shared_port_teardown(struct dsa_port *dp)
+{
+	dsa_port_disable(dp);
+	if (dp->dn)
+		dsa_shared_port_link_unregister_of(dp);
+}
+
+static int dsa_user_port_setup(struct dsa_port *dp)
+{
+	of_get_mac_address(dp->dn, dp->mac);
+
+	return dsa_user_create(dp);
+}
+
+static void dsa_user_port_teardown(struct dsa_port *dp)
+{
+	if (!dp->user)
+		return;
+
+	dsa_user_destroy(dp->user);
+	dp->user = NULL;
+}
+
+static int dsa_port_setup(struct dsa_port *dp)
+{
+	int err;
 
 	if (dp->setup)
 		return 0;
@@ -476,38 +533,17 @@ static int dsa_port_setup(struct dsa_port *dp)
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
-		dsa_port_disable(dp);
+		err = dsa_unused_port_setup(dp);
 		break;
 	case DSA_PORT_TYPE_CPU:
 	case DSA_PORT_TYPE_DSA:
-		if (dp->dn) {
-			err = dsa_shared_port_link_register_of(dp);
-			if (err)
-				break;
-			dsa_port_link_registered = true;
-		} else {
-			dev_warn(ds->dev,
-				 "skipping link registration for %s port %d\n",
-				 dsa_port_is_cpu(dp) ? "CPU" : "DSA",
-				 dp->index);
-		}
-
-		err = dsa_port_enable(dp, NULL);
-		if (err)
-			break;
-		dsa_port_enabled = true;
-
+		err = dsa_shared_port_setup(dp);
 		break;
 	case DSA_PORT_TYPE_USER:
-		of_get_mac_address(dp->dn, dp->mac);
-		err = dsa_user_create(dp);
+		err = dsa_user_port_setup(dp);
 		break;
 	}
 
-	if (err && dsa_port_enabled)
-		dsa_port_disable(dp);
-	if (err && dsa_port_link_registered)
-		dsa_shared_port_link_unregister_of(dp);
 	if (err) {
 		dsa_port_devlink_teardown(dp);
 		return err;
@@ -525,18 +561,14 @@ static void dsa_port_teardown(struct dsa_port *dp)
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
+		dsa_unused_port_teardown(dp);
 		break;
 	case DSA_PORT_TYPE_CPU:
 	case DSA_PORT_TYPE_DSA:
-		dsa_port_disable(dp);
-		if (dp->dn)
-			dsa_shared_port_link_unregister_of(dp);
+		dsa_shared_port_teardown(dp);
 		break;
 	case DSA_PORT_TYPE_USER:
-		if (dp->user) {
-			dsa_user_destroy(dp->user);
-			dp->user = NULL;
-		}
+		dsa_user_port_teardown(dp);
 		break;
 	}
 
-- 
2.34.1


[-- Attachment #4: 0003-net-dsa-move-call-to-driver-port_setup-after-creatio.patch --]
[-- Type: text/x-diff, Size: 4534 bytes --]

From afebdb6e4b52101e863aa36a2fedb5828d93ed4c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 1 Apr 2024 18:25:51 +0300
Subject: [PATCH 3/3] net: dsa: move call to driver port_setup after creation
 of netdev

The driver-facing method port_setup() is a good place to add the LEDs of
a port to the netdev representing the port. However, when port_setup()
is called in dsa_port_devlink_setup(), the netdev does not exist
yet. That only happens in dsa_user_create(), which is later in
dsa_port_setup().

Move the call to port_setup() out of dsa_port_devlink_setup() and to
the end of dsa_port_setup() where the netdev will exist. For the other
port types, the call to port_setup() and port_teardown() remains where
it was before (functionally speaking), but now it needs to be open-coded
in their respective setup/teardown logic.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/devlink.c | 17 +-------------
 net/dsa/dsa.c     | 59 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
index 431bf52290a1..9c3dc6319269 100644
--- a/net/dsa/devlink.c
+++ b/net/dsa/devlink.c
@@ -294,20 +294,12 @@ int dsa_port_devlink_setup(struct dsa_port *dp)
 	struct dsa_switch_tree *dst = dp->ds->dst;
 	struct devlink_port_attrs attrs = {};
 	struct devlink *dl = dp->ds->devlink;
-	struct dsa_switch *ds = dp->ds;
 	const unsigned char *id;
 	unsigned char len;
-	int err;
 
 	memset(dlp, 0, sizeof(*dlp));
 	devlink_port_init(dl, dlp);
 
-	if (ds->ops->port_setup) {
-		err = ds->ops->port_setup(ds, dp->index);
-		if (err)
-			return err;
-	}
-
 	id = (const unsigned char *)&dst->index;
 	len = sizeof(dst->index);
 
@@ -331,14 +323,7 @@ int dsa_port_devlink_setup(struct dsa_port *dp)
 	}
 
 	devlink_port_attrs_set(dlp, &attrs);
-	err = devlink_port_register(dl, dlp, dp->index);
-	if (err) {
-		if (ds->ops->port_teardown)
-			ds->ops->port_teardown(ds, dp->index);
-		return err;
-	}
-
-	return 0;
+	return devlink_port_register(dl, dlp, dp->index);
 }
 
 void dsa_port_devlink_teardown(struct dsa_port *dp)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5d65da9a1971..d8aa869e17ba 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -462,6 +462,15 @@ static void dsa_tree_teardown_cpu_ports(struct dsa_switch_tree *dst)
 
 static int dsa_unused_port_setup(struct dsa_port *dp)
 {
+	struct dsa_switch *ds = dp->ds;
+	int err;
+
+	if (ds->ops->port_setup) {
+		err = ds->ops->port_setup(ds, dp->index);
+		if (err)
+			return err;
+	}
+
 	dsa_port_disable(dp);
 
 	return 0;
@@ -469,6 +478,10 @@ static int dsa_unused_port_setup(struct dsa_port *dp)
 
 static void dsa_unused_port_teardown(struct dsa_port *dp)
 {
+	struct dsa_switch *ds = dp->ds;
+
+	if (ds->ops->port_teardown)
+		ds->ops->port_teardown(ds, dp->index);
 }
 
 static int dsa_shared_port_setup(struct dsa_port *dp)
@@ -490,8 +503,23 @@ static int dsa_shared_port_setup(struct dsa_port *dp)
 			 dp->index);
 	}
 
+	if (ds->ops->port_setup) {
+		err = ds->ops->port_setup(ds, dp->index);
+		if (err)
+			goto unregister_link;
+	}
+
 	err = dsa_port_enable(dp, NULL);
-	if (err && link_registered)
+	if (err)
+		goto port_teardown;
+
+	return 0;
+
+port_teardown:
+	if (ds->ops->port_teardown)
+		ds->ops->port_teardown(ds, dp->index);
+unregister_link:
+	if (link_registered)
 		dsa_shared_port_link_unregister_of(dp);
 
 	return err;
@@ -499,23 +527,50 @@ static int dsa_shared_port_setup(struct dsa_port *dp)
 
 static void dsa_shared_port_teardown(struct dsa_port *dp)
 {
+	struct dsa_switch *ds = dp->ds;
+
 	dsa_port_disable(dp);
+	if (ds->ops->port_teardown)
+		ds->ops->port_teardown(ds, dp->index);
 	if (dp->dn)
 		dsa_shared_port_link_unregister_of(dp);
 }
 
 static int dsa_user_port_setup(struct dsa_port *dp)
 {
+	struct dsa_switch *ds = dp->ds;
+	int err;
+
 	of_get_mac_address(dp->dn, dp->mac);
 
-	return dsa_user_create(dp);
+	err = dsa_user_create(dp);
+	if (err)
+		return err;
+
+	if (ds->ops->port_setup) {
+		err = ds->ops->port_setup(ds, dp->index);
+		if (err)
+			goto user_destroy;
+	}
+
+	return 0;
+
+user_destroy:
+	dsa_user_destroy(dp->user);
+	dp->user = NULL;
+	return err;
 }
 
 static void dsa_user_port_teardown(struct dsa_port *dp)
 {
+	struct dsa_switch *ds = dp->ds;
+
 	if (!dp->user)
 		return;
 
+	if (ds->ops->port_teardown)
+		ds->ops->port_teardown(ds, dp->index);
+
 	dsa_user_destroy(dp->user);
 	dp->user = NULL;
 }
-- 
2.34.1

Re: [PATCH net-next v3 2/7] net: Add helpers for netdev LEDs

[Vladimir Oltean]

On Mon, Apr 01, 2024 at 08:35:47AM -0500, Andrew Lunn wrote:
> Add a set of helpers for parsing the standard device tree properties
> for LEDs as part of an ethernet device, and registering them with the
> LED subsystem. This code can be used by any sort of netdev driver,
> including plain MAC, DSA switches or pure switchdev switch driver.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/net/netdev_leds.h |  45 +++++++++++
>  net/Kconfig               |  11 +++
>  net/core/Makefile         |   1 +
>  net/core/netdev-leds.c    | 201 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 258 insertions(+)
> 
> diff --git a/include/net/netdev_leds.h b/include/net/netdev_leds.h
> new file mode 100644
> index 000000000000..239f492f29f5
> --- /dev/null
> +++ b/include/net/netdev_leds.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Helpers used for creating and managing LEDs on a netdev MAC
> + * driver.
> + */
> +
> +#ifndef _NET_NETDEV_LEDS_H
> +#define _NET_NETDEV_LEDS_H
> +

An object file which has "#include <net/netdev_leds.h>" as its only
line of source code does not compile. All headers should be
self-contained in terms of #include dependencies.

../include/net/netdev_leds.h:11:31: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
        int (*brightness_set)(struct net_device *ndev, u8 led,
                                     ^
../include/net/netdev_leds.h:11:49: error: unknown type name 'u8'
        int (*brightness_set)(struct net_device *ndev, u8 led,
                                                       ^
../include/net/netdev_leds.h:12:15: warning: declaration of 'enum led_brightness' will not be visible outside of this function [-Wvisibility]
                              enum led_brightness brightness);
                                   ^
../include/net/netdev_leds.h:13:26: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
        int (*blink_set)(struct net_device *ndev, u8 led,
                                ^
../include/net/netdev_leds.h:13:44: error: unknown type name 'u8'
        int (*blink_set)(struct net_device *ndev, u8 led,
                                                  ^
../include/net/netdev_leds.h:15:40: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
        int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
                                              ^
../include/net/netdev_leds.h:15:58: error: unknown type name 'u8'
        int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
                                                                ^
../include/net/netdev_leds.h:17:31: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
        int (*hw_control_set)(struct net_device *ndev, u8 led,
                                     ^
../include/net/netdev_leds.h:17:49: error: unknown type name 'u8'
        int (*hw_control_set)(struct net_device *ndev, u8 led,
                                                       ^
../include/net/netdev_leds.h:19:31: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
        int (*hw_control_get)(struct net_device *ndev, u8 led,
                                     ^
../include/net/netdev_leds.h:19:49: error: unknown type name 'u8'
        int (*hw_control_get)(struct net_device *ndev, u8 led,
                                                       ^
../include/net/netdev_leds.h:24:30: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
                             ^
../include/net/netdev_leds.h:24:55: warning: declaration of 'struct device_node' will not be visible outside of this function [-Wvisibility]
int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
                                                      ^
../include/net/netdev_leds.h:25:16: warning: declaration of 'struct list_head' will not be visible outside of this function [-Wvisibility]
                      struct list_head *list, struct netdev_leds_ops *ops,
                             ^
../include/net/netdev_leds.h:28:34: warning: declaration of 'struct list_head' will not be visible outside of this function [-Wvisibility]
void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);
                                 ^
../include/net/netdev_leds.h:28:58: warning: declaration of 'struct net_device' will not be visible outside of this function [-Wvisibility]
void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);

> +struct netdev_leds_ops {
> +	int (*brightness_set)(struct net_device *ndev, u8 led,
> +			      enum led_brightness brightness);
> +	int (*blink_set)(struct net_device *ndev, u8 led,
> +			 unsigned long *delay_on,  unsigned long *delay_off);

One single space between arguments.

> +	int (*hw_control_is_supported)(struct net_device *ndev, u8 led,
> +				       unsigned long flags);
> +	int (*hw_control_set)(struct net_device *ndev, u8 led,
> +			      unsigned long flags);
> +	int (*hw_control_get)(struct net_device *ndev, u8 led,
> +			      unsigned long *flags);
> +};
> +
> +#ifdef CONFIG_NETDEV_LEDS
> +int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
> +		      struct list_head *list, struct netdev_leds_ops *ops,
> +		      int max_leds);

Should we be even more opaque in the API, aka instead of requiring the
user to explicitly hold a struct list_head, just give it an opaque
struct netdev_led_group * which holds that (as a hidden implementation
detail)?

The API structure could be simply styled as
"struct netdev_led_group *netdev_led_group_create()" and
"void netdev_led_group_destroy(const struct netdev_led_group *group)",
and it would allow for future editing of the actual implementation.

Just an idea.

> +
> +void netdev_leds_teardown(struct list_head *list, struct net_device *ndev);
> +
> +#else
> +static inline int netdev_leds_setup(struct net_device *ndev,
> +				    struct device_node *np,
> +				    struct list_head *list,
> +				    struct netdev_leds_ops *ops)
> +{
> +	return 0;
> +}
> +
> +static inline void netdev_leds_teardown(struct list_head *list,
> +					struct net_device *ndev)
> +{
> +}
> +#endif /* CONFIG_NETDEV_LEDS */
> +
> +#endif /* _NET_PORT_LEDS_H */
> diff --git a/net/Kconfig b/net/Kconfig
> index 3e57ccf0da27..753dfd11f014 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -516,4 +516,15 @@ config NET_TEST
>  
>  	  If unsure, say N.
>  
> +config NETDEV_LEDS
> +	bool "NETDEV helper code for MAC LEDs"
> +	select LEDS_CLASS
> +	select LEDS_TRIGGERS
> +	select LEDS_TRIGGER_NETDEV
> +	help
> +	  NICs and Switches often contain LED controllers. When the LEDs

switches

> +	  are part of the MAC, the MAC driver, aka netdev driver, should
> +	  make the LEDs available. NETDEV_LEDS offers a small library
> +	  of code to help MAC drivers do this.
> +
>  endif   # if NET
> diff --git a/net/core/Makefile b/net/core/Makefile
> index 21d6fbc7e884..d04ce07541b5 100644
> --- a/net/core/Makefile
> +++ b/net/core/Makefile
> @@ -42,3 +42,4 @@ obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
>  obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
>  obj-$(CONFIG_OF)	+= of_net.o
>  obj-$(CONFIG_NET_TEST) += net_test.o
> +obj-$(CONFIG_NETDEV_LEDS) += netdev-leds.o
> diff --git a/net/core/netdev-leds.c b/net/core/netdev-leds.c
> new file mode 100644
> index 000000000000..802dd819a991
> --- /dev/null
> +++ b/net/core/netdev-leds.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/leds.h>
> +#include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <net/netdev_leds.h>
> +
> +struct netdev_led {
> +	struct list_head led_list;
> +	struct led_classdev led_cdev;
> +	struct netdev_leds_ops *ops;
> +	struct net_device *ndev;
> +	u8 index;
> +};
> +
> +#define to_netdev_led(d) container_of(d, struct netdev_led, led_cdev)
> +
> +static int netdev_brightness_set(struct led_classdev *led_cdev,
> +				 enum led_brightness value)
> +{
> +	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> +	return netdev_led->ops->brightness_set(netdev_led->ndev,
> +					       netdev_led->index,
> +					       value);
> +}
> +
> +static int netdev_blink_set(struct led_classdev *led_cdev,
> +			    unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> +	return netdev_led->ops->blink_set(netdev_led->ndev,
> +					  netdev_led->index,
> +					  delay_on, delay_off);
> +}
> +
> +static __maybe_unused int
> +netdev_hw_control_is_supported(struct led_classdev *led_cdev,
> +			       unsigned long flags)
> +{
> +	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> +	return netdev_led->ops->hw_control_is_supported(netdev_led->ndev,
> +							netdev_led->index,
> +							flags);
> +}
> +
> +static __maybe_unused int netdev_hw_control_set(struct led_classdev *led_cdev,
> +						unsigned long flags)
> +{
> +	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> +	return netdev_led->ops->hw_control_set(netdev_led->ndev,
> +					       netdev_led->index,
> +					       flags);
> +}
> +
> +static __maybe_unused int netdev_hw_control_get(struct led_classdev *led_cdev,
> +						unsigned long *flags)
> +{
> +	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> +	return netdev_led->ops->hw_control_get(netdev_led->ndev,
> +					       netdev_led->index,
> +					       flags);
> +}
> +
> +static struct device *
> +netdev_hw_control_get_device(struct led_classdev *led_cdev)
> +{
> +	struct netdev_led *netdev_led = to_netdev_led(led_cdev);
> +
> +	return &netdev_led->ndev->dev;
> +}
> +
> +static int netdev_led_setup(struct net_device *ndev, struct device_node *led,
> +			    struct list_head *list, struct netdev_leds_ops *ops,
> +			    int max_leds)
> +{
> +	struct led_init_data init_data = {};
> +	struct device *dev = &ndev->dev;
> +	struct netdev_led *netdev_led;
> +	struct led_classdev *cdev;
> +	u32 index;
> +	int err;
> +
> +	netdev_led = devm_kzalloc(dev, sizeof(*netdev_led), GFP_KERNEL);
> +	if (!netdev_led)
> +		return -ENOMEM;
> +
> +	netdev_led->ndev = ndev;
> +	netdev_led->ops = ops;
> +	cdev = &netdev_led->led_cdev;
> +
> +	err = of_property_read_u32(led, "reg", &index);
> +	if (err)
> +		return err;
> +
> +	if (index >= max_leds)
> +		return -EINVAL;
> +
> +	netdev_led->index = index;
> +
> +	if (ops->brightness_set)
> +		cdev->brightness_set_blocking = netdev_brightness_set;
> +	if (ops->blink_set)
> +		cdev->blink_set = netdev_blink_set;
> +#ifdef CONFIG_LEDS_TRIGGERS
> +	if (ops->hw_control_is_supported)
> +		cdev->hw_control_is_supported = netdev_hw_control_is_supported;
> +	if (ops->hw_control_set)
> +		cdev->hw_control_set = netdev_hw_control_set;
> +	if (ops->hw_control_get)
> +		cdev->hw_control_get = netdev_hw_control_get;
> +	cdev->hw_control_trigger = "netdev";
> +#endif
> +	cdev->hw_control_get_device = netdev_hw_control_get_device;
> +	cdev->max_brightness = 1;
> +	init_data.fwnode = of_fwnode_handle(led);
> +	init_data.devname_mandatory = true;
> +
> +	init_data.devicename = dev_name(dev);
> +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> +	if (err)
> +		return err;
> +
> +	INIT_LIST_HEAD(&netdev_led->led_list);
> +	list_add(&netdev_led->led_list, list);
> +
> +	return 0;
> +}
> +
> +/**
> + * netdev_leds_setup - Parse DT node and create LEDs for netdev
> + *
> + * @ndev: struct netdev for the MAC
> + * @np: ethernet-node in device tree
> + * @list: list to add LEDs to
> + * @ops: structure of ops to manipulate the LED.
> + * @max_leds: maximum number of LEDs support by netdev.
> + *
> + * Parse the device tree node, as described in
> + * ethernet-controller.yaml, and find any LEDs. For each LED found,
> + * ensure the reg value is less than max_leds, create an LED and
> + * register it with the LED subsystem. The LED will be added to the
> + * list, which can be shared by all netdevs of the device. The ops
> + * structure contains the callbacks needed to control the LEDs.
> + *
> + * Return 0 in success, otherwise an negative error code.
> + */
> +int netdev_leds_setup(struct net_device *ndev, struct device_node *np,
> +		      struct list_head *list, struct netdev_leds_ops *ops,
> +		      int max_leds)
> +{
> +	struct device_node *leds, *led;
> +	int err;
> +
> +	leds = of_get_child_by_name(np, "leds");
> +	if (!leds)
> +		return 0;
> +
> +	for_each_available_child_of_node(leds, led) {
> +		err = netdev_led_setup(ndev, led, list, ops, max_leds);
> +		if (err) {
> +			of_node_put(led);

What is the refcounting scheme for the "leds" node? Its refcount is left
incremented both on success, and on error here. It is not decremented
anywhere.

> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(netdev_leds_setup);
> +
> +/**
> + * netdev_leds_teardown - Remove LEDs for a netdev
> + *
> + * @list: list to add LEDs to teardown
> + * @ndev: The netdev for which LEDs should be removed
> + *
> + * Unregister all LEDs for a given netdev, freeing up any allocated
> + * memory.
> + */
> +void netdev_leds_teardown(struct list_head *list, struct net_device *ndev)
> +{
> +	struct netdev_led *netdev_led;
> +	struct led_classdev *cdev;
> +	struct device *dev;
> +
> +	list_for_each_entry(netdev_led, list, led_list) {
> +		if (netdev_led->ndev != ndev)
> +			continue;

I don't exactly see what's the advantage, in your proposal, of allowing
the API to bundle up the LED groups of multiple netdevs into a single
struct list_head. I see switches like mv88e6xxx have a single list for
the entire chip rather than one per port. It makes for a less
straightforward implementation here (we could have just wiped out the
entire list, if we knew there's a single group in it).

Also, what's the locking scheme expected by the API? The setup and
teardown methods are not reentrant.

> +		dev = &netdev_led->ndev->dev;
> +		cdev = &netdev_led->led_cdev;
> +		devm_led_classdev_unregister(dev, cdev);

I think devres-using API functions should be prefixed with devm_ in
their name for callers' awareness, rather than this aspect being silent.
And, if a netdev_leds_teardown() exists, I suspect devres usage isn't
even necessary, you could use list_for_each_entry_safe() and also
kfree() the netdev_led.

> +	}
> +}
> +EXPORT_SYMBOL_GPL(netdev_leds_teardown);
> 
> -- 
> 2.43.0
> 

Re: [PATCH net-next v3 5/7] net: dsa: Add helpers to convert netdev to ds or port index

[Vladimir Oltean]

On Mon, Apr 01, 2024 at 08:35:50AM -0500, Andrew Lunn wrote:
> The LED helpers make use of a struct netdev. Add helpers a DSA driver
> can use to convert a netdev to a struct dsa_switch and the port index.
> 
> To do this, dsa_user_to_port() has to be made available out side of
> net/dev, to convert the inline function in net/dsa/user.h into a
> normal function, and export it.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

I think the API we have today is sufficient: we have dsa_port_to_netdev(),
introduced at Vivien's request rather than exporting dsa_user_to_port().

Also, I believe that having a single API function which returns a single
struct dsa_port *, from which we force the caller to get the dp->ds and
dp->index, is better (cheaper) than requiring 2 API functions, one for
getting the ds and the other for the index.

Re: [PATCH net-next v3 6/7] dsa: mv88e6xxx: Create port/netdev LEDs

[Vladimir Oltean]

On Mon, Apr 01, 2024 at 08:35:51AM -0500, Andrew Lunn wrote:
> @@ -4006,6 +4106,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>  
>  static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
>  {
> +	struct dsa_port *dp = dsa_to_port(ds, port);
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int err;
>  
> @@ -4016,13 +4117,26 @@ static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
>  			return err;
>  	}
>  
> -	return mv88e6xxx_setup_devlink_regions_port(ds, port);
> +	err  = mv88e6xxx_setup_devlink_regions_port(ds, port);
> +	if (err)
> +		return err;
> +
> +	if (dp->dn) {
> +		err = netdev_leds_setup(dp->user, dp->dn, &chip->leds,
> +					&mv88e6xxx_netdev_leds_ops, 2);

This (dereferencing dp->user regardless of dp->type) is a dangerous
thing to do. Let alone the fact that dp->user is NULL for DSA ports. It
is actually in a union with struct net_device *conduit, for CPU ports.
So you're also setting up LEDs for the conduit interface here...

Please make it conditional on dsa_port_is_user(), and same for teardown.

> +		if (err)
> +			mv88e6xxx_teardown_devlink_regions_port(ds, port);
> +	}
> +	return err;
>  }
>  
>  static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
>  {
> +	struct dsa_port *dp = dsa_to_port(ds, port);
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  
> +	netdev_leds_teardown(&chip->leds, dp->user);
> +
>  	mv88e6xxx_teardown_devlink_regions_port(ds, port);
>  
>  	if (chip->info->ops->pcs_ops &&
> @@ -6397,6 +6511,7 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>  	INIT_LIST_HEAD(&chip->mdios);
>  	idr_init(&chip->policies);
>  	INIT_LIST_HEAD(&chip->msts);
> +	INIT_LIST_HEAD(&chip->leds);
>  
>  	return chip;
>  }
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 64f8bde68ccf..b70e74203b31 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -432,6 +432,9 @@ struct mv88e6xxx_chip {
>  
>  	/* Bridge MST to SID mappings */
>  	struct list_head msts;
> +
> +	/* LEDs associated to the ports */
> +	struct list_head leds;
>  };
>  
>  struct mv88e6xxx_bus_ops {
> @@ -668,6 +671,15 @@ struct mv88e6xxx_ops {
>  	int (*led_blink_set)(struct mv88e6xxx_chip *chip, int port, u8 led,
>  			     unsigned long *delay_on,
>  			     unsigned long *delay_off);
> +	int (*led_hw_control_is_supported)(struct mv88e6xxx_chip *chip,
> +					   int port, u8 led,
> +					   unsigned long flags);
> +	int (*led_hw_control_set)(struct mv88e6xxx_chip *chip,
> +				  int port, u8 led,
> +				  unsigned long flags);
> +	int (*led_hw_control_get)(struct mv88e6xxx_chip *chip,
> +				  int port, u8 led,
> +				  unsigned long *flags);
>  };
>  
>  struct mv88e6xxx_irq_ops {
> 
> -- 
> 2.43.0
> 

Re: [PATCH net-next v3 5/7] net: dsa: Add helpers to convert netdev to ds or port index

[Russell King (Oracle)]

On Tue, Apr 02, 2024 at 01:56:52PM +0300, Vladimir Oltean wrote:
> On Mon, Apr 01, 2024 at 08:35:50AM -0500, Andrew Lunn wrote:
> > The LED helpers make use of a struct netdev. Add helpers a DSA driver
> > can use to convert a netdev to a struct dsa_switch and the port index.
> > 
> > To do this, dsa_user_to_port() has to be made available out side of
> > net/dev, to convert the inline function in net/dsa/user.h into a
> > normal function, and export it.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> 
> I think the API we have today is sufficient: we have dsa_port_to_netdev(),
> introduced at Vivien's request rather than exporting dsa_user_to_port().
> 
> Also, I believe that having a single API function which returns a single
> struct dsa_port *, from which we force the caller to get the dp->ds and
> dp->index, is better (cheaper) than requiring 2 API functions, one for
> getting the ds and the other for the index.

Yes, I would tend to agree having done this for my experimental phylink
changes. struct dsa_port seems to make the most sense:

static inline struct dsa_port *
dsa_phylink_to_port(struct phylink_config *config)
{
        return container_of(config, struct dsa_port, pl_config);
}

which then means e.g.:

static void mv88e6xxx_mac_config(struct phylink_config *config,
                                 unsigned int mode,
                                 const struct phylink_link_state *state)
{
        struct dsa_port *dp = dsa_phylink_to_port(config);
        struct mv88e6xxx_chip *chip = dp->ds->priv;
        int port = dp->index;
        int err = 0;

        mv88e6xxx_reg_lock(chip);

        if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(chip, port)) {
                err = mv88e6xxx_port_config_interface(chip, port,
                                                      state->interface);
                if (err && err != -EOPNOTSUPP)
                        goto err_unlock;
        }

err_unlock:
        mv88e6xxx_reg_unlock(chip);

        if (err && err != -EOPNOTSUPP)
                dev_err(dp->ds->dev, "p%d: failed to configure MAC/PCS\n",
                        port);
}

vs the current code:

static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
                                 unsigned int mode,
                                 const struct phylink_link_state *state)
{
        struct mv88e6xxx_chip *chip = ds->priv;
        int err = 0;

        mv88e6xxx_reg_lock(chip);

        if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(chip, port)) {
                err = mv88e6xxx_port_config_interface(chip, port,
                                                      state->interface);
                if (err && err != -EOPNOTSUPP)
                        goto err_unlock;
        }

err_unlock:
        mv88e6xxx_reg_unlock(chip);

        if (err && err != -EOPNOTSUPP)
                dev_err(ds->dev, "p%d: failed to configure MAC/PCS\n", port);
}

-- 
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 v3 5/7] net: dsa: Add helpers to convert netdev to ds or port index

[Vladimir Oltean]

On Tue, Apr 02, 2024 at 02:31:34PM +0100, Russell King (Oracle) wrote:
> Yes, I would tend to agree having done this for my experimental phylink
> changes. struct dsa_port seems to make the most sense:
> 
> static inline struct dsa_port *
> dsa_phylink_to_port(struct phylink_config *config)
> {
>         return container_of(config, struct dsa_port, pl_config);
> }
> 
> which then means e.g.:
> 
> static void mv88e6xxx_mac_config(struct phylink_config *config,
>                                  unsigned int mode,
>                                  const struct phylink_link_state *state)
> {
>         struct dsa_port *dp = dsa_phylink_to_port(config);
>         struct mv88e6xxx_chip *chip = dp->ds->priv;
>         int port = dp->index;
>         int err = 0;
> 
>         mv88e6xxx_reg_lock(chip);
> 
>         if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(chip, port)) {
>                 err = mv88e6xxx_port_config_interface(chip, port,
>                                                       state->interface);
>                 if (err && err != -EOPNOTSUPP)
>                         goto err_unlock;
>         }
> 
> err_unlock:
>         mv88e6xxx_reg_unlock(chip);
> 
>         if (err && err != -EOPNOTSUPP)
>                 dev_err(dp->ds->dev, "p%d: failed to configure MAC/PCS\n",
>                         port);
> }

Looks ok, looking forward to seeing more of it. Maybe a slight
preference to keeping "struct dsa_switch *ds" as a local variable of its
own, even if this means declaring and initializing "chip" on separate
lines.

Re: [PATCH net-next v3 2/7] net: Add helpers for netdev LEDs

[Andrew Lunn]

> Should we be even more opaque in the API, aka instead of requiring the
> user to explicitly hold a struct list_head, just give it an opaque
> struct netdev_led_group * which holds that (as a hidden implementation
> detail)?
> 
> The API structure could be simply styled as
> "struct netdev_led_group *netdev_led_group_create()" and
> "void netdev_led_group_destroy(const struct netdev_led_group *group)",
> and it would allow for future editing of the actual implementation.
> 
> Just an idea.

Hi Vladimir

I'm not sure making it opaque brings much for this case. It is so
simple. Things like phylink should be opaque, there is a lot of
internal state which if used in a MAC driver is going to cause all
sorts of problem, and is likely to be used wrong. But here?

So i will probably leave this transparent.

Thanks for the other comments. I'm slowly working on them as time
permits.

       Andrew