On Wed, Dec 13, 2023 at 01:05:21PM +0100, Marek Behún wrote:
> 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.
>
Just working on doing this change!
Just need to do some test on how is_visible behave with changing the
device_name.
--
Ansuel
