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 <[email protected]>
Reviewed-by: Marek Behún <[email protected]>
---
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 v6:
- Improve comments wording
- Add Reviewed-by tag
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 836610292b37..f082a952bd4d 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"
@@ -65,12 +67,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;
@@ -218,13 +223,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 avoid rtnl lock every time
+ * modes are refreshed on any change event
+ */
+ linkmode_copy(trigger_data->supported_link_modes, cmd.link_modes.supported);
}
static ssize_t device_name_show(struct device *dev,
@@ -292,6 +303,10 @@ static ssize_t device_name_store(struct device *dev,
if (ret < 0)
return ret;
+
+ /* Refresh link_speed visibility */
+ sysfs_update_group(&dev->kobj, &netdev_trig_link_speed_attrs_group);
+
return size;
}
@@ -455,15 +470,62 @@ static ssize_t offloaded_show(struct device *dev,
static DEVICE_ATTR_RO(offloaded);
-static struct attribute *netdev_trig_attrs[] = {
- &dev_attr_device_name.attr,
- &dev_attr_link.attr,
+#define CHECK_LINK_MODE_ATTR(link_speed) \
+ do { \
+ if (attr == &dev_attr_link_##link_speed.attr && \
+ link_ksettings.base.speed == SPEED_##link_speed) \
+ return attr->mode; \
+ } while (0)
+
+static umode_t netdev_trig_link_speed_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct led_netdev_data *trigger_data;
+ unsigned long *supported_link_modes;
+ u32 mode;
+
+ trigger_data = led_trigger_get_drvdata(dev);
+ supported_link_modes = trigger_data->supported_link_modes;
+
+ /* Search in the supported link mode mask a matching supported mode.
+ * Stop at the first matching entry as we care only to check if a particular
+ * speed is supported and not the kind.
+ */
+ for_each_set_bit(mode, supported_link_modes, __ETHTOOL_LINK_MODE_MASK_NBITS) {
+ struct ethtool_link_ksettings link_ksettings;
+
+ ethtool_params_from_link_mode(&link_ksettings, mode);
+
+ CHECK_LINK_MODE_ATTR(10);
+ CHECK_LINK_MODE_ATTR(100);
+ CHECK_LINK_MODE_ATTR(1000);
+ CHECK_LINK_MODE_ATTR(2500);
+ CHECK_LINK_MODE_ATTR(5000);
+ CHECK_LINK_MODE_ATTR(10000);
+ }
+
+ return 0;
+}
+
+static struct attribute *netdev_trig_link_speed_attrs[] = {
&dev_attr_link_10.attr,
&dev_attr_link_100.attr,
&dev_attr_link_1000.attr,
&dev_attr_link_2500.attr,
&dev_attr_link_5000.attr,
&dev_attr_link_10000.attr,
+ NULL
+};
+
+static const struct attribute_group netdev_trig_link_speed_attrs_group = {
+ .attrs = netdev_trig_link_speed_attrs,
+ .is_visible = netdev_trig_link_speed_visible,
+};
+
+static struct attribute *netdev_trig_attrs[] = {
+ &dev_attr_device_name.attr,
+ &dev_attr_link.attr,
&dev_attr_full_duplex.attr,
&dev_attr_half_duplex.attr,
&dev_attr_rx.attr,
@@ -472,7 +534,16 @@ static struct attribute *netdev_trig_attrs[] = {
&dev_attr_offloaded.attr,
NULL
};
-ATTRIBUTE_GROUPS(netdev_trig);
+
+static const struct attribute_group netdev_trig_attrs_group = {
+ .attrs = netdev_trig_attrs,
+};
+
+static const struct attribute_group *netdev_trig_groups[] = {
+ &netdev_trig_attrs_group,
+ &netdev_trig_link_speed_attrs_group,
+ NULL,
+};
static int netdev_trig_notify(struct notifier_block *nb,
unsigned long evt, void *dv)
@@ -481,6 +552,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
netdev_notifier_info_to_dev((struct netdev_notifier_info *)dv);
struct led_netdev_data *trigger_data =
container_of(nb, struct led_netdev_data, notifier);
+ struct led_classdev *led_cdev = trigger_data->led_cdev;
if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
&& evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
@@ -515,6 +587,10 @@ static int netdev_trig_notify(struct notifier_block *nb,
case NETDEV_UP:
case NETDEV_CHANGE:
get_device_state(trigger_data);
+ /* Refresh link_speed visibility */
+ if (evt == NETDEV_CHANGE)
+ sysfs_update_group(&led_cdev->dev->kobj,
+ &netdev_trig_link_speed_attrs_group);
break;
}
--
2.40.1
Document now hidable link speed modes for the LED netdev trigger.
Link speed modes are now showed only if the named network device
supports them and are hidden if not.
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Marek Behún <[email protected]>
---
Changes v6:
- Add Reviewed-by tag
Changes v2:
- Add this patch
.../ABI/testing/sysfs-class-led-trigger-netdev | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
index a6c307c4befa..ed46b37ab8a2 100644
--- a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
@@ -88,6 +88,8 @@ Description:
speed of 10MBps of the named network device.
Setting this value also immediately changes the LED state.
+ Present only if the named network device supports 10Mbps link speed.
+
What: /sys/class/leds/<led>/link_100
Date: Jun 2023
KernelVersion: 6.5
@@ -101,6 +103,8 @@ Description:
speed of 100Mbps of the named network device.
Setting this value also immediately changes the LED state.
+ Present only if the named network device supports 100Mbps link speed.
+
What: /sys/class/leds/<led>/link_1000
Date: Jun 2023
KernelVersion: 6.5
@@ -114,6 +118,8 @@ Description:
speed of 1000Mbps of the named network device.
Setting this value also immediately changes the LED state.
+ Present only if the named network device supports 1000Mbps link speed.
+
What: /sys/class/leds/<led>/link_2500
Date: Nov 2023
KernelVersion: 6.8
@@ -127,6 +133,8 @@ Description:
speed of 2500Mbps of the named network device.
Setting this value also immediately changes the LED state.
+ Present only if the named network device supports 2500Mbps link speed.
+
What: /sys/class/leds/<led>/link_5000
Date: Nov 2023
KernelVersion: 6.8
@@ -140,6 +148,8 @@ Description:
speed of 5000Mbps of the named network device.
Setting this value also immediately changes the LED state.
+ Present only if the named network device supports 5000Mbps link speed.
+
What: /sys/class/leds/<led>/link_10000
Date: Nov 2023
KernelVersion: 6.8
@@ -153,6 +163,8 @@ Description:
speed of 10000Mbps of the named network device.
Setting this value also immediately changes the LED state.
+ Present only if the named network device supports 10000Mbps link speed.
+
What: /sys/class/leds/<led>/half_duplex
Date: Jun 2023
KernelVersion: 6.5
--
2.40.1
On Thu, Dec 21, 2023 at 06:11:24PM +0100, Christian Marangi 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 <[email protected]>
> Reviewed-by: Marek Beh?n <[email protected]>
Any news for this?
--
Ansuel
On Wed, 10 Jan 2024, Christian Marangi wrote:
> On Thu, Dec 21, 2023 at 06:11:24PM +0100, Christian Marangi 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 <[email protected]>
> > Reviewed-by: Marek Behún <[email protected]>
>
> Any news for this?
Not yet. It's on the list. Holidays, merge window, etc.
--
Lee Jones [李琼斯]
A review from Andrew is always helpful for netdev related items.
On Thu, 21 Dec 2023, Christian Marangi 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 <[email protected]>
> Reviewed-by: Marek Behún <[email protected]>
> ---
> 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 v6:
> - Improve comments wording
> - Add Reviewed-by tag
> 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(-)
Generally fine, just a couple of nits.
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 836610292b37..f082a952bd4d 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"
> @@ -65,12 +67,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;
> @@ -218,13 +223,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 avoid rtnl lock every time
> + * modes are refreshed on any change event
> + */
Nit: Standard kernel comments throughout please.
This is not the Net subsystem.
> + linkmode_copy(trigger_data->supported_link_modes, cmd.link_modes.supported);
> }
>
> static ssize_t device_name_show(struct device *dev,
> @@ -292,6 +303,10 @@ static ssize_t device_name_store(struct device *dev,
>
> if (ret < 0)
> return ret;
> +
> + /* Refresh link_speed visibility */
> + sysfs_update_group(&dev->kobj, &netdev_trig_link_speed_attrs_group);
> +
> return size;
> }
>
> @@ -455,15 +470,62 @@ static ssize_t offloaded_show(struct device *dev,
>
> static DEVICE_ATTR_RO(offloaded);
>
> -static struct attribute *netdev_trig_attrs[] = {
> - &dev_attr_device_name.attr,
> - &dev_attr_link.attr,
> +#define CHECK_LINK_MODE_ATTR(link_speed) \
> + do { \
> + if (attr == &dev_attr_link_##link_speed.attr && \
> + link_ksettings.base.speed == SPEED_##link_speed) \
> + return attr->mode; \
> + } while (0)
> +
> +static umode_t netdev_trig_link_speed_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct led_netdev_data *trigger_data;
> + unsigned long *supported_link_modes;
> + u32 mode;
> +
> + trigger_data = led_trigger_get_drvdata(dev);
> + supported_link_modes = trigger_data->supported_link_modes;
> +
> + /* Search in the supported link mode mask a matching supported mode.
> + * Stop at the first matching entry as we care only to check if a particular
> + * speed is supported and not the kind.
> + */
Here too.
> + for_each_set_bit(mode, supported_link_modes, __ETHTOOL_LINK_MODE_MASK_NBITS) {
> + struct ethtool_link_ksettings link_ksettings;
> +
> + ethtool_params_from_link_mode(&link_ksettings, mode);
> +
> + CHECK_LINK_MODE_ATTR(10);
> + CHECK_LINK_MODE_ATTR(100);
> + CHECK_LINK_MODE_ATTR(1000);
> + CHECK_LINK_MODE_ATTR(2500);
> + CHECK_LINK_MODE_ATTR(5000);
> + CHECK_LINK_MODE_ATTR(10000);
> + }
> +
> + return 0;
> +}
> +
> +static struct attribute *netdev_trig_link_speed_attrs[] = {
> &dev_attr_link_10.attr,
> &dev_attr_link_100.attr,
> &dev_attr_link_1000.attr,
> &dev_attr_link_2500.attr,
> &dev_attr_link_5000.attr,
> &dev_attr_link_10000.attr,
> + NULL
> +};
> +
> +static const struct attribute_group netdev_trig_link_speed_attrs_group = {
> + .attrs = netdev_trig_link_speed_attrs,
> + .is_visible = netdev_trig_link_speed_visible,
> +};
> +
> +static struct attribute *netdev_trig_attrs[] = {
> + &dev_attr_device_name.attr,
> + &dev_attr_link.attr,
> &dev_attr_full_duplex.attr,
> &dev_attr_half_duplex.attr,
> &dev_attr_rx.attr,
> @@ -472,7 +534,16 @@ static struct attribute *netdev_trig_attrs[] = {
> &dev_attr_offloaded.attr,
> NULL
> };
> -ATTRIBUTE_GROUPS(netdev_trig);
> +
> +static const struct attribute_group netdev_trig_attrs_group = {
> + .attrs = netdev_trig_attrs,
> +};
> +
> +static const struct attribute_group *netdev_trig_groups[] = {
> + &netdev_trig_attrs_group,
> + &netdev_trig_link_speed_attrs_group,
> + NULL,
> +};
>
> static int netdev_trig_notify(struct notifier_block *nb,
> unsigned long evt, void *dv)
> @@ -481,6 +552,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
> netdev_notifier_info_to_dev((struct netdev_notifier_info *)dv);
> struct led_netdev_data *trigger_data =
> container_of(nb, struct led_netdev_data, notifier);
> + struct led_classdev *led_cdev = trigger_data->led_cdev;
>
> if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
> && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
> @@ -515,6 +587,10 @@ static int netdev_trig_notify(struct notifier_block *nb,
> case NETDEV_UP:
> case NETDEV_CHANGE:
> get_device_state(trigger_data);
> + /* Refresh link_speed visibility */
> + if (evt == NETDEV_CHANGE)
> + sysfs_update_group(&led_cdev->dev->kobj,
> + &netdev_trig_link_speed_attrs_group);
> break;
> }
>
> --
> 2.40.1
--
Lee Jones [李琼斯]
On Thu, 21 Dec 2023, Christian Marangi wrote:
> Document now hidable link speed modes for the LED netdev trigger.
>
> Link speed modes are now showed only if the named network device
> supports them and are hidden if not.
>
> Signed-off-by: Christian Marangi <[email protected]>
> Reviewed-by: Marek Behún <[email protected]>
> ---
> Changes v6:
> - Add Reviewed-by tag
> Changes v2:
> - Add this patch
>
> .../ABI/testing/sysfs-class-led-trigger-netdev | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
Reviewed-by: Lee Jones <[email protected]>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
> index a6c307c4befa..ed46b37ab8a2 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
> @@ -88,6 +88,8 @@ Description:
> speed of 10MBps of the named network device.
> Setting this value also immediately changes the LED state.
>
> + Present only if the named network device supports 10Mbps link speed.
> +
> What: /sys/class/leds/<led>/link_100
> Date: Jun 2023
> KernelVersion: 6.5
> @@ -101,6 +103,8 @@ Description:
> speed of 100Mbps of the named network device.
> Setting this value also immediately changes the LED state.
>
> + Present only if the named network device supports 100Mbps link speed.
> +
> What: /sys/class/leds/<led>/link_1000
> Date: Jun 2023
> KernelVersion: 6.5
> @@ -114,6 +118,8 @@ Description:
> speed of 1000Mbps of the named network device.
> Setting this value also immediately changes the LED state.
>
> + Present only if the named network device supports 1000Mbps link speed.
> +
> What: /sys/class/leds/<led>/link_2500
> Date: Nov 2023
> KernelVersion: 6.8
> @@ -127,6 +133,8 @@ Description:
> speed of 2500Mbps of the named network device.
> Setting this value also immediately changes the LED state.
>
> + Present only if the named network device supports 2500Mbps link speed.
> +
> What: /sys/class/leds/<led>/link_5000
> Date: Nov 2023
> KernelVersion: 6.8
> @@ -140,6 +148,8 @@ Description:
> speed of 5000Mbps of the named network device.
> Setting this value also immediately changes the LED state.
>
> + Present only if the named network device supports 5000Mbps link speed.
> +
> What: /sys/class/leds/<led>/link_10000
> Date: Nov 2023
> KernelVersion: 6.8
> @@ -153,6 +163,8 @@ Description:
> speed of 10000Mbps of the named network device.
> Setting this value also immediately changes the LED state.
>
> + Present only if the named network device supports 10000Mbps link speed.
> +
> What: /sys/class/leds/<led>/half_duplex
> Date: Jun 2023
> KernelVersion: 6.5
> --
> 2.40.1
>
--
Lee Jones [李琼斯]