[RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Luiz Angelo Daros de Luca]

The rtl8366rb switch family has 4 LED groups, with one LED from each
group for each of its 6 ports. LEDs in this family can be controlled
manually using a bitmap or triggered by hardware. It's important to note
that hardware triggers are configured at the LED group level, meaning
all LEDs in the same group share the same hardware triggers settings.

The first part of this series involves dropping most of the existing
code, as, except for disabling the LEDs, it was not working as expected.
If not disabled, the LEDs will retain their default settings after a
switch reset, which may be sufficient for many devices.

The second part introduces the LED driver to control the switch LEDs
from sysfs or device-tree. This driver still allows the LEDs to retain
their default settings, but it will shift to the software-based OS LED
triggers if any configuration is changed. Subsequently, the LEDs will
operate as normal LEDs until the switch undergoes another reset.

Netdev LED trigger supports offloading to hardware triggers.
Unfortunately, this isn't possible with the current LED API for this
switch family. When the hardware trigger is enabled, it applies to all
LEDs in the LED group while the LED API decides to offload based on only
the state of a single LED. To avoid inconsistency between LEDs,
offloading would need to check if all LEDs in the group share the same
compatible settings and atomically enable offload for all LEDs.

This patch series has a minor conflict with the other realtek
refacatoring series at the priv->ds access.

-       dsa_switch_for_each_port(dp, priv->ds) {
+       dsa_switch_for_each_port(dp, &priv->ds) {

[RFC net-next 1/2] net: dsa: realtek: keep default LED state in rtl8366rb

[Luiz Angelo Daros de Luca]

This switch family supports four LEDs for each of its six ports. Each
LED group is composed of one of these four LEDs from all six ports. LED
groups can be configured to display hardware information, such as link
activity, or manually controlled through a bitmap in registers
RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG.

After a reset, the default LED group configuration for groups 0 to 3
indicates link activity at 1000M, 100M, and 10M, or
RTL8366RB_LED_CTRL_REG as 0x5432. These configurations are commonly used
for LED indications. However, the driver was replacing that
configuration to use manually controlled LEDs (RTL8366RB_LED_FORCE)
without providing a way for the OS to control them. The default
configuration is deemed more useful than fixed, uncontrollable turned-on
LEDs.

The driver was enabling/disabling LEDs during port_enable/disable.
However, these events occur when the port is administratively controlled
(up or down) and are not related to link presence. Additionally, when a
port N was disabled, the driver was turning off all LEDs for group N,
not only the corresponding LED for port N in any of those 4 groups. In
such cases, if port 0 is WAN and brought down, the LEDs for all ports in
LED group 0 would be turned off. As another side effect, the driver was
wrongly warning that port 5 didn't have an LED ("no LED for port 5").
Since showing the administrative state of ports is not an orthodox way
to use LEDs, it was not worth it to fix it and all this code was
dropped.

The code to disable LEDs was simplified only changing each LED group to
the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and
RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED
group is configured with RTL8366RB_LED_FORCE and they don't need to be
cleaned. The code still references an LED controlled by
RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has
actually used it. Also, some magic numbers were replaced by macros.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/rtl8366rb.c | 87 +++++++----------------------
 1 file changed, 20 insertions(+), 67 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index e3b6a470ca67..874e04cf2e0d 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -182,7 +182,12 @@
 #define RTL8366RB_LED_BLINKRATE_222MS		0x0004
 #define RTL8366RB_LED_BLINKRATE_446MS		0x0005
 
+/* LED trigger event for each group */
 #define RTL8366RB_LED_CTRL_REG			0x0431
+#define RTL8366RB_LED_CTRL_OFFSET(led_group)	\
+	(4 * (led_group))
+#define RTL8366RB_LED_CTRL_MASK(led_group)	\
+	(0xf << RTL8366RB_LED_CTRL_OFFSET(led_group))
 #define RTL8366RB_LED_OFF			0x0
 #define RTL8366RB_LED_DUP_COL			0x1
 #define RTL8366RB_LED_LINK_ACT			0x2
@@ -199,6 +204,11 @@
 #define RTL8366RB_LED_LINK_TX			0xd
 #define RTL8366RB_LED_MASTER			0xe
 #define RTL8366RB_LED_FORCE			0xf
+
+/* The RTL8366RB_LED_X_X registers are used to manually set the LED state only
+ * when the corresponding LED group in RTL8366RB_LED_CTRL_REG is
+ * RTL8366RB_LED_FORCE. Otherwise, it is ignored.
+ */
 #define RTL8366RB_LED_0_1_CTRL_REG		0x0432
 #define RTL8366RB_LED_1_OFFSET			6
 #define RTL8366RB_LED_2_3_CTRL_REG		0x0433
@@ -998,28 +1008,20 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	 */
 	if (priv->leds_disabled) {
 		/* Turn everything off */
-		regmap_update_bits(priv->map,
-				   RTL8366RB_LED_0_1_CTRL_REG,
-				   0x0FFF, 0);
-		regmap_update_bits(priv->map,
-				   RTL8366RB_LED_2_3_CTRL_REG,
-				   0x0FFF, 0);
 		regmap_update_bits(priv->map,
 				   RTL8366RB_INTERRUPT_CONTROL_REG,
 				   RTL8366RB_P4_RGMII_LED,
 				   0);
-		val = RTL8366RB_LED_OFF;
-	} else {
-		/* TODO: make this configurable per LED */
-		val = RTL8366RB_LED_FORCE;
-	}
-	for (i = 0; i < 4; i++) {
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_CTRL_REG,
-					 0xf << (i * 4),
-					 val << (i * 4));
-		if (ret)
-			return ret;
+
+		for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
+			val = RTL8366RB_LED_OFF << RTL8366RB_LED_CTRL_OFFSET(i);
+			ret = regmap_update_bits(priv->map,
+						 RTL8366RB_LED_CTRL_REG,
+						 RTL8366RB_LED_CTRL_MASK(i),
+						 val);
+			if (ret)
+				return ret;
+		}
 	}
 
 	ret = rtl8366_reset_vlan(priv);
@@ -1166,52 +1168,6 @@ rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
 	}
 }
 
-static void rb8366rb_set_port_led(struct realtek_priv *priv,
-				  int port, bool enable)
-{
-	u16 val = enable ? 0x3f : 0;
-	int ret;
-
-	if (priv->leds_disabled)
-		return;
-
-	switch (port) {
-	case 0:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_0_1_CTRL_REG,
-					 0x3F, val);
-		break;
-	case 1:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_0_1_CTRL_REG,
-					 0x3F << RTL8366RB_LED_1_OFFSET,
-					 val << RTL8366RB_LED_1_OFFSET);
-		break;
-	case 2:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_2_3_CTRL_REG,
-					 0x3F, val);
-		break;
-	case 3:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_2_3_CTRL_REG,
-					 0x3F << RTL8366RB_LED_3_OFFSET,
-					 val << RTL8366RB_LED_3_OFFSET);
-		break;
-	case 4:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_INTERRUPT_CONTROL_REG,
-					 RTL8366RB_P4_RGMII_LED,
-					 enable ? RTL8366RB_P4_RGMII_LED : 0);
-		break;
-	default:
-		dev_err(priv->dev, "no LED for port %d\n", port);
-		return;
-	}
-	if (ret)
-		dev_err(priv->dev, "error updating LED on port %d\n", port);
-}
-
 static int
 rtl8366rb_port_enable(struct dsa_switch *ds, int port,
 		      struct phy_device *phy)
