[PATCH] leds: trigger: netdev: Check offload ability on interface up

[PATCH] leds: trigger: netdev: Check offload ability on interface up

[Marek Vasut]

The trigger_data->hw_control indicates whether the LED is controlled by HW
offload, i.e. the PHY. The trigger_data->hw_control = can_hw_control() is
currently called only from netdev_led_attr_store(), i.e. when writing any
sysfs attribute of the netdev trigger instance associated with a PHY LED.

The can_hw_control() calls validate_net_dev() which internally calls
led_cdev->hw_control_get_device(), which is phy_led_hw_control_get_device()
for PHY LEDs. The phy_led_hw_control_get_device() returns NULL if the PHY
is not attached.

At least in case of DWMAC (STM32MP, iMX8M, ...), the PHY device is attached
only when the interface is brought up and is detached again when the
interface is brought down. In case e.g. udev rules configure the netdev
LED trigger sysfs attributes before the interface is brought up, then when
the interface is brought up, the LEDs are not blinking.

This is because trigger_data->hw_control = can_hw_control() was called
when udev wrote the sysfs attribute files, before the interface was up,
so can_hw_control() resp. validate_net_dev() returned false, and the
trigger_data->hw_control = can_hw_control() was never called again to
update the trigger_data->hw_control content and let the offload take
over the LED blinking.

Call data->hw_control = can_hw_control() from netdev_trig_notify() to
update the offload capability of the LED when the UP notification arrives.
This makes the LEDs blink after the interface is brought up.

On STM32MP13xx with RTL8211F, it is enough to have the following udev rule
in place, boot the machine with cable plugged in, and the LEDs won't work
without this patch once the interface is brought up, even if they should:
"
ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="end0"
ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name}="end0"
"

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Daniel Golle <daniel@makrotopia.org>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Lee Jones <lee@kernel.org>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: kernel@dh-electronics.com
Cc: linux-leds@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: netdev@vger.kernel.org
---
 drivers/leds/trigger/ledtrig-netdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 4b0863db901a9..c15efe3e50780 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -605,6 +605,8 @@ static int netdev_trig_notify(struct notifier_block *nb,
 		trigger_data->net_dev = NULL;
 		break;
 	case NETDEV_UP:
+		trigger_data->hw_control = can_hw_control(trigger_data);
+		fallthrough;
 	case NETDEV_CHANGE:
 		get_device_state(trigger_data);
 		/* Refresh link_speed visibility */
-- 
2.45.2

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Andrew Lunn]

On Tue, Oct 01, 2024 at 04:45:23AM +0200, Marek Vasut wrote:
> The trigger_data->hw_control indicates whether the LED is controlled by HW
> offload, i.e. the PHY. The trigger_data->hw_control = can_hw_control() is
> currently called only from netdev_led_attr_store(), i.e. when writing any
> sysfs attribute of the netdev trigger instance associated with a PHY LED.
> 
> The can_hw_control() calls validate_net_dev() which internally calls
> led_cdev->hw_control_get_device(), which is phy_led_hw_control_get_device()
> for PHY LEDs. The phy_led_hw_control_get_device() returns NULL if the PHY
> is not attached.
> 
> At least in case of DWMAC (STM32MP, iMX8M, ...), the PHY device is attached
> only when the interface is brought up and is detached again when the
> interface is brought down. In case e.g. udev rules configure the netdev
> LED trigger sysfs attributes before the interface is brought up, then when
> the interface is brought up, the LEDs are not blinking.
> 
> This is because trigger_data->hw_control = can_hw_control() was called
> when udev wrote the sysfs attribute files, before the interface was up,
> so can_hw_control() resp. validate_net_dev() returned false, and the
> trigger_data->hw_control = can_hw_control() was never called again to
> update the trigger_data->hw_control content and let the offload take
> over the LED blinking.
> 
> Call data->hw_control = can_hw_control() from netdev_trig_notify() to
> update the offload capability of the LED when the UP notification arrives.
> This makes the LEDs blink after the interface is brought up.

