Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1237440 > unrolled thread

[PATCH v2 0/3] leds: add device trigger

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] leds: add device trigger Maciek Borzecki <maciek.borzecki@gmail.com> - 2015-10-01 16:10 +0200
    [PATCH v2 2/3] leds: add debugfs to device trigger Maciek Borzecki <maciek.borzecki@gmail.com> - 2015-10-01 16:10 +0200
    Re: [PATCH v2 0/3] leds: add device trigger Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-10-02 16:30 +0200
      Re: [PATCH v2 0/3] leds: add device trigger Maciek Borzecki <maciek.borzecki@gmail.com> - 2015-10-02 18:30 +0200

#1237440 — [PATCH v2 0/3] leds: add device trigger

A series of patches that add yet another LED trigger, a device
activity trigger.

The motivation is to have a LED trigger that is associated with a
device and can be fired from cetrain points in the code that have been
chosen by the developer. With this this device activity trigger it is
possible for instance to easily hook up a tty driver for a console to
blink one LED, yet another serial port to blink a second LED and
writes to a block device to trigger a third LED.

The patches have been tested on Wandboard Quad.

The first patch adds the actual trigger. Each device wishing to use
the trigger has to be explicitly registered by calling
ledtrig_dev_add(), and passing it's dev_t. The intention is that the
trigger will be used in scenarios that are impossible to foresee at
this moment, and are likely to be approach in a case by case manner
anyway.

The second patch adds couple of debugfs helpers.

The third patch adds documentation and notes on debugfs interface.

Example hooks into tty driver can be seen here:
https://github.com/bboozzoo/linux/commit/d8a875673e37b27d9c9066febe7633382f97d8af

Changes since v1:
  - fixed debugfs user address space access
  - added unregister debugfs attribute
  - documentation update

Maciek Borzecki (3):
  leds: add device activity LED triggers
  leds: add debugfs to device trigger
  Documentation: leds: document ledtrig-device trigger

 Documentation/leds/00-INDEX           |   3 +
 Documentation/leds/ledtrig-device.txt |  35 ++++
 drivers/leds/trigger/Kconfig          |   8 +
 drivers/leds/trigger/Makefile         |   1 +
 drivers/leds/trigger/ledtrig-device.c | 326 ++++++++++++++++++++++++++++++++++
 include/linux/leds.h                  |  10 ++
 6 files changed, 383 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-device.txt
 create mode 100644 drivers/leds/trigger/ledtrig-device.c

-- 
2.6.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1237442 — [PATCH v2 2/3] leds: add debugfs to device trigger

Add debugfs entries for device activity trigger. Three entries are
created under /sys/kernel/debug/ledtrig-dev when the driver gets
loaded. These are:

  devices - cat'ing will produce a list of currently registered devices
            in <major>:<minor> format, a line for each device.

  register - echo'ing <major>:<minor> will create a trigger for this
             device

  unregister - echo'ing <major>:<minor> will delete a trigger for this
               device

  trigger - echo'ing <major>:<minor> will fire a trigger for this device

Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---
 drivers/leds/trigger/ledtrig-device.c | 145 +++++++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-device.c b/drivers/leds/trigger/ledtrig-device.c
index dbb8d7d2b4a0258149c581a040c416d412d9ceeb..68f61fb6ebb05ceaee36c58f1a0244a66bf34847 100644
--- a/drivers/leds/trigger/ledtrig-device.c
+++ b/drivers/leds/trigger/ledtrig-device.c
@@ -16,15 +16,19 @@
 #include <linux/list.h>
 #include <linux/rwsem.h>
 #include <linux/kdev_t.h>
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <asm/uaccess.h>
 
 #define BLINK_DELAY 30
 static unsigned long blink_delay = BLINK_DELAY;
 
 static DECLARE_RWSEM(devs_list_lock);
 static LIST_HEAD(devs_list);
+static struct dentry *debug_root;
 
 #define MAX_NAME_LEN 20
