Re: [PATCH v5 1/2] leds: trigger: netdev: display only supported link speed attribute

[Marek Behún]

On Wed, 20 Dec 2023 23:48:26 +0100
Christian Marangi <ansuelsmth@xxxxxxxxx> wrote:

> With the addition of more link speed mode to the netdev trigger, it was
> pointed out that there may be a problem with bloating the attribute list
> with modes that won't ever be supported by the trigger as the attached
> device name doesn't support them.
> 
> To clear and address this problem, change the logic where these
> additional trigger modes are listed.
> 
> Since the netdev trigger REQUIRE a device name to be set, attach to the
> device name change function additional logic to parse the supported link
> speed modes using ethtool APIs and show only the supported link speed
> modes attribute.
> 
> Link speed attribute are refreshed on device_name set and on
> NETDEV_CHANGE events.
> 
> This only apply to the link speed modes and every other mode is still
> provided by default.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> ---
> Took a while but I found a way to not use phy_speeds.
> 
> Saddly that is way too specific to PHYs and we can't add PHYLIB as
> a dependency for leds.
> 
> I instead found a handy way to keep everything to ethtool, it's a bit
> worse optimization wise but does the same work. (the perf penality
> is really minimal as we only parse supported speeds and it's difficult
> to have a device that have tons of speeds modes)
> 
> Changes v5:
> - Add macro to make code less ugly
> Changes v4:
> - Rework to use an alternative to phy_speeds API
> Changes v3:
> - Use phy_speeds API to parse the ethtool mask
> Changes v2:
> - Use is_visibile instead of removing/adding attr
> 
>  drivers/leds/trigger/ledtrig-netdev.c | 88 +++++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index bd68da15c723..f0f946747ff5 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -18,10 +18,12 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
> +#include <linux/linkmode.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
>  #include <linux/mutex.h>
> +#include <linux/phy.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/timer.h>
>  #include "../leds.h"
> @@ -55,12 +57,15 @@ struct led_netdev_data {
>  
>  	unsigned long mode;
>  	int link_speed;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_link_modes);
>  	u8 duplex;
>  
>  	bool carrier_link_up;
>  	bool hw_control;
>  };
>  
> +static const struct attribute_group netdev_trig_link_speed_attrs_group;
> +
>  static void set_baseline_state(struct led_netdev_data *trigger_data)
>  {
>  	int current_brightness;
> @@ -208,13 +213,19 @@ static void get_device_state(struct led_netdev_data *trigger_data)
>  	struct ethtool_link_ksettings cmd;
>  
>  	trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
> -	if (!trigger_data->carrier_link_up)
> +
> +	if (__ethtool_get_link_ksettings(trigger_data->net_dev, &cmd))
>  		return;
>  
> -	if (!__ethtool_get_link_ksettings(trigger_data->net_dev, &cmd)) {
> +	if (trigger_data->carrier_link_up) {
>  		trigger_data->link_speed = cmd.base.speed;
>  		trigger_data->duplex = cmd.base.duplex;
>  	}
> +
> +	/* Have a local copy of the link speed supported to not rtnl lock every time

                                                         ^ to avoid

> +	 * Modes are refreshed on any change event to handle mode changes

           ^ capital letter but not beginning of sentence

If you change these two, you can add my R-b tag to both patches.


I am also wondering if this sysfs ABI could be extended for multi-color
LEDs.

For example:
  echo green >link_1000
  echo yellow >link_100

Or something like that. But that is a completely different discussion.