[PATCH v3 0/3] tty/leds: implement a trigger for ttys

[PATCH v3 0/3] tty/leds: implement a trigger for ttys

[Uwe Kleine-König]

Hello,

next iteration of the tty-led-trigger series. This addresses the review
comments I got from Greg.

This depends on patch "tty: drop useless variable initialisation in
tty_kopen()" which is already on tty-testing.

Best regards
Uwe

Uwe Kleine-König (3):
  tty: rename tty_kopen() and add new function tty_kopen_shared()
  tty: new helper function tty_get_icount()
  leds: trigger: implement a tty trigger

 drivers/leds/trigger/Kconfig        |   7 ++
 drivers/leds/trigger/Makefile       |   1 +
 drivers/leds/trigger/ledtrig-tty.c  | 159 ++++++++++++++++++++++++++++
 drivers/staging/speakup/spk_ttyio.c |   2 +-
 drivers/tty/tty_io.c                |  87 +++++++++++----
 include/linux/tty.h                 |   7 +-
 6 files changed, 238 insertions(+), 25 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

-- 
2.24.0

[PATCH v3 1/3] tty: rename tty_kopen() and add new function tty_kopen_shared()

[Uwe Kleine-König]

Introduce a new function tty_kopen_shared() that yields a struct
tty_struct. The semantic difference to tty_kopen() is that the tty is
expected to be used already. So rename tty_kopen() to
tty_kopen_exclusive() for clearness, adapt the single user and put the
common code in a new static helper function.

tty_kopen_shared is to be used to implement an LED trigger for tty
devices in one of the next patches.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/staging/speakup/spk_ttyio.c |  2 +-
 drivers/tty/tty_io.c                | 58 ++++++++++++++++++++---------
 include/linux/tty.h                 |  5 ++-
 3 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c
index 5a9eff08cb96..e9db06eae875 100644
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -147,7 +147,7 @@ static int spk_ttyio_initialise_ldisc(struct spk_synth *synth)
 	if (ret)
 		return ret;
 
-	tty = tty_kopen(dev);
+	tty = tty_kopen_exclusive(dev);
 	if (IS_ERR(tty))
 		return PTR_ERR(tty);
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a1453fe10862..b718666ce73c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1875,22 +1875,7 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 	return driver;
 }
 
-/**
- *	tty_kopen	-	open a tty device for kernel
- *	@device: dev_t of device to open
- *
- *	Opens tty exclusively for kernel. Performs the driver lookup,
- *	makes sure it's not already opened and performs the first-time
- *	tty initialization.
- *
- *	Returns the locked initialized &tty_struct
- *
- *	Claims the global tty_mutex to serialize:
- *	  - concurrent first-time tty initialization
- *	  - concurrent tty driver removal w/ lookup
- *	  - concurrent tty removal from driver table
- */
-struct tty_struct *tty_kopen(dev_t device)
+static struct tty_struct *tty_kopen(dev_t device, int shared)
 {
 	struct tty_struct *tty;
 	struct tty_driver *driver;
@@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
 
 	/* check whether we're reopening an existing tty */
 	tty = tty_driver_lookup_tty(driver, NULL, index);
-	if (IS_ERR(tty))
+	if (IS_ERR(tty) || shared)
 		goto out;
 
 	if (tty) {
@@ -1923,7 +1908,44 @@ struct tty_struct *tty_kopen(dev_t device)
 	tty_driver_kref_put(driver);
 	return tty;
 }
-EXPORT_SYMBOL_GPL(tty_kopen);
+
+/**
+ *	tty_kopen_exclusive	-	open a tty device for kernel
+ *	@device: dev_t of device to open
+ *
+ *	Opens tty exclusively for kernel. Performs the driver lookup,
+ *	makes sure it's not already opened and performs the first-time
+ *	tty initialization.
+ *
+ *	Returns the locked initialized &tty_struct
+ *
+ *	Claims the global tty_mutex to serialize:
+ *	  - concurrent first-time tty initialization
+ *	  - concurrent tty driver removal w/ lookup
+ *	  - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen_exclusive(dev_t device)
+{
+	return tty_kopen(device, 0);
+}
+EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
+
+/**
+ *	tty_kopen_shared	-	open a tty device for shared in-kernel use
+ *	@device: dev_t of device to open
+ *
+ *	Opens an already existing tty
+ *	rnel. Performs the driver lookup,
+ *	makes sure it's not already opened and performs the first-time
+ *	tty initialization.
+ *
+ *	Locking is identical to tty_kopen() above.
+ */
+struct tty_struct *tty_kopen_shared(dev_t device)
+{
+	return tty_kopen(device, 1);
+}
+EXPORT_SYMBOL_GPL(tty_kopen_shared);
 
 /**
  *	tty_open_by_driver	-	open a tty device
diff --git a/include/linux/tty.h b/include/linux/tty.h
index bfa4e2ee94a9..d0bcf3226fb2 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -411,7 +411,8 @@ extern struct tty_struct *get_current_tty(void);
 /* tty_io.c */
 extern int __init tty_init(void);
 extern const char *tty_name(const struct tty_struct *tty);
-extern struct tty_struct *tty_kopen(dev_t device);
+extern struct tty_struct *tty_kopen_exclusive(dev_t device);
+extern struct tty_struct *tty_kopen_shared(dev_t device);
 extern void tty_kclose(struct tty_struct *tty);
 extern int tty_dev_name_to_number(const char *name, dev_t *number);
 extern int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout);