-
 struct ledtrig_dev_data {
 	char name[MAX_NAME_LEN];
 	dev_t dev;
@@ -114,8 +118,11 @@ void ledtrig_dev_add(dev_t dev)
 		/* register with led triggers */
 		led_trigger_register_simple(new_dev_trig->name,
 					    &new_dev_trig->trig);
-	else
+	else {
+		pr_warn("device %u:%u already registered\n",
+			MAJOR(dev), MINOR(dev));
 		kfree(new_dev_trig);
+	}
 }
 EXPORT_SYMBOL(ledtrig_dev_add);
 
@@ -167,13 +174,147 @@ static void ledtrig_dev_remove_all(void)
 	up_write(&devs_list_lock);
 }
 
+static int ledtrig_dev_devices_show(struct seq_file *s, void *unused)
+{
+	struct ledtrig_dev_data *dev_trig;
+
+	down_read(&devs_list_lock);
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		seq_printf(s, "%u:%u\n", MAJOR(dev_trig->dev),
+			MINOR(dev_trig->dev));
+	}
+	up_read(&devs_list_lock);
+	return 0;
+}
+
+static int ledtrig_dev_devices_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, ledtrig_dev_devices_show,
+			   &inode->i_private);
+}
+
+static const struct file_operations debug_devices_ops = {
+	.owner = THIS_MODULE,
+	.open = ledtrig_dev_devices_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release
+};
+
+static int get_dev_from_user(const char __user *buf, size_t size,
+			dev_t *dev)
+{
+	char temp[MAX_NAME_LEN];
+	unsigned int major;
+	unsigned int minor;
+	int ret;
+
+	WARN_ON(dev == NULL);
+	if (dev == NULL)
+		return -EINVAL;
+
+	if (size > sizeof(temp) || size == 0)
+		return -EINVAL;
+
+	if (copy_from_user(temp, buf, size) != 0)
+		return -EINVAL;
+
+	ret = sscanf(temp, "%u:%u", &major, &minor);
+	if (ret < 2)
+		return -EINVAL;
+
+	*dev = MKDEV(major, minor);
+	return 0;
+}
+
+static ssize_t ledtrig_dev_register_write(struct file *filp,
+					const char __user *buf,
+					size_t size, loff_t *off)
+{
+	dev_t dev;
+	int ret;
+
+	ret = get_dev_from_user(buf, size, &dev);
+	if (ret < 0)
+		return ret;
+
+	pr_debug("register device %u:%u\n", MAJOR(dev), MINOR(dev));
+	ledtrig_dev_add(dev);
+
+	return size;
+}
+
+static const struct file_operations debug_register_ops = {
+	.owner = THIS_MODULE,
+	.write = ledtrig_dev_register_write,
+};
+
+static ssize_t ledtrig_dev_unregister_write(struct file *filp,
+					    const char __user *buf,
+					    size_t size, loff_t *off)
+{
+	dev_t dev;
+	int ret;
+
+	ret = get_dev_from_user(buf, size, &dev);
+	if (ret < 0)
+		return ret;
+
+	pr_debug("unregister device %u:%u\n", MAJOR(dev), MINOR(dev));
+	ledtrig_dev_del(dev);
+
+	return size;
+}
+
+static const struct file_operations debug_unregister_ops = {
+	.owner = THIS_MODULE,
+	.write = ledtrig_dev_unregister_write,
+};
+
+static ssize_t ledtrig_dev_trigger_write(struct file *filp,
+					const char __user *buf,
+					size_t size, loff_t *off)
+{
+	dev_t dev;
+	int ret;
+
+	ret = get_dev_from_user(buf, size, &dev);
+	if (ret < 0)
+		return ret;
+
+	pr_debug("trigger device %u:%u\n", MAJOR(dev), MINOR(dev));
+	ledtrig_dev_activity(dev);
+
+	return size;
+}
+
+static const struct file_operations debug_trigger_ops = {
+	.owner = THIS_MODULE,
+	.write = ledtrig_dev_trigger_write,
+};
+
 static int __init ledtrig_dev_init(void)
 {
+	debug_root = debugfs_create_dir("ledtrig-dev", NULL);
+
+	if (debug_root) {
+		debugfs_create_file("devices", 0444, debug_root, NULL,
+				&debug_devices_ops);
+		debugfs_create_file("register", 0200, debug_root, NULL,
+				&debug_register_ops);
+		debugfs_create_file("unregister", 0200, debug_root, NULL,
+				&debug_unregister_ops);
+		debugfs_create_file("trigger", 0200, debug_root, NULL,
+				&debug_trigger_ops);
+	}
+
 	return 0;
 }
 
 static void __exit ledtrig_dev_exit(void)
 {
+	debugfs_remove_recursive(debug_root);
+
 	ledtrig_dev_remove_all();
 }
 