@@ -1225,7 +1181,6 @@ rtl8366rb_port_enable(struct dsa_switch *ds, int port,
 	if (ret)
 		return ret;
 
-	rb8366rb_set_port_led(priv, port, true);
 	return 0;
 }
 
@@ -1240,8 +1195,6 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
 				 BIT(port));
 	if (ret)
 		return;
-
-	rb8366rb_set_port_led(priv, port, false);
 }
 
 static int
-- 
2.43.0

[RFC net-next 2/2] net: dsa: realtek: add LED drivers for rtl8366rb

[Luiz Angelo Daros de Luca]

This commit introduces LED drivers for rtl8366rb, allowing LEDs to be
described in the device tree using the same format as qca8k. Each port
can configure up to 4 LEDs.

If LEDs use the default state "keep", they will keep the default
behavior after a reset. Changing the brightness of one LED will cause
the entire LED group to switch to manually controlled LEDs. Once in this
mode, there is no way to revert to hardware-controlled LEDs (except by
resetting the switch).

Software triggers function as expected with manually controlled LEDs.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/rtl8366rb.c | 269 +++++++++++++++++++++++++---
 1 file changed, 245 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 874e04cf2e0d..f71cb59b8048 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -188,31 +188,21 @@
 	(4 * (led_group))
 #define RTL8366RB_LED_CTRL_MASK(led_group)	\
 	(0xf << RTL8366RB_LED_CTRL_OFFSET(led_group))
-#define RTL8366RB_LED_OFF			0x0
-#define RTL8366RB_LED_DUP_COL			0x1
-#define RTL8366RB_LED_LINK_ACT			0x2
-#define RTL8366RB_LED_SPD1000			0x3
-#define RTL8366RB_LED_SPD100			0x4
-#define RTL8366RB_LED_SPD10			0x5
-#define RTL8366RB_LED_SPD1000_ACT		0x6
-#define RTL8366RB_LED_SPD100_ACT		0x7
-#define RTL8366RB_LED_SPD10_ACT			0x8
-#define RTL8366RB_LED_SPD100_10_ACT		0x9
-#define RTL8366RB_LED_FIBER			0xa
-#define RTL8366RB_LED_AN_FAULT			0xb
-#define RTL8366RB_LED_LINK_RX			0xc
-#define RTL8366RB_LED_LINK_TX			0xd
-#define RTL8366RB_LED_MASTER			0xe
-#define RTL8366RB_LED_FORCE			0xf
 
 /* The RTL8366RB_LED_X_X registers are used to manually set the LED state only
  * when the corresponding LED group in RTL8366RB_LED_CTRL_REG is
  * RTL8366RB_LED_FORCE. Otherwise, it is ignored.
  */
 #define RTL8366RB_LED_0_1_CTRL_REG		0x0432
-#define RTL8366RB_LED_1_OFFSET			6
 #define RTL8366RB_LED_2_3_CTRL_REG		0x0433
-#define RTL8366RB_LED_3_OFFSET			6
+#define RTL8366RB_LED_X_X_CTRL_REG(led_group)	\
+	((led_group) <= 1 ? \
+		RTL8366RB_LED_0_1_CTRL_REG : \
+		RTL8366RB_LED_2_3_CTRL_REG)
+#define RTL8366RB_LED_0_X_CTRL_MASK		GENMASK(5, 0)
+#define RTL8366RB_LED_X_1_CTRL_MASK		GENMASK(11, 6)
+#define RTL8366RB_LED_2_X_CTRL_MASK		GENMASK(5, 0)
+#define RTL8366RB_LED_X_3_CTRL_MASK		GENMASK(11, 6)
 
 #define RTL8366RB_MIB_COUNT			33
 #define RTL8366RB_GLOBAL_MIB_COUNT		1
@@ -356,14 +346,44 @@
 #define RTL8366RB_GREEN_FEATURE_TX	BIT(0)
 #define RTL8366RB_GREEN_FEATURE_RX	BIT(2)
 
+enum rtl8366_led_mode {
+	RTL8366RB_LED_OFF		= 0x0,
+	RTL8366RB_LED_DUP_COL		= 0x1,
+	RTL8366RB_LED_LINK_ACT		= 0x2,
+	RTL8366RB_LED_SPD1000		= 0x3,
+	RTL8366RB_LED_SPD100		= 0x4,
+	RTL8366RB_LED_SPD10		= 0x5,
+	RTL8366RB_LED_SPD1000_ACT	= 0x6,
+	RTL8366RB_LED_SPD100_ACT	= 0x7,
+	RTL8366RB_LED_SPD10_ACT		= 0x8,
+	RTL8366RB_LED_SPD100_10_ACT	= 0x9,
+	RTL8366RB_LED_FIBER		= 0xa,
+	RTL8366RB_LED_AN_FAULT		= 0xb,
+	RTL8366RB_LED_LINK_RX		= 0xc,
+	RTL8366RB_LED_LINK_TX		= 0xd,
+	RTL8366RB_LED_MASTER		= 0xe,
+	RTL8366RB_LED_FORCE		= 0xf,
+
+	__RTL8366RB_LED_MAX
+};
+
+struct rtl8366rb_led {
+	u8 port_num;
+	u8 led_group;
+	struct realtek_priv *priv;
+	struct led_classdev cdev;
+};
+
 /**
  * struct rtl8366rb - RTL8366RB-specific data
  * @max_mtu: per-port max MTU setting
  * @pvid_enabled: if PVID is set for respective port
+ * @leds: per-port and per-ledgroup led info
  */
 struct rtl8366rb {
 	unsigned int max_mtu[RTL8366RB_NUM_PORTS];
 	bool pvid_enabled[RTL8366RB_NUM_PORTS];
+	struct rtl8366rb_led leds[RTL8366RB_NUM_PORTS][RTL8366RB_NUM_LEDGROUPS];
 };
 
 static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
@@ -806,6 +826,207 @@ static int rtl8366rb_jam_table(const struct rtl8366rb_jam_tbl_entry *jam_table,
 	return 0;
 }
 