@@ -436,7 +437,7 @@ static inline int __init tty_init(void)
 { return 0; }
 static inline const char *tty_name(const struct tty_struct *tty)
 { return "(none)"; }
-static inline struct tty_struct *tty_kopen(dev_t device)
+static inline struct tty_struct *tty_kopen_exclusive(dev_t device)
 { return ERR_PTR(-ENODEV); }
 static inline void tty_kclose(struct tty_struct *tty)
 { }
-- 
2.24.0

[PATCH v3 2/3] tty: new helper function tty_get_icount()

[Uwe Kleine-König]

For a given struct tty_struct this yields the corresponding statistics
about sent and received characters (and some more) which is needed to
implement an LED trigger for tty devices.

The new function is then used to simplify tty_tiocgicount().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/tty_io.c | 29 +++++++++++++++++++++++++----
 include/linux/tty.h  |  2 ++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b718666ce73c..3e74e6cec1a6 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2492,15 +2492,36 @@ static int tty_tiocmset(struct tty_struct *tty, unsigned int cmd,
 	return tty->ops->tiocmset(tty, set, clear);
 }
 
+/**
+ *	tty_get_icount		-	get tty statistics
+ *	@tty: tty device
+ *	@icount: output parameter
+ *
+ *	Gets a copy of the tty's icount statistics.
+ *
+ *	Locking: none (up to the driver)
+ */
+int tty_get_icount(struct tty_struct *tty,
+		   struct serial_icounter_struct *icount)
+{
+	memset(icount, 0, sizeof(*icount));
+
+	if (tty->ops->get_icount)
+		return tty->ops->get_icount(tty, icount);
+	else
+		return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tty_get_icount);
+
 static int tty_tiocgicount(struct tty_struct *tty, void __user *arg)
 {
-	int retval = -EINVAL;
 	struct serial_icounter_struct icount;
-	memset(&icount, 0, sizeof(icount));
-	if (tty->ops->get_icount)
-		retval = tty->ops->get_icount(tty, &icount);
+	int retval;
+
+	retval = tty_get_icount(tty, &icount);
 	if (retval != 0)
 		return retval;
+
 	if (copy_to_user(arg, &icount, sizeof(icount)))
 		return -EFAULT;
 	return 0;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index d0bcf3226fb2..40ea5c409619 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -495,6 +495,8 @@ extern void tty_unthrottle(struct tty_struct *tty);
 extern int tty_throttle_safe(struct tty_struct *tty);
 extern int tty_unthrottle_safe(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
+extern int tty_get_icount(struct tty_struct *tty,
+			  struct serial_icounter_struct *icount);
 extern int is_current_pgrp_orphaned(void);
 extern void tty_hangup(struct tty_struct *tty);
 extern void tty_vhangup(struct tty_struct *tty);
-- 
2.24.0

[PATCH v3 3/3] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

Usage is as follows:

	myled=ledname
	tty=ttyS0

	echo tty > /sys/class/leds/$myled/trigger
	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev

. When this new trigger is active it periodically checks the tty's
statistics and when it changed since the last check the led is flashed
once.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/trigger/Kconfig       |   7 ++
 drivers/leds/trigger/Makefile      |   1 +
 drivers/leds/trigger/ledtrig-tty.c | 159 +++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index ce9429ca6dde..40ff08c93f56 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -144,4 +144,11 @@ config LEDS_TRIGGER_AUDIO
 	  the audio mute and mic-mute changes.
 	  If unsure, say N
 
+config LEDS_TRIGGER_TTY
+	tristate "LED Trigger for TTY devices"
+	depends on TTY
+	help
+	  This allows LEDs to be controlled by activity on ttys which includes
+	  serial devices like /dev/ttyS0.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 733a83e2a718..25c4db97cdd4 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
+obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
new file mode 100644
index 000000000000..0157aa0b2ce3
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <uapi/linux/serial.h>
+
+struct ledtrig_tty_data {
+	struct led_classdev *led_cdev;
+	struct delayed_work dwork;
+	struct tty_struct *tty;
+	dev_t device;
+	int rx, tx;
+};
+
+static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)
+{
+	cancel_delayed_work_sync(&trigger_data->dwork);
+}
+
+static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
+{
+	if (!trigger_data->tty)
+		return;
+
+	schedule_delayed_work(&trigger_data->dwork, 0);
+}
+
+static ssize_t dev_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	ssize_t len = 0;
+
+	if (trigger_data->tty)
+		len = sprintf(buf, "%u\n", trigger_data->device);
+
+	return len;
+}
+
+static ssize_t dev_store(struct device *dev,
+			 struct device_attribute *attr, const char *buf,
+			 size_t size)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	struct tty_struct *tty;
+	unsigned major, minor;
+	int ret;
+
+	if (size == 0 || (size == 1 && buf[0] == '\n')) {
+		tty = NULL;
+	} else {
+		ret = sscanf(buf, "%u:%u", &major, &minor);
+		if (ret < 2) {
+			dev_err(dev, "invalid value\n");
+			return -EINVAL;
+		}
+
+		tty = tty_kopen_shared(MKDEV(major, minor));
+		if (IS_ERR(tty)) {
+			dev_err(dev, "failed to open tty: %pe\n", tty);
+			return PTR_ERR(tty);
+		}
+	}
+
+	ledtrig_tty_halt(trigger_data);
+
+	tty_kref_put(trigger_data->tty);
+	trigger_data->tty = tty;
+	trigger_data->device = MKDEV(major, minor);
+
+	if (tty) {
+		struct serial_icounter_struct icount;
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (!ret) {
+			trigger_data->tx = icount.tx;
+			trigger_data->rx = icount.rx;
+		}
+	}
+
+	ledtrig_tty_restart(trigger_data);
+
+	return size;
+}
+static DEVICE_ATTR_RW(dev);
+
+static void ledtrig_tty_work(struct work_struct *work)
+{
+	struct ledtrig_tty_data *trigger_data =
+		container_of(work, struct ledtrig_tty_data, dwork.work);
+	struct serial_icounter_struct icount;
+	int ret;
+
+	if (!trigger_data->tty) {
+		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+		return;
+	}
+
+	ret = tty_get_icount(trigger_data->tty, &icount);
+	if (ret)
+		return;
+
+	if (icount.rx != trigger_data->rx ||
+	    icount.tx != trigger_data->tx) {
+		unsigned long delay_on = 100, delay_off = 100;
+
+		led_blink_set_oneshot(trigger_data->led_cdev,
+				      &delay_on, &delay_off, 0);
+
+		trigger_data->rx = icount.rx;
+		trigger_data->tx = icount.tx;
+	}
+
+	schedule_delayed_work(&trigger_data->dwork, msecs_to_jiffies(100));
+}
+
+static struct attribute *ledtrig_tty_attrs[] = {
+	&dev_attr_dev.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ledtrig_tty);
+
+static int ledtrig_tty_activate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_tty_data *trigger_data;
+
+	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+	if (!trigger_data)
+		return -ENOMEM;
+
+	led_set_trigger_data(led_cdev, trigger_data);
+
+	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
+	trigger_data->led_cdev = led_cdev;
+
+	return 0;
+}
+
+static void ledtrig_tty_deactivate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_tty_data *trigger_data = led_get_trigger_data(led_cdev);
+
+	cancel_delayed_work_sync(&trigger_data->dwork);
+
+	kfree(trigger_data);
+}
+
+struct led_trigger ledtrig_tty = {
+	.name = "tty",
+	.activate = ledtrig_tty_activate,
+	.deactivate = ledtrig_tty_deactivate,
+	.groups = ledtrig_tty_groups,
+};
+module_led_trigger(ledtrig_tty);
+
+MODULE_AUTHOR("Uwe Kleine-König <u.kleine-koenig@pengutronix.de>");
+MODULE_DESCRIPTION("UART LED trigger");
+MODULE_LICENSE("GPL v2");
-- 
2.24.0