Have you run this code with lockdep enabled? There have been some
deadlocks, or potential deadlocks in this area.

> 
> On STM32MP13xx with RTL8211F, it is enough to have the following udev rule
> in place, boot the machine with cable plugged in, and the LEDs won't work
> without this patch once the interface is brought up, even if they should:
> "
> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="end0"
> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name}="end0"
> "

Nice use of udev. I had not thought about using it for this.

	Andrew

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Marek Vasut]

On 10/3/24 1:21 AM, Andrew Lunn wrote:
> On Tue, Oct 01, 2024 at 04:45:23AM +0200, Marek Vasut wrote:
>> The trigger_data->hw_control indicates whether the LED is controlled by HW
>> offload, i.e. the PHY. The trigger_data->hw_control = can_hw_control() is
>> currently called only from netdev_led_attr_store(), i.e. when writing any
>> sysfs attribute of the netdev trigger instance associated with a PHY LED.
>>
>> The can_hw_control() calls validate_net_dev() which internally calls
>> led_cdev->hw_control_get_device(), which is phy_led_hw_control_get_device()
>> for PHY LEDs. The phy_led_hw_control_get_device() returns NULL if the PHY
>> is not attached.
>>
>> At least in case of DWMAC (STM32MP, iMX8M, ...), the PHY device is attached
>> only when the interface is brought up and is detached again when the
>> interface is brought down. In case e.g. udev rules configure the netdev
>> LED trigger sysfs attributes before the interface is brought up, then when
>> the interface is brought up, the LEDs are not blinking.
>>
>> This is because trigger_data->hw_control = can_hw_control() was called
>> when udev wrote the sysfs attribute files, before the interface was up,
>> so can_hw_control() resp. validate_net_dev() returned false, and the
>> trigger_data->hw_control = can_hw_control() was never called again to
>> update the trigger_data->hw_control content and let the offload take
>> over the LED blinking.
>>
>> Call data->hw_control = can_hw_control() from netdev_trig_notify() to
>> update the offload capability of the LED when the UP notification arrives.
>> This makes the LEDs blink after the interface is brought up.
> 
> Have you run this code with lockdep enabled? There have been some
> deadlocks, or potential deadlocks in this area.

Now I did on next 20241002 , no lockdep splat reported .

>> On STM32MP13xx with RTL8211F, it is enough to have the following udev rule
>> in place, boot the machine with cable plugged in, and the LEDs won't work
>> without this patch once the interface is brought up, even if they should:
>> "
>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="end0"
>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name}="end0"
>> "
> 
> Nice use of udev. I had not thought about using it for this.
Is there some other way to configure the netdev-triggered PHY LEDs ?
I still feel the udev rule is somewhat brittle and fragile, and also not 
available early enough for default PHY LED configuration, i.e. the LEDs 
are not blinking when I use e.g. ip=/nfsroot= when booting from NFS root 
until the userspace started, which is not nice. The only alternative I 
can imagine is default configuration in DT, which was already rejected a 
few years ago.

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Andrew Lunn]

> > Nice use of udev. I had not thought about using it for this.

> Is there some other way to configure the netdev-triggered PHY LEDs ?
> I still feel the udev rule is somewhat brittle and fragile, and also not
> available early enough for default PHY LED configuration, i.e. the LEDs are
> not blinking when I use e.g. ip=/nfsroot= when booting from NFS root until
> the userspace started, which is not nice. The only alternative I can imagine
> is default configuration in DT, which was already rejected a few years ago.

Device tree is the only early way i can think of, especially for NFS
root.

What has clearly been rejected is each vendor having their own DT
binding. But i think we might have more success with one generic
binding for all MAC/PHY LEDs.

The way i was thinking about it, was to describe the label on the
front panel. That is hardware you are describing, so fits DT.

We are clearly in the grey area for DT, so i can understand some push
back on this from the DT Maintainers.

	Andrew

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Marek Vasut]

