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

["Heiko Stübner" <heiko@sntech.de>]

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;
>  
>