Re: [PATCH v3 3/3] leds: trigger: implement a tty trigger

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 04:07:36PM +0100, Uwe Kleine-König wrote:
> Usage is as follows:
> 
> 	myled=ledname
> 	tty=ttyS0
> 
> 	echo tty > /sys/class/leds/$myled/trigger
> 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev

Is this the correct instructions?  Aren't you looking for a major/minor
number instead in your sysfs file?

> 
> . When this new trigger is active it periodically checks the tty's
> statistics and when it changed since the last check the led is flashed
> once.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/leds/trigger/Kconfig       |   7 ++
>  drivers/leds/trigger/Makefile      |   1 +
>  drivers/leds/trigger/ledtrig-tty.c | 159 +++++++++++++++++++++++++++++
>  3 files changed, 167 insertions(+)
>  create mode 100644 drivers/leds/trigger/ledtrig-tty.c
> 
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index ce9429ca6dde..40ff08c93f56 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -144,4 +144,11 @@ config LEDS_TRIGGER_AUDIO
>  	  the audio mute and mic-mute changes.
>  	  If unsure, say N
>  
> +config LEDS_TRIGGER_TTY
> +	tristate "LED Trigger for TTY devices"
> +	depends on TTY
> +	help
> +	  This allows LEDs to be controlled by activity on ttys which includes
> +	  serial devices like /dev/ttyS0.
> +
>  endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 733a83e2a718..25c4db97cdd4 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
>  obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
>  obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
>  obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
> +obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> new file mode 100644
> index 000000000000..0157aa0b2ce3
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <uapi/linux/serial.h>
> +
> +struct ledtrig_tty_data {
> +	struct led_classdev *led_cdev;
> +	struct delayed_work dwork;
> +	struct tty_struct *tty;
> +	dev_t device;
> +	int rx, tx;
> +};
> +
> +static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)
> +{
> +	cancel_delayed_work_sync(&trigger_data->dwork);
> +}
> +
> +static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
> +{
> +	if (!trigger_data->tty)
> +		return;
> +
> +	schedule_delayed_work(&trigger_data->dwork, 0);
> +}
> +
> +static ssize_t dev_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	ssize_t len = 0;
> +
> +	if (trigger_data->tty)
> +		len = sprintf(buf, "%u\n", trigger_data->device);

