| Message ID | 20231214122041.17439-1-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] leds: trigger: netdev: display only supported link speed attribute | expand |
Hi Christian, kernel test robot noticed the following build errors: [auto build test ERROR on lee-leds/for-leds-next] [cannot apply to net-next/main net/main linus/master v6.7-rc5 next-20231215] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/docs-ABI-sysfs-class-led-trigger-netdev-Document-now-hidable-link_/20231214-202215 base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next patch link: https://lore.kernel.org/r/20231214122041.17439-1-ansuelsmth%40gmail.com patch subject: [PATCH v3 1/2] leds: trigger: netdev: display only supported link speed attribute config: i386-randconfig-011-20231217 (https://download.01.org/0day-ci/archive/20231217/202312170606.NWH5SzQD-lkp@intel.com/config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/202312170606.NWH5SzQD-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312170606.NWH5SzQD-lkp@intel.com/ All errors (new ones prefixed by >>): ld: drivers/leds/trigger/ledtrig-netdev.o: in function `get_device_state': >> drivers/leds/trigger/ledtrig-netdev.c:230: undefined reference to `phy_speeds' vim +230 drivers/leds/trigger/ledtrig-netdev.c 211 212 static void get_device_state(struct led_netdev_data *trigger_data) 213 { 214 struct ethtool_link_ksettings cmd; 215 int speeds_num; 216 217 trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev); 218 219 if (__ethtool_get_link_ksettings(trigger_data->net_dev, &cmd)) 220 return; 221 222 if (trigger_data->carrier_link_up) { 223 trigger_data->link_speed = cmd.base.speed; 224 trigger_data->duplex = cmd.base.duplex; 225 } 226 227 /* Have a local copy of the link speed supported to not rtnl lock every time 228 * Modes are refreshed on any change event to handle mode changes 229 */ > 230 speeds_num = phy_speeds(trigger_data->supported_link_speeds, 231 ARRAY_SIZE(trigger_data->supported_link_speeds), 232 cmd.link_modes.supported); 233 trigger_data->supported_link_speeds_num = speeds_num; 234 } 235
Hi Christian, kernel test robot noticed the following build errors: [auto build test ERROR on lee-leds/for-leds-next] [cannot apply to net-next/main net/main linus/master v6.7-rc5 next-20231215] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/docs-ABI-sysfs-class-led-trigger-netdev-Document-now-hidable-link_/20231214-202215 base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next patch link: https://lore.kernel.org/r/20231214122041.17439-1-ansuelsmth%40gmail.com patch subject: [PATCH v3 1/2] leds: trigger: netdev: display only supported link speed attribute config: i386-buildonly-randconfig-002-20231217 (https://download.01.org/0day-ci/archive/20231217/202312170610.BnWo2SnP-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/202312170610.BnWo2SnP-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312170610.BnWo2SnP-lkp@intel.com/ All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: phy_speeds >>> referenced by ledtrig-netdev.c >>> drivers/leds/trigger/ledtrig-netdev.o:(netdev_trig_notify) in archive vmlinux.a >>> referenced by ledtrig-netdev.c >>> drivers/leds/trigger/ledtrig-netdev.o:(netdev_trig_notify) in archive vmlinux.a >>> referenced by ledtrig-netdev.c >>> drivers/leds/trigger/ledtrig-netdev.o:(set_device_name) in archive vmlinux.a
On Sun, Dec 17, 2023 at 06:23:05AM +0800, kernel test robot wrote: > Hi Christian, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on lee-leds/for-leds-next] > [cannot apply to net-next/main net/main linus/master v6.7-rc5 next-20231215] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/docs-ABI-sysfs-class-led-trigger-netdev-Document-now-hidable-link_/20231214-202215 > base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next > patch link: https://lore.kernel.org/r/20231214122041.17439-1-ansuelsmth%40gmail.com > patch subject: [PATCH v3 1/2] leds: trigger: netdev: display only supported link speed attribute > config: i386-randconfig-011-20231217 (https://download.01.org/0day-ci/archive/20231217/202312170606.NWH5SzQD-lkp@intel.com/config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231217/202312170606.NWH5SzQD-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202312170606.NWH5SzQD-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > ld: drivers/leds/trigger/ledtrig-netdev.o: in function `get_device_state': > >> drivers/leds/trigger/ledtrig-netdev.c:230: undefined reference to `phy_speeds' > > > vim +230 drivers/leds/trigger/ledtrig-netdev.c > > 211 > 212 static void get_device_state(struct led_netdev_data *trigger_data) > 213 { > 214 struct ethtool_link_ksettings cmd; > 215 int speeds_num; > 216 > 217 trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev); > 218 > 219 if (__ethtool_get_link_ksettings(trigger_data->net_dev, &cmd)) > 220 return; > 221 > 222 if (trigger_data->carrier_link_up) { > 223 trigger_data->link_speed = cmd.base.speed; > 224 trigger_data->duplex = cmd.base.duplex; > 225 } > 226 > 227 /* Have a local copy of the link speed supported to not rtnl lock every time > 228 * Modes are refreshed on any change event to handle mode changes > 229 */ > > 230 speeds_num = phy_speeds(trigger_data->supported_link_speeds, > 231 ARRAY_SIZE(trigger_data->supported_link_speeds), > 232 cmd.link_modes.supported); > 233 trigger_data->supported_link_speeds_num = speeds_num; > 234 } > 235 Ugh didn't think that LEDs netdev trigger doesn't have a dependency on PHY... Andrew any idea about this? I can see 2 solution (or maybe 3???): - Add the dependency for PHY - Move phy_speeds net_utils.c (with the settings table moved there) - Implement a custom function in ledtrig-netdev.c It's sad since the phy_speed was just what we needed to implement this ins a ""clean way"".
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index bd68da15c723..345e522bc44a 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" @@ -55,12 +57,16 @@ struct led_netdev_data { unsigned long mode; int link_speed; + unsigned int supported_link_speeds[20]; + int supported_link_speeds_num; 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; @@ -206,15 +212,25 @@ static bool can_hw_control(struct led_netdev_data *trigger_data) static void get_device_state(struct led_netdev_data *trigger_data) { struct ethtool_link_ksettings cmd; + int speeds_num; 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 not rtnl lock every time + * Modes are refreshed on any change event to handle mode changes + */ + speeds_num = phy_speeds(trigger_data->supported_link_speeds, + ARRAY_SIZE(trigger_data->supported_link_speeds), + cmd.link_modes.supported); + trigger_data->supported_link_speeds_num = speeds_num; } static ssize_t device_name_show(struct device *dev, @@ -257,6 +273,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; + trigger_data->supported_link_speeds_num = 0; if (trigger_data->net_dev != NULL) { rtnl_lock(); get_device_state(trigger_data); @@ -282,6 +299,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 +461,72 @@ 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 int *supported_link_speeds; + int speeds_num, i; + + trigger_data = led_trigger_get_drvdata(dev); + supported_link_speeds = trigger_data->supported_link_speeds; + speeds_num = trigger_data->supported_link_speeds_num; + + for (i = 0; i < speeds_num; i++) { + int speed = supported_link_speeds[i]; + + switch (speed) { + case SPEED_10: + if (attr == &dev_attr_link_10.attr) + return attr->mode; + break; + case SPEED_100: + if (attr == &dev_attr_link_100.attr) + return attr->mode; + break; + case SPEED_1000: + if (attr == &dev_attr_link_1000.attr) + return attr->mode; + break; + case SPEED_2500: + if (attr == &dev_attr_link_2500.attr) + return attr->mode; + break; + case SPEED_5000: + if (attr == &dev_attr_link_5000.attr) + return attr->mode; + break; + case SPEED_10000: + if (attr == &dev_attr_link_10000.attr) + return attr->mode; + break; + default: + return 0; + } + } + + 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 +535,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 +553,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 +588,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; }
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 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 | 104 ++++++++++++++++++++++++-- 1 file changed, 98 insertions(+), 6 deletions(-)