| Message ID | 99e7d3304c6bba7f4863a4a80764a869855f2085.1701143925.git.daniel@makrotopia.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] leds: trigger: netdev: extend speeds up to 10G | expand |
On Tue, 28 Nov 2023 04:00:10 +0000, Daniel Golle wrote: > Add 2.5G, 5G and 10G as available speeds to the netdev LED trigger. > > Applied, thanks! [1/2] leds: trigger: netdev: extend speeds up to 10G commit: bc8e1da69a68d9871773b657d18400a7941cbdef [2/2] docs: ABI: sysfs-class-led-trigger-netdev: add new modes and entry commit: f07894d3b384344c43be1bcf61ef8e2fded0efe5 -- Lee Jones [李琼斯]
On Thu, 7 Dec 2023 18:11:29 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Dec 07, 2023 at 05:29:23PM +0100, Marek Behún wrote: > > On Tue, 28 Nov 2023 04:00:10 +0000 > > Daniel Golle <daniel@makrotopia.org> wrote: > > > > > Add 2.5G, 5G and 10G as available speeds to the netdev LED trigger. > > > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > So what will happen when there are more speeds? Will we create a > > separate file for each speed? > > > > Will we have a separate sysfs file for 10, 100, 1000, 2500, 5000, > > 10000, 20000, 25000, 40000, 50000, 56000, 100000, 200000, 400000, > > 800000 ? > > > > These are all speeds from include/uapi/linux/ethtool.h. > > > > Maybe we should have reused ethtool link mode bits, or something... > > That gets pretty ugly. The bits are not in any logical order, since > they just get appended onto the end as needed. > > > Also, the files should only be present if the requested speed is > > supported by the net device. So if 2500 mbps is not supported, there > > should no be link_2500. > > Yes, this would be nice. We have the information in the phy_setting > settings[] table in phy-core.c. What if the netdev does not have a PHY? The MAC also has speed information. Maybe create a function bool dev_supports_speed(netdev, speed) ? Marek
> What if the netdev does not have a PHY? The MAC also has speed > information. The ethtool API provides a list of link modes the MAC supports: /usr/sbin/ethtool enp2s0 Settings for enp2s0: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full The MAC driver can provide this information by calling into phylib or phylink, or it can do it some other way. In fact, none of the LED code goes direct to the PHY when determining when to blink in software, its all via the struct net_device. We should use the ethtool API to determine which speed sysfs files should exist. Andrew
On Fri, Dec 01, 2023 at 10:57:41AM +0000, Lee Jones wrote: > On Tue, 28 Nov 2023 04:00:10 +0000, Daniel Golle wrote: > > Add 2.5G, 5G and 10G as available speeds to the netdev LED trigger. > > > > > > Applied, thanks! > > [1/2] leds: trigger: netdev: extend speeds up to 10G > commit: bc8e1da69a68d9871773b657d18400a7941cbdef > [2/2] docs: ABI: sysfs-class-led-trigger-netdev: add new modes and entry > commit: f07894d3b384344c43be1bcf61ef8e2fded0efe5 > Hi, Lee I'm working on adding LEDs support for qca8081 PHY. This PHY supports 2500 link speed. Is it possible to have an immutable branch for this series so we can have this in net-next? Jakub can you also help with this?
On Mon, Dec 11, 2023 at 08:46:56AM -0800, Jakub Kicinski wrote: > On Mon, 11 Dec 2023 16:57:15 +0100 Christian Marangi wrote: > > > [1/2] leds: trigger: netdev: extend speeds up to 10G > > > commit: bc8e1da69a68d9871773b657d18400a7941cbdef > > > [2/2] docs: ABI: sysfs-class-led-trigger-netdev: add new modes and entry > > > commit: f07894d3b384344c43be1bcf61ef8e2fded0efe5 > > > > > > > Hi, Lee > > > > I'm working on adding LEDs support for qca8081 PHY. This PHY supports > > 2500 link speed. > > > > Is it possible to have an immutable branch for this series so we can > > have this in net-next? > > > > Jakub can you also help with this? > > I'm guessing that if it's already applied - it's already applied. > Soo that it's problematic to also have on net-next? (Sorry for the stupid question) > Lee, we seem to be getting quite a few LEDs/netdev patches - do you > reckon we should ask Konstantin for a separate repo to which we can > both apply, and then merge that into our respective trees? Because > stacking the changes on stable branches may get weird and/or error > prone. Or is separate tree an overkill at this stage? IDK..
On Mon, Dec 11, 2023 at 02:05:46PM -0800, Jakub Kicinski wrote: > On Mon, 11 Dec 2023 22:53:55 +0100 Christian Marangi wrote: > > Soo that it's problematic to also have on net-next? (Sorry for the > > stupid question) > > Unless I pull from Lee the patch would be duplicated, we'd have two > commits with different hashes and the same diff. And if I pull we'd > get a lot of netdev-unrelated stuff into net-next: > Thanks for the explaination... Sad... guess I have to wait, hoped I could have the qca808x series before proposing the at803x driver split but I guess it's not possible... Maybe I can try pushing the change for link_1000 for now and later add link_2500 ? > $ git merge f07894d3b384344c43be1bcf61ef8e2fded0efe5 > Auto-merging drivers/leds/trigger/ledtrig-netdev.c > Merge made by the 'ort' strategy. > .../ABI/testing/sysfs-class-led-trigger-netdev | 39 ++ > .../ABI/testing/sysfs-class-led-trigger-tty | 56 ++ > .../bindings/leds/allwinner,sun50i-a100-ledc.yaml | 137 +++++ > Documentation/devicetree/bindings/leds/common.yaml | 2 +- > drivers/leds/Kconfig | 21 + > drivers/leds/Makefile | 2 + > drivers/leds/leds-max5970.c | 109 ++++ > drivers/leds/leds-sun50i-a100.c | 580 +++++++++++++++++++++ > drivers/leds/leds-syscon.c | 3 +- > drivers/leds/leds-tca6507.c | 30 +- > drivers/leds/rgb/leds-qcom-lpg.c | 52 +- > drivers/leds/trigger/ledtrig-gpio.c | 26 +- > drivers/leds/trigger/ledtrig-netdev.c | 32 +- > drivers/leds/trigger/ledtrig-tty.c | 247 +++++++-- > drivers/tty/tty_io.c | 28 +- > include/linux/leds.h | 3 + > include/linux/tty.h | 1 + > 17 files changed, 1247 insertions(+), 121 deletions(-) > create mode 100644 Documentation/devicetree/bindings/leds/allwinner,sun50i-a100-ledc.yaml > create mode 100644 drivers/leds/leds-max5970.c > create mode 100644 drivers/leds/leds-sun50i-a100.c
On Wed, 13 Dec 2023 11:27:05 +0000 Lee Jones <lee@kernel.org> wrote: > On Mon, 11 Dec 2023, Jakub Kicinski wrote: > > > On Mon, 11 Dec 2023 22:53:55 +0100 Christian Marangi wrote: > > > Soo that it's problematic to also have on net-next? (Sorry for the > > > stupid question) > > > > Unless I pull from Lee the patch would be duplicated, we'd have two > > commits with different hashes and the same diff. And if I pull we'd > > get a lot of netdev-unrelated stuff into net-next: > > > > $ git merge f07894d3b384344c43be1bcf61ef8e2fded0efe5 > > Auto-merging drivers/leds/trigger/ledtrig-netdev.c > > Merge made by the 'ort' strategy. > > .../ABI/testing/sysfs-class-led-trigger-netdev | 39 ++ > > .../ABI/testing/sysfs-class-led-trigger-tty | 56 ++ > > .../bindings/leds/allwinner,sun50i-a100-ledc.yaml | 137 +++++ > > Documentation/devicetree/bindings/leds/common.yaml | 2 +- > > drivers/leds/Kconfig | 21 + > > drivers/leds/Makefile | 2 + > > drivers/leds/leds-max5970.c | 109 ++++ > > drivers/leds/leds-sun50i-a100.c | 580 +++++++++++++++++++++ > > drivers/leds/leds-syscon.c | 3 +- > > drivers/leds/leds-tca6507.c | 30 +- > > drivers/leds/rgb/leds-qcom-lpg.c | 52 +- > > drivers/leds/trigger/ledtrig-gpio.c | 26 +- > > drivers/leds/trigger/ledtrig-netdev.c | 32 +- > > drivers/leds/trigger/ledtrig-tty.c | 247 +++++++-- > > drivers/tty/tty_io.c | 28 +- > > include/linux/leds.h | 3 + > > include/linux/tty.h | 1 + > > 17 files changed, 1247 insertions(+), 121 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/leds/allwinner,sun50i-a100-ledc.yaml > > create mode 100644 drivers/leds/leds-max5970.c > > create mode 100644 drivers/leds/leds-sun50i-a100.c > > No, please don't do that. None of the branches I maintain are stable. > > It allows me to do things like this: > > The following changes since commit b85ea95d086471afb4ad062012a4d73cd328fa86: > > Linux 6.7-rc1 (2023-11-12 16:19:07 -0800) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git ib-leds-netdev-v6.8 > > for you to fetch changes up to ee8bfb47222a5cc59dee345b7369c5f2068e78cd: > > docs: ABI: sysfs-class-led-trigger-netdev: Add new modes and entry (2023-12-13 11:24:55 +0000) > > ---------------------------------------------------------------- > Immutable branch between LEDs and NetDev due for the v6.8 merge window > > ---------------------------------------------------------------- > Daniel Golle (2): > leds: trigger: netdev: Extend speeds up to 10G > docs: ABI: sysfs-class-led-trigger-netdev: Add new modes and entry Please don't pull this. The sysfs documentation for the link_* files does not specify that they are available only if the underlying speeds are supported. Let's first fix this and then merge it. Christian sent the patch leds: trigger: netdev: display only supported link speed attribute It needs some updating first, but only after it is fixed should this be merged. Marek
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index e358e77e4b38f..bd68da15c723e 100644 --- a/drivers/leds/trigger/ledtrig-netdev.c +++ b/drivers/leds/trigger/ledtrig-netdev.c @@ -99,6 +99,18 @@ static void set_baseline_state(struct led_netdev_data *trigger_data) trigger_data->link_speed == SPEED_1000) blink_on = true; + if (test_bit(TRIGGER_NETDEV_LINK_2500, &trigger_data->mode) && + trigger_data->link_speed == SPEED_2500) + blink_on = true; + + if (test_bit(TRIGGER_NETDEV_LINK_5000, &trigger_data->mode) && + trigger_data->link_speed == SPEED_5000) + blink_on = true; + + if (test_bit(TRIGGER_NETDEV_LINK_10000, &trigger_data->mode) && + trigger_data->link_speed == SPEED_10000) + blink_on = true; + if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &trigger_data->mode) && trigger_data->duplex == DUPLEX_HALF) blink_on = true; @@ -286,6 +298,9 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf, case TRIGGER_NETDEV_LINK_10: case TRIGGER_NETDEV_LINK_100: case TRIGGER_NETDEV_LINK_1000: + case TRIGGER_NETDEV_LINK_2500: + case TRIGGER_NETDEV_LINK_5000: + case TRIGGER_NETDEV_LINK_10000: case TRIGGER_NETDEV_HALF_DUPLEX: case TRIGGER_NETDEV_FULL_DUPLEX: case TRIGGER_NETDEV_TX: @@ -316,6 +331,9 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf, case TRIGGER_NETDEV_LINK_10: case TRIGGER_NETDEV_LINK_100: case TRIGGER_NETDEV_LINK_1000: + case TRIGGER_NETDEV_LINK_2500: + case TRIGGER_NETDEV_LINK_5000: + case TRIGGER_NETDEV_LINK_10000: case TRIGGER_NETDEV_HALF_DUPLEX: case TRIGGER_NETDEV_FULL_DUPLEX: case TRIGGER_NETDEV_TX: @@ -334,7 +352,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf, if (test_bit(TRIGGER_NETDEV_LINK, &mode) && (test_bit(TRIGGER_NETDEV_LINK_10, &mode) || test_bit(TRIGGER_NETDEV_LINK_100, &mode) || - test_bit(TRIGGER_NETDEV_LINK_1000, &mode))) + test_bit(TRIGGER_NETDEV_LINK_1000, &mode) || + test_bit(TRIGGER_NETDEV_LINK_2500, &mode) || + test_bit(TRIGGER_NETDEV_LINK_5000, &mode) || + test_bit(TRIGGER_NETDEV_LINK_10000, &mode))) return -EINVAL; cancel_delayed_work_sync(&trigger_data->work); @@ -364,6 +385,9 @@ DEFINE_NETDEV_TRIGGER(link, TRIGGER_NETDEV_LINK); DEFINE_NETDEV_TRIGGER(link_10, TRIGGER_NETDEV_LINK_10); DEFINE_NETDEV_TRIGGER(link_100, TRIGGER_NETDEV_LINK_100); DEFINE_NETDEV_TRIGGER(link_1000, TRIGGER_NETDEV_LINK_1000); +DEFINE_NETDEV_TRIGGER(link_2500, TRIGGER_NETDEV_LINK_2500); +DEFINE_NETDEV_TRIGGER(link_5000, TRIGGER_NETDEV_LINK_5000); +DEFINE_NETDEV_TRIGGER(link_10000, TRIGGER_NETDEV_LINK_10000); DEFINE_NETDEV_TRIGGER(half_duplex, TRIGGER_NETDEV_HALF_DUPLEX); DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX); DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX); @@ -422,6 +446,9 @@ static struct attribute *netdev_trig_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, &dev_attr_full_duplex.attr, &dev_attr_half_duplex.attr, &dev_attr_rx.attr, @@ -519,6 +546,9 @@ static void netdev_trig_work(struct work_struct *work) test_bit(TRIGGER_NETDEV_LINK_10, &trigger_data->mode) || test_bit(TRIGGER_NETDEV_LINK_100, &trigger_data->mode) || test_bit(TRIGGER_NETDEV_LINK_1000, &trigger_data->mode) || + test_bit(TRIGGER_NETDEV_LINK_2500, &trigger_data->mode) || + test_bit(TRIGGER_NETDEV_LINK_5000, &trigger_data->mode) || + test_bit(TRIGGER_NETDEV_LINK_10000, &trigger_data->mode) || test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &trigger_data->mode) || test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &trigger_data->mode); interval = jiffies_to_msecs( diff --git a/include/linux/leds.h b/include/linux/leds.h index aa16dc2a8230f..1bdf7f5a0d7c0 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -588,6 +588,9 @@ enum led_trigger_netdev_modes { TRIGGER_NETDEV_LINK_10, TRIGGER_NETDEV_LINK_100, TRIGGER_NETDEV_LINK_1000, + TRIGGER_NETDEV_LINK_2500, + TRIGGER_NETDEV_LINK_5000, + TRIGGER_NETDEV_LINK_10000, TRIGGER_NETDEV_HALF_DUPLEX, TRIGGER_NETDEV_FULL_DUPLEX, TRIGGER_NETDEV_TX,