diff mbox series

[v7,09/11] leds: trigger: netdev: add additional hardware only triggers

Message ID 20221214235438.30271-10-ansuelsmth@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series Adds support for PHY LEDs with offload triggers | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1173 this patch: 1173
netdev/cc_maintainers success CCed 3 of 3 maintainers
netdev/build_clang success Errors and warnings before: 259 this patch: 259
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1212 this patch: 1212
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Christian Marangi Dec. 14, 2022, 11:54 p.m. UTC
Add additional hardware only triggers commonly supported by switch LEDs.

Additional modes:
link_10: LED on with link up AND speed 10mbps
link_100: LED on with link up AND speed 100mbps
link_1000: LED on with link up AND speed 1000mbps
half_duplex: LED on with link up AND half_duplex mode
full_duplex: LED on with link up AND full duplex mode

Additional blink interval modes:
blink_2hz: LED blink on any even at 2Hz (250ms)
blink_4hz: LED blink on any even at 4Hz (125ms)
blink_8hz: LED blink on any even at 8Hz (62ms)

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 58 +++++++++++++++++++++++++++
 include/linux/leds.h                  | 10 +++++
 2 files changed, 68 insertions(+)

Comments

Russell King (Oracle) Dec. 15, 2022, 5:35 p.m. UTC | #1
On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> Add additional hardware only triggers commonly supported by switch LEDs.
> 
> Additional modes:
> link_10: LED on with link up AND speed 10mbps
> link_100: LED on with link up AND speed 100mbps
> link_1000: LED on with link up AND speed 1000mbps
> half_duplex: LED on with link up AND half_duplex mode
> full_duplex: LED on with link up AND full duplex mode

Looking at Marvell 88e151x, I don't think this is usable there.
We have the option of supporting link_1000 on one of the LEDs,
link_100 on another, and link_10 on the other. It's rather rare
for all three leds to be wired though.

This is also a PHY where "activity" mode is supported (illuminated
or blinking if any traffic is transmitted or received) but may not
support individual directional traffic in hardware. However, it
does support forcing the LED on or off, so software mode can handle
those until the user selects a combination of modes that are
supported in the hardware.

> Additional blink interval modes:
> blink_2hz: LED blink on any even at 2Hz (250ms)
> blink_4hz: LED blink on any even at 4Hz (125ms)
> blink_8hz: LED blink on any even at 8Hz (62ms)

This seems too restrictive. For example, Marvell 88e151x supports
none of these, but does support 42, 84, 170, 340, 670ms.
Christian Marangi Dec. 16, 2022, 5:17 p.m. UTC | #2
On Thu, Dec 15, 2022 at 05:35:47PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> > Add additional hardware only triggers commonly supported by switch LEDs.
> > 
> > Additional modes:
> > link_10: LED on with link up AND speed 10mbps
> > link_100: LED on with link up AND speed 100mbps
> > link_1000: LED on with link up AND speed 1000mbps
> > half_duplex: LED on with link up AND half_duplex mode
> > full_duplex: LED on with link up AND full duplex mode
> 
> Looking at Marvell 88e151x, I don't think this is usable there.
> We have the option of supporting link_1000 on one of the LEDs,
> link_100 on another, and link_10 on the other. It's rather rare
> for all three leds to be wired though.

On qca8k it's just anarchy. You can apply the same rule table to
whatever led you want. They are all the same... OEM decide what to do
(add white led, amber, green...)

Most common configuration is 2 leds, one react with port 1000 and the
other with port 100. But each led can be set to be powered on to any
speed by enabling 10 100 and 1000 rule.

Rejecting a configuration falls in the driver returning error on
configure.

> 
> This is also a PHY where "activity" mode is supported (illuminated
> or blinking if any traffic is transmitted or received) but may not
> support individual directional traffic in hardware. However, it
> does support forcing the LED on or off, so software mode can handle
> those until the user selects a combination of modes that are
> supported in the hardware.
> 
> > Additional blink interval modes:
> > blink_2hz: LED blink on any even at 2Hz (250ms)
> > blink_4hz: LED blink on any even at 4Hz (125ms)
> > blink_8hz: LED blink on any even at 8Hz (62ms)
> 
> This seems too restrictive. For example, Marvell 88e151x supports
> none of these, but does support 42, 84, 170, 340, 670ms.
> 

Eh this is really bad, it was an idea to support blink internal for each
event but I expected other switch had something different... But the
generic function is still there...

Wonder if we should consider adding generic naming like SLOW, NORMAL(?),
FAST. And just assign them from the driver side?

Permit to have some kind of configuration while also keeping it generic
enough? 