+static int rb8366rb_set_ledgroup_mode(struct realtek_priv *priv,
+				      u8 led_group,
+				      enum rtl8366_led_mode mode)
+{
+	int ret;
+	u32 val;
+
+	val = mode << RTL8366RB_LED_CTRL_OFFSET(led_group);
+
+	ret = regmap_update_bits(priv->map,
+				 RTL8366RB_LED_CTRL_REG,
+				 RTL8366RB_LED_CTRL_MASK(led_group),
+				 val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static inline u32 rtl8366rb_led_group_port_mask(u8 led_group, u8 port)
+{
+	switch (led_group) {
+	case 0:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	case 1:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	case 2:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	case 3:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	default:
+		return 0;
+	}
+}
+
+static int rb8366rb_get_port_led(struct rtl8366rb_led *led, bool enable)
+{
+	struct realtek_priv *priv = led->priv;
+	u8 led_group = led->led_group;
+	u8 port_num = led->port_num;
+	int ret;
+	u32 val;
+
+	if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+		dev_err(priv->dev, "Invalid LED group %d for port %d",
+			led_group, port_num);
+		return -EINVAL;
+	}
+
+	ret = regmap_read(priv->map, RTL8366RB_LED_X_X_CTRL_REG(led_group),
+			  &val);
+	if (ret) {
+		dev_err(priv->dev, "error reading LED on port %d group %d\n",
+			led_group, port_num);
+		return ret;
+	}
+
+	return !!(val & rtl8366rb_led_group_port_mask(led_group, port_num));
+}
+
+static int rb8366rb_set_port_led(struct rtl8366rb_led *led, bool enable)
+{
+	struct realtek_priv *priv = led->priv;
+	u8 led_group = led->led_group;
+	u8 port_num = led->port_num;
+	int ret;
+
+	if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+		dev_err(priv->dev, "Invalid LED group %d for port %d",
+			led_group, port_num);
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(priv->map,
+				 RTL8366RB_LED_X_X_CTRL_REG(led_group),
+				 rtl8366rb_led_group_port_mask(led_group,
+							       port_num),
+				 enable ? 0xffff : 0);
+	if (ret) {
+		dev_err(priv->dev, "error updating LED on port %d group %d\n",
+			led_group, port_num);
+		return ret;
+	}
+
+	/* Change the LED group to manual controlled LEDs if required */
+	ret = rb8366rb_set_ledgroup_mode(priv, led_group, RTL8366RB_LED_FORCE);
+
+	if (ret) {
+		dev_err(priv->dev, "error updating LED GROUP group %d\n",
+			led_group);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int
+rtl8366rb_cled_brightness_set_blocking(struct led_classdev *ldev,
+				       enum led_brightness brightness)
+{
+	struct rtl8366rb_led *led = container_of(ldev, struct rtl8366rb_led,
+						 cdev);
+
+	return rb8366rb_set_port_led(led, brightness == LED_ON);
+}
+
+static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
+			       struct fwnode_handle *led_fwnode)
+{
+	struct rtl8366rb *rb = priv->chip_data;
+	struct led_init_data init_data = { };
+	struct rtl8366rb_led *led;
+	enum led_default_state state;
+	u32 led_group;
+	int ret;
+
+	ret = fwnode_property_read_u32(led_fwnode, "reg", &led_group);
+	if (ret)
+		return ret;
+
+	if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+		dev_warn(priv->dev, "Invalid LED reg %d defined for port %d",
+			 led_group, dp->index);
+		return -EINVAL;
+	}
+
+	led = &rb->leds[dp->index][led_group];
+	led->port_num = dp->index;
+	led->led_group = led_group;
+	led->priv = priv;
+
+	state = led_init_default_state_get(led_fwnode);
+	switch (state) {
+	case LEDS_DEFSTATE_ON:
+		led->cdev.brightness = 1;
+		rb8366rb_set_port_led(led, 1);
+		break;
+	case LEDS_DEFSTATE_KEEP:
+		led->cdev.brightness =
+			rb8366rb_get_port_led(led, 1);
+		break;
+	case LEDS_DEFSTATE_OFF:
+	default:
+		led->cdev.brightness = 0;
+		rb8366rb_set_port_led(led, 0);
+	}
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking =
+		rtl8366rb_cled_brightness_set_blocking;
+	init_data.fwnode = led_fwnode;
+	init_data.devname_mandatory = true;
+
+	init_data.devicename = kasprintf(GFP_KERNEL, "Realtek-%d:0%d:%d",
+					 dp->ds->index, dp->index, led_group);
+	if (!init_data.devicename)
+		return -ENOMEM;
+
+	ret = devm_led_classdev_register_ext(priv->dev, &led->cdev, &init_data);
+	if (ret) {
+		dev_warn(priv->dev, "Failed to init LED %d for port %d",
+			 led_group, dp->index);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rtl8366rb_setup_leds(struct realtek_priv *priv)
+{
+	struct device_node *leds_np, *led_np;
+	struct dsa_port *dp;
+	int ret;
+
+	dsa_switch_for_each_port(dp, priv->ds) {
+		if (!dp->dn)
+			continue;
+
+		leds_np = of_get_child_by_name(dp->dn, "leds");
+		if (!leds_np) {
+			dev_dbg(priv->dev, "No leds defined for port %d",
+				dp->index);
+			continue;
+		}
+
+		for_each_child_of_node(leds_np, led_np) {
+			ret = rtl8366rb_setup_led(priv, dp,
+						  of_fwnode_handle(led_np));
+			if (ret) {
+				of_node_put(led_np);
+				break;
+			}
+		}
+
+		of_node_put(leds_np);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 static int rtl8366rb_setup(struct dsa_switch *ds)
 {
 	struct realtek_priv *priv = ds->priv;
@@ -814,7 +1035,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	u32 chip_ver = 0;
 	u32 chip_id = 0;
 	int jam_size;
-	u32 val;
 	int ret;
 	int i;
 
@@ -1014,14 +1234,15 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 				   0);
 
 		for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
-			val = RTL8366RB_LED_OFF << RTL8366RB_LED_CTRL_OFFSET(i);
-			ret = regmap_update_bits(priv->map,
-						 RTL8366RB_LED_CTRL_REG,
-						 RTL8366RB_LED_CTRL_MASK(i),
-						 val);
+			ret = rb8366rb_set_ledgroup_mode(priv, i,
+							 RTL8366RB_LED_OFF);
 			if (ret)
 				return ret;
 		}
+	} else {
+		ret = rtl8366rb_setup_leds(priv);
+		if (ret)
+			return ret;
 	}
 
 	ret = rtl8366_reset_vlan(priv);
-- 
2.43.0

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Luiz Angelo Daros de Luca]

> The rtl8366rb switch family has 4 LED groups, with one LED from each
> group for each of its 6 ports. LEDs in this family can be controlled
> manually using a bitmap or triggered by hardware. It's important to note
> that hardware triggers are configured at the LED group level, meaning
> all LEDs in the same group share the same hardware triggers settings.
>
> The first part of this series involves dropping most of the existing
> code, as, except for disabling the LEDs, it was not working as expected.
> If not disabled, the LEDs will retain their default settings after a
> switch reset, which may be sufficient for many devices.
>
> The second part introduces the LED driver to control the switch LEDs
> from sysfs or device-tree. This driver still allows the LEDs to retain
> their default settings, but it will shift to the software-based OS LED
> triggers if any configuration is changed. Subsequently, the LEDs will
> operate as normal LEDs until the switch undergoes another reset.
>
> Netdev LED trigger supports offloading to hardware triggers.
> Unfortunately, this isn't possible with the current LED API for this
> switch family. When the hardware trigger is enabled, it applies to all
> LEDs in the LED group while the LED API decides to offload based on only
> the state of a single LED. To avoid inconsistency between LEDs,
> offloading would need to check if all LEDs in the group share the same
> compatible settings and atomically enable offload for all LEDs.

Hi Christian,

I tried to implement something close to your work with qca8k and LED
hw control. However, I couldn't find a solution that would work with
the existing API. The HW led configuration in realtek switches is
shared with all LEDs in a group. Before activating the hw control, all
LEDs in the same group must share the same netdev trigger config, use
the correct device and also use a compatible netdev trigger settings.
In order to check that, I would need to expose some internal netdev
trigger info that is only available through sysfs (and I believe sysfs
is not suitable to be used from the kernel). Even if I got all LEDs
with the correct settings, I would need to atomicly switch all LEDs to
use the hw control or, at least, I would need to stop all update jobs
because if the OS changes a LED brightness, it might be interpreted as
the OS disabling the hw control:

/*
...
* Deactivate hardware blink control by setting brightness to LED_OFF via
* the brightness_set() callback.
*
...
*/
int (*hw_control_set)(struct led_classdev *led_cdev,
 unsigned long flags);

Do you have any idea how to implement it?

BTW, during my tests with a single LED, ignoring the LED group
situation, I noticed that the OS was sending a brightness_set(LED_OFF)
after I changed the trigger to netdev, a moment after hw_control_set
was called. It doesn't make sense to enable hw control just to disable
it afterwards. The call came from set_brightness_delayed(). Maybe it
is because my test device is pretty slow and the previous trigger
event always gets queued. Touching any settings after that worked as
expected without the spurious brightness_set(LED_OFF). Did you see
something like this?

Regards,

Luiz

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Christian Marangi]

On Sat, Jan 06, 2024 at 04:47:10PM -0300, Luiz Angelo Daros de Luca wrote:
> > The rtl8366rb switch family has 4 LED groups, with one LED from each
> > group for each of its 6 ports. LEDs in this family can be controlled
> > manually using a bitmap or triggered by hardware. It's important to note
> > that hardware triggers are configured at the LED group level, meaning
> > all LEDs in the same group share the same hardware triggers settings.
> >
> > The first part of this series involves dropping most of the existing
> > code, as, except for disabling the LEDs, it was not working as expected.
> > If not disabled, the LEDs will retain their default settings after a
> > switch reset, which may be sufficient for many devices.
> >
> > The second part introduces the LED driver to control the switch LEDs
> > from sysfs or device-tree. This driver still allows the LEDs to retain
> > their default settings, but it will shift to the software-based OS LED
> > triggers if any configuration is changed. Subsequently, the LEDs will
> > operate as normal LEDs until the switch undergoes another reset.
> >
> > Netdev LED trigger supports offloading to hardware triggers.
> > Unfortunately, this isn't possible with the current LED API for this
> > switch family. When the hardware trigger is enabled, it applies to all
> > LEDs in the LED group while the LED API decides to offload based on only
> > the state of a single LED. To avoid inconsistency between LEDs,
> > offloading would need to check if all LEDs in the group share the same
> > compatible settings and atomically enable offload for all LEDs.
> 
> Hi Christian,
> 
> I tried to implement something close to your work with qca8k and LED
> hw control. However, I couldn't find a solution that would work with
> the existing API. The HW led configuration in realtek switches is
> shared with all LEDs in a group. Before activating the hw control, all
> LEDs in the same group must share the same netdev trigger config, use
> the correct device and also use a compatible netdev trigger settings.
> In order to check that, I would need to expose some internal netdev
> trigger info that is only available through sysfs (and I believe sysfs
> is not suitable to be used from the kernel). Even if I got all LEDs
> with the correct settings, I would need to atomicly switch all LEDs to
> use the hw control or, at least, I would need to stop all update jobs
> because if the OS changes a LED brightness, it might be interpreted as
> the OS disabling the hw control:
> 

Saddly we still don't have the concept of LED groups, but from what I
notice 99% of the time switch have limitation of HW control but single
LED can still be controlled separately.

With this limitation you can use the is_supported function and some priv
struct to enforce and reject unsupported configuration.
netdev trigger will then fallback to software in this case. (I assume on
real world scenario to have all the LED in the group to be set to the
common rule set resulting in is_supported never rejecting it)

Also consider this situation, it's the first LED touched that enables HW
control that drive everything. LED configuration are not enabled all at
once. You can totally introduce a priv struct that cache the current
modes and on the other LEDs make sure the requested mode match the cache
one.

And I guess this limitation should be printed and documented in DT.

> /*
> ...
> * Deactivate hardware blink control by setting brightness to LED_OFF via
> * the brightness_set() callback.
> *
> ...
> */
> int (*hw_control_set)(struct led_classdev *led_cdev,
>  unsigned long flags);
> 
> Do you have any idea how to implement it?
> 
> BTW, during my tests with a single LED, ignoring the LED group
> situation, I noticed that the OS was sending a brightness_set(LED_OFF)
> after I changed the trigger to netdev, a moment after hw_control_set
> was called. It doesn't make sense to enable hw control just to disable
> it afterwards. The call came from set_brightness_delayed(). Maybe it
> is because my test device is pretty slow and the previous trigger
> event always gets queued. Touching any settings after that worked as
> expected without the spurious brightness_set(LED_OFF). Did you see
> something like this?
>

Consider that brightness_set is called whatever a trigger is changed,
the logic is in the generic LED handling. Setting OFF and then enabling
hw control should not change a thing. In other driver tho I notice an
extra measure is needed to reset any HW control rule already applied by
default.

-- 
	Ansuel

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Luiz Angelo Daros de Luca]

