On Sat, 9 Dec 2023 16:07:24 +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 added.
>
> 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 add only the supported link speed
> modes attribute.
>
> This only apply to the link speed modes and every other mode is still
> provided by default.
>
> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> ---
> drivers/leds/trigger/ledtrig-netdev.c | 56 +++++++++++++++++++++++----
> 1 file changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 09e75fd9f2bc..ce84808e231c 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -61,6 +61,8 @@ struct led_netdev_data {
> bool hw_control;
> };
>
> +static int add_link_speed_attr(struct led_netdev_data *trigger_data);
> +
> static void set_baseline_state(struct led_netdev_data *trigger_data)
> {
> int current_brightness;
> @@ -262,8 +264,10 @@ static int set_device_name(struct led_netdev_data *trigger_data,
> trigger_data->carrier_link_up = false;
> trigger_data->link_speed = SPEED_UNKNOWN;
> trigger_data->duplex = DUPLEX_UNKNOWN;
> - if (trigger_data->net_dev)
> + if (trigger_data->net_dev) {
> get_device_state(trigger_data);
> + add_link_speed_attr(trigger_data);
> + }
>
> trigger_data->last_activity = 0;
>
> @@ -396,6 +400,50 @@ DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX);
> DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
> DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
>
> +static int add_link_speed_attr(struct led_netdev_data *trigger_data)
> +{
> + struct led_classdev *led_cdev = trigger_data->led_cdev;
> + struct device *dev = led_cdev->dev;
> + struct ethtool_link_ksettings cmd;
> + int ret;
> +
> + /* First remove any entry previously added */
> + device_remove_file(dev, &dev_attr_link_10);
> + device_remove_file(dev, &dev_attr_link_100);
> + device_remove_file(dev, &dev_attr_link_1000);
> + device_remove_file(dev, &dev_attr_link_2500);
> + device_remove_file(dev, &dev_attr_link_5000);
> + device_remove_file(dev, &dev_attr_link_10000);
Noooooo!
> + ret = __ethtool_get_link_ksettings(trigger_data->net_dev, &cmd);
> + if (ret)
> + return ret;
> +
> + /* Add only supported entry */
> + if (test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, cmd.link_modes.supported) ||
> + test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, cmd.link_modes.supported))
> + device_create_file(dev, &dev_attr_link_10);
> +
> + if (test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, cmd.link_modes.supported) ||
> + test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, cmd.link_modes.supported))
> + device_create_file(dev, &dev_attr_link_100);
> +
> + if (test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, cmd.link_modes.supported) ||
> + test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, cmd.link_modes.supported))
> + device_create_file(dev, &dev_attr_link_1000);
> +
> + if (test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, cmd.link_modes.supported))
> + device_create_file(dev, &dev_attr_link_2500);
> +
> + if (test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, cmd.link_modes.supported))
> + device_create_file(dev, &dev_attr_link_5000);
> +
> + if (test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, cmd.link_modes.supported))
> + device_create_file(dev, &dev_attr_link_10000);
> +
> + return 0;
> +}
This should be done via the is_visible sysfs attribute_group method.
Also documentation for the link_* files should be changed so that it
says that the files are present only if those speeds are available.
Marek