Just as reference... The blink mode already had a compromise since qca8k
can support a way to blink differently based on the link speed.
Andrew Lunn Dec. 21, 2022, 12:12 a.m. UTC | #3
On Thu, Dec 15, 2022 at 05:35:47PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> > Add additional hardware only triggers commonly supported by switch LEDs.
> > 
> > Additional modes:
> > link_10: LED on with link up AND speed 10mbps
> > link_100: LED on with link up AND speed 100mbps
> > link_1000: LED on with link up AND speed 1000mbps
> > half_duplex: LED on with link up AND half_duplex mode
> > full_duplex: LED on with link up AND full duplex mode
> 
> Looking at Marvell 88e151x, I don't think this is usable there.
> We have the option of supporting link_1000 on one of the LEDs,
> link_100 on another, and link_10 on the other. It's rather rare
> for all three leds to be wired though.

The 88e151x will need to enumerate what it actually supports from the
above list, per LED. I also think we can carefully expand the list
above, adding a few more modes. We just need to ensure what is added
is reasonably generic, modes we expect multiple PHY to support. What
we need to avoid is adding every single mode a PHY supports, but no
other PHY has.

> This is also a PHY where "activity" mode is supported (illuminated
> or blinking if any traffic is transmitted or received) but may not
> support individual directional traffic in hardware. However, it
> does support forcing the LED on or off, so software mode can handle
> those until the user selects a combination of modes that are
> supported in the hardware.
> 
> > Additional blink interval modes:
> > blink_2hz: LED blink on any even at 2Hz (250ms)
> > blink_4hz: LED blink on any even at 4Hz (125ms)
> > blink_8hz: LED blink on any even at 8Hz (62ms)
> 
> This seems too restrictive. For example, Marvell 88e151x supports
> none of these, but does support 42, 84, 170, 340, 670ms.

I would actually drop this whole idea of being able to configure the
blink period. It seems like it is going to cause problems. I expect
most PHYs actual share the period across multiple LEDs, which you
cannot easily model here.

So i would have the driver hard coded to pick a frequency at thats' it.

   Andrew
Christian Marangi Dec. 21, 2022, 12:56 p.m. UTC | #4
On Wed, Dec 21, 2022 at 01:12:16AM +0100, Andrew Lunn wrote:
> On Thu, Dec 15, 2022 at 05:35:47PM +0000, Russell King (Oracle) wrote:
> > On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> > > Add additional hardware only triggers commonly supported by switch LEDs.
> > > 
> > > Additional modes:
> > > link_10: LED on with link up AND speed 10mbps
> > > link_100: LED on with link up AND speed 100mbps
> > > link_1000: LED on with link up AND speed 1000mbps
> > > half_duplex: LED on with link up AND half_duplex mode
> > > full_duplex: LED on with link up AND full duplex mode
> > 
> > Looking at Marvell 88e151x, I don't think this is usable there.
> > We have the option of supporting link_1000 on one of the LEDs,
> > link_100 on another, and link_10 on the other. It's rather rare
> > for all three leds to be wired though.
> 
> The 88e151x will need to enumerate what it actually supports from the
> above list, per LED. I also think we can carefully expand the list
> above, adding a few more modes. We just need to ensure what is added
> is reasonably generic, modes we expect multiple PHY to support. What
> we need to avoid is adding every single mode a PHY supports, but no
> other PHY has.
> 
> > This is also a PHY where "activity" mode is supported (illuminated
> > or blinking if any traffic is transmitted or received) but may not
> > support individual directional traffic in hardware. However, it
> > does support forcing the LED on or off, so software mode can handle
> > those until the user selects a combination of modes that are
> > supported in the hardware.
> > 
> > > Additional blink interval modes:
> > > blink_2hz: LED blink on any even at 2Hz (250ms)
> > > blink_4hz: LED blink on any even at 4Hz (125ms)
> > > blink_8hz: LED blink on any even at 8Hz (62ms)
> > 
> > This seems too restrictive. For example, Marvell 88e151x supports
> > none of these, but does support 42, 84, 170, 340, 670ms.
> 
> I would actually drop this whole idea of being able to configure the
> blink period. It seems like it is going to cause problems. I expect
> most PHYs actual share the period across multiple LEDs, which you
> cannot easily model here.
> 
> So i would have the driver hard coded to pick a frequency at thats' it.
> 

Yes I think "for now" it's the only way and just drop blink
configuration support.
diff mbox series

Patch

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 3a3b77bb41fb..880d5e65f7a2 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -29,8 +29,20 @@ 
  *
  * device_name - network device name to monitor
  * interval - duration of LED blink, in milliseconds