On 10/3/24 2:05 PM, Andrew Lunn wrote:
>>> Nice use of udev. I had not thought about using it for this.
> 
>> Is there some other way to configure the netdev-triggered PHY LEDs ?
>> I still feel the udev rule is somewhat brittle and fragile, and also not
>> available early enough for default PHY LED configuration, i.e. the LEDs are
>> not blinking when I use e.g. ip=/nfsroot= when booting from NFS root until
>> the userspace started, which is not nice. The only alternative I can imagine
>> is default configuration in DT, which was already rejected a few years ago.
> 
> Device tree is the only early way i can think of, especially for NFS
> root.
> 
> What has clearly been rejected is each vendor having their own DT
> binding. But i think we might have more success with one generic
> binding for all MAC/PHY LEDs.

Right now I have this (for one of the PHY LEDs):

led@0 {
         reg = <0>;
         color = <LED_COLOR_ID_GREEN>;
         function = LED_FUNCTION_WAN;
         linux,default-trigger = "netdev";
};

What about be useful is to set the link_10/100/1000 and rx/tx flags here 
somehow. It cannot be 'function' because that is already used to define 
the port purpose.

Maybe something like 'led-pattern' property used by 'pattern' trigger 
would work ? Some sort of "led-netdev-flags = LINK10 | LINK100;" ?

> The way i was thinking about it, was to describe the label on the
> front panel. That is hardware you are describing, so fits DT.
> 
> We are clearly in the grey area for DT, so i can understand some push
> back on this from the DT Maintainers.

It would be a policy, yes.

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Marek Vasut]

On 10/3/24 2:47 AM, Marek Vasut wrote:

Hello again,

>>> On STM32MP13xx with RTL8211F, it is enough to have the following udev 
>>> rule
>>> in place, boot the machine with cable plugged in, and the LEDs won't 
>>> work
>>> without this patch once the interface is brought up, even if they 
>>> should:
>>> "
>>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", 
>>> ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", 
>>> ATTR{link_1000}="1", ATTR{device_name}="end0"
>>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", 
>>> ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name} 
>>> ="end0"
>>> "
>>
>> Nice use of udev. I had not thought about using it for this.

I might have been a bit too hasty with this. The following is only a 
quick preliminary FYI, I am still investigating the details.

I observe on 6.18-rc6 (ST STM32MP13xx , so STM32 DWMAC ethernet, and 
RTL8211F PHY), that if I use the these udev rules (SoC has two MACs, 
there are two rules for each MAC, and 2 rules for each of two LEDs on 
each MAC PHY , therefore four rules in total ; the rules for both MACs 
are identical):

"
ACTION=="add|change", SUBSYSTEM=="leds", 
KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", 
ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", 
ATTR{device_name}="ethsom0"
ACTION=="add|change", SUBSYSTEM=="leds", 
KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", 
ATTR{tx}="1", ATTR{device_name}="ethsom0"

ACTION=="add|change", SUBSYSTEM=="leds", 
KERNEL=="stmmac-1:01:green:lan", ATTR{trigger}="netdev", 
ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", 
ATTR{device_name}="ethsom1"
ACTION=="add|change", SUBSYSTEM=="leds", 
KERNEL=="stmmac-1:01:yellow:lan", ATTR{trigger}="netdev", ATTR{rx}="1", 
ATTR{tx}="1", ATTR{device_name}="ethsom1"
"

I get this backtrace. Notice the "sysfs: cannot create duplicate 
filename ..." part , I suspect there is some subtle race condition ?