-- 
2.6.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238320

Hi Maciek,

I tested your trigger, and it works fine, but I wonder if
it really improves the things.

Basically, similarly as Josh, I have doubts related to associating
triggers with dev_t. It requires from user to figure out how they can
obtain valid dev_t. For example, in case of v4l2 jpeg codec, I had to
spend some time looking for the location of struct device containing
dev_t related to the exposed encoder and decoder nodes, whereas addition
of debugging instruction should be easy and intuitive.

It is much simpler to register a trigger with human readable names.
What is more, use of dev_t makes the user thinking that there is
some mechanism involved, that automatically associates device with
trigger, and after some time of code investigation one gets
disillusioned and realizes that dev_t is used only to uniquely identify
registered triggers.

I tried to add trigger to the JPEG codec interrupt handler using
ledtrig-oneshot, and below is the result:

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 9690f9d..af826d3 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -28,6 +28,7 @@
  #include <media/v4l2-ioctl.h>
  #include <media/videobuf2-core.h>
  #include <media/videobuf2-dma-contig.h>
+#include <linux/leds.h>

  #include "jpeg-core.h"
  #include "jpeg-hw-s5p.h"
@@ -35,6 +36,9 @@
  #include "jpeg-hw-exynos3250.h"
  #include "jpeg-regs.h"

+static unsigned long blink_delay = 30;
+struct led_trigger *jpeg_led_trig;
+
  static struct s5p_jpeg_fmt sjpeg_formats[] = {
         {
                 .name           = "JPEG JFIF",
@@ -2318,6 +2322,7 @@ static irqreturn_t s5p_jpeg_irq(int irq, void *dev_id)
         return IRQ_HANDLED;
  }

+
  static irqreturn_t exynos4_jpeg_irq(int irq, void *priv)
  {
         unsigned int int_status;
@@ -2375,7 +2380,10 @@ static irqreturn_t exynos4_jpeg_irq(int irq, void 
*priv)
         v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
         curr_ctx->subsampling = exynos4_jpeg_get_frame_fmt(jpeg->regs);

+       led_trigger_blink_oneshot(jpeg_led_trig, &blink_delay, 
&blink_delay, 0);
+
         spin_unlock(&jpeg->slock);
+
         return IRQ_HANDLED;
  }

@@ -2589,6 +2597,8 @@ static int s5p_jpeg_probe(struct platform_device 
*pdev)

         v4l2_info(&jpeg->v4l2_dev, "Samsung S5P JPEG codec\n");

+       led_trigger_register_simple("jpeg-trig", &jpeg_led_trig);
+
         return 0;

  enc_vdev_register_rollback:
@@ -2633,6 +2643,8 @@ static int s5p_jpeg_remove(struct platform_device 
*pdev)
         if (!IS_ERR(jpeg->sclk))
                 clk_put(jpeg->sclk);

+       led_trigger_unregister_simple(jpeg_led_trig);
+
         return 0;
  }

It needs addition of 6 lines of code versus 2 lines in case
of ledtrig-device. Not a big deal, taking into account that we
don't have to spend additional time looking for the suitable
struct device.

Please note that in case of drivers that expose many dev nodes,
there are cases like interrupt handlers, where struct device
can't be automatically retrieved, but it needs additional
analysis to find out which dev node given call to ISR referes to.
In this case we would have to check current mode (encoding/decoding)
and call either

	ledtrig_dev_activity(jpeg->vfd_encoder->dev.devt)
or
	ledtrig_dev_activity(jpeg->vfd_decoder->dev.devt).


