[PATCH v1 net-next] leds: trigger: netdev: Add support for tx_err and rx_err notification with LEDs

[PATCH v1 net-next] leds: trigger: netdev: Add support for tx_err and rx_err notification with LEDs

[Lukasz Majewski]

This patch provides support for enabling blinking of LEDs when RX or TX
errors are detected.

Approach taken in this patch is similar to one for TX or RX data
transmission indication (i.e. TRIGGER_NETDEV_TX/RX attribute).

One can inspect transmission errors with:
ip -s link show eth0

Example LED configuration:
cd /sys/devices/platform/amba_pl@0/a001a000.leds/leds/
echo netdev > mode:blue/trigger && \
echo eth0 > mode:blue/device_name && \
echo 1 > mode:blue/tx_err

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/leds/trigger/ledtrig-netdev.c | 24 +++++++++++++++++++++---
 include/linux/leds.h                  |  2 ++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 22bba8e97642..4b0863db901a 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -39,6 +39,8 @@
  *         (has carrier) or not
  * tx -  LED blinks on transmitted data
  * rx -  LED blinks on receive data
+ * tx_err -  LED blinks on transmit error
+ * rx_err -  LED blinks on receive error
  *
  * Note: If the user selects a mode that is not supported by hw, default
  * behavior is to fall back to software control of the LED. However not every
@@ -144,7 +146,9 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 		 * checking stats
 		 */
 		if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ||
-		    test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
+		    test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) ||
+		    test_bit(TRIGGER_NETDEV_TX_ERR, &trigger_data->mode) ||
+		    test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode))
 			schedule_delayed_work(&trigger_data->work, 0);
 	}
 }
@@ -337,6 +341,8 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
+	case TRIGGER_NETDEV_TX_ERR:
+	case TRIGGER_NETDEV_RX_ERR:
 		bit = attr;
 		break;
 	default:
@@ -371,6 +377,8 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	case TRIGGER_NETDEV_FULL_DUPLEX:
 	case TRIGGER_NETDEV_TX:
 	case TRIGGER_NETDEV_RX:
+	case TRIGGER_NETDEV_TX_ERR:
+	case TRIGGER_NETDEV_RX_ERR:
 		bit = attr;
 		break;
 	default:
@@ -429,6 +437,8 @@ 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);
+DEFINE_NETDEV_TRIGGER(tx_err, TRIGGER_NETDEV_TX_ERR);
+DEFINE_NETDEV_TRIGGER(rx_err, TRIGGER_NETDEV_RX_ERR);
 
 static ssize_t interval_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
@@ -538,6 +548,8 @@ static struct attribute *netdev_trig_attrs[] = {
 	&dev_attr_half_duplex.attr,
 	&dev_attr_rx.attr,
 	&dev_attr_tx.attr,
+	&dev_attr_rx_err.attr,
+	&dev_attr_tx_err.attr,
 	&dev_attr_interval.attr,
 	&dev_attr_offloaded.attr,
 	NULL
@@ -628,7 +640,9 @@ static void netdev_trig_work(struct work_struct *work)
 
 	/* If we are not looking for RX/TX then return  */
 	if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
-	    !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
+	    !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) &&
+	    !test_bit(TRIGGER_NETDEV_TX_ERR, &trigger_data->mode) &&
+	    !test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode))
 		return;
 
 	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
@@ -636,7 +650,11 @@ static void netdev_trig_work(struct work_struct *work)
 	    (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ?
 		dev_stats->tx_packets : 0) +
 	    (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) ?