+ *            (in hardware mode 2hz (62ms), 4hz (125ms) or 8hz (250ms)
+ *             are supported)
  * link -  LED's normal state reflects whether the link is up
  *         (has carrier) or not
+ * link_10 - LED's normal state reflects whether the link is
+ *           up and at 10mbps speed (hardware only)
+ * link_100 - LED's normal state reflects whether the link is
+ *            up and at 100mbps speed (hardware only)
+ * link_1000 - LED's normal state reflects whether the link is
+ *             up and at 1000mbps speed (hardware only)
+ * half_duplex - LED's normal state reflects whether the link is
+ *               up and hafl duplex (hardware only)
+ * full_duplex - LED's normal state reflects whether the link is
+ *               up and full duplex (hardware only)
  * tx -  LED blinks on transmitted data
  * rx -  LED blinks on receive data
  * available_mode - Display available mode and how they can be handled
@@ -64,8 +76,19 @@  struct netdev_led_attr_detail {
 
 static struct netdev_led_attr_detail attr_details[] = {
 	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
+	{ .name = "link_10", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_10},
+	{ .name = "link_100", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_100},
+	{ .name = "link_1000", .hardware_only = true, .bit = TRIGGER_NETDEV_LINK_1000},
+	{ .name = "half_duplex", .hardware_only = true, .bit = TRIGGER_NETDEV_HALF_DUPLEX},
+	{ .name = "full_duplex", .hardware_only = true, .bit = TRIGGER_NETDEV_FULL_DUPLEX},
 	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
 	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},
+	{ .name = "hw blink 2hz (interval set to 62)", .hardware_only = true,
+	  .bit = TRIGGER_NETDEV_BLINK_2HZ},
+	{ .name = "hw blink 4hz (interval set to 125)", .hardware_only = true,
+	  .bit = TRIGGER_NETDEV_BLINK_4HZ},
+	{ .name = "hw blink 8hz (interval set to 250)", .hardware_only = true,
+	  .bit = TRIGGER_NETDEV_BLINK_8HZ},
 };
 
 static bool validate_baseline_state(struct led_netdev_data *trigger_data)
@@ -259,6 +282,11 @@  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_HALF_DUPLEX:
+	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
 		bit = attr;
@@ -284,6 +312,11 @@  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_HALF_DUPLEX:
+	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
 		bit = attr;
@@ -324,6 +357,11 @@  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(half_duplex, TRIGGER_NETDEV_HALF_DUPLEX);
+DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX);
 DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
 DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
 
@@ -356,6 +394,21 @@  static ssize_t interval_store(struct device *dev,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
+	if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
+		/* Interval are handled as triggers. Reset them. */
+		trigger_data->mode &= ~(BIT(TRIGGER_NETDEV_BLINK_8HZ) |
+					BIT(TRIGGER_NETDEV_BLINK_4HZ) |
+					BIT(TRIGGER_NETDEV_BLINK_2HZ));
+
+		/* Support a common value of 2Hz, 4Hz and 8Hz. */
+		if (value > 5 && value <= 62) /* 8Hz */
+			trigger_data->mode |= BIT(TRIGGER_NETDEV_BLINK_8HZ);
+		else if (value > 63 && value <= 125) /* 4Hz */
+			trigger_data->mode |= BIT(TRIGGER_NETDEV_BLINK_4HZ);
+		else /* 2Hz */
+			trigger_data->mode |= BIT(TRIGGER_NETDEV_BLINK_2HZ);
+	}
+
 	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
 
 	if (!validate_baseline_state(trigger_data)) {
@@ -404,6 +457,11 @@  static DEVICE_ATTR_RO(available_mode);
 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_half_duplex.attr,
+	&dev_attr_full_duplex.attr,
 	&dev_attr_rx.attr,
 	&dev_attr_tx.attr,
 	&dev_attr_interval.attr,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 13862f8b1e07..5fcc6d233757 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -551,8 +551,18 @@  static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 /* Trigger specific enum */
 enum led_trigger_netdev_modes {
 	TRIGGER_NETDEV_LINK = 1,
+	TRIGGER_NETDEV_LINK_10,
+	TRIGGER_NETDEV_LINK_100,
+	TRIGGER_NETDEV_LINK_1000,
+	TRIGGER_NETDEV_HALF_DUPLEX,
+	TRIGGER_NETDEV_FULL_DUPLEX,
 	TRIGGER_NETDEV_TX,
 	TRIGGER_NETDEV_RX,
+
+	/* Hardware Interval options */
+	TRIGGER_NETDEV_BLINK_2HZ,
+	TRIGGER_NETDEV_BLINK_4HZ,
+	TRIGGER_NETDEV_BLINK_8HZ,
 };
 
 /* Trigger specific functions */