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 <ansuelsmth@xxxxxxxxx>
> Reviewed-by: Marek Behún <kabel@xxxxxxxxxx>
> ---
> 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 [李琼斯]