Will that print a dev_t in a format that userspace can make sense of it?
Should you split it up with MAJOR:MINOR instead?



> +
> +	return len;
> +}
> +
> +static ssize_t dev_store(struct device *dev,
> +			 struct device_attribute *attr, const char *buf,
> +			 size_t size)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	struct tty_struct *tty;
> +	unsigned major, minor;
> +	int ret;
> +
> +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> +		tty = NULL;
> +	} else {
> +		ret = sscanf(buf, "%u:%u", &major, &minor);
> +		if (ret < 2) {
> +			dev_err(dev, "invalid value\n");

Can I DoS the syslog with this?  :)

> +			return -EINVAL;
> +		}
> +
> +		tty = tty_kopen_shared(MKDEV(major, minor));
> +		if (IS_ERR(tty)) {
> +			dev_err(dev, "failed to open tty: %pe\n", tty);

Same here, dev_dbg() perhaps?

Other than these minor things, looks good to me.

Your tty changes are fine, if I can get an ack from the led maintainers
about a working patch 3, I'll be glad to take all 3 in my tree.

thanks,

greg k-h

Re: [PATCH v3 3/3] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

On Tue, Dec 17, 2019 at 04:27:24PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 17, 2019 at 04:07:36PM +0100, Uwe Kleine-König wrote:
> > Usage is as follows:
> > 
> > 	myled=ledname
> > 	tty=ttyS0
> > 
> > 	echo tty > /sys/class/leds/$myled/trigger
> > 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> 
> Is this the correct instructions?  Aren't you looking for a major/minor
> number instead in your sysfs file?

This is correct, yes, at least it works as intended on my machine.

/sys/class/tty/$tty/dev produces $major:$minor and that's what the
led-trigger consumes.

> > . When this new trigger is active it periodically checks the tty's
> > statistics and when it changed since the last check the led is flashed
> > once.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/leds/trigger/Kconfig       |   7 ++
> >  drivers/leds/trigger/Makefile      |   1 +
> >  drivers/leds/trigger/ledtrig-tty.c | 159 +++++++++++++++++++++++++++++
> >  3 files changed, 167 insertions(+)
> >  create mode 100644 drivers/leds/trigger/ledtrig-tty.c
> > 
> > diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> > index ce9429ca6dde..40ff08c93f56 100644
> > --- a/drivers/leds/trigger/Kconfig
> > +++ b/drivers/leds/trigger/Kconfig
> > @@ -144,4 +144,11 @@ config LEDS_TRIGGER_AUDIO
> >  	  the audio mute and mic-mute changes.
> >  	  If unsure, say N
> >  
> > +config LEDS_TRIGGER_TTY
> > +	tristate "LED Trigger for TTY devices"
> > +	depends on TTY
> > +	help
> > +	  This allows LEDs to be controlled by activity on ttys which includes
> > +	  serial devices like /dev/ttyS0.
> > +
> >  endif # LEDS_TRIGGERS
> > diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> > index 733a83e2a718..25c4db97cdd4 100644
> > --- a/drivers/leds/trigger/Makefile
> > +++ b/drivers/leds/trigger/Makefile
> > @@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
> >  obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
> >  obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
> >  obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
> > +obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
> > diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> > new file mode 100644
> > index 000000000000..0157aa0b2ce3
> > --- /dev/null
> > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <uapi/linux/serial.h>
> > +
> > +struct ledtrig_tty_data {
> > +	struct led_classdev *led_cdev;
> > +	struct delayed_work dwork;
> > +	struct tty_struct *tty;
> > +	dev_t device;
> > +	int rx, tx;
> > +};
> > +
> > +static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)
> > +{
> > +	cancel_delayed_work_sync(&trigger_data->dwork);
> > +}
> > +
> > +static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
> > +{
> > +	if (!trigger_data->tty)
> > +		return;
> > +
> > +	schedule_delayed_work(&trigger_data->dwork, 0);
> > +}
> > +
> > +static ssize_t dev_show(struct device *dev,
> > +			struct device_attribute *attr, char *buf)
> > +{
> > +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	ssize_t len = 0;
> > +
> > +	if (trigger_data->tty)
> > +		len = sprintf(buf, "%u\n", trigger_data->device);
> 
> Will that print a dev_t in a format that userspace can make sense of it?
> Should you split it up with MAJOR:MINOR instead?

Ah, yes, this needs fixing to match the format that is used in .store.

> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t dev_store(struct device *dev,
> > +			 struct device_attribute *attr, const char *buf,
> > +			 size_t size)
> > +{
> > +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	struct tty_struct *tty;
> > +	unsigned major, minor;
> > +	int ret;
> > +
> > +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> > +		tty = NULL;
> > +	} else {
> > +		ret = sscanf(buf, "%u:%u", &major, &minor);
> > +		if (ret < 2) {
> > +			dev_err(dev, "invalid value\n");
> 
> Can I DoS the syslog with this?  :)

Only if you can write to the sysfs file in which case you can DoS the
syslog anyhow :-)

> > +			return -EINVAL;
> > +		}
> > +
> > +		tty = tty_kopen_shared(MKDEV(major, minor));
> > +		if (IS_ERR(tty)) {
> > +			dev_err(dev, "failed to open tty: %pe\n", tty);
> 
> Same here, dev_dbg() perhaps?
> 
> Other than these minor things, looks good to me.
> 
> Your tty changes are fine, if I can get an ack from the led maintainers
> about a working patch 3, I'll be glad to take all 3 in my tree.

You'll get a v4 in a moment. Thanks for your prompt reviews.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 3/3] leds: trigger: implement a tty trigger

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 05:23:13PM +0100, Uwe Kleine-König wrote:
> On Tue, Dec 17, 2019 at 04:27:24PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 17, 2019 at 04:07:36PM +0100, Uwe Kleine-König wrote:
> > > Usage is as follows:
> > > 
> > > 	myled=ledname
> > > 	tty=ttyS0
> > > 
> > > 	echo tty > /sys/class/leds/$myled/trigger
> > > 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> > 
> > Is this the correct instructions?  Aren't you looking for a major/minor
> > number instead in your sysfs file?
> 
> This is correct, yes, at least it works as intended on my machine.
> 
> /sys/class/tty/$tty/dev produces $major:$minor and that's what the
> led-trigger consumes.

Ugh, nevermind, I totally read that wrong, I was thinking "echo" instead
of cat.  My fault, what you wrote is correct.  Should that be documented
somewhere in a Documentation/ABI/ file so that people know how to use
this new sysfs file?  How are led triggers documented?

thanks,

greg k-h

Re: [PATCH v3 3/3] leds: trigger: implement a tty trigger

[Jacek Anaszewski]

On 12/17/19 6:21 PM, Greg Kroah-Hartman wrote:
> On Tue, Dec 17, 2019 at 05:23:13PM +0100, Uwe Kleine-König wrote:
>> On Tue, Dec 17, 2019 at 04:27:24PM +0100, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 17, 2019 at 04:07:36PM +0100, Uwe Kleine-König wrote:
>>>> Usage is as follows:
>>>>
>>>> 	myled=ledname
>>>> 	tty=ttyS0
>>>>
>>>> 	echo tty > /sys/class/leds/$myled/trigger
>>>> 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
>>>
>>> Is this the correct instructions?  Aren't you looking for a major/minor
>>> number instead in your sysfs file?
>>
>> This is correct, yes, at least it works as intended on my machine.
>>
>> /sys/class/tty/$tty/dev produces $major:$minor and that's what the
>> led-trigger consumes.
> 
> Ugh, nevermind, I totally read that wrong, I was thinking "echo" instead
> of cat.  My fault, what you wrote is correct.  Should that be documented
> somewhere in a Documentation/ABI/ file so that people know how to use
> this new sysfs file?  How are led triggers documented?

LED triggers have their corresponding entries in Documentation/ABI.

Uwe, you already did that for netdev trigger:

Documentation/ABI/testing/sysfs-class-led-trigger-netdev

It would be nice to have one for this too.

There are also less formal docs in Documentation/leds, e.g.:

Documentation/leds/ledtrig-usbport.rst

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 3/3] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