When comparing the steps required from user space side to activate
the triggers, it looks as follows:


ledtrig-oneshot (we have one trigger for encoding and decoding mode):
=================
#cd /sys/class/leds/aat1290
#cat triggers
#[none] jpeg-trig
#echo "jpeg-trig" > trigger

ledtrig-dev approach (we have to define trigger per dev_t, we choose 
encoder):
=====================
#grep .  /sys/class/video4linux/video*/name
#/sys/class/video4linux/video7/name:s5p-jpeg-enc
#/sys/class/video4linux/video8/name:s5p-jpeg-dec
#ls -l /dev/video7
#crw-rw---T 1 root video 81, 8 Jan  1 03:32 /dev/video8
#echo "81:7" > /sys/kernel/debug/ledtrig-dev/register
#cd /sys/class/leds/aat1290
#cat triggers
#[none] dev-81:7
#echo "dev-81:7" > trigger

It seems that ledtrig-dev is more troublesome in use.

Please let me know if I missed something.

On 10/01/2015 04:04 PM, Maciek Borzecki wrote:
> A series of patches that add yet another LED trigger, a device
> activity trigger.
>
> The motivation is to have a LED trigger that is associated with a
> device and can be fired from cetrain points in the code that have been
> chosen by the developer. With this this device activity trigger it is
> possible for instance to easily hook up a tty driver for a console to
> blink one LED, yet another serial port to blink a second LED and
> writes to a block device to trigger a third LED.
>
> The patches have been tested on Wandboard Quad.
>
> The first patch adds the actual trigger. Each device wishing to use
> the trigger has to be explicitly registered by calling
> ledtrig_dev_add(), and passing it's dev_t. The intention is that the
> trigger will be used in scenarios that are impossible to foresee at
> this moment, and are likely to be approach in a case by case manner
> anyway.
>
> The second patch adds couple of debugfs helpers.
>
> The third patch adds documentation and notes on debugfs interface.
>
> Example hooks into tty driver can be seen here:
> https://github.com/bboozzoo/linux/commit/d8a875673e37b27d9c9066febe7633382f97d8af
>
> Changes since v1:
>    - fixed debugfs user address space access
>    - added unregister debugfs attribute
>    - documentation update
>
> Maciek Borzecki (3):
>    leds: add device activity LED triggers
>    leds: add debugfs to device trigger
>    Documentation: leds: document ledtrig-device trigger
>
>   Documentation/leds/00-INDEX           |   3 +
>   Documentation/leds/ledtrig-device.txt |  35 ++++
>   drivers/leds/trigger/Kconfig          |   8 +
>   drivers/leds/trigger/Makefile         |   1 +
>   drivers/leds/trigger/ledtrig-device.c | 326 ++++++++++++++++++++++++++++++++++
>   include/linux/leds.h                  |  10 ++
>   6 files changed, 383 insertions(+)
>   create mode 100644 Documentation/leds/ledtrig-device.txt
>   create mode 100644 drivers/leds/trigger/ledtrig-device.c
>


-- 
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238390

