[PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Marek Vasut]

Document netdev trigger specific netdev-trigger-mode property which
is used to configure the netdev trigger mode flags. Those mode flags
define events on which the LED acts upon when the hardware offload is
enabled. This is traditionally configured via sysfs, but that depends
on udev rules which are available either too late or never in case of
non-Linux systems.

For each LED with linux,default-trigger = "netdev" described in DT, this
optional netdev-trigger-mode property supplies the default configuration
of the PHY LED mode via DT. This property should be set to a subset of
TRIGGER_NETDEV_* flags.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Lee Jones <lee@kernel.org>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-leds@vger.kernel.org
---
 Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
index 3e8319e443392..1f1148fdf20c0 100644
--- a/Documentation/devicetree/bindings/leds/common.yaml
+++ b/Documentation/devicetree/bindings/leds/common.yaml
@@ -233,6 +233,12 @@ properties:
       Maximum timeout in microseconds after which the flash LED is turned off.
       Required for flash LED nodes with configurable timeout.
 
+  # Requires netdev trigger
+  netdev-trigger-mode:
+    description:
+      The netdev LED trigger default mode flags, use TRIGGER_NETDEV_ * flags.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
 allOf:
   - if:
       required:
-- 
2.45.2

[PATCH 2/2] leds: trigger: netdev: Introduce OF mode configuration using netdev-trigger-mode property

[Marek Vasut]

Introduce netdev trigger specific netdev-trigger-mode property which
is used to configure the netdev trigger mode flags. Those mode flags
define events on which the LED acts upon when the hardware offload is
enabled. This is traditionally configured via sysfs, but that depends
on userspace, which is available too late and makes ethernet PHY LEDs
not work e.g. when NFS root is being mounted.

For each LED with linux,default-trigger = "netdev" described in DT, the
optional netdev-trigger-mode property supplies the default configuration
of the PHY LED mode via DT. This property should be set to a subset of
TRIGGER_NETDEV_* flags.

For each LED with linux,default-trigger = "netdev" described in DT, the
netdev trigger is activated during kernel boot. The trigger is extended
the parse the netdev-trigger-mode property and set it as a default value
of trigger_data->mode.

It is not possible to immediately configure the LED mode, because the
interface to which the PHY and the LED is connected to might not be
attached to the PHY yet. The netdev_trig_notify() is called when the
PHY got attached to interface, extend netdev_trig_notify() to detect
the condition where the LED does have netdev trigger configured in DT
but the mode was not yet configured and configure the baseline mode
from the notifier. This can reuse most of set_device_name() except for
the rtnl_lock() which cannot be claimed in the notifier, so split the
relevant core code into set_device_name_locked() and call only the core
code.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Lee Jones <lee@kernel.org>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-leds@vger.kernel.org
---
 drivers/leds/trigger/ledtrig-netdev.c | 51 ++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index c15efe3e50780..20dfc9506c0a6 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/rtnetlink.h>
 #include <linux/timer.h>
@@ -256,19 +257,9 @@ static ssize_t device_name_show(struct device *dev,
 	return len;
 }
 
