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.