On Tue, Dec 17, 2019 at 09:27:13PM +0100, Jacek Anaszewski wrote:
> On 12/17/19 6:21 PM, Greg Kroah-Hartman wrote:
> > On Tue, Dec 17, 2019 at 05:23:13PM +0100, Uwe Kleine-König wrote:
> >> On Tue, Dec 17, 2019 at 04:27:24PM +0100, Greg Kroah-Hartman wrote:
> >>> On Tue, Dec 17, 2019 at 04:07:36PM +0100, Uwe Kleine-König wrote:
> >>>> Usage is as follows:
> >>>>
> >>>> 	myled=ledname
> >>>> 	tty=ttyS0
> >>>>
> >>>> 	echo tty > /sys/class/leds/$myled/trigger
> >>>> 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> >>>
> >>> Is this the correct instructions?  Aren't you looking for a major/minor
> >>> number instead in your sysfs file?
> >>
> >> This is correct, yes, at least it works as intended on my machine.
> >>
> >> /sys/class/tty/$tty/dev produces $major:$minor and that's what the
> >> led-trigger consumes.
> > 
> > Ugh, nevermind, I totally read that wrong, I was thinking "echo" instead
> > of cat.  My fault, what you wrote is correct.  Should that be documented
> > somewhere in a Documentation/ABI/ file so that people know how to use
> > this new sysfs file?  How are led triggers documented?
> 
> LED triggers have their corresponding entries in Documentation/ABI.
> 
> Uwe, you already did that for netdev trigger:
> 
> Documentation/ABI/testing/sysfs-class-led-trigger-netdev
> 
> It would be nice to have one for this too.
> 
> There are also less formal docs in Documentation/leds, e.g.:
> 
> Documentation/leds/ledtrig-usbport.rst

