| Message ID | 20241001024731.140069-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | leds: trigger: netdev: Check offload ability on interface up | expand |
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
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.
> > 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
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
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.
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 */
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(+)