> > Hi Christian,
> >
> > I tried to implement something close to your work with qca8k and LED
> > hw control. However, I couldn't find a solution that would work with
> > the existing API. The HW led configuration in realtek switches is
> > shared with all LEDs in a group. Before activating the hw control, all
> > LEDs in the same group must share the same netdev trigger config, use
> > the correct device and also use a compatible netdev trigger settings.
> > In order to check that, I would need to expose some internal netdev
> > trigger info that is only available through sysfs (and I believe sysfs
> > is not suitable to be used from the kernel). Even if I got all LEDs
> > with the correct settings, I would need to atomicly switch all LEDs to
> > use the hw control or, at least, I would need to stop all update jobs
> > because if the OS changes a LED brightness, it might be interpreted as
> > the OS disabling the hw control:
> >
>
> Saddly we still don't have the concept of LED groups, but from what I
> notice 99% of the time switch have limitation of HW control but single
> LED can still be controlled separately.

Individually, I can only turn them on/off. That is enough for software
control but not for hardware control. When you set a LED group to
blink on link activity, all LEDs will be affected.

> With this limitation you can use the is_supported function and some priv
> struct to enforce and reject unsupported configuration.
> netdev trigger will then fallback to software in this case. (I assume on
> real world scenario to have all the LED in the group to be set to the
> common rule set resulting in is_supported never rejecting it)

Maybe I wasn't clear enough about what the HW provides me. I have 4
16-bit registers:

REG1: a single blink rate used by all LEDs in all groups
REG2: configures the trigger for each group, 4-bit each, with one
special 4-bit value being "fixed", equivalent to "none" in Linux LED
trigger
REG3: bitmap to manually control LEDs in group 0 and 1 only when their
group trigger is configured as fixed.
REG3: bitmap to manually control LEDs in group 2 and 3 only when their
group trigger is configured as fixed.

And that's it.

