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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d0411d89-5c83-47b4-bef9-904b63cbc2c0@denx.de>
Date: Thu, 3 Oct 2024 02:47:48 +0200
From: Marek Vasut <marex@...x.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: linux-leds@...r.kernel.org,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Christian Marangi <ansuelsmth@...il.com>,
 Christophe Roullier <christophe.roullier@...s.st.com>,
 Daniel Golle <daniel@...rotopia.org>, Heiner Kallweit
 <hkallweit1@...il.com>, Lee Jones <lee@...nel.org>,
 Lukasz Majewski <lukma@...x.de>, Pavel Machek <pavel@....cz>,
 kernel@...electronics.com, linux-stm32@...md-mailman.stormreply.com,
 netdev@...r.kernel.org
Subject: Re: [PATCH] leds: trigger: netdev: Check offload ability on interface
 up

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.