"
sysfs: cannot create duplicate filename 
'/devices/platform/soc/5c007000.bus/5800e000.ethernet/mdio_bus/stmmac-1/stmmac-1:01/leds/stmmac-1:01:green:lan/link_10'
CPU: 0 UID: 0 PID: 153 Comm: (udev-worker) Not tainted 6.18.0-rc6 #1 
PREEMPT
Hardware name: STM32 (Device Tree Support)
Call trace:
  unwind_backtrace from show_stack+0x18/0x1c
  show_stack from dump_stack_lvl+0x54/0x68
  dump_stack_lvl from sysfs_warn_dup+0x58/0x6c
  sysfs_warn_dup from sysfs_add_file_mode_ns+0xf0/0x130
  sysfs_add_file_mode_ns from internal_create_group+0x344/0x480
  internal_create_group from internal_create_groups+0x48/0x6c
  internal_create_groups from led_trigger_set+0x1e4/0x278
  led_trigger_set from led_trigger_write+0xe0/0x118
  led_trigger_write from sysfs_kf_bin_write+0x98/0xa0
  sysfs_kf_bin_write from kernfs_fop_write_iter+0x14c/0x198
  kernfs_fop_write_iter from vfs_write+0x170/0x1d4
  vfs_write from ksys_write+0x7c/0xd0
  ksys_write from ret_fast_syscall+0x0/0x54
Exception stack(0xedbf1fa8 to 0xedbf1ff0)
1fa0:                   00000006 bec4476c 00000015 bec4476c 00000006 
00000001
1fc0: 00000006 bec4476c 000e7698 00000004 00000006 fffffff7 00000000 
000d1710
1fe0: 00000004 bec44578 b6c34397 b6bb15e6
leds stmmac-1:01:green:lan: Failed to add trigger attributes
"

If I find out more, I will get back to this thread.

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Marek Vasut]

On 11/17/25 12:18 PM, Marek Vasut wrote:

Hello one more time,

>>>> On STM32MP13xx with RTL8211F, it is enough to have the following 
>>>> udev rule
>>>> in place, boot the machine with cable plugged in, and the LEDs won't 
>>>> work
>>>> without this patch once the interface is brought up, even if they 
>>>> should:
>>>> "
>>>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", 
>>>> ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", 
>>>> ATTR{link_1000}="1", ATTR{device_name}="end0"
>>>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", 
>>>> ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", 
>>>> ATTR{device_name} ="end0"
>>>> "
>>>
>>> Nice use of udev. I had not thought about using it for this.
> 
> I might have been a bit too hasty with this. The following is only a 
> quick preliminary FYI, I am still investigating the details.
> 
> I observe on 6.18-rc6 (ST STM32MP13xx , so STM32 DWMAC ethernet, and 
> RTL8211F PHY), that if I use the these udev rules (SoC has two MACs, 
> there are two rules for each MAC, and 2 rules for each of two LEDs on 
> each MAC PHY , therefore four rules in total ; the rules for both MACs 
> are identical):
> 
> "
> ACTION=="add|change", SUBSYSTEM=="leds", 
> KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10} 
> ="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="ethsom0"
> ACTION=="add|change", SUBSYSTEM=="leds", 
> KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", 
> ATTR{tx}="1", ATTR{device_name}="ethsom0"
> 
> ACTION=="add|change", SUBSYSTEM=="leds", 
> KERNEL=="stmmac-1:01:green:lan", ATTR{trigger}="netdev", ATTR{link_10} 
> ="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="ethsom1"
> ACTION=="add|change", SUBSYSTEM=="leds", 
> KERNEL=="stmmac-1:01:yellow:lan", ATTR{trigger}="netdev", ATTR{rx}="1", 
> ATTR{tx}="1", ATTR{device_name}="ethsom1"
> "
> 
> I get this backtrace. Notice the "sysfs: cannot create duplicate 
> filename ..." part , I suspect there is some subtle race condition ?
> 
> "
> sysfs: cannot create duplicate filename '/devices/platform/ 
> soc/5c007000.bus/5800e000.ethernet/mdio_bus/stmmac-1/stmmac-1:01/leds/ 
> stmmac-1:01:green:lan/link_10'
> CPU: 0 UID: 0 PID: 153 Comm: (udev-worker) Not tainted 6.18.0-rc6 #1 
> PREEMPT
> Hardware name: STM32 (Device Tree Support)
> Call trace:
>   unwind_backtrace from show_stack+0x18/0x1c
>   show_stack from dump_stack_lvl+0x54/0x68
>   dump_stack_lvl from sysfs_warn_dup+0x58/0x6c
>   sysfs_warn_dup from sysfs_add_file_mode_ns+0xf0/0x130
>   sysfs_add_file_mode_ns from internal_create_group+0x344/0x480
>   internal_create_group from internal_create_groups+0x48/0x6c
>   internal_create_groups from led_trigger_set+0x1e4/0x278
>   led_trigger_set from led_trigger_write+0xe0/0x118
>   led_trigger_write from sysfs_kf_bin_write+0x98/0xa0
>   sysfs_kf_bin_write from kernfs_fop_write_iter+0x14c/0x198
>   kernfs_fop_write_iter from vfs_write+0x170/0x1d4
>   vfs_write from ksys_write+0x7c/0xd0
>   ksys_write from ret_fast_syscall+0x0/0x54
> Exception stack(0xedbf1fa8 to 0xedbf1ff0)
> 1fa0:                   00000006 bec4476c 00000015 bec4476c 00000006 
> 00000001
> 1fc0: 00000006 bec4476c 000e7698 00000004 00000006 fffffff7 00000000 
> 000d1710
> 1fe0: 00000004 bec44578 b6c34397 b6bb15e6
> leds stmmac-1:01:green:lan: Failed to add trigger attributes
> "
> 
> If I find out more, I will get back to this thread.