I can keep track of the netdev trigger form calls to "is_supported
function". I can also check if all LEDs are still using the netdev
trigger. However, I cannot detect if the user changed the device to
something else not related to the corresponding port as the netdev
trigger will not check the compatibility if the device does not match.
I would still need to expose at least some of the netdev trigger
internal data.

> Also consider this situation, it's the first LED touched that enables HW
> control that drive everything. LED configuration are not enabled all at
> once. You can totally introduce a priv struct that cache the current
> modes and on the other LEDs make sure the requested mode match the cache
> one.

Considering that I can externally check that all LEDs have a netdev
trigger settings compatible with the HW control, once the last LED is
configured, I could return true for the hw_control_is_supported. When
hw_control_set is called, I could configure the hardware accordingly,
which would affect all LEDs in that group. However, the OS will still
use the software control for the other LEDs in that same group. That
way, once a netdev event turns off one LED, that message is the same
clue the LED driver receives to disable the hardware control. It will
undo the hardware change I just made. I could use
led_brightness_set(OFF) on those other LEDs during hw_control_set to
disable their software controlled triggers (actually changing the
trigger to "none"), but it might be a race condition of who stops the
other. And even then, the other LEDs will keep an inconsistent
configuration state, with "none" as their trigger.

I need:
1) expose the required info or allow an external caller to test a LED
configuration for compatibility (avoiding recursion).
2) something from hw_control_set() that stops the software triggers in
other LEDs without destroying their configuration.
3) something that could enable hw_control on those other LEDs

> And I guess this limitation should be printed and documented in DT.
>
> > /*
> > ...
> > * Deactivate hardware blink control by setting brightness to LED_OFF via
> > * the brightness_set() callback.
> > *
> > ...
> > */
> > int (*hw_control_set)(struct led_classdev *led_cdev,
> >  unsigned long flags);
> >
> > Do you have any idea how to implement it?
> >
> > BTW, during my tests with a single LED, ignoring the LED group
> > situation, I noticed that the OS was sending a brightness_set(LED_OFF)
> > after I changed the trigger to netdev, a moment after hw_control_set
> > was called. It doesn't make sense to enable hw control just to disable
> > it afterwards. The call came from set_brightness_delayed(). Maybe it
> > is because my test device is pretty slow and the previous trigger
> > event always gets queued. Touching any settings after that worked as
> > expected without the spurious brightness_set(LED_OFF). Did you see
> > something like this?
> >
>
> Consider that brightness_set is called whatever a trigger is changed,
> the logic is in the generic LED handling. Setting OFF and then enabling
> hw control should not change a thing. In other driver tho I notice an
> extra measure is needed to reset any HW control rule already applied by
> default.

It would be OK to call brightness_set(LED_OFF) if that is guaranteed
to happen before hw_control_set(). The problem is that the
brightness_set(LED_OFF) happens *after* hw_control_set() was called.
It looks like a race condition.

Regards,

Luiz

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Christian Marangi]

On Mon, Jan 08, 2024 at 02:47:22AM -0300, Luiz Angelo Daros de Luca wrote:
> > > Hi Christian,
> > >
> > > I tried to implement something close to your work with qca8k and LED
> > > hw control. However, I couldn't find a solution that would work with
> > > the existing API. The HW led configuration in realtek switches is
> > > shared with all LEDs in a group. Before activating the hw control, all
> > > LEDs in the same group must share the same netdev trigger config, use
> > > the correct device and also use a compatible netdev trigger settings.
> > > In order to check that, I would need to expose some internal netdev
> > > trigger info that is only available through sysfs (and I believe sysfs
> > > is not suitable to be used from the kernel). Even if I got all LEDs
> > > with the correct settings, I would need to atomicly switch all LEDs to
> > > use the hw control or, at least, I would need to stop all update jobs
> > > because if the OS changes a LED brightness, it might be interpreted as
> > > the OS disabling the hw control:
> > >
> >
> > Saddly we still don't have the concept of LED groups, but from what I
> > notice 99% of the time switch have limitation of HW control but single
> > LED can still be controlled separately.
>
> Individually, I can only turn them on/off. That is enough for software
> control but not for hardware control. When you set a LED group to
> blink on link activity, all LEDs will be affected.
>

Assuming we have the same 2005 datasheet, yes the LED situation is
complex for this switch. (If you have something better please link)

> > With this limitation you can use the is_supported function and some priv
> > struct to enforce and reject unsupported configuration.
> > netdev trigger will then fallback to software in this case. (I assume on
> > real world scenario to have all the LED in the group to be set to the
> > common rule set resulting in is_supported never rejecting it)
> 
> Maybe I wasn't clear enough about what the HW provides me. I have 4
> 16-bit registers:
> 
> REG1: a single blink rate used by all LEDs in all groups
> REG2: configures the trigger for each group, 4-bit each, with one
> special 4-bit value being "fixed", equivalent to "none" in Linux LED
> trigger
> REG3: bitmap to manually control LEDs in group 0 and 1 only when their
> group trigger is configured as fixed.
> REG3: bitmap to manually control LEDs in group 2 and 3 only when their
> group trigger is configured as fixed.
> 
> And that's it.
> 
> I can keep track of the netdev trigger form calls to "is_supported
> function". I can also check if all LEDs are still using the netdev
> trigger. However, I cannot detect if the user changed the device to
> something else not related to the corresponding port as the netdev
> trigger will not check the compatibility if the device does not match.
> I would still need to expose at least some of the netdev trigger
> internal data.
> 

We can make some assumption and use refcount tho. For very exotic
configuration it will always fallback to software (and make hw control
impossible) but for more generic one we can benefit of it.

- We can only enable HW control on the LED group.

This means that for the group we need to make sure that:
1. We have the correct device set to each LED
2. We have an acceptable mode requesterd.

With these 2 prereq, we can correctly enable HW control for the LED
group.

HW control is enable only IF the device netdev currently set match what
hw_control_get_device returns. With this, we can assume for HW control
request the correct netdev is always set.

We also use refcount to check how many LED are actually ""enabled"".
With this count we can understand if we can enable HW control for the
LED group or return false from is_supported.

And with HW control enabled, we would reject all kind of invalid
settings and print a warning to alert the user of the limitation and
maybe how to remove it.

> > Also consider this situation, it's the first LED touched that enables HW
> > control that drive everything. LED configuration are not enabled all at
> > once. You can totally introduce a priv struct that cache the current
> > modes and on the other LEDs make sure the requested mode match the cache
> > one.
> 
> Considering that I can externally check that all LEDs have a netdev
> trigger settings compatible with the HW control, once the last LED is
> configured, I could return true for the hw_control_is_supported. When
> hw_control_set is called, I could configure the hardware accordingly,
> which would affect all LEDs in that group. However, the OS will still
> use the software control for the other LEDs in that same group. That
> way, once a netdev event turns off one LED, that message is the same
> clue the LED driver receives to disable the hardware control. It will
> undo the hardware change I just made. I could use
> led_brightness_set(OFF) on those other LEDs during hw_control_set to
> disable their software controlled triggers (actually changing the
> trigger to "none"), but it might be a race condition of who stops the
> other. And even then, the other LEDs will keep an inconsistent
> configuration state, with "none" as their trigger.
> 
> I need:
> 1) expose the required info or allow an external caller to test a LED
> configuration for compatibility (avoiding recursion).
> 2) something from hw_control_set() that stops the software triggers in
> other LEDs without destroying their configuration.
> 3) something that could enable hw_control on those other LEDs
> 