-static int set_device_name(struct led_netdev_data *trigger_data,
-			   const char *name, size_t size)
+static void set_device_name_locked(struct led_netdev_data *trigger_data,
+				  const char *name, size_t size)
 {
-	if (size >= IFNAMSIZ)
-		return -EINVAL;
-
-	cancel_delayed_work_sync(&trigger_data->work);
-
-	/*
-	 * Take RTNL lock before trigger_data lock to prevent potential
-	 * deadlock with netdev notifier registration.
-	 */
-	rtnl_lock();
 	mutex_lock(&trigger_data->lock);
 
 	if (trigger_data->net_dev) {
@@ -298,6 +289,24 @@ static int set_device_name(struct led_netdev_data *trigger_data,
 		set_baseline_state(trigger_data);
 
 	mutex_unlock(&trigger_data->lock);
+}
+
+static int set_device_name(struct led_netdev_data *trigger_data,
+			   const char *name, size_t size)
+{
+	if (size >= IFNAMSIZ)
+		return -EINVAL;
+
+	cancel_delayed_work_sync(&trigger_data->work);
+
+	/*
+	 * Take RTNL lock before trigger_data lock to prevent potential
+	 * deadlock with netdev notifier registration.
+	 */
+	rtnl_lock();
+
+	set_device_name_locked(trigger_data, name, size);
+
 	rtnl_unlock();
 
 	return 0;
@@ -579,6 +588,20 @@ static int netdev_trig_notify(struct notifier_block *nb,
 	    && evt != NETDEV_CHANGENAME)
 		return NOTIFY_DONE;
 
+	if (evt == NETDEV_REGISTER && !trigger_data->device_name[0] &&
+	    led_cdev->hw_control_get && led_cdev->hw_control_set &&
+	    led_cdev->hw_control_is_supported) {
+		struct device *ndev = led_cdev->hw_control_get_device(led_cdev);
+		if (ndev) {
+			const char *name = dev_name(ndev);
+
+			trigger_data->hw_control = true;
+
+			cancel_delayed_work_sync(&trigger_data->work);
+			set_device_name_locked(trigger_data, name, strlen(name));
+		}
+	}
+
 	if (!(dev == trigger_data->net_dev ||
 	      (evt == NETDEV_CHANGENAME && !strcmp(dev->name, trigger_data->device_name)) ||
 	      (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))
@@ -689,6 +712,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	struct led_netdev_data *trigger_data;
 	unsigned long mode = 0;
 	struct device *dev;
+	u32 val;
 	int rc;
 
 	trigger_data = kzalloc(sizeof(struct led_netdev_data), GFP_KERNEL);
@@ -706,7 +730,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	trigger_data->net_dev = NULL;
 	trigger_data->device_name[0] = 0;
 
-	trigger_data->mode = 0;
+	rc = of_property_read_u32(led_cdev->dev->of_node, "netdev-trigger-mode", &val);
+	trigger_data->mode = rc ? 0 : val;
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(NETDEV_LED_DEFAULT_INTERVAL));
 	trigger_data->last_activity = 0;
 
-- 
2.45.2

Re: [PATCH 2/2] leds: trigger: netdev: Introduce OF mode configuration using netdev-trigger-mode property

[Andrew Lunn]

> It is not possible to immediately configure the LED mode, because the
> interface to which the PHY and the LED is connected to might not be
> attached to the PHY yet. The netdev_trig_notify() is called when the
> PHY got attached to interface, extend netdev_trig_notify() to detect
> the condition where the LED does have netdev trigger configured in DT
> but the mode was not yet configured and configure the baseline mode
> from the notifier. This can reuse most of set_device_name() except for
> the rtnl_lock() which cannot be claimed in the notifier, so split the
> relevant core code into set_device_name_locked() and call only the core
> code.

Why cannot it be claimed? Because it has already been claimed? If so,
please add an ASSERT_RTNL() in the locked function to document
this. Or is there a lock inversion here?

> -static int set_device_name(struct led_netdev_data *trigger_data,
> -			   const char *name, size_t size)
> +static void set_device_name_locked(struct led_netdev_data *trigger_data,
> +				  const char *name, size_t size)
>  {
> -	if (size >= IFNAMSIZ)
> -		return -EINVAL;
> -

The code you cannot see in the context does:

        memcpy(trigger_data->device_name, name, size);

If we don't have this size check, is it possible to overrun the
buffer?

It might be better to split this patch into two, one doing the
refactoring of this function, and include an explanation of the
locking and why it is safe not to include this size check.

	Andrew

Re: [PATCH 2/2] leds: trigger: netdev: Introduce OF mode configuration using netdev-trigger-mode property

[Marek Vasut]

On 1/16/25 2:47 PM, Andrew Lunn wrote:
>> It is not possible to immediately configure the LED mode, because the
>> interface to which the PHY and the LED is connected to might not be
>> attached to the PHY yet. The netdev_trig_notify() is called when the
>> PHY got attached to interface, extend netdev_trig_notify() to detect
>> the condition where the LED does have netdev trigger configured in DT
>> but the mode was not yet configured and configure the baseline mode
>> from the notifier. This can reuse most of set_device_name() except for
>> the rtnl_lock() which cannot be claimed in the notifier, so split the
>> relevant core code into set_device_name_locked() and call only the core
>> code.
> 
> Why cannot it be claimed? Because it has already been claimed?

Yes

> If so,
> please add an ASSERT_RTNL() in the locked function to document
> this.

Added

> Or is there a lock inversion here?

Not to my knowledge, the rtnl lock should always be locked first and the 
trigger_data->lock mutex second.

>> -static int set_device_name(struct led_netdev_data *trigger_data,
>> -			   const char *name, size_t size)
>> +static void set_device_name_locked(struct led_netdev_data *trigger_data,
>> +				  const char *name, size_t size)
>>   {
>> -	if (size >= IFNAMSIZ)
>> -		return -EINVAL;
>> -
> 
> The code you cannot see in the context does:
> 
>          memcpy(trigger_data->device_name, name, size);
> 
> If we don't have this size check, is it possible to overrun the
> buffer?

Yes, good point, added.

> It might be better to split this patch into two, one doing the
> refactoring of this function, and include an explanation of the
> locking and why it is safe not to include this size check.
Does this still apply with the ASSERT_RTNL() in place and the check in 
the _locked() function reinstated ?

Re: [PATCH 2/2] leds: trigger: netdev: Introduce OF mode configuration using netdev-trigger-mode property

[Heiko Stübner]

Hi Marek,

Am Montag, 13. Januar 2025, 01:23:38 Mitteleuropäische Sommerzeit schrieb Marek Vasut:
> Introduce netdev trigger specific netdev-trigger-mode property which
> is used to configure the netdev trigger mode flags. Those mode flags
> define events on which the LED acts upon when the hardware offload is
> enabled. This is traditionally configured via sysfs, but that depends
> on userspace, which is available too late and makes ethernet PHY LEDs
> not work e.g. when NFS root is being mounted.
> 
> For each LED with linux,default-trigger = "netdev" described in DT, the
> optional netdev-trigger-mode property supplies the default configuration
> of the PHY LED mode via DT. This property should be set to a subset of
> TRIGGER_NETDEV_* flags.
> 
> For each LED with linux,default-trigger = "netdev" described in DT, the
> netdev trigger is activated during kernel boot. The trigger is extended
> the parse the netdev-trigger-mode property and set it as a default value
> of trigger_data->mode.
> 
> It is not possible to immediately configure the LED mode, because the
> interface to which the PHY and the LED is connected to might not be
> attached to the PHY yet. The netdev_trig_notify() is called when the
> PHY got attached to interface, extend netdev_trig_notify() to detect
> the condition where the LED does have netdev trigger configured in DT
> but the mode was not yet configured and configure the baseline mode
> from the notifier. This can reuse most of set_device_name() except for
> the rtnl_lock() which cannot be claimed in the notifier, so split the
> relevant core code into set_device_name_locked() and call only the core
> code.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 51 ++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index c15efe3e50780..20dfc9506c0a6 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  #include <linux/phy.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/timer.h>
> @@ -256,19 +257,9 @@ static ssize_t device_name_show(struct device *dev,
>  	return len;
>  }
>  
> -static int set_device_name(struct led_netdev_data *trigger_data,
> -			   const char *name, size_t size)
> +static void set_device_name_locked(struct led_netdev_data *trigger_data,
> +				  const char *name, size_t size)
>  {
> -	if (size >= IFNAMSIZ)
> -		return -EINVAL;
> -
> -	cancel_delayed_work_sync(&trigger_data->work);
> -
> -	/*
> -	 * Take RTNL lock before trigger_data lock to prevent potential
> -	 * deadlock with netdev notifier registration.
> -	 */
> -	rtnl_lock();
>  	mutex_lock(&trigger_data->lock);
>  
>  	if (trigger_data->net_dev) {
> @@ -298,6 +289,24 @@ static int set_device_name(struct led_netdev_data *trigger_data,
>  		set_baseline_state(trigger_data);
>  
>  	mutex_unlock(&trigger_data->lock);
> +}
> +
> +static int set_device_name(struct led_netdev_data *trigger_data,
> +			   const char *name, size_t size)
> +{
> +	if (size >= IFNAMSIZ)
> +		return -EINVAL;
> +
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
> +	/*
> +	 * Take RTNL lock before trigger_data lock to prevent potential
> +	 * deadlock with netdev notifier registration.
> +	 */
> +	rtnl_lock();
> +
> +	set_device_name_locked(trigger_data, name, size);
> +
>  	rtnl_unlock();
>  
>  	return 0;
> @@ -579,6 +588,20 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  	    && evt != NETDEV_CHANGENAME)
>  		return NOTIFY_DONE;
>  
> +	if (evt == NETDEV_REGISTER && !trigger_data->device_name[0] &&
> +	    led_cdev->hw_control_get && led_cdev->hw_control_set &&
> +	    led_cdev->hw_control_is_supported) {
> +		struct device *ndev = led_cdev->hw_control_get_device(led_cdev);
> +		if (ndev) {
> +			const char *name = dev_name(ndev);
> +
> +			trigger_data->hw_control = true;
> +
> +			cancel_delayed_work_sync(&trigger_data->work);
> +			set_device_name_locked(trigger_data, name, strlen(name));
> +		}
> +	}
> +

hmm, somehow this did not work for me as is, because the devicename
never makes it to the trigger. It seems because
	phy_led_hw_control_get_device()
of course only returns a device after the phy got attached somewhere and
NULL otherwise.

Testsystem is a Qnap TS233 NAS with RK3568 SoC using a dwmac on
6.16 kernel and 3 LEDs attached to the Motorcomm PHY.


On boot into regular Debian I see some separate steps, generating
events in the netdev trigger:

- dwmac probes and probes the phy
I see a number of expected NETDEV_REGISTER events


- systemd renames the interface to end0
[    6.502455] rk_gmac-dwmac fe2a0000.ethernet end0: renamed from eth0
[    6.509696] netdev_trig_notify evt 11

- dhclient end0
[   62.156033] rk_gmac-dwmac fe2a0000.ethernet end0: Register MEM_TYPE_PAGE_POOL RxQ-0
[   62.165292] phy_attach_direct
   ... only here phydev->attached_dev is set ...
[...]
[   62.240004] rk_gmac-dwmac fe2a0000.ethernet end0: configuring for phy/rgmii link mode
[   62.250517] netdev_trig_notify evt 1
[   65.315407] rk_gmac-dwmac fe2a0000.ethernet end0: Link is Up - 1Gbps/Full - flow control rx/tx
[   65.315415] netdev_trig_notify evt 4


changing from
	evt == NETDEV_REGISTER
to
	evt == NETDEV_UP
in the conditional up there, makes the device_name resolve work for me.
But right now, I have no clue if that is a bit no-no :-)

Or do we want a NETDEV_PHY_ATTACH (+_DETACH) event type ?


I'll try to poke things more
Heiko



>  	if (!(dev == trigger_data->net_dev ||
>  	      (evt == NETDEV_CHANGENAME && !strcmp(dev->name, trigger_data->device_name)) ||
>  	      (evt == NETDEV_REGISTER && !strcmp(dev->name, trigger_data->device_name))))
> @@ -689,6 +712,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>  	struct led_netdev_data *trigger_data;
>  	unsigned long mode = 0;
>  	struct device *dev;
> +	u32 val;
>  	int rc;
>  
>  	trigger_data = kzalloc(sizeof(struct led_netdev_data), GFP_KERNEL);
> @@ -706,7 +730,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>  	trigger_data->net_dev = NULL;
>  	trigger_data->device_name[0] = 0;
>  
> -	trigger_data->mode = 0;
> +	rc = of_property_read_u32(led_cdev->dev->of_node, "netdev-trigger-mode", &val);
> +	trigger_data->mode = rc ? 0 : val;
>  	atomic_set(&trigger_data->interval, msecs_to_jiffies(NETDEV_LED_DEFAULT_INTERVAL));
>  	trigger_data->last_activity = 0;
>  
> 

Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Andrew Lunn]

On Mon, Jan 13, 2025 at 01:23:37AM +0100, Marek Vasut wrote:
> Document netdev trigger specific netdev-trigger-mode property which
> is used to configure the netdev trigger mode flags. Those mode flags
> define events on which the LED acts upon when the hardware offload is
> enabled. This is traditionally configured via sysfs, but that depends
> on udev rules which are available either too late or never in case of
> non-Linux systems.
> 
> For each LED with linux,default-trigger = "netdev" described in DT, this
> optional netdev-trigger-mode property supplies the default configuration
> of the PHY LED mode via DT. This property should be set to a subset of
> TRIGGER_NETDEV_* flags.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Lee Jones <lee@kernel.org>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-leds@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> index 3e8319e443392..1f1148fdf20c0 100644
> --- a/Documentation/devicetree/bindings/leds/common.yaml
> +++ b/Documentation/devicetree/bindings/leds/common.yaml
> @@ -233,6 +233,12 @@ properties:
>        Maximum timeout in microseconds after which the flash LED is turned off.
>        Required for flash LED nodes with configurable timeout.
>  
> +  # Requires netdev trigger
> +  netdev-trigger-mode:
> +    description:
> +      The netdev LED trigger default mode flags, use TRIGGER_NETDEV_ * flags.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +
>  allOf:
>    - if:
>        required:
> -- 

An example would be good.

In order to be able to use TRIGGER_NETDEV_* i assume you are doing an
include which is outside of the usual dt-bindings directory. I don't
know of the DT Maintainers opinion on that.

	Andrew

Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Christian Marangi]

On Thu, Jan 16, 2025 at 02:32:13PM +0100, Andrew Lunn wrote:
> On Mon, Jan 13, 2025 at 01:23:37AM +0100, Marek Vasut wrote:
> > Document netdev trigger specific netdev-trigger-mode property which
> > is used to configure the netdev trigger mode flags. Those mode flags
> > define events on which the LED acts upon when the hardware offload is
> > enabled. This is traditionally configured via sysfs, but that depends
> > on udev rules which are available either too late or never in case of
> > non-Linux systems.
> > 
> > For each LED with linux,default-trigger = "netdev" described in DT, this
> > optional netdev-trigger-mode property supplies the default configuration
> > of the PHY LED mode via DT. This property should be set to a subset of
> > TRIGGER_NETDEV_* flags.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > ---
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: Christian Marangi <ansuelsmth@gmail.com>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: Heiner Kallweit <hkallweit1@gmail.com>
> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > Cc: Lee Jones <lee@kernel.org>
> > Cc: Lukasz Majewski <lukma@denx.de>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-leds@vger.kernel.org
> > ---
> >  Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> > index 3e8319e443392..1f1148fdf20c0 100644
> > --- a/Documentation/devicetree/bindings/leds/common.yaml
> > +++ b/Documentation/devicetree/bindings/leds/common.yaml
> > @@ -233,6 +233,12 @@ properties:
> >        Maximum timeout in microseconds after which the flash LED is turned off.
> >        Required for flash LED nodes with configurable timeout.
> >  
> > +  # Requires netdev trigger
> > +  netdev-trigger-mode:
> > +    description:
> > +      The netdev LED trigger default mode flags, use TRIGGER_NETDEV_ * flags.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +
> >  allOf:
> >    - if:
> >        required:
> > -- 
> 
> An example would be good.
> 
> In order to be able to use TRIGGER_NETDEV_* i assume you are doing an
> include which is outside of the usual dt-bindings directory. I don't
> know of the DT Maintainers opinion on that.
>

Well I think we can just move those include to dt-bindings or at worst
define new one (maybe less driver specific) and reference the internal
one... Should not be a problem in theory.

-- 
	Ansuel

Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Marek Vasut]

On 1/17/25 5:00 PM, Christian Marangi wrote:
> On Thu, Jan 16, 2025 at 02:32:13PM +0100, Andrew Lunn wrote:
>> On Mon, Jan 13, 2025 at 01:23:37AM +0100, Marek Vasut wrote:
>>> Document netdev trigger specific netdev-trigger-mode property which
>>> is used to configure the netdev trigger mode flags. Those mode flags
>>> define events on which the LED acts upon when the hardware offload is
>>> enabled. This is traditionally configured via sysfs, but that depends
>>> on udev rules which are available either too late or never in case of
>>> non-Linux systems.
>>>
>>> For each LED with linux,default-trigger = "netdev" described in DT, this
>>> optional netdev-trigger-mode property supplies the default configuration
>>> of the PHY LED mode via DT. This property should be set to a subset of
>>> TRIGGER_NETDEV_* flags.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Christian Marangi <ansuelsmth@gmail.com>
>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>> Cc: Lee Jones <lee@kernel.org>
>>> Cc: Lukasz Majewski <lukma@denx.de>
>>> Cc: Pavel Machek <pavel@ucw.cz>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: linux-leds@vger.kernel.org
>>> ---
>>>   Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
>>> index 3e8319e443392..1f1148fdf20c0 100644
>>> --- a/Documentation/devicetree/bindings/leds/common.yaml
>>> +++ b/Documentation/devicetree/bindings/leds/common.yaml
>>> @@ -233,6 +233,12 @@ properties:
>>>         Maximum timeout in microseconds after which the flash LED is turned off.
>>>         Required for flash LED nodes with configurable timeout.
>>>   
>>> +  # Requires netdev trigger
>>> +  netdev-trigger-mode:
>>> +    description:
>>> +      The netdev LED trigger default mode flags, use TRIGGER_NETDEV_ * flags.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +
>>>   allOf:
>>>     - if:
>>>         required:
>>> -- 
>>
>> An example would be good.
>>
>> In order to be able to use TRIGGER_NETDEV_* i assume you are doing an
>> include which is outside of the usual dt-bindings directory. I don't
>> know of the DT Maintainers opinion on that.

I am indeed.

> Well I think we can just move those include to dt-bindings or at worst
> define new one (maybe less driver specific) and reference the internal
> one... Should not be a problem in theory.
I can do that. I'll try to handle input from Andrew in the next few 
days, sorry for the slow replies.

Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Marek Vasut]

On 1/16/25 2:32 PM, Andrew Lunn wrote:
> On Mon, Jan 13, 2025 at 01:23:37AM +0100, Marek Vasut wrote:
>> Document netdev trigger specific netdev-trigger-mode property which
>> is used to configure the netdev trigger mode flags. Those mode flags
>> define events on which the LED acts upon when the hardware offload is
>> enabled. This is traditionally configured via sysfs, but that depends
>> on udev rules which are available either too late or never in case of
>> non-Linux systems.
>>
>> For each LED with linux,default-trigger = "netdev" described in DT, this
>> optional netdev-trigger-mode property supplies the default configuration
>> of the PHY LED mode via DT. This property should be set to a subset of
>> TRIGGER_NETDEV_* flags.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Christian Marangi <ansuelsmth@gmail.com>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>> Cc: Lee Jones <lee@kernel.org>
>> Cc: Lukasz Majewski <lukma@denx.de>
>> Cc: Pavel Machek <pavel@ucw.cz>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-leds@vger.kernel.org
>> ---
>>   Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
>> index 3e8319e443392..1f1148fdf20c0 100644
>> --- a/Documentation/devicetree/bindings/leds/common.yaml
>> +++ b/Documentation/devicetree/bindings/leds/common.yaml
>> @@ -233,6 +233,12 @@ properties:
>>         Maximum timeout in microseconds after which the flash LED is turned off.
>>         Required for flash LED nodes with configurable timeout.
>>   
>> +  # Requires netdev trigger
>> +  netdev-trigger-mode:
>> +    description:
>> +      The netdev LED trigger default mode flags, use TRIGGER_NETDEV_ * flags.
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +
>>   allOf:
>>     - if:
>>         required:
>> -- 
> 
> An example would be good.
> 
> In order to be able to use TRIGGER_NETDEV_* i assume you are doing an
> include which is outside of the usual dt-bindings directory. I don't
> know of the DT Maintainers opinion on that.
I think the question here is more ... shall I introduce new set of 
LED_NETDEV_nnn flags in e.g. include/dt-bindings/leds/common.h , so the 
flags won't be Linux netdev trigger specific ?

Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Heiko Stübner]

Hi,

Am Montag, 13. Januar 2025, 01:23:37 Mitteleuropäische Sommerzeit schrieb Marek Vasut:
> Document netdev trigger specific netdev-trigger-mode property which
> is used to configure the netdev trigger mode flags. Those mode flags
> define events on which the LED acts upon when the hardware offload is
> enabled. This is traditionally configured via sysfs, but that depends
> on udev rules which are available either too late or never in case of
> non-Linux systems.
> 
> For each LED with linux,default-trigger = "netdev" described in DT, this
> optional netdev-trigger-mode property supplies the default configuration
> of the PHY LED mode via DT. This property should be set to a subset of
> TRIGGER_NETDEV_* flags.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

while this is already half a year old, neither me nor b4 have found a
newer thread, so I hope this is still the most recent one to reply to.


> ---
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Lee Jones <lee@kernel.org>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-leds@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> index 3e8319e443392..1f1148fdf20c0 100644
> --- a/Documentation/devicetree/bindings/leds/common.yaml
> +++ b/Documentation/devicetree/bindings/leds/common.yaml
> @@ -233,6 +233,12 @@ properties:
>        Maximum timeout in microseconds after which the flash LED is turned off.
>        Required for flash LED nodes with configurable timeout.
>  
> +  # Requires netdev trigger
> +  netdev-trigger-mode:
> +    description:
> +      The netdev LED trigger default mode flags, use TRIGGER_NETDEV_ * flags.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +

as DT is supposed to be a hardware description, I think throwing arbitary
binary values around is not very readable - especially as the above would
be a combination of setting-bits for the TRIGGER_NETDEV_* things.

Instead I'd think using boolean dt props would reflect the binary "or"
way better and also keep all the bitwise nastiness out of the dt.

Also "netdev" is a Linux thing, and therefore also set in the
"linux,default-trigger" property, so I'd think any added netdev-props
should probably also have a linux,* prefix.


So in sum, I think the following might look better?

  linux,netdev-trigger-link:
    description:
      LED is lit on established link
    type: boolean

  linux,netdev-trigger-link-10:
    description:
      LED is lit on established link with 10MBit
    type: boolean

  linux,netdev-trigger-link-100:
    description:
      LED is lit on established link with 100MBit
    type: boolean

[...]

  linux,netdev-trigger-link-tx:
    description:
      LED is triggered when sending data
    type: boolean

  linux,netdev-trigger-link-rx:
    description:
      LED is triggered when receiving data
    type: boolean

[...]

for each element of the led_trigger_netdev_modes enum [0], with the node
then looking something like:

               leds {
                       #address-cells = <1>;
                       #size-cells = <0>;

                       /* Network LED on the front panel */
                       led@0 {
                               reg = <0>;
                               color = <LED_COLOR_ID_AMBER>;
                               function = LED_FUNCTION_LAN;
                               linux,default-trigger = "netdev";
                               linux,netdev-trigger-rx;
                               linux,netdev-trigger-tx;
                       };


Heiko


[0] https://elixir.bootlin.com/linux/v6.16/source/include/linux/leds.h#L603

Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Andrew Lunn]

> as DT is supposed to be a hardware description, I think throwing arbitary
> binary values around is not very readable - especially as the above would
> be a combination of setting-bits for the TRIGGER_NETDEV_* things.

I tend to agree with you. This is a tricky area, since it does appear
in most part to be configuration, not hardware.

What i think you should actually be describing is the label on the
case next to the LED.

Taking a random example:

https://www.downloads.netgear.com/files/GDC/Unmanaged_Switches/GS105Pv3_GS105PPv3_GS108LP_GS108PP_GS116LP_GS116PP_DS.pdf

The case says:

Left LED: Link/ACT mode
Green = Link at 1000M
Yellow = Link at 10/100M
Blink = ACT

Right: PoE Mode
Green = Powered
Yellow = Fault

So there is in fact four LEDs. Two of them are actually nothing to do
with netdev. This shows how flexible 'PHY' LEDs are, they can in fact
be used for anything. We currently don't have a PoE trigger, but it
should not be too hard to add.

For the two actual netdev LEDs, we need to describe the text of the
label. The naming of the DT property also needs to emphasise this is
the label. And if the case has no label, you should not be putting
properties in DT, the LEDs don't actually have any fixed meaning, it
is user space policy to set them.

As you said, there has not been any obvious progress on such a DT
binding for 6 months or more. I would probably interpret that as its
not particularly important. Maybe it actually makes more sense to work
on user space tools, to make it easier to configure these LEDs. Maybe
extend ethtool with a --get-led and --set-led. It is not always so
easy to following the symlinks in /sys, PHY LEDs appear in a different
place to MAC LEDs typical of switches etc. Once we have ethtool
support, it becomes easier to add entries to /etc/network/interfaces,
or udev rules, etc.

	Andrew

Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

[Marek Vasut]

On 8/9/25 6:29 PM, Andrew Lunn wrote:
>> as DT is supposed to be a hardware description, I think throwing arbitary
>> binary values around is not very readable - especially as the above would
>> be a combination of setting-bits for the TRIGGER_NETDEV_* things.
> 
> I tend to agree with you. This is a tricky area, since it does appear
> in most part to be configuration, not hardware.
> 
> What i think you should actually be describing is the label on the
> case next to the LED.
> 
> Taking a random example:
> 
> https://www.downloads.netgear.com/files/GDC/Unmanaged_Switches/GS105Pv3_GS105PPv3_GS108LP_GS108PP_GS116LP_GS116PP_DS.pdf
> 
> The case says:
> 
> Left LED: Link/ACT mode
> Green = Link at 1000M
> Yellow = Link at 10/100M
> Blink = ACT
> 
> Right: PoE Mode
> Green = Powered
> Yellow = Fault

This is meant to configure the netdev trigger, i.e. how Linux configures 
the PHY LED behavior (blink at 100/Full, not blink at 10/Full etc.), not 
the LED labels.

It is already possible to configure which trigger should be used for 
each LED in DT, it is also possible to configure trigger settings for 
some LED triggers in DT, but it is not possible to configure the netdev 
trigger in DT.

> So there is in fact four LEDs. Two of them are actually nothing to do
> with netdev. This shows how flexible 'PHY' LEDs are, they can in fact
> be used for anything. We currently don't have a PoE trigger, but it
> should not be too hard to add.
> 
> For the two actual netdev LEDs, we need to describe the text of the
> label. The naming of the DT property also needs to emphasise this is
> the label. And if the case has no label, you should not be putting
> properties in DT, the LEDs don't actually have any fixed meaning, it
> is user space policy to set them.
> 
> As you said, there has not been any obvious progress on such a DT
> binding for 6 months or more. I would probably interpret that as its
> not particularly important.

This is still in the backlog pipeline, which is too deep now, I'm sorry.

> Maybe it actually makes more sense to work
> on user space tools

The LEDs have to be configured before userspace is even started, that's 
what this patchset attempts to solve. Consider the state of the system 
before (proper) userspace that would configure the LEDs is even started, 
e.g. initramfs and other such edge cases. DT seems like the only place 
where this could be described, and it is done so for other LED triggers 
already.

Userspace configuration is solved by udev rules, that's not a problem.

-- 
Best regards,
Marek Vasut