Message-ID: <0d6d9c75-4ef6-80b4-dba8-0523b0c27aa5@gmail.com>
Date:   Mon, 11 Dec 2017 22:53:28 +0100
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Ben Whitten <ben.whitten@...il.com>, rpurdie@...ys.net,
        pavel@....cz
Cc:     linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH v5] leds: trigger: Introduce a NETDEV trigger

Hi Ben,

Thanks for the update.

On 12/10/2017 10:17 PM, Ben Whitten wrote:
> This commit introduces a NETDEV trigger for named device
> activity. Available triggers are link, rx, and tx.
> 
> Signed-off-by: Ben Whitten <ben.whitten@...il.com>
> 
> ---
> Changes in v5:
> Adjust header comment style to be consistent
> Changes in v4:
> Adopt SPDX licence header
> Changes in v3:
> Cancel the software blink prior to a oneshot re-queue
> Changes in v2:
> Sort includes and redate documentation
> Correct licence
> Remove macro and replace with generic function using enums
> Convert blink logic in stats work to use led_blink_oneshot
> Uses configured brightness instead of FULL
> ---
>  .../ABI/testing/sysfs-class-led-trigger-netdev     |  45 ++
>  drivers/leds/trigger/Kconfig                       |   7 +
>  drivers/leds/trigger/Makefile                      |   1 +
>  drivers/leds/trigger/ledtrig-netdev.c              | 496 +++++++++++++++++++++
>  4 files changed, 549 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-netdev
>  create mode 100644 drivers/leds/trigger/ledtrig-netdev.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
> new file mode 100644
> index 0000000..451af6d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
> @@ -0,0 +1,45 @@
> +What:		/sys/class/leds/<led>/device_name
> +Date:		Dec 2017
> +KernelVersion:	4.16
> +Contact:	linux-leds@...r.kernel.org
> +Description:
> +		Specifies the network device name to monitor.
> +
> +What:		/sys/class/leds/<led>/interval
> +Date:		Dec 2017
> +KernelVersion:	4.16
> +Contact:	linux-leds@...r.kernel.org
> +Description:
> +		Specifies the duration of the LED blink in milliseconds.
> +		Defaults to 50 ms.
> +
> +What:		/sys/class/leds/<led>/link
> +Date:		Dec 2017
> +KernelVersion:	4.16
> +Contact:	linux-leds@...r.kernel.org
> +Description:
> +		Signal the link state of the named network device.
> +		If set to 0 (default), the LED's normal state is off.
> +		If set to 1, the LED's normal state reflects the link state
> +		of the named network device.
> +		Setting this value also immediately changes the LED state.
> +
> +What:		/sys/class/leds/<led>/tx
> +Date:		Dec 2017
> +KernelVersion:	4.16
> +Contact:	linux-leds@...r.kernel.org
> +Description:
> +		Signal transmission of data on the named network device.
> +		If set to 0 (default), the LED will not blink on transmission.
> +		If set to 1, the LED will blink for the milliseconds specified
> +		in interval to signal transmission.
> +
> +What:		/sys/class/leds/<led>/rx
> +Date:		Dec 2017
> +KernelVersion:	4.16
> +Contact:	linux-leds@...r.kernel.org
> +Description:
> +		Signal reception of data on the named network device.
> +		If set to 0 (default), the LED will not blink on reception.
> +		If set to 1, the LED will blink for the milliseconds specified
> +		in interval to signal reception.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 3f9ddb9..4ec1853 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -126,4 +126,11 @@ config LEDS_TRIGGER_PANIC
>  	  a different trigger.
>  	  If unsure, say Y.
>  
> +config LEDS_TRIGGER_NETDEV
> +	tristate "LED Netdev Trigger"
> +	depends on NET && LEDS_TRIGGERS
> +	help
> +	  This allows LEDs to be controlled by network device activity.
> +	  If unsure, say Y.
> +
>  endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 9f2e868..59e163d 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
>  obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
>  obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
>  obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
> +obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> new file mode 100644
> index 0000000..6df4781
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -0,0 +1,496 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2017 Ben Whitten <ben.whitten@...il.com>
> +// Copyright 2007 Oliver Jowett <oliver@...ncloud.com>
> +//
> +// LED Kernel Netdev Trigger
> +//
> +// Toggles the LED to reflect the link and traffic state of a named net device
> +//
> +// Derived from ledtrig-timer.c which is:
> +//  Copyright 2005-2006 Openedhand Ltd.
> +//  Author: Richard Purdie <rpurdie@...nedhand.com>
> +
> +#include <linux/atomic.h>
> +#include <linux/ctype.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
> +#include "../leds.h"
> +
> +/*
> + * Configurable sysfs attributes:
> + *
> + * device_name - network device name to monitor
> + * interval - duration of LED blink, in milliseconds
> + * link -  LED's normal state reflects whether the link is up
> + *         (has carrier) or not
> + * tx -  LED blinks on transmitted data
> + * rx -  LED blinks on receive data
> + *
> + */
> +
> +struct led_netdev_data {
> +	spinlock_t lock;
> +
> +	struct delayed_work work;
> +	struct notifier_block notifier;
> +
> +	struct led_classdev *led_cdev;
> +	struct net_device *net_dev;
> +
> +	char device_name[IFNAMSIZ];
> +	atomic_t interval;
> +	unsigned int last_activity;
> +
> +	unsigned long mode;
> +#define NETDEV_LED_LINK	0
> +#define NETDEV_LED_TX	1
> +#define NETDEV_LED_RX	2
> +#define NETDEV_LED_MODE_LINKUP	3
> +};
> +
> +enum netdev_led_attr {
> +	NETDEV_ATTR_LINK,
> +	NETDEV_ATTR_TX,
> +	NETDEV_ATTR_RX
> +};
> +
> +static void set_baseline_state(struct led_netdev_data *trigger_data)
> +{
> +	int current_brightness;
> +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> +
> +	current_brightness = led_cdev->brightness;
> +	if (current_brightness)
> +		led_cdev->blink_brightness = current_brightness;
> +	if (!led_cdev->blink_brightness)
> +		led_cdev->blink_brightness = led_cdev->max_brightness;
> +
> +	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
> +		led_set_brightness(led_cdev, LED_OFF);
> +	else {
> +		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
> +			led_set_brightness(led_cdev,
> +					   led_cdev->blink_brightness);
> +		else
> +			led_set_brightness(led_cdev, LED_OFF);
> +
> +		/* If we are looking for RX/TX start periodically
> +		 * checking stats
> +		 */
> +		if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
> +		    test_bit(NETDEV_LED_RX, &trigger_data->mode))
> +			schedule_delayed_work(&trigger_data->work, 0);
> +	}
> +}
> +
> +static ssize_t device_name_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_netdev_data *trigger_data = led_cdev->trigger_data;
> +	ssize_t len;
> +
> +	spin_lock_bh(&trigger_data->lock);
> +	len = sprintf(buf, "%s\n", trigger_data->device_name);
> +	spin_unlock_bh(&trigger_data->lock);
> +
> +	return len;
> +}
> +
> +static ssize_t device_name_store(struct device *dev,
> +				 struct device_attribute *attr, const char *buf,
> +				 size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_netdev_data *trigger_data = led_cdev->trigger_data;
> +
> +	if (size >= IFNAMSIZ)
> +		return -EINVAL;
> +
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
> +	spin_lock_bh(&trigger_data->lock);
> +
> +	if (trigger_data->net_dev) {
> +		dev_put(trigger_data->net_dev);
> +		trigger_data->net_dev = NULL;
> +	}
> +
> +	strncpy(trigger_data->device_name, buf, size);
> +	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
> +		trigger_data->device_name[size - 1] = 0;
> +
> +	if (trigger_data->device_name[0] != 0)
> +		trigger_data->net_dev =
> +		    dev_get_by_name(&init_net, trigger_data->device_name);
> +
> +	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +	if (trigger_data->net_dev != NULL)
> +		if (netif_carrier_ok(trigger_data->net_dev))
> +			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +
> +	trigger_data->last_activity = 0;
> +
> +	set_baseline_state(trigger_data);
> +	spin_unlock_bh(&trigger_data->lock);
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(device_name);
> +
> +static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
> +	enum netdev_led_attr attr)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_netdev_data *trigger_data = led_cdev->trigger_data;
> +	int bit;
> +
> +	switch (attr) {
> +	case NETDEV_ATTR_LINK:
> +		bit = NETDEV_LED_LINK;
> +		break;
> +	case NETDEV_ATTR_TX:
> +		bit = NETDEV_LED_TX;
> +		break;
> +	case NETDEV_ATTR_RX:
> +		bit = NETDEV_LED_RX;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
> +}
> +
> +static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
> +	size_t size, enum netdev_led_attr attr)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_netdev_data *trigger_data = led_cdev->trigger_data;
> +	unsigned long state;
> +	int ret;
> +	int bit;
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		return ret;
> +
> +	switch (attr) {
> +	case NETDEV_ATTR_LINK:
> +		bit = NETDEV_LED_LINK;
> +		break;
> +	case NETDEV_ATTR_TX:
> +		bit = NETDEV_LED_TX;
> +		break;
> +	case NETDEV_ATTR_RX:
> +		bit = NETDEV_LED_RX;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
> +	if (state)
> +		set_bit(bit, &trigger_data->mode);
> +	else
> +		clear_bit(bit, &trigger_data->mode);
> +
> +	set_baseline_state(trigger_data);
> +
> +	return size;
> +}
> +
> +static ssize_t link_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_LINK);
> +}
> +
> +static ssize_t link_store(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_LINK);
> +}
> +
> +static DEVICE_ATTR_RW(link);
> +
> +static ssize_t tx_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_TX);
> +}
> +
> +static ssize_t tx_store(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_TX);
> +}
> +
> +static DEVICE_ATTR_RW(tx);
> +
> +static ssize_t rx_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_RX);
> +}
> +
> +static ssize_t rx_store(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_RX);
> +}
> +
> +static DEVICE_ATTR_RW(rx);
> +
> +static ssize_t interval_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_netdev_data *trigger_data = led_cdev->trigger_data;
> +
> +	return sprintf(buf, "%u\n",
> +		       jiffies_to_msecs(atomic_read(&trigger_data->interval)));
> +}
> +
> +static ssize_t interval_store(struct device *dev,
> +			      struct device_attribute *attr, const char *buf,
> +			      size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_netdev_data *trigger_data = led_cdev->trigger_data;
> +	unsigned long value;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &value);
> +	if (ret)
> +		return ret;
> +
> +	/* impose some basic bounds on the timer interval */
> +	if (value >= 5 && value <= 10000) {
> +		cancel_delayed_work_sync(&trigger_data->work);
> +
> +		atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> +		set_baseline_state(trigger_data);	/* resets timer */
> +	}
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(interval);
> +
> +static int netdev_trig_notify(struct notifier_block *nb,
> +			      unsigned long evt, void *dv)
> +{
> +	struct net_device *dev =
> +		netdev_notifier_info_to_dev((struct netdev_notifier_info *)dv);
> +	struct led_netdev_data *trigger_data = container_of(nb,
> +							    struct
> +							    led_netdev_data,
> +							    notifier);
> +
> +	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
> +	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
> +	    && evt != NETDEV_CHANGENAME)
> +		return NOTIFY_DONE;
> +
> +	if (strcmp(dev->name, trigger_data->device_name))
> +		return NOTIFY_DONE;
> +
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
> +	spin_lock_bh(&trigger_data->lock);
> +
> +	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +	switch (evt) {
> +	case NETDEV_REGISTER:
> +		if (trigger_data->net_dev)
> +			dev_put(trigger_data->net_dev);
> +		dev_hold(dev);
> +		trigger_data->net_dev = dev;
> +		break;
> +	case NETDEV_CHANGENAME:
> +	case NETDEV_UNREGISTER:
> +		if (trigger_data->net_dev) {
> +			dev_put(trigger_data->net_dev);
> +			trigger_data->net_dev = NULL;
> +		}
> +		break;
> +	case NETDEV_UP:
> +	case NETDEV_CHANGE:
> +		if (netif_carrier_ok(dev))
> +			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +		break;
> +	}
> +
> +	set_baseline_state(trigger_data);
> +
> +	spin_unlock_bh(&trigger_data->lock);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +/* here's the real work! */
> +static void netdev_trig_work(struct work_struct *work)
> +{
> +	struct led_netdev_data *trigger_data = container_of(work,
> +							    struct
> +							    led_netdev_data,
> +							    work.work);
> +	struct rtnl_link_stats64 *dev_stats;
> +	unsigned int new_activity;
> +	struct rtnl_link_stats64 temp;
> +	unsigned long interval;
> +	int invert;
> +
> +	/* If we dont have a device, insure we are off */
> +	if (!trigger_data->net_dev) {
> +		led_set_brightness(trigger_data->led_cdev, LED_OFF);
> +		return;
> +	}
> +
> +	/* If we are not looking for RX/TX then return  */
> +	if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
> +	    !test_bit(NETDEV_LED_RX, &trigger_data->mode))
> +		return;
> +
> +	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
> +	new_activity =
> +	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
> +		dev_stats->tx_packets : 0) +
> +	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
> +		dev_stats->rx_packets : 0);
> +
> +	if (trigger_data->last_activity != new_activity) {
> +		led_stop_software_blink(trigger_data->led_cdev);
> +
> +		invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
> +		interval = jiffies_to_msecs(
> +				atomic_read(&trigger_data->interval));
> +		/* base state is ON (link present) */
> +		led_blink_set_oneshot(trigger_data->led_cdev,
> +				      &interval,
> +				      &interval,
> +				      invert);
> +		trigger_data->last_activity = new_activity;
> +	}
> +
> +	schedule_delayed_work(&trigger_data->work,
> +			(atomic_read(&trigger_data->interval)*2));
> +}
> +
> +static void netdev_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct led_netdev_data *trigger_data;
> +	int rc;
> +
> +	trigger_data = kzalloc(sizeof(struct led_netdev_data), GFP_KERNEL);
> +	if (!trigger_data)
> +		return;
> +
> +	spin_lock_init(&trigger_data->lock);
> +
> +	trigger_data->notifier.notifier_call = netdev_trig_notify;
> +	trigger_data->notifier.priority = 10;
> +
> +	INIT_DELAYED_WORK(&trigger_data->work, netdev_trig_work);
> +
> +	trigger_data->led_cdev = led_cdev;
> +	trigger_data->net_dev = NULL;
> +	trigger_data->device_name[0] = 0;
> +
> +	trigger_data->mode = 0;
> +	atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
> +	trigger_data->last_activity = 0;
> +
> +	led_cdev->trigger_data = trigger_data;
> +
> +	rc = device_create_file(led_cdev->dev, &dev_attr_device_name);
> +	if (rc)
> +		goto err_out;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_link);
> +	if (rc)
> +		goto err_out_device_name;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_rx);
> +	if (rc)
> +		goto err_out_link;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_tx);
> +	if (rc)
> +		goto err_out_rx;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_interval);
> +	if (rc)
> +		goto err_out_tx;
> +	rc = register_netdevice_notifier(&trigger_data->notifier);
> +	if (rc)
> +		goto err_out_interval;
> +	return;
> +
> +err_out_interval:
> +	device_remove_file(led_cdev->dev, &dev_attr_interval);
> +err_out_tx:
> +	device_remove_file(led_cdev->dev, &dev_attr_tx);
> +err_out_rx:
> +	device_remove_file(led_cdev->dev, &dev_attr_rx);
> +err_out_link:
> +	device_remove_file(led_cdev->dev, &dev_attr_link);
> +err_out_device_name:
> +	device_remove_file(led_cdev->dev, &dev_attr_device_name);
> +err_out:
> +	led_cdev->trigger_data = NULL;
> +	kfree(trigger_data);
> +}
> +
> +static void netdev_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct led_netdev_data *trigger_data = led_cdev->trigger_data;
> +
> +	if (trigger_data) {
> +		unregister_netdevice_notifier(&trigger_data->notifier);
> +
> +		device_remove_file(led_cdev->dev, &dev_attr_device_name);
> +		device_remove_file(led_cdev->dev, &dev_attr_link);
> +		device_remove_file(led_cdev->dev, &dev_attr_rx);
> +		device_remove_file(led_cdev->dev, &dev_attr_tx);
> +		device_remove_file(led_cdev->dev, &dev_attr_interval);
> +
> +		cancel_delayed_work_sync(&trigger_data->work);
> +
> +		if (trigger_data->net_dev)
> +			dev_put(trigger_data->net_dev);
> +
> +		kfree(trigger_data);
> +	}
> +}
> +
> +static struct led_trigger netdev_led_trigger = {
> +	.name = "netdev",
> +	.activate = netdev_trig_activate,
> +	.deactivate = netdev_trig_deactivate,
> +};
> +
> +static int __init netdev_trig_init(void)
> +{
> +	return led_trigger_register(&netdev_led_trigger);
> +}
> +
> +static void __exit netdev_trig_exit(void)
> +{
> +	led_trigger_unregister(&netdev_led_trigger);
> +}
> +
> +module_init(netdev_trig_init);
> +module_exit(netdev_trig_exit);
> +
> +MODULE_AUTHOR("Ben Whitten <ben.whitten@...il.com>");
> +MODULE_AUTHOR("Oliver Jowett <oliver@...ncloud.com>");
> +MODULE_DESCRIPTION("Netdev LED trigger");
> +MODULE_LICENSE("GPL v2");
> 

Applied to the for-next branch of linux-leds.git.

-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists