[v2,1/2] leds: trigger: netdev: display only supported link speed attribute
Commit Message
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@gmail.com>
---
Changes v2:
- Use is_visibile instead of removing/adding attr
drivers/leds/trigger/ledtrig-netdev.c | 91 +++++++++++++++++++++++++--
1 file changed, 85 insertions(+), 6 deletions(-)
Comments
> + /* Display only supported entry */
> + if (attr == &dev_attr_link_10.attr &&
> + (test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported_link_speed) ||
> + test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported_link_speed)))
> + return attr->mode;
> +
> + if (attr == &dev_attr_link_100.attr &&
> + (test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported_link_speed) ||
> + test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported_link_speed)))
> + return attr->mode;
> +
> + if (attr == &dev_attr_link_1000.attr &&
> + (test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported_link_speed) ||
> + test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported_link_speed)))
> + return attr->mode;
> +
> + if (attr == &dev_attr_link_2500.attr &&
> + test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported_link_speed))
> + return attr->mode;
> +
> + if (attr == &dev_attr_link_5000.attr &&
> + test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported_link_speed))
> + return attr->mode;
> +
> + if (attr == &dev_attr_link_10000.attr &&
> + test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported_link_speed))
> + return attr->mode;
Why only the T modes? There are much more ethtool modes for these
speeds, for example at least 5 modes for 1000 mbps speed:
1000baseT_Half
1000baseT_Full
1000baseKX_Full
1000baseX_Full
1000baseT1_Full
There are also 2 possible modes for 2500 mbps
2500baseT
2500baseX
Ditto for 10 mbps and 100 mbps.
So if you're doing this, why not do it properly?
There is an aarray
static const struct phy_setting settings[]
in
drivers/net/phy/phy-core.c
and a function phy_speeds() which will tell you which speeds are
supported for a given ethtool mode bitmap.
Or you can add another function there,
bool phy_speed_supported(unsigned long mask, unsigned int speed)
and use that one.
Marek
On Wed, Dec 13, 2023 at 04:37:57PM +0100, Marek Behún wrote:
> > + /* Display only supported entry */
> > + if (attr == &dev_attr_link_10.attr &&
> > + (test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported_link_speed) ||
> > + test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported_link_speed)))
> > + return attr->mode;
> > +
> > + if (attr == &dev_attr_link_100.attr &&
> > + (test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported_link_speed) ||
> > + test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported_link_speed)))
> > + return attr->mode;
> > +
> > + if (attr == &dev_attr_link_1000.attr &&
> > + (test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported_link_speed) ||
> > + test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported_link_speed)))
> > + return attr->mode;
> > +
> > + if (attr == &dev_attr_link_2500.attr &&
> > + test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported_link_speed))
> > + return attr->mode;
> > +
> > + if (attr == &dev_attr_link_5000.attr &&
> > + test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported_link_speed))
> > + return attr->mode;
> > +
> > + if (attr == &dev_attr_link_10000.attr &&
> > + test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported_link_speed))
> > + return attr->mode;
>
> Why only the T modes? There are much more ethtool modes for these
> speeds, for example at least 5 modes for 1000 mbps speed:
> 1000baseT_Half
> 1000baseT_Full
> 1000baseKX_Full
> 1000baseX_Full
> 1000baseT1_Full
>
> There are also 2 possible modes for 2500 mbps
> 2500baseT
> 2500baseX
>
> Ditto for 10 mbps and 100 mbps.
>
> So if you're doing this, why not do it properly?
My concern was filling the thing with all kind of modes and have an if
hell.
>
> There is an aarray
> static const struct phy_setting settings[]
> in
> drivers/net/phy/phy-core.c
> and a function phy_speeds() which will tell you which speeds are
> supported for a given ethtool mode bitmap.
>
> Or you can add another function there,
> bool phy_speed_supported(unsigned long mask, unsigned int speed)
> and use that one.
>
Ok this is very handy and just perfect for the task. Problem is that
adding a function makes this again a cross subsystem patch and
problematic to merge...
And having phy_speed_supported would make the function very heavy as the
settings struct needs to be parsed multiple times...
Still I see the settings struct doesn't provide a way to comunicate the
sum of all the modes. I already have a patch in mind for that (usual
enum define hell) but that is again cross subsystem thing...
I see phy declare a array of 50 element to have enough space for all the
link speed modes. Think I will have to do the same here and update
later to a better implementation.
@@ -18,6 +18,7 @@
#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>
@@ -55,12 +56,15 @@ struct led_netdev_data {
unsigned long mode;
int link_speed;
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(supported_link_speed);
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 +212,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 advertised to not rtnl lock every time
+ * Modes are refreshed on any change event to handle mode changes
+ */
+ linkmode_copy(trigger_data->supported_link_speed, cmd.link_modes.supported);
}
static ssize_t device_name_show(struct device *dev,
@@ -257,6 +267,7 @@ 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;
+ linkmode_zero(trigger_data->supported_link_speed);
if (trigger_data->net_dev != NULL) {
rtnl_lock();
get_device_state(trigger_data);
@@ -282,6 +293,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;
}
@@ -440,15 +455,65 @@ 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,
+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_speed;
+
+ trigger_data = led_trigger_get_drvdata(dev);
+ supported_link_speed = trigger_data->supported_link_speed;
+
+ /* Display only supported entry */
+ if (attr == &dev_attr_link_10.attr &&
+ (test_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported_link_speed) ||
+ test_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported_link_speed)))
+ return attr->mode;
+
+ if (attr == &dev_attr_link_100.attr &&
+ (test_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported_link_speed) ||
+ test_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported_link_speed)))
+ return attr->mode;
+
+ if (attr == &dev_attr_link_1000.attr &&
+ (test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported_link_speed) ||
+ test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported_link_speed)))
+ return attr->mode;
+
+ if (attr == &dev_attr_link_2500.attr &&
+ test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported_link_speed))
+ return attr->mode;
+
+ if (attr == &dev_attr_link_5000.attr &&
+ test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported_link_speed))
+ return attr->mode;
+
+ if (attr == &dev_attr_link_10000.attr &&
+ test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported_link_speed))
+ return attr->mode;
+
+ 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,
@@ -457,7 +522,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)
@@ -466,6 +540,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
@@ -500,6 +575,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;
}