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

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

[Uwe Kleine-König]

Hello,

the only change since v4 (sent starting with Message-Id:
<20191217165816.19324-1-u.kleine-koenig@pengutronix.de> is that I now
added a file Documentation/ABI/testing/sysfs-class-led-trigger-tty in
patch 3.

As before this depends on patch "tty: drop useless variable
initialisation in tty_kopen()" which is already on tty-next.

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

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

-- 
2.24.0

[PATCH v5 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 v5 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 v5 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>
---
 .../ABI/testing/sysfs-class-led-trigger-tty   |   6 +
 drivers/leds/trigger/Kconfig                  |   7 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-tty.c            | 153 ++++++++++++++++++
 4 files changed, 167 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
new file mode 100644
index 000000000000..f56f9721c317
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -0,0 +1,6 @@
+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
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..e9bbad52cc3f
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,153 @@
+// 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)
+{
+	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, "%d:%d\n", MAJOR(trigger_data->device),
+			      MINOR(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)
+			return -EINVAL;
+
+		tty = tty_kopen_shared(MKDEV(major, minor));
+		if (IS_ERR(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 v5 3/3] leds: trigger: implement a tty trigger

[Pavel Machek]

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

Hi!

> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -0,0 +1,6 @@
> +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

Ok, sounds reasonable.

> +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)
> +			return -EINVAL;

If user writes 1:2:badparsingofdata into the file, it will pass, right?

> +		tty = tty_kopen_shared(MKDEV(major, minor));
> +		if (IS_ERR(tty))
> +			return PTR_ERR(tty);
> +	}

Do you need to do some kind of tty_kclose()? What happens if the
device disappears, for example because the USB modem is unplugged?

> +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;
> +	}

Since you are polling this, anyway, can you just manipulate brightness
directly instead of using _oneshot()? _oneshot() will likely invoke
another set of workqueues.

LED triggers were meant to operate directly from the events, not based
on statistics like this.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Uwe Kleine-König]

Hello Pavel,

On Sat, Dec 21, 2019 at 07:40:47PM +0100, Pavel Machek wrote:
> > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> > @@ -0,0 +1,6 @@
> > +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
> 
> Ok, sounds reasonable.
> 
> > +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)
> > +			return -EINVAL;
> 
> If user writes 1:2:badparsingofdata into the file, it will pass, right?

Yes, and it will have the same effect as writing 1:2. I wonder if this
is bad.

> > +		tty = tty_kopen_shared(MKDEV(major, minor));
> > +		if (IS_ERR(tty))
> > +			return PTR_ERR(tty);
> > +	}
> 
> Do you need to do some kind of tty_kclose()? What happens if the
> device disappears, for example because the USB modem is unplugged?

Only tty_kref_put is needed to close.

> > +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;
> > +	}
> 
> Since you are polling this, anyway, can you just manipulate brightness
> directly instead of using _oneshot()? _oneshot() will likely invoke
> another set of workqueues.

I copied that from the netdev trigger. I failed to find a suitable
helper function, did I miss that or does it need creating?
 
> LED triggers were meant to operate directly from the events, not based
> on statistics like this.

Ditto; just copied from the netdev trigger. I tried to find a suitable
place to add a trigger in the core, but this is hard without having to
modify all drivers; additionally this is in thier hot path. So I
considered using statistics a good idea. Greg also liked it and someone
before us for the network trigger, too ...

Best regards
Uwe

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

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

[Greg Kroah-Hartman]

On Mon, Dec 23, 2019 at 11:08:28AM +0100, Uwe Kleine-König wrote:
> Hello Pavel,
> 
> On Sat, Dec 21, 2019 at 07:40:47PM +0100, Pavel Machek wrote:
> > > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> > > @@ -0,0 +1,6 @@
> > > +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
> > 
> > Ok, sounds reasonable.
> > 
> > > +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)
> > > +			return -EINVAL;
> > 
> > If user writes 1:2:badparsingofdata into the file, it will pass, right?
> 
> Yes, and it will have the same effect as writing 1:2. I wonder if this
> is bad.
> 
> > > +		tty = tty_kopen_shared(MKDEV(major, minor));
> > > +		if (IS_ERR(tty))
> > > +			return PTR_ERR(tty);
> > > +	}
> > 
> > Do you need to do some kind of tty_kclose()? What happens if the
> > device disappears, for example because the USB modem is unplugged?
> 
> Only tty_kref_put is needed to close.
> 
> > > +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;
> > > +	}
> > 
> > Since you are polling this, anyway, can you just manipulate brightness
> > directly instead of using _oneshot()? _oneshot() will likely invoke
> > another set of workqueues.
> 
> I copied that from the netdev trigger. I failed to find a suitable
> helper function, did I miss that or does it need creating?
>  
> > LED triggers were meant to operate directly from the events, not based
> > on statistics like this.
> 
> Ditto; just copied from the netdev trigger. I tried to find a suitable
> place to add a trigger in the core, but this is hard without having to
> modify all drivers; additionally this is in thier hot path. So I
> considered using statistics a good idea. Greg also liked it and someone
> before us for the network trigger, too ...

This still looks ok to me, any objections to me merging it in my tty
tree?

thanks,

greg k-h

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

[Uwe Kleine-König]