I think it would be problematic for other LED to do changes. I need to
check how LED multicolor work... In a sense they are LED group so maybe
in LED core we have a way to group LED and share some info with the
others.

> > And I guess this limitation should be printed and documented in DT.
> >
> > > /*
> > > ...
> > > * Deactivate hardware blink control by setting brightness to LED_OFF via
> > > * the brightness_set() callback.
> > > *
> > > ...
> > > */
> > > int (*hw_control_set)(struct led_classdev *led_cdev,
> > >  unsigned long flags);
> > >
> > > Do you have any idea how to implement it?
> > >
> > > BTW, during my tests with a single LED, ignoring the LED group
> > > situation, I noticed that the OS was sending a brightness_set(LED_OFF)
> > > after I changed the trigger to netdev, a moment after hw_control_set
> > > was called. It doesn't make sense to enable hw control just to disable
> > > it afterwards. The call came from set_brightness_delayed(). Maybe it
> > > is because my test device is pretty slow and the previous trigger
> > > event always gets queued. Touching any settings after that worked as
> > > expected without the spurious brightness_set(LED_OFF). Did you see
> > > something like this?
> > >
> >
> > Consider that brightness_set is called whatever a trigger is changed,
> > the logic is in the generic LED handling. Setting OFF and then enabling
> > hw control should not change a thing. In other driver tho I notice an
> > extra measure is needed to reset any HW control rule already applied by
> > default.
> 
> It would be OK to call brightness_set(LED_OFF) if that is guaranteed
> to happen before hw_control_set(). The problem is that the
> brightness_set(LED_OFF) happens *after* hw_control_set() was called.
> It looks like a race condition.
> 

Totally require some further investigation, it seems strange tho that
the your system is that slow.

-- 
	Ansuel

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Luiz Angelo Daros de Luca]

> On Mon, Jan 08, 2024 at 02:47:22AM -0300, Luiz Angelo Daros de Luca wrote:
> > > > Hi Christian,
> > > >
> > > > I tried to implement something close to your work with qca8k and LED
> > > > hw control. However, I couldn't find a solution that would work with
> > > > the existing API. The HW led configuration in realtek switches is
> > > > shared with all LEDs in a group. Before activating the hw control, all
> > > > LEDs in the same group must share the same netdev trigger config, use
> > > > the correct device and also use a compatible netdev trigger settings.
> > > > In order to check that, I would need to expose some internal netdev
> > > > trigger info that is only available through sysfs (and I believe sysfs
> > > > is not suitable to be used from the kernel). Even if I got all LEDs
> > > > with the correct settings, I would need to atomicly switch all LEDs to
> > > > use the hw control or, at least, I would need to stop all update jobs
> > > > because if the OS changes a LED brightness, it might be interpreted as
> > > > the OS disabling the hw control:
> > > >
> > >
> > > Saddly we still don't have the concept of LED groups, but from what I
> > > notice 99% of the time switch have limitation of HW control but single
> > > LED can still be controlled separately.
> >
> > Individually, I can only turn them on/off. That is enough for software
> > control but not for hardware control. When you set a LED group to
> > blink on link activity, all LEDs will be affected.
> >
>
> Assuming we have the same 2005 datasheet, yes the LED situation is
> complex for this switch. (If you have something better please link)

I wouldn't say complex, but limited.

The manual for rtl8365mb
(https://cdn.jsdelivr.net/gh/libc0607/Realtek_switch_hacking@files/Realtek_Unmanaged_Switch_ProgrammingGuide.pdf,
page 64) shows the vendor API for controlling LEDs. It is similar for
both families.
You have only 3 functions:

int32 rtk_led_blinkRate_set(rtk_led_blink_rate_t blinkRate)
int32 rtk_led_groupConfig_set(rtk_led_group_t group, rtk_led_congig_t config)
int32 rtk_led_enable_set(rtk_led_group_t group, rtk_portmask_t portmask)

rtk_led_blinkRate_set sets the blink rate but it does not mention
group or port, so it is global.
rtk_led_groupConfig_set defines the HW trigger but only by group.
rtk_led_enable_set uses a mask to manually set a LED

It closely matches the register behavior.

> > > Also consider this situation, it's the first LED touched that enables HW
> > > control that drive everything. LED configuration are not enabled all at
> > > once. You can totally introduce a priv struct that cache the current
> > > modes and on the other LEDs make sure the requested mode match the cache
> > > one.
> >
> > Considering that I can externally check that all LEDs have a netdev
> > trigger settings compatible with the HW control, once the last LED is
> > configured, I could return true for the hw_control_is_supported. When
> > hw_control_set is called, I could configure the hardware accordingly,
> > which would affect all LEDs in that group. However, the OS will still
> > use the software control for the other LEDs in that same group. That
> > way, once a netdev event turns off one LED, that message is the same
> > clue the LED driver receives to disable the hardware control. It will
> > undo the hardware change I just made. I could use
> > led_brightness_set(OFF) on those other LEDs during hw_control_set to
> > disable their software controlled triggers (actually changing the
> > trigger to "none"), but it might be a race condition of who stops the
> > other. And even then, the other LEDs will keep an inconsistent
> > configuration state, with "none" as their trigger.
> >
> > I need:
> > 1) expose the required info or allow an external caller to test a LED
> > configuration for compatibility (avoiding recursion).
> > 2) something from hw_control_set() that stops the software triggers in
> > other LEDs without destroying their configuration.
> > 3) something that could enable hw_control on those other LEDs
> >
>
> I think it would be problematic for other LED to do changes. I need to
> check how LED multicolor work... In a sense they are LED group so maybe
> in LED core we have a way to group LED and share some info with the
> others.

That's the main issue. I can expose the needed info to check if all
LEDs agree in a compatible configuration. However, once that happens,
I must stop all sw control and enable the hw control. Something like:

lock a group of leds
for all leds
   if not devname is correct
     fallback to sw control
   if settings are not compatible
     fallback to sw control
   if settings is different from other LEDs
     fallback to sw control
for all leds
   stop sw control work
for all leds
   enable hw control
set the group hw trigger
unlock the group of leds

And I need something that would also work to disable hw control once
the first LED changes anything, breaking the compatibility.

> > > > BTW, during my tests with a single LED, ignoring the LED group
> > > > situation, I noticed that the OS was sending a brightness_set(LED_OFF)
> > > > after I changed the trigger to netdev, a moment after hw_control_set
> > > > was called. It doesn't make sense to enable hw control just to disable
> > > > it afterwards. The call came from set_brightness_delayed(). Maybe it
> > > > is because my test device is pretty slow and the previous trigger
> > > > event always gets queued. Touching any settings after that worked as
> > > > expected without the spurious brightness_set(LED_OFF). Did you see
> > > > something like this?
> > > >
> > >
> > > Consider that brightness_set is called whatever a trigger is changed,
> > > the logic is in the generic LED handling. Setting OFF and then enabling
> > > hw control should not change a thing. In other driver tho I notice an
> > > extra measure is needed to reset any HW control rule already applied by
> > > default.
> >
> > It would be OK to call brightness_set(LED_OFF) if that is guaranteed
> > to happen before hw_control_set(). The problem is that the
> > brightness_set(LED_OFF) happens *after* hw_control_set() was called.
> > It looks like a race condition.
> >
>
> Totally require some further investigation, it seems strange tho that
> the your system is that slow.