-		dev_stats->rx_packets : 0);
+		dev_stats->rx_packets : 0) +
+	    (test_bit(TRIGGER_NETDEV_TX_ERR, &trigger_data->mode) ?
+		dev_stats->tx_errors : 0) +
+	    (test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode) ?
+		dev_stats->rx_errors : 0);
 
 	if (trigger_data->last_activity != new_activity) {
 		led_stop_software_blink(trigger_data->led_cdev);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6300313c46b7..c4087c15e61d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -574,6 +574,8 @@ enum led_trigger_netdev_modes {
 	TRIGGER_NETDEV_FULL_DUPLEX,
 	TRIGGER_NETDEV_TX,
 	TRIGGER_NETDEV_RX,
+	TRIGGER_NETDEV_TX_ERR,
+	TRIGGER_NETDEV_RX_ERR,
 
 	/* Keep last */
 	__TRIGGER_NETDEV_MAX,
-- 
2.39.2

Re: [PATCH v1 net-next] leds: trigger: netdev: Add support for tx_err and rx_err notification with LEDs

[Andrew Lunn]

On Wed, Jul 10, 2024 at 12:06:51PM +0200, Lukasz Majewski wrote:
> This patch provides support for enabling blinking of LEDs when RX or TX
> errors are detected.
> 
> Approach taken in this patch is similar to one for TX or RX data
> transmission indication (i.e. TRIGGER_NETDEV_TX/RX attribute).
> 
> One can inspect transmission errors with:
> ip -s link show eth0
> 
> Example LED configuration:
> cd /sys/devices/platform/amba_pl@0/a001a000.leds/leds/
> echo netdev > mode:blue/trigger && \
> echo eth0 > mode:blue/device_name && \
> echo 1 > mode:blue/tx_err

When i look at the machines around me, they all have an error count of
0. Do you have a real customer use case for this? What sort of systems
do you have which do have sufficient errors to justify an LED?

There is no standardisation of LEDs. Every vendor implements something
different. What i don't want is lots of different blink patterns,
which nobody ever uses.

	Andrew

Re: [PATCH v1 net-next] leds: trigger: netdev: Add support for tx_err and rx_err notification with LEDs

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

Hi Andrew,

> On Wed, Jul 10, 2024 at 12:06:51PM +0200, Lukasz Majewski wrote:
> > This patch provides support for enabling blinking of LEDs when RX
> > or TX errors are detected.
> > 
> > Approach taken in this patch is similar to one for TX or RX data
> > transmission indication (i.e. TRIGGER_NETDEV_TX/RX attribute).
> > 
> > One can inspect transmission errors with:
> > ip -s link show eth0
> > 
> > Example LED configuration:
> > cd /sys/devices/platform/amba_pl@0/a001a000.leds/leds/
> > echo netdev > mode:blue/trigger && \
> > echo eth0 > mode:blue/device_name && \
> > echo 1 > mode:blue/tx_err  
> 
> When i look at the machines around me, they all have an error count of
> 0. 

This is mostly true for ethernet. However, it happens on some low-level
drivers that errors field is not zero when e.g. the received frame is
malformed due to harsh work environment.

> Do you have a real customer use case for this? 

Yes.

The problem is apparent mostly with can interfaces and transmission for
it.

> What sort of systems
> do you have which do have sufficient errors to justify an LED?

In my case it is an embedded industrial/automotive controller with 10+
RGB LEDs. It only has LEDs to communicate with user.

The way how and when they blink shows the status of the device.

With this patch one is able to spot if some transmission is failing
(among other things).

> 
> There is no standardisation of LEDs. Every vendor implements something
> different. What i don't want is lots of different blink patterns,
> which nobody ever uses.

As you can assess from the code - this patch extends neatly the current
code, with use case valid for my customer.

> 
> 	Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 net-next] leds: trigger: netdev: Add support for tx_err and rx_err notification with LEDs

[Andrew Lunn]

On Wed, Jul 10, 2024 at 12:06:51PM +0200, Lukasz Majewski wrote:
> This patch provides support for enabling blinking of LEDs when RX or TX
> errors are detected.
> 
> Approach taken in this patch is similar to one for TX or RX data
> transmission indication (i.e. TRIGGER_NETDEV_TX/RX attribute).
> 
> One can inspect transmission errors with:
> ip -s link show eth0
> 
> Example LED configuration:
> cd /sys/devices/platform/amba_pl@0/a001a000.leds/leds/
> echo netdev > mode:blue/trigger && \
> echo eth0 > mode:blue/device_name && \
> echo 1 > mode:blue/tx_err
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: (subset) [PATCH v1 net-next] leds: trigger: netdev: Add support for tx_err and rx_err notification with LEDs

[Lee Jones]

On Wed, 10 Jul 2024 12:06:51 +0200, Lukasz Majewski wrote:
> This patch provides support for enabling blinking of LEDs when RX or TX
> errors are detected.
> 
> Approach taken in this patch is similar to one for TX or RX data
> transmission indication (i.e. TRIGGER_NETDEV_TX/RX attribute).
> 
> One can inspect transmission errors with:
> ip -s link show eth0
> 
> [...]

Applied, thanks!

[1/1] leds: trigger: netdev: Add support for tx_err and rx_err notification with LEDs
      commit: 8ed8ccdc279cf840a456c9dddb572fbee45ab6ae

--
Lee Jones [李琼斯]