This is a continue of [1]. It was decided to take a more gradual
approach to implement LEDs support for switch and phy starting with
basic support and then implementing the hw control part when we have all
the prereq done.
This should be the final part for the netdev trigger.
I added net-next tag and added netdev mailing list since I was informed
that this should be merged with netdev branch.
We collect some info around and we found a good set of modes that are
common in almost all the PHY and Switch.
These modes are:
- Modes for dedicated link speed(10, 100, 1000 mbps). Additional mode
can be added later following this example.
- Modes for half and full duplex.
The original idea was to add hw control only modes.
While the concept makes sense in practice it would results in lots of
additional code and extra check to make sure we are setting correct modes.
With the suggestion from Andrew it was pointed out that using the ethtool
APIs we can actually get the current link speed and duplex and this
effectively removed the problem of having hw control only modes since we
can fallback to software.
Since these modes are supported by software, we can skip providing an
user for this in the LED driver to support hw control for these new modes
(that will come right after this is merged) and prevent this to be another
multi subsystem series.
For link speed and duplex we use ethtool APIs.
To call ethtool APIs, rtnl lock is needed but this can be skipped on
handling netdev events as the lock is already held.
[1] https://lore.kernel.org/lkml/[email protected]/
Changes v4:
- Add net-next tag
- Add additional patch to expose hw_control via sysfs
- CC netdev mailing list
Changes v3:
- Add Andrew review tag
- Use SPEED_UNKNOWN to init link_speed
- Fix using HALF_DUPLEX as duplex init and use DUPLEX_UNKNOWN instead
Changes v2:
- Drop ACTIVITY patch as it can be handled internally in the LED driver
- Reduce duplicate code and move the link state to a dedicated helper
Christian Marangi (3):
leds: trigger: netdev: add additional specific link speed mode
leds: trigger: netdev: add additional specific link duplex mode
leds: trigger: netdev: expose hw_control status via sysfs
drivers/leds/trigger/ledtrig-netdev.c | 114 +++++++++++++++++++++++---
include/linux/leds.h | 5 ++
2 files changed, 109 insertions(+), 10 deletions(-)
--
2.40.1
Add additional modes for specific link speed. Use ethtool APIs to get the
current link speed and enable the LED accordingly. Under netdev event
handler the rtnl lock is already held and is not needed to be set to
access ethtool APIs.
This is especially useful for PHY and Switch that supports LEDs hw
control for specific link speed. (example scenario a PHY that have 2 LED
connected one green and one orange where the green is turned on with
1000mbps speed and orange is turned on with 10mpbs speed)
On mode set from sysfs we check if we have enabled split link speed mode
and reject enabling generic link mode to prevent wrong and redundant
configuration.
Rework logic on the set baseline state to support these new modes to
select if we need to turn on or off the LED.
Add additional modes:
- link_10: Turn on LED when link speed is 10mbps
- link_100: Turn on LED when link speed is 100mbps
- link_1000: Turn on LED when link speed is 1000mbps
Signed-off-by: Christian Marangi <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 80 +++++++++++++++++++++++----
include/linux/leds.h | 3 +
2 files changed, 73 insertions(+), 10 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index c9b040bacbb0..8e6132f069af 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -13,6 +13,7 @@
#include <linux/atomic.h>
#include <linux/ctype.h>
#include <linux/device.h>
+#include <linux/ethtool.h>
#include <linux/init.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
@@ -21,6 +22,7 @@
#include <linux/module.h>
#include <linux/netdevice.h>
#include <linux/mutex.h>
+#include <linux/rtnetlink.h>
#include <linux/timer.h>
#include "../leds.h"
@@ -52,6 +54,8 @@ struct led_netdev_data {
unsigned int last_activity;
unsigned long mode;
+ int link_speed;
+
bool carrier_link_up;
bool hw_control;
};
@@ -77,7 +81,24 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
if (!trigger_data->carrier_link_up) {
led_set_brightness(led_cdev, LED_OFF);
} else {
+ bool blink_on = false;
+
if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
+ blink_on = true;
+
+ if (test_bit(TRIGGER_NETDEV_LINK_10, &trigger_data->mode) &&
+ trigger_data->link_speed == SPEED_10)
+ blink_on = true;
+
+ if (test_bit(TRIGGER_NETDEV_LINK_100, &trigger_data->mode) &&
+ trigger_data->link_speed == SPEED_100)
+ blink_on = true;
+
+ if (test_bit(TRIGGER_NETDEV_LINK_1000, &trigger_data->mode) &&
+ trigger_data->link_speed == SPEED_1000)
+ blink_on = true;
+
+ if (blink_on)
led_set_brightness(led_cdev,
led_cdev->blink_brightness);
else
@@ -161,6 +182,18 @@ static bool can_hw_control(struct led_netdev_data *trigger_data)
return true;
}
+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)
+ return;
+
+ if (!__ethtool_get_link_ksettings(trigger_data->net_dev, &cmd))
+ trigger_data->link_speed = cmd.base.speed;
+}
+
static ssize_t device_name_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -196,8 +229,12 @@ static int set_device_name(struct led_netdev_data *trigger_data,
dev_get_by_name(&init_net, trigger_data->device_name);
trigger_data->carrier_link_up = false;
- if (trigger_data->net_dev != NULL)
- trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
+ trigger_data->link_speed = SPEED_UNKNOWN;
+ if (trigger_data->net_dev != NULL) {
+ rtnl_lock();
+ get_device_state(trigger_data);
+ rtnl_unlock();
+ }
trigger_data->last_activity = 0;
@@ -234,6 +271,9 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
switch (attr) {
case TRIGGER_NETDEV_LINK:
+ case TRIGGER_NETDEV_LINK_10:
+ case TRIGGER_NETDEV_LINK_100:
+ case TRIGGER_NETDEV_LINK_1000:
case TRIGGER_NETDEV_TX:
case TRIGGER_NETDEV_RX:
bit = attr;
@@ -249,7 +289,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
size_t size, enum led_trigger_netdev_modes attr)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
- unsigned long state;
+ unsigned long state, mode = trigger_data->mode;
int ret;
int bit;
@@ -259,6 +299,9 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
switch (attr) {
case TRIGGER_NETDEV_LINK:
+ case TRIGGER_NETDEV_LINK_10:
+ case TRIGGER_NETDEV_LINK_100:
+ case TRIGGER_NETDEV_LINK_1000:
case TRIGGER_NETDEV_TX:
case TRIGGER_NETDEV_RX:
bit = attr;
@@ -267,13 +310,20 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
return -EINVAL;
}
- cancel_delayed_work_sync(&trigger_data->work);
-
if (state)
- set_bit(bit, &trigger_data->mode);
+ set_bit(bit, &mode);
else
- clear_bit(bit, &trigger_data->mode);
+ clear_bit(bit, &mode);
+
+ 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)))
+ return -EINVAL;
+
+ cancel_delayed_work_sync(&trigger_data->work);
+ trigger_data->mode = mode;
trigger_data->hw_control = can_hw_control(trigger_data);
set_baseline_state(trigger_data);
@@ -295,6 +345,9 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
static DEVICE_ATTR_RW(trigger_name)
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(tx, TRIGGER_NETDEV_TX);
DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
@@ -338,6 +391,9 @@ static DEVICE_ATTR_RW(interval);
static struct attribute *netdev_trig_attrs[] = {
&dev_attr_device_name.attr,
&dev_attr_link.attr,
+ &dev_attr_link_10.attr,
+ &dev_attr_link_100.attr,
+ &dev_attr_link_1000.attr,
&dev_attr_rx.attr,
&dev_attr_tx.attr,
&dev_attr_interval.attr,
@@ -368,9 +424,10 @@ static int netdev_trig_notify(struct notifier_block *nb,
mutex_lock(&trigger_data->lock);
trigger_data->carrier_link_up = false;
+ trigger_data->link_speed = SPEED_UNKNOWN;
switch (evt) {
case NETDEV_CHANGENAME:
- trigger_data->carrier_link_up = netif_carrier_ok(dev);
+ get_device_state(trigger_data);
fallthrough;
case NETDEV_REGISTER:
dev_put(trigger_data->net_dev);
@@ -383,7 +440,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
break;
case NETDEV_UP:
case NETDEV_CHANGE:
- trigger_data->carrier_link_up = netif_carrier_ok(dev);
+ get_device_state(trigger_data);
break;
}
@@ -426,7 +483,10 @@ static void netdev_trig_work(struct work_struct *work)
if (trigger_data->last_activity != new_activity) {
led_stop_software_blink(trigger_data->led_cdev);
- invert = test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode);
+ invert = test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode) ||
+ 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);
interval = jiffies_to_msecs(
atomic_read(&trigger_data->interval));
/* base state is ON (link present) */
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 4b3d8bda1fff..39f15b1e772c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -582,6 +582,9 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
/* Trigger specific enum */
enum led_trigger_netdev_modes {
TRIGGER_NETDEV_LINK = 0,
+ TRIGGER_NETDEV_LINK_10,
+ TRIGGER_NETDEV_LINK_100,
+ TRIGGER_NETDEV_LINK_1000,
TRIGGER_NETDEV_TX,
TRIGGER_NETDEV_RX,
--
2.40.1
On Sat, 17 Jun 2023, Christian Marangi wrote:
> This is a continue of [1]. It was decided to take a more gradual
> approach to implement LEDs support for switch and phy starting with
> basic support and then implementing the hw control part when we have all
> the prereq done.
>
> This should be the final part for the netdev trigger.
> I added net-next tag and added netdev mailing list since I was informed
> that this should be merged with netdev branch.
>
> We collect some info around and we found a good set of modes that are
> common in almost all the PHY and Switch.
>
> These modes are:
> - Modes for dedicated link speed(10, 100, 1000 mbps). Additional mode
> can be added later following this example.
> - Modes for half and full duplex.
>
> The original idea was to add hw control only modes.
> While the concept makes sense in practice it would results in lots of
> additional code and extra check to make sure we are setting correct modes.
>
> With the suggestion from Andrew it was pointed out that using the ethtool
> APIs we can actually get the current link speed and duplex and this
> effectively removed the problem of having hw control only modes since we
> can fallback to software.
>
> Since these modes are supported by software, we can skip providing an
> user for this in the LED driver to support hw control for these new modes
> (that will come right after this is merged) and prevent this to be another
> multi subsystem series.
>
> For link speed and duplex we use ethtool APIs.
>
> To call ethtool APIs, rtnl lock is needed but this can be skipped on
> handling netdev events as the lock is already held.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> Changes v4:
> - Add net-next tag
> - Add additional patch to expose hw_control via sysfs
> - CC netdev mailing list
> Changes v3:
> - Add Andrew review tag
> - Use SPEED_UNKNOWN to init link_speed
> - Fix using HALF_DUPLEX as duplex init and use DUPLEX_UNKNOWN instead
> Changes v2:
> - Drop ACTIVITY patch as it can be handled internally in the LED driver
> - Reduce duplicate code and move the link state to a dedicated helper
>
> Christian Marangi (3):
> leds: trigger: netdev: add additional specific link speed mode
> leds: trigger: netdev: add additional specific link duplex mode
> leds: trigger: netdev: expose hw_control status via sysfs
>
> drivers/leds/trigger/ledtrig-netdev.c | 114 +++++++++++++++++++++++---
> include/linux/leds.h | 5 ++
> 2 files changed, 109 insertions(+), 10 deletions(-)
Seeing as we're on -rc7 already, any reason why we shouldn't hold off
and simply apply these against LEDs once v6.5 is released?
--
Lee Jones [李琼斯]
> Seeing as we're on -rc7 already, any reason why we shouldn't hold off
> and simply apply these against LEDs once v6.5 is released?
Each subsystem has its own policies. netdev tends to accept patches
right up until the merge window opens, sometimes even a couple of days
into the merge window for low risk changes. Maybe this is because
netdev is fast moving, two weeks of not merging results in a big
backlog of patches, making it a bumpy restart once merging is started
again. And is some of those late patches breaks something, there is
still 7 weeks to fix it.
Since this is cross subsystems i would expect both subsystems
Maintainers to agree to a merge or not. If you want to be more
conservative than netdev, wait until after the next merge window,
please say so.
If you do decided to wait, you are going to need to create another
stable branch to pull into netdev. I know it is not a huge overhead,
but it is still work, coordination etc.
Andrew
On Sat, Jun 17, 2023 at 01:53:52PM +0200, Christian Marangi wrote:
> This is a continue of [1]. It was decided to take a more gradual
> approach to implement LEDs support for switch and phy starting with
> basic support and then implementing the hw control part when we have all
> the prereq done.
>
> This should be the final part for the netdev trigger.
> I added net-next tag and added netdev mailing list since I was informed
> that this should be merged with netdev branch.
>
> We collect some info around and we found a good set of modes that are
> common in almost all the PHY and Switch.
>
> These modes are:
> - Modes for dedicated link speed(10, 100, 1000 mbps). Additional mode
> can be added later following this example.
> - Modes for half and full duplex.
>
> The original idea was to add hw control only modes.
> While the concept makes sense in practice it would results in lots of
> additional code and extra check to make sure we are setting correct modes.
>
> With the suggestion from Andrew it was pointed out that using the ethtool
> APIs we can actually get the current link speed and duplex and this
> effectively removed the problem of having hw control only modes since we
> can fallback to software.
>
> Since these modes are supported by software, we can skip providing an
> user for this in the LED driver to support hw control for these new modes
> (that will come right after this is merged) and prevent this to be another
> multi subsystem series.
>
> For link speed and duplex we use ethtool APIs.
>
> To call ethtool APIs, rtnl lock is needed but this can be skipped on
> handling netdev events as the lock is already held.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
Hi Christian,
I am sorry if I am missing something obvious here,
but this series does not appear to apply on top of net-next.
Please consider rebasing and reposting.
As you probably know, you can include the reviewed-by tags
provided by Andrew for this posting, unless there are
substantial changes.
--
pw-bot: changes-requested
On Mon, Jun 19, 2023 at 10:27:05PM +0200, Simon Horman wrote:
> On Sat, Jun 17, 2023 at 01:53:52PM +0200, Christian Marangi wrote:
> > This is a continue of [1]. It was decided to take a more gradual
> > approach to implement LEDs support for switch and phy starting with
> > basic support and then implementing the hw control part when we have all
> > the prereq done.
> >
> > This should be the final part for the netdev trigger.
> > I added net-next tag and added netdev mailing list since I was informed
> > that this should be merged with netdev branch.
> >
> > We collect some info around and we found a good set of modes that are
> > common in almost all the PHY and Switch.
> >
> > These modes are:
> > - Modes for dedicated link speed(10, 100, 1000 mbps). Additional mode
> > can be added later following this example.
> > - Modes for half and full duplex.
> >
> > The original idea was to add hw control only modes.
> > While the concept makes sense in practice it would results in lots of
> > additional code and extra check to make sure we are setting correct modes.
> >
> > With the suggestion from Andrew it was pointed out that using the ethtool
> > APIs we can actually get the current link speed and duplex and this
> > effectively removed the problem of having hw control only modes since we
> > can fallback to software.
> >
> > Since these modes are supported by software, we can skip providing an
> > user for this in the LED driver to support hw control for these new modes
> > (that will come right after this is merged) and prevent this to be another
> > multi subsystem series.
> >
> > For link speed and duplex we use ethtool APIs.
> >
> > To call ethtool APIs, rtnl lock is needed but this can be skipped on
> > handling netdev events as the lock is already held.
> >
> > [1] https://lore.kernel.org/lkml/[email protected]/
>
> Hi Christian,
>
> I am sorry if I am missing something obvious here,
> but this series does not appear to apply on top of net-next.
>
> Please consider rebasing and reposting.
>
> As you probably know, you can include the reviewed-by tags
> provided by Andrew for this posting, unless there are
> substantial changes.
>
> --
> pw-bot: changes-requested
>
Hi, sorry for the mistake. I just sent v5 and added the additional
Review-by tag.
--
Ansuel
On Mon, 19 Jun 2023, Andrew Lunn wrote:
> > Seeing as we're on -rc7 already, any reason why we shouldn't hold off
> > and simply apply these against LEDs once v6.5 is released?
>
> Each subsystem has its own policies. netdev tends to accept patches
> right up until the merge window opens, sometimes even a couple of days
> into the merge window for low risk changes. Maybe this is because
> netdev is fast moving, two weeks of not merging results in a big
> backlog of patches, making it a bumpy restart once merging is started
> again. And is some of those late patches breaks something, there is
> still 7 weeks to fix it.
>
> Since this is cross subsystems i would expect both subsystems
> Maintainers to agree to a merge or not. If you want to be more
> conservative than netdev, wait until after the next merge window,
> please say so.
>
> If you do decided to wait, you are going to need to create another
> stable branch to pull into netdev. I know it is not a huge overhead,
> but it is still work, coordination etc.
Can you clarify you last point for me please?
--
Lee Jones [李琼斯]
> > If you do decided to wait, you are going to need to create another
> > stable branch to pull into netdev. I know it is not a huge overhead,
> > but it is still work, coordination etc.
>
> Can you clarify you last point for me please?
This patchset extends the conditions on which the trigger blinks the
LED. It adds a couple more values to enum led_trigger_netdev_modes in
include/linux/leds.h. Once it gets merged, i will have a followup
patch extending the Marvell PHY driver to make us of them. It will
need these additional enum values. I also expect other PHY drivers to
gain support for them. Probably the dp83867.c driver since Alexander
Stein already has a patch merged adding support for what the current
API supports.
If we merge this patchset now via netdev, -rc1 should have everything
we need for this continuing development work. If we wait to merge
these patches until -rc1, only the LED tree has the needed patches, so
these network drivers will need a stable branch we can pull into
netdev.
Both ways work, we can do either. But it is probably easier for
everybody to merge now via netdev.
Andrew
On Tue, 20 Jun 2023, Andrew Lunn wrote:
> > > If you do decided to wait, you are going to need to create another
> > > stable branch to pull into netdev. I know it is not a huge overhead,
> > > but it is still work, coordination etc.
> >
> > Can you clarify you last point for me please?
>
> This patchset extends the conditions on which the trigger blinks the
> LED. It adds a couple more values to enum led_trigger_netdev_modes in
> include/linux/leds.h. Once it gets merged, i will have a followup
> patch extending the Marvell PHY driver to make us of them. It will
> need these additional enum values. I also expect other PHY drivers to
> gain support for them. Probably the dp83867.c driver since Alexander
> Stein already has a patch merged adding support for what the current
> API supports.
>
> If we merge this patchset now via netdev, -rc1 should have everything
> we need for this continuing development work. If we wait to merge
> these patches until -rc1, only the LED tree has the needed patches, so
> these network drivers will need a stable branch we can pull into
> netdev.
>
> Both ways work, we can do either. But it is probably easier for
> everybody to merge now via netdev.
Got it, thanks.
--
Lee Jones [李琼斯]