I'd put into Documentation/ABI/testing/sysfs-class-led-trigger-tty:

	What:           /sys/class/leds/<led>/dev
	Date:           Dec 2019
	KernelVersion:  5.6
	Contact:        linux-leds@vger.kernel.org
	Description:
			Specifies $major:$minor of the triggering tty

Does this look reasonable?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 3/3] leds: trigger: implement a tty trigger

[Jacek Anaszewski]

On 12/17/19 10:08 PM, Uwe Kleine-König wrote:
> On Tue, Dec 17, 2019 at 09:27:13PM +0100, Jacek Anaszewski wrote:
>> On 12/17/19 6:21 PM, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 17, 2019 at 05:23:13PM +0100, Uwe Kleine-König wrote:
>>>> On Tue, Dec 17, 2019 at 04:27:24PM +0100, Greg Kroah-Hartman wrote:
>>>>> On Tue, Dec 17, 2019 at 04:07:36PM +0100, Uwe Kleine-König wrote:
>>>>>> Usage is as follows:
>>>>>>
>>>>>> 	myled=ledname
>>>>>> 	tty=ttyS0
>>>>>>
>>>>>> 	echo tty > /sys/class/leds/$myled/trigger
>>>>>> 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
>>>>>
>>>>> Is this the correct instructions?  Aren't you looking for a major/minor
>>>>> number instead in your sysfs file?
>>>>
>>>> This is correct, yes, at least it works as intended on my machine.
>>>>
>>>> /sys/class/tty/$tty/dev produces $major:$minor and that's what the
>>>> led-trigger consumes.
>>>
>>> Ugh, nevermind, I totally read that wrong, I was thinking "echo" instead
>>> of cat.  My fault, what you wrote is correct.  Should that be documented
>>> somewhere in a Documentation/ABI/ file so that people know how to use
>>> this new sysfs file?  How are led triggers documented?
>>
>> LED triggers have their corresponding entries in Documentation/ABI.
>>
>> Uwe, you already did that for netdev trigger:
>>
>> Documentation/ABI/testing/sysfs-class-led-trigger-netdev
>>
>> It would be nice to have one for this too.
>>
>> There are also less formal docs in Documentation/leds, e.g.:
>>
>> Documentation/leds/ledtrig-usbport.rst
> 
> I'd put into Documentation/ABI/testing/sysfs-class-led-trigger-tty:
> 
> 	What:           /sys/class/leds/<led>/dev
> 	Date:           Dec 2019
> 	KernelVersion:  5.6
> 	Contact:        linux-leds@vger.kernel.org
> 	Description:
> 			Specifies $major:$minor of the triggering tty

Yes, that should be fine.

-- 
Best regards,
Jacek Anaszewski