I've been tracking it all the way to kernfs, but so far without much 
success.

I found commit 52c47742f79d ("leds: triggers: send uevent when changing 
triggers") which indicates the udev rules above are likely wrong, but 
they still shouldn't corrupt sysfs the way they do, right ?

If you have any hint how to find out what is actually going on, I would 
be much grateful. I already tried KASAN on this and LOCKDEP, but neither 
triggers. I was also adding a lot of trace_printk() into netdev LED 
trigger, but all I could find is the link_* attributes are removed, then 
added again, and the kernel complains the link_10 attribute already exists.

I also noticed that if I try to list /sys/...stmmac-1:01:green:lan/ 
directory after the splat with netdev trigger set, the result of the 
listing is not always the same, sometimes there are the netdev trigger 
attributes, sometimes not, and sometimes they are corrupted.

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Andrew Lunn]

On Tue, Oct 01, 2024 at 04:45:23AM +0200, Marek Vasut wrote:
> The trigger_data->hw_control indicates whether the LED is controlled by HW
> offload, i.e. the PHY. The trigger_data->hw_control = can_hw_control() is
> currently called only from netdev_led_attr_store(), i.e. when writing any
> sysfs attribute of the netdev trigger instance associated with a PHY LED.
> 
> The can_hw_control() calls validate_net_dev() which internally calls
> led_cdev->hw_control_get_device(), which is phy_led_hw_control_get_device()
> for PHY LEDs. The phy_led_hw_control_get_device() returns NULL if the PHY
> is not attached.
> 
> At least in case of DWMAC (STM32MP, iMX8M, ...), the PHY device is attached
> only when the interface is brought up and is detached again when the
> interface is brought down. In case e.g. udev rules configure the netdev
> LED trigger sysfs attributes before the interface is brought up, then when
> the interface is brought up, the LEDs are not blinking.
> 
> This is because trigger_data->hw_control = can_hw_control() was called
> when udev wrote the sysfs attribute files, before the interface was up,
> so can_hw_control() resp. validate_net_dev() returned false, and the
> trigger_data->hw_control = can_hw_control() was never called again to
> update the trigger_data->hw_control content and let the offload take
> over the LED blinking.
> 
> Call data->hw_control = can_hw_control() from netdev_trig_notify() to
> update the offload capability of the LED when the UP notification arrives.
> This makes the LEDs blink after the interface is brought up.
> 
> On STM32MP13xx with RTL8211F, it is enough to have the following udev rule
> in place, boot the machine with cable plugged in, and the LEDs won't work
> without this patch once the interface is brought up, even if they should:
> "
> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="end0"
> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name}="end0"
> "
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Marek Vasut]