On Mon, Jan 06, 2020 at 07:59:18PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Dec 23, 2019 at 11:08:28AM +0100, Uwe Kleine-König wrote:
> > Hello Pavel,
> > 
> > On Sat, Dec 21, 2019 at 07:40:47PM +0100, Pavel Machek wrote:
> > > > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> > > > @@ -0,0 +1,6 @@
> > > > +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
> > > 
> > > Ok, sounds reasonable.
> > > 
> > > > +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)
> > > > +			return -EINVAL;
> > > 
> > > If user writes 1:2:badparsingofdata into the file, it will pass, right?
> > 
> > Yes, and it will have the same effect as writing 1:2. I wonder if this
> > is bad.
> > 
> > > > +		tty = tty_kopen_shared(MKDEV(major, minor));
> > > > +		if (IS_ERR(tty))
> > > > +			return PTR_ERR(tty);
> > > > +	}
> > > 
> > > Do you need to do some kind of tty_kclose()? What happens if the
> > > device disappears, for example because the USB modem is unplugged?
> > 
> > Only tty_kref_put is needed to close.
> > 
> > > > +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;
> > > > +	}
> > > 
> > > Since you are polling this, anyway, can you just manipulate brightness
> > > directly instead of using _oneshot()? _oneshot() will likely invoke
> > > another set of workqueues.
> > 
> > I copied that from the netdev trigger. I failed to find a suitable
> > helper function, did I miss that or does it need creating?
> >  
> > > LED triggers were meant to operate directly from the events, not based
> > > on statistics like this.
> > 
> > Ditto; just copied from the netdev trigger. I tried to find a suitable
> > place to add a trigger in the core, but this is hard without having to
> > modify all drivers; additionally this is in thier hot path. So I
> > considered using statistics a good idea. Greg also liked it and someone
> > before us for the network trigger, too ...
> 
> This still looks ok to me, any objections to me merging it in my tty
> tree?

Thanks for caring. If Pavel objects in principle to using statistics to
trigger the trigger, there will be no user of the new tty functions. I
don't care much about this, so feel free to apply the tty part, but I
would have expected that "no user" would be a stopper.

Best regards
Uwe

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

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

[Greg Kroah-Hartman]

On Tue, Jan 07, 2020 at 08:29:42AM +0100, Uwe Kleine-König wrote:
> On Mon, Jan 06, 2020 at 07:59:18PM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Dec 23, 2019 at 11:08:28AM +0100, Uwe Kleine-König wrote:
> > > Hello Pavel,
> > > 
> > > On Sat, Dec 21, 2019 at 07:40:47PM +0100, Pavel Machek wrote:
> > > > > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> > > > > @@ -0,0 +1,6 @@
> > > > > +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
> > > > 
> > > > Ok, sounds reasonable.
> > > > 
> > > > > +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)
> > > > > +			return -EINVAL;
> > > > 
> > > > If user writes 1:2:badparsingofdata into the file, it will pass, right?
> > > 
> > > Yes, and it will have the same effect as writing 1:2. I wonder if this
> > > is bad.
> > > 
> > > > > +		tty = tty_kopen_shared(MKDEV(major, minor));
> > > > > +		if (IS_ERR(tty))
> > > > > +			return PTR_ERR(tty);
> > > > > +	}
> > > > 
> > > > Do you need to do some kind of tty_kclose()? What happens if the
> > > > device disappears, for example because the USB modem is unplugged?
> > > 
> > > Only tty_kref_put is needed to close.
> > > 
> > > > > +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;
> > > > > +	}
> > > > 
> > > > Since you are polling this, anyway, can you just manipulate brightness
> > > > directly instead of using _oneshot()? _oneshot() will likely invoke
> > > > another set of workqueues.
> > > 
> > > I copied that from the netdev trigger. I failed to find a suitable
> > > helper function, did I miss that or does it need creating?
> > >  
> > > > LED triggers were meant to operate directly from the events, not based
> > > > on statistics like this.
> > > 
> > > Ditto; just copied from the netdev trigger. I tried to find a suitable
> > > place to add a trigger in the core, but this is hard without having to
> > > modify all drivers; additionally this is in thier hot path. So I
> > > considered using statistics a good idea. Greg also liked it and someone
> > > before us for the network trigger, too ...
> > 
> > This still looks ok to me, any objections to me merging it in my tty
> > tree?
> 
> Thanks for caring. If Pavel objects in principle to using statistics to
> trigger the trigger, there will be no user of the new tty functions. I
> don't care much about this, so feel free to apply the tty part, but I
> would have expected that "no user" would be a stopper.

Yes, "no user" would prevent me from merging this series, but I like the
"user" of this api, so would be glad to merge it at the same time.

thanks,

greg k-h

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

[Pavel Machek]

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

Hi!

> > > > +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)
> > > > +			return -EINVAL;
> > > 
> > > If user writes 1:2:badparsingofdata into the file, it will pass, right?
> > 
> > Yes, and it will have the same effect as writing 1:2. I wonder if this
> > is bad.

It is.

> > > > +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;
> > > > +	}
> > > 
> > > Since you are polling this, anyway, can you just manipulate brightness
> > > directly instead of using _oneshot()? _oneshot() will likely invoke
> > > another set of workqueues.
> > 
> > I copied that from the netdev trigger. I failed to find a suitable
> > helper function, did I miss that or does it need creating?

You don't need helper function. You are periodically checking the
statistics, anyway, so use it to turn the LED on/off directly, without
oneshot helper, which uses another workqueue internally.

> > > LED triggers were meant to operate directly from the events, not based
> > > on statistics like this.
> > 
> > Ditto; just copied from the netdev trigger. I tried to find a suitable
> > place to add a trigger in the core, but this is hard without having to
> > modify all drivers; additionally this is in thier hot path. So I
> > considered using statistics a good idea. Greg also liked it and someone
> > before us for the network trigger, too ...
> 
> This still looks ok to me, any objections to me merging it in my tty
> tree?

Yes.

I guess using statistics is kind-off acceptable, but the stuff I quote
above is not.

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]