I got some stacks. When I change the trigger to netdev, I get 2 calls
to set the hw control:

[  625.601449] CPU: 0 PID: 2607 Comm: ash Not tainted 6.1.59 #0
[  625.607153] Stack : 809b0000 77e70000 00000000 800c1e80 00000431
00000004 00000000 00000000
[  625.615627]         80c45c74 80980000 80850000 806ffc2c 80e23b88
00000001 80c45c18 f51e58b8
[  625.624094]         00000000 00000000 806ffc2c 80c45b48 ffffefff
00000000 00000000 ffffffea
[  625.632561]         00000112 80c45b54 00000112 807c99c0 00000001
806ffc2c 809ec080 00000005
[  625.641029]         ffff7fff 80840000 809b0000 77e70000 00000018
8039785c 00000000 80980000
[  625.649495]         ...
[  625.651965] Call Trace:
[  625.654422] [<80066e4c>] show_stack+0x28/0xf0
[  625.658848] [<8061b800>] dump_stack_lvl+0x38/0x60
[  625.663599] [<8189b890>] rtl8366rb_cled_hw_control_set+0xdc/0xf8 [rtl8366]
[  625.670578] [<8041dfb0>] netdev_trig_notify+0x114/0x280
[  625.675867] [<80450d14>] call_netdevice_register_net_notifiers+0x54/0x104
[  625.682729] [<804542dc>] register_netdevice_notifier+0x98/0x130
[  625.688702] [<8041dbf8>] netdev_trig_activate+0x160/0x1b0
[  625.694152] [<8041b948>] led_trigger_set+0xf8/0x254
[  625.699070] [<8041c2a4>] led_trigger_write+0xd4/0x148
[  625.704163] [<8026966c>] sysfs_kf_bin_write+0x80/0xbc
[  625.709263] [<80268438>] kernfs_fop_write_iter+0x118/0x244
[  625.714801] [<801e787c>] vfs_write+0x1fc/0x3c0
[  625.719301] [<801e7bdc>] ksys_write+0x70/0x124
[  625.723791] [<8006e1e4>] syscall_common+0x34/0x58

[  625.761665] CPU: 0 PID: 2607 Comm: ash Not tainted 6.1.59 #0
[  625.767374] Stack : 809b0000 77e70000 00000000 800c1e80 00000431
00000004 00000000 00000000
[  625.775848]         80c45c74 80980000 80850000 806ffc2c 80e23b88
00000001 80c45c18 f51e58b8
[  625.784315]         00000000 00000000 806ffc2c 80c45b48 ffffefff
00000000 00000000 ffffffea
[  625.792782]         00000130 80c45b54 00000130 807c99c0 00000001
806ffc2c 809ec080 00000001
[  625.801249]         ffff7fff 80840000 809b0000 77e70000 00000018
8039785c 00000000 80980000
[  625.809716]         ...
[  625.812186] Call Trace:
[  625.814643] [<80066e4c>] show_stack+0x28/0xf0
[  625.819068] [<8061b800>] dump_stack_lvl+0x38/0x60
[  625.823820] [<8189b890>] rtl8366rb_cled_hw_control_set+0xdc/0xf8 [rtl8366]
[  625.830800] [<8041dfb0>] netdev_trig_notify+0x114/0x280
[  625.836088] [<80450d94>] call_netdevice_register_net_notifiers+0xd4/0x104
[  625.842950] [<804542dc>] register_netdevice_notifier+0x98/0x130
[  625.848923] [<8041dbf8>] netdev_trig_activate+0x160/0x1b0
[  625.854374] [<8041b948>] led_trigger_set+0xf8/0x254
[  625.859300] [<8041c2a4>] led_trigger_write+0xd4/0x148
[  625.864401] [<8026966c>] sysfs_kf_bin_write+0x80/0xbc
[  625.869502] [<80268438>] kernfs_fop_write_iter+0x118/0x244
[  625.875040] [<801e787c>] vfs_write+0x1fc/0x3c0
[  625.879539] [<801e7bdc>] ksys_write+0x70/0x124
[  625.884030] [<8006e1e4>] syscall_common+0x34/0x58

That is not really a problem but I my guess is that it is calling for
both NETDEV_REGISTER and NETDEV_UP as both eventually call
set_baseline_state(). Shouldn't we avoid one of them?

But after that, I get:

[  625.900712] CPU: 0 PID: 2626 Comm: kworker/0:2 Not tainted 6.1.59 #0
[  625.907154] Workqueue: events set_brightness_delayed
[  625.912178] Stack : 81000205 81a4ab40 817f9da4 800c1e80 8199e4e0
806ffc2c 807c0000 81a4ab00
[  625.920652]         81000200 00000000 80c08c5c 800c1f7c 80e21cc8
00000001 817f9d60 f37f8d2c
[  625.929119]         00000000 00000000 806ffc2c 817f9c78 ffffefff
00000000 00000000 ffffffea
[  625.937587]         00000148 817f9c84 00000148 807c99c0 00000001
806ffc2c 00000000 81000200
[  625.946054]         00000000 80c08c5c 81000205 81a4ab40 00000018
8039785c 00000000 80980000
[  625.954521]         ...
[  625.956990] Call Trace:
[  625.959448] [<80066e4c>] show_stack+0x28/0xf0
[  625.963873] [<8061b800>] dump_stack_lvl+0x38/0x60
[  625.968624] [<81899ec8>]
rtl8366rb_cled_brightness_set_blocking+0x68/0x88 [rtl8366]
[  625.976389] [<8041a018>] set_brightness_delayed+0x84/0xec
[  625.981833] [<8009f724>] process_one_work+0x254/0x484
[  625.986934] [<8009fed0>] worker_thread+0x178/0x5a4
[  625.991766] [<800a61a0>] kthread+0xec/0x114
[  625.996004] [<800620b8>] ret_from_kernel_thread+0x14/0x1c

This one is the issue. It turns off the LED, forcing me to disable the
hw control I just configured (twice). Unfortunately, a scheduled work
breaks the stack, not showing who actually requested it.

I only saw set_brightness_delayed being used by a work created in
led_init_core(), called by led_classdev_register_ext. That work might
be scheduled by led_set_brightness(). So, a call to
led_set_brightness() moments before setting netdev trigger might
reproduce the issue I see.

I'll try to get who scheduled the work and the stack from there. It
might already pinpoint the cause. Checking if the work is pending
during netdev trigger activation might also help. I'll also try to
flush (or cancel) the work before activating the new trigger. I just
don't know if I can flush (and that way, blocking) inside
led_set_brightness().

I just bricked my device and I couldn't continue the tests. I might
need to reserve an extra hour to fix that in the next days.

Regards,

Luiz

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Luiz Angelo Daros de Luca]