On 10/02 16:27, Jacek Anaszewski wrote:
> Hi Maciek,
>
> I tested your trigger, and it works fine, but I wonder if
> it really improves the things.
>
> Basically, similarly as Josh, I have doubts related to associating
> triggers with dev_t. It requires from user to figure out how they can
> obtain valid dev_t. For example, in case of v4l2 jpeg codec, I had to
> spend some time looking for the location of struct device containing
> dev_t related to the exposed encoder and decoder nodes, whereas addition
> of debugging instruction should be easy and intuitive.
>
> It is much simpler to register a trigger with human readable names.
> What is more, use of dev_t makes the user thinking that there is
> some mechanism involved, that automatically associates device with
> trigger, and after some time of code investigation one gets
> disillusioned and realizes that dev_t is used only to uniquely identify
> registered triggers.
>
> I tried to add trigger to the JPEG codec interrupt handler using
> ledtrig-oneshot, and below is the result:
>
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 9690f9d..af826d3 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -28,6 +28,7 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/videobuf2-core.h>
>  #include <media/videobuf2-dma-contig.h>
> +#include <linux/leds.h>
>
>  #include "jpeg-core.h"
>  #include "jpeg-hw-s5p.h"
> @@ -35,6 +36,9 @@
>  #include "jpeg-hw-exynos3250.h"
>  #include "jpeg-regs.h"
>
> +static unsigned long blink_delay = 30;
> +struct led_trigger *jpeg_led_trig;
> +
>  static struct s5p_jpeg_fmt sjpeg_formats[] = {
>         {
>                 .name           = "JPEG JFIF",
> @@ -2318,6 +2322,7 @@ static irqreturn_t s5p_jpeg_irq(int irq, void *dev_id)
>         return IRQ_HANDLED;
>  }
>
> +
>  static irqreturn_t exynos4_jpeg_irq(int irq, void *priv)
>  {
>         unsigned int int_status;
> @@ -2375,7 +2380,10 @@ static irqreturn_t exynos4_jpeg_irq(int irq, void
> *priv)
>         v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
>         curr_ctx->subsampling = exynos4_jpeg_get_frame_fmt(jpeg->regs);
>
> +       led_trigger_blink_oneshot(jpeg_led_trig, &blink_delay, &blink_delay,
> 0);
> +
>         spin_unlock(&jpeg->slock);
> +
>         return IRQ_HANDLED;
>  }
>
> @@ -2589,6 +2597,8 @@ static int s5p_jpeg_probe(struct platform_device
> *pdev)
>
>         v4l2_info(&jpeg->v4l2_dev, "Samsung S5P JPEG codec\n");
>
> +       led_trigger_register_simple("jpeg-trig", &jpeg_led_trig);
> +
>         return 0;
>
>  enc_vdev_register_rollback:
> @@ -2633,6 +2643,8 @@ static int s5p_jpeg_remove(struct platform_device
> *pdev)
>         if (!IS_ERR(jpeg->sclk))
>                 clk_put(jpeg->sclk);
>
> +       led_trigger_unregister_simple(jpeg_led_trig);
> +
>         return 0;
>  }
>
> It needs addition of 6 lines of code versus 2 lines in case
> of ledtrig-device. Not a big deal, taking into account that we
> don't have to spend additional time looking for the suitable
> struct device.
>
> Please note that in case of drivers that expose many dev nodes,
> there are cases like interrupt handlers, where struct device
> can't be automatically retrieved, but it needs additional
> analysis to find out which dev node given call to ISR referes to.
> In this case we would have to check current mode (encoding/decoding)
> and call either
>
> 	ledtrig_dev_activity(jpeg->vfd_encoder->dev.devt)
> or
> 	ledtrig_dev_activity(jpeg->vfd_decoder->dev.devt).
>
>
> When comparing the steps required from user space side to activate
> the triggers, it looks as follows:
>
>
> ledtrig-oneshot (we have one trigger for encoding and decoding mode):
> =================
> #cd /sys/class/leds/aat1290
> #cat triggers
> #[none] jpeg-trig
> #echo "jpeg-trig" > trigger
>
> ledtrig-dev approach (we have to define trigger per dev_t, we choose
> encoder):
> =====================
> #grep .  /sys/class/video4linux/video*/name
> #/sys/class/video4linux/video7/name:s5p-jpeg-enc
> #/sys/class/video4linux/video8/name:s5p-jpeg-dec
> #ls -l /dev/video7
> #crw-rw---T 1 root video 81, 8 Jan  1 03:32 /dev/video8
> #echo "81:7" > /sys/kernel/debug/ledtrig-dev/register
> #cd /sys/class/leds/aat1290
> #cat triggers
> #[none] dev-81:7
> #echo "dev-81:7" > trigger
>
> It seems that ledtrig-dev is more troublesome in use.
>
> Please let me know if I missed something.
>

Thanks for your comments. You've raised some fair points.

Although my use case is a bit different (multiple serial ports, each
with a LED, port <-> LED mapping is user configurable, hooks at tty
layer), I may have unnecessarily overengineered the solution. Indeed
simply patching driver structures with `struct led_trigger` will work as
well.

Regards,
--
Maciek Borzecki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web