[v3,1/2] leds: trigger: netdev: extend speeds up to 10G
Commit Message
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>
---
v3: no changes
v2: add missing sysfs entries
drivers/leds/trigger/ledtrig-netdev.c | 32 ++++++++++++++++++++++++++-
include/linux/leds.h | 3 +++
2 files changed, 34 insertions(+), 1 deletion(-)
Comments
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 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...
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.
Marek
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.
Andrew
On Thu, Dec 07, 2023 at 06:11:29PM +0100, Andrew Lunn 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 ?
Yes, why not?
I also doubt we are doing to have 800GBit/s on RJ-45 copper any time
soon. And speed-indication is generally only needed for media which
supports more than one speed (ie. twisted copper pair).
> >
> > 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.
I agree on that, it would be an additional patch though because that
obviously goes beyond adding some more speeds.
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, 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.
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 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, 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
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
> 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 ?
Yes, that is O.K. It should do link_2500 in software, and since it is
a static on/off, should cost nothing.
Andrew
@@ -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(
@@ -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,