> I'll try to get who scheduled the work and the stack from there. It
> might already pinpoint the cause. Checking if the work is pending
> during netdev trigger activation might also help. I'll also try to
> flush (or cancel) the work before activating the new trigger. I just
> don't know if I can flush (and that way, blocking) inside
> led_set_brightness().
>

Hi Christian,

I got it. It was actually from the netdev trigger. During activation we have:

        /* Check if hw control is active by default on the LED.
         * Init already enabled mode in hw control.
         */
        if (supports_hw_control(led_cdev)) {
                dev = led_cdev->hw_control_get_device(led_cdev);
                if (dev) {
                        const char *name = dev_name(dev);

                        set_device_name(trigger_data, name, strlen(name));
                        trigger_data->hw_control = true;

                        rc = led_cdev->hw_control_get(led_cdev, &mode);
                        if (!rc)
                                trigger_data->mode = mode;
                }
        }

The set_device_name calls set_baseline_state() that, at this point,
will start to monitor the device using sw control
(trigger_data->hw_control is only set afterwards). In
set_baseline_state(), it will call led_set_brightness in most
codepaths (all of them if trigger_data->hw_control is false). With
link down (and other situations), it will call
led_set_brightness(led_cdev, LED_OFF). If that led_set_brightness
takes some time to be processed, it will happen after the hw control
was configured, undoing what it previously just did.

Is there any good reason to call set_device_name before the led mode
and hw_control are defined? Will this break anything?

diff --git a/drivers/leds/trigger/ledtrig-netdev.c
b/drivers/leds/trigger/ledtrig-netdev.c
index d76214fa9ad8..6f72d55c187a 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -572,12 +572,13 @@ static int netdev_trig_activate(struct
led_classdev *led_cdev)
               if (dev) {
                       const char *name = dev_name(dev);

-                       set_device_name(trigger_data, name, strlen(name));
                       trigger_data->hw_control = true;

                       rc = led_cdev->hw_control_get(led_cdev, &mode);
                       if (!rc)
                               trigger_data->mode = mode;
+
+                       set_device_name(trigger_data, name, strlen(name));
               }
       }

With this patch, it will not undo the trigger setting in hardware
anymore. However, it now calls the hw_control_set 3 times during
activation:

1) set_device_name
2) register_netdevice_notifier on NETDEV_REGISTER
3) register_netdevice_notifier on NETDEV_UP

Anyway, calling it multiple times doesn't break anything.

Regards,

Luiz

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Heiner Kallweit]

On 13.01.2024 15:24, Luiz Angelo Daros de Luca wrote:
>> I'll try to get who scheduled the work and the stack from there. It
>> might already pinpoint the cause. Checking if the work is pending
>> during netdev trigger activation might also help. I'll also try to
>> flush (or cancel) the work before activating the new trigger. I just
>> don't know if I can flush (and that way, blocking) inside
>> led_set_brightness().
>>
> 
> Hi Christian,
> 
> I got it. It was actually from the netdev trigger. During activation we have:
> 
>         /* Check if hw control is active by default on the LED.
>          * Init already enabled mode in hw control.
>          */
>         if (supports_hw_control(led_cdev)) {
>                 dev = led_cdev->hw_control_get_device(led_cdev);
>                 if (dev) {
>                         const char *name = dev_name(dev);
> 
>                         set_device_name(trigger_data, name, strlen(name));
>                         trigger_data->hw_control = true;
> 
>                         rc = led_cdev->hw_control_get(led_cdev, &mode);
>                         if (!rc)
>                                 trigger_data->mode = mode;
>                 }
>         }
> 
> The set_device_name calls set_baseline_state() that, at this point,
> will start to monitor the device using sw control
> (trigger_data->hw_control is only set afterwards). In
> set_baseline_state(), it will call led_set_brightness in most
> codepaths (all of them if trigger_data->hw_control is false). With
> link down (and other situations), it will call
> led_set_brightness(led_cdev, LED_OFF). If that led_set_brightness
> takes some time to be processed, it will happen after the hw control
> was configured, undoing what it previously just did.
> 
> Is there any good reason to call set_device_name before the led mode
> and hw_control are defined? Will this break anything?
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> b/drivers/leds/trigger/ledtrig-netdev.c
> index d76214fa9ad8..6f72d55c187a 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -572,12 +572,13 @@ static int netdev_trig_activate(struct
> led_classdev *led_cdev)
>                if (dev) {
>                        const char *name = dev_name(dev);
> 
> -                       set_device_name(trigger_data, name, strlen(name));
>                        trigger_data->hw_control = true;
> 
>                        rc = led_cdev->hw_control_get(led_cdev, &mode);
>                        if (!rc)
>                                trigger_data->mode = mode;
> +
> +                       set_device_name(trigger_data, name, strlen(name));
>                }
>        }
> 
> With this patch, it will not undo the trigger setting in hardware
> anymore. However, it now calls the hw_control_set 3 times during
> activation:
> 
This is addressed by the following patch, it should show up in linux-next
after the merge window.

https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/?h=for-leds-next-next&id=5df2b4ed10a4ea636bb5ace99712a7d0c6226a55

> 1) set_device_name
> 2) register_netdevice_notifier on NETDEV_REGISTER
> 3) register_netdevice_notifier on NETDEV_UP
> 
> Anyway, calling it multiple times doesn't break anything.
> 
> Regards,
> 
> Luiz
> 

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Luiz Angelo Daros de Luca]

> This is addressed by the following patch, it should show up in linux-next
> after the merge window.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/?h=for-leds-next-next&id=5df2b4ed10a4ea636bb5ace99712a7d0c6226a55

Hello Heiner,

Yes, exactly that. Thanks. I wish it had appeared some days ago. :-)

Regards,

Luiz

Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

[Linus Walleij]

Hi Luiz,

On Sat, Jan 6, 2024 at 7:47 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> The rtl8366rb switch family has 4 LED groups, with one LED from each
> group for each of its 6 ports. LEDs in this family can be controlled
> manually using a bitmap or triggered by hardware. It's important to note
> that hardware triggers are configured at the LED group level, meaning
> all LEDs in the same group share the same hardware triggers settings.
>
> The first part of this series involves dropping most of the existing
> code, as, except for disabling the LEDs, it was not working as expected.
> If not disabled, the LEDs will retain their default settings after a
> switch reset, which may be sufficient for many devices.
>
> The second part introduces the LED driver to control the switch LEDs
> from sysfs or device-tree. This driver still allows the LEDs to retain
> their default settings, but it will shift to the software-based OS LED
> triggers if any configuration is changed. Subsequently, the LEDs will
> operate as normal LEDs until the switch undergoes another reset.
>
> Netdev LED trigger supports offloading to hardware triggers.
> Unfortunately, this isn't possible with the current LED API for this
> switch family. When the hardware trigger is enabled, it applies to all
> LEDs in the LED group while the LED API decides to offload based on only
> the state of a single LED. To avoid inconsistency between LEDs,
> offloading would need to check if all LEDs in the group share the same
> compatible settings and atomically enable offload for all LEDs.

I think these patches look great, and the driver certainly look better
after these changes than before them so if you resend without
RFC, please feel free to add my:

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

HW triggers may be hard to implement but plain software control
is not bad either so this is already way better than what we had
before.

HW control can always be discussed and added later.

Yours,
Linus Walleij