On 10/3/24 2:06 PM, Andrew Lunn wrote:
> On Tue, Oct 01, 2024 at 04:45:23AM +0200, Marek Vasut wrote:
>> The trigger_data->hw_control indicates whether the LED is controlled by HW
>> offload, i.e. the PHY. The trigger_data->hw_control = can_hw_control() is
>> currently called only from netdev_led_attr_store(), i.e. when writing any
>> sysfs attribute of the netdev trigger instance associated with a PHY LED.
>>
>> The can_hw_control() calls validate_net_dev() which internally calls
>> led_cdev->hw_control_get_device(), which is phy_led_hw_control_get_device()
>> for PHY LEDs. The phy_led_hw_control_get_device() returns NULL if the PHY
>> is not attached.
>>
>> At least in case of DWMAC (STM32MP, iMX8M, ...), the PHY device is attached
>> only when the interface is brought up and is detached again when the
>> interface is brought down. In case e.g. udev rules configure the netdev
>> LED trigger sysfs attributes before the interface is brought up, then when
>> the interface is brought up, the LEDs are not blinking.
>>
>> This is because trigger_data->hw_control = can_hw_control() was called
>> when udev wrote the sysfs attribute files, before the interface was up,
>> so can_hw_control() resp. validate_net_dev() returned false, and the
>> trigger_data->hw_control = can_hw_control() was never called again to
>> update the trigger_data->hw_control content and let the offload take
>> over the LED blinking.
>>
>> Call data->hw_control = can_hw_control() from netdev_trig_notify() to
>> update the offload capability of the LED when the UP notification arrives.
>> This makes the LEDs blink after the interface is brought up.
>>
>> On STM32MP13xx with RTL8211F, it is enough to have the following udev rule
>> in place, boot the machine with cable plugged in, and the LEDs won't work
>> without this patch once the interface is brought up, even if they should:
>> "
>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="end0"
>> ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name}="end0"
>> "
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Is there anything blocking this patch from being picked up ?

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Andrew Lunn]

On Fri, Dec 13, 2024 at 11:15:09PM +0100, Marek Vasut wrote:
> On 10/3/24 2:06 PM, Andrew Lunn wrote:
> > On Tue, Oct 01, 2024 at 04:45:23AM +0200, Marek Vasut wrote:
> > > The trigger_data->hw_control indicates whether the LED is controlled by HW
> > > offload, i.e. the PHY. The trigger_data->hw_control = can_hw_control() is
> > > currently called only from netdev_led_attr_store(), i.e. when writing any
> > > sysfs attribute of the netdev trigger instance associated with a PHY LED.
> > > 
> > > The can_hw_control() calls validate_net_dev() which internally calls
> > > led_cdev->hw_control_get_device(), which is phy_led_hw_control_get_device()
> > > for PHY LEDs. The phy_led_hw_control_get_device() returns NULL if the PHY
> > > is not attached.
> > > 
> > > At least in case of DWMAC (STM32MP, iMX8M, ...), the PHY device is attached
> > > only when the interface is brought up and is detached again when the
> > > interface is brought down. In case e.g. udev rules configure the netdev
> > > LED trigger sysfs attributes before the interface is brought up, then when
> > > the interface is brought up, the LEDs are not blinking.
> > > 
> > > This is because trigger_data->hw_control = can_hw_control() was called
> > > when udev wrote the sysfs attribute files, before the interface was up,
> > > so can_hw_control() resp. validate_net_dev() returned false, and the
> > > trigger_data->hw_control = can_hw_control() was never called again to
> > > update the trigger_data->hw_control content and let the offload take
> > > over the LED blinking.
> > > 
> > > Call data->hw_control = can_hw_control() from netdev_trig_notify() to
> > > update the offload capability of the LED when the UP notification arrives.
> > > This makes the LEDs blink after the interface is brought up.
> > > 
> > > On STM32MP13xx with RTL8211F, it is enough to have the following udev rule
> > > in place, boot the machine with cable plugged in, and the LEDs won't work
> > > without this patch once the interface is brought up, even if they should:
> > > "
> > > ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="end0"
> > > ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name}="end0"
> > > "
> > > 
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Is there anything blocking this patch from being picked up ?

I think this should be going via the LED Maintainer. Please check with
Lee.

	Andrew

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Lee Jones]

On Sat, 14 Dec 2024, Andrew Lunn wrote:

> On Fri, Dec 13, 2024 at 11:15:09PM +0100, Marek Vasut wrote:
> > On 10/3/24 2:06 PM, Andrew Lunn wrote:
> > > On Tue, Oct 01, 2024 at 04:45:23AM +0200, Marek Vasut wrote:
> > > > The trigger_data->hw_control indicates whether the LED is controlled by HW
> > > > offload, i.e. the PHY. The trigger_data->hw_control = can_hw_control() is
> > > > currently called only from netdev_led_attr_store(), i.e. when writing any
> > > > sysfs attribute of the netdev trigger instance associated with a PHY LED.
> > > > 
> > > > The can_hw_control() calls validate_net_dev() which internally calls
> > > > led_cdev->hw_control_get_device(), which is phy_led_hw_control_get_device()
> > > > for PHY LEDs. The phy_led_hw_control_get_device() returns NULL if the PHY
> > > > is not attached.
> > > > 
> > > > At least in case of DWMAC (STM32MP, iMX8M, ...), the PHY device is attached
> > > > only when the interface is brought up and is detached again when the
> > > > interface is brought down. In case e.g. udev rules configure the netdev
> > > > LED trigger sysfs attributes before the interface is brought up, then when
> > > > the interface is brought up, the LEDs are not blinking.
> > > > 
> > > > This is because trigger_data->hw_control = can_hw_control() was called
> > > > when udev wrote the sysfs attribute files, before the interface was up,
> > > > so can_hw_control() resp. validate_net_dev() returned false, and the
> > > > trigger_data->hw_control = can_hw_control() was never called again to
> > > > update the trigger_data->hw_control content and let the offload take
> > > > over the LED blinking.
> > > > 
> > > > Call data->hw_control = can_hw_control() from netdev_trig_notify() to
> > > > update the offload capability of the LED when the UP notification arrives.
> > > > This makes the LEDs blink after the interface is brought up.
> > > > 
> > > > On STM32MP13xx with RTL8211F, it is enough to have the following udev rule
> > > > in place, boot the machine with cable plugged in, and the LEDs won't work
> > > > without this patch once the interface is brought up, even if they should:
> > > > "
> > > > ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:green:wan", ATTR{trigger}="netdev", ATTR{link_10}="1", ATTR{link_100}="1", ATTR{link_1000}="1", ATTR{device_name}="end0"
> > > > ACTION=="add", SUBSYSTEM=="leds", KERNEL=="stmmac-0:01:yellow:wan", ATTR{trigger}="netdev", ATTR{rx}="1", ATTR{tx}="1", ATTR{device_name}="end0"
> > > > "
> > > > 
> > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > 
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Is there anything blocking this patch from being picked up ?
> 
> I think this should be going via the LED Maintainer. Please check with

It looked like the conversation was continuing.

If you have everything tied up, rather than relying on maintainers to
keep up with the branching conversations of 100's of patch-sets, it's
best to collect the tags you have and submit a [RESEND].

-- 
Lee Jones [李琼斯]

Re: [PATCH] leds: trigger: netdev: Check offload ability on interface up

[Andrew Lunn]

> It looked like the conversation was continuing.

Yes, it did, for nice to have additional functionality. But i think
the base patch is good to go, and it has my Reviewed-by:.

I expect there will be a resend coming soon :-)

	Andrew