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

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

[Uwe Kleine-König]

Hello,

back in November last year I sent my last approach to implement an led
trigger for UARTs (Message-Id:
20181106213739.29628-1-u.kleine-koenig@pengutronix.de). This was parked
on my todo list since then somewhere far from the top.

This is a new approach. Similar to the netdev trigger the tty trigger
introduced here periodically checks the statistics related to a given
tty and if that changed the LED is flashed once.

I had to introduce two functions in the tty layer to be able to get my
hands on the needed data in the first two patches, the third patch then
adds the trigger.

Best regards
Uwe

Uwe Kleine-König (3):
  tty: new helper 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 | 146 +++++++++++++++++++++++++++++
 drivers/tty/tty_io.c               |  47 ++++++++-
 include/linux/tty.h                |   3 +
 5 files changed, 200 insertions(+), 4 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

-- 
2.24.0

[PATCH v2 1/3] tty: new helper function tty_kopen_shared

[Uwe Kleine-König]

This function gives a struct tty_struct for a given device number.

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index d9f54c7d94f2..584025117cd3 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1925,6 +1925,33 @@ struct tty_struct *tty_kopen(dev_t device)
 }
 EXPORT_SYMBOL_GPL(tty_kopen);
 
+/*
+ * Caller gets a reference on (non-error) ttys, that must be disposed using
+ * tty_kref_put().
+ */
+struct tty_struct *tty_kopen_shared(dev_t device)
+{
+	struct tty_struct *tty;
+	struct tty_driver *driver;
+	int index = -1;
+
+	mutex_lock(&tty_mutex);
+	driver = tty_lookup_driver(device, NULL, &index);
+	if (IS_ERR(driver)) {
+		tty = ERR_CAST(driver);
+		goto err_lookup_driver;
+	}
+
+	tty = tty_driver_lookup_tty(driver, NULL, index);
+
+	tty_driver_kref_put(driver);
+err_lookup_driver:
+
+	mutex_unlock(&tty_mutex);
+	return tty;
+}
+EXPORT_SYMBOL(tty_kopen_shared);
+
 /**
  *	tty_open_by_driver	-	open a tty device
  *	@device: dev_t of device to open
diff --git a/include/linux/tty.h b/include/linux/tty.h
index bfa4e2ee94a9..616268eb1613 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -412,6 +412,7 @@ extern struct tty_struct *get_current_tty(void);
 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_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);
-- 
2.24.0

[PATCH v2 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)

Use the function to simplify tty_tiocgicount().

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 584025117cd3..df3942eb3468 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2497,15 +2497,27 @@ static int tty_tiocmset(struct tty_struct *tty, unsigned int cmd,
 	return tty->ops->tiocmset(tty, set, clear);
 }
 
+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(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 616268eb1613..3c6f58032f57 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 v2 3/3] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

Usage is as follows:

	myled=ledname
	tty=ttyS0

	echo tty > /sys/class/led/$myled/trigger
	cat /sys/class/tty/$tty/dev > /sys/class/led/$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 | 146 +++++++++++++++++++++++++++++
 3 files changed, 154 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..3f3197366700
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,146 @@
+// 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;
+	struct serial_icounter_struct icount;
+};
+
+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);
+
+	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 (icount.rx > trigger_data->icount.rx ||
+	    icount.tx > trigger_data->icount.tx) {
+		unsigned long delay_on = 100, delay_off = 100;
+
+		led_blink_set_oneshot(trigger_data->led_cdev,
+				      &delay_on, &delay_off, 0);
+
+		trigger_data->icount = icount;
+	}
+
+	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 v2 1/3] tty: new helper function tty_kopen_shared

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 09:17:16AM +0100, Uwe Kleine-König wrote:
> This function gives a struct tty_struct for a given device number.

That says _what_ this does, but not why :)

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/tty_io.c | 27 +++++++++++++++++++++++++++
>  include/linux/tty.h  |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index d9f54c7d94f2..584025117cd3 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1925,6 +1925,33 @@ struct tty_struct *tty_kopen(dev_t device)
>  }
>  EXPORT_SYMBOL_GPL(tty_kopen);
>  
> +/*
> + * Caller gets a reference on (non-error) ttys, that must be disposed using
> + * tty_kref_put().
> + */

It's a global function, can you please use kerneldoc?
And please describe it as well as tty_kopen() is.

> +struct tty_struct *tty_kopen_shared(dev_t device)
> +{
> +	struct tty_struct *tty;
> +	struct tty_driver *driver;
> +	int index = -1;
> +
> +	mutex_lock(&tty_mutex);
> +	driver = tty_lookup_driver(device, NULL, &index);
> +	if (IS_ERR(driver)) {
> +		tty = ERR_CAST(driver);
> +		goto err_lookup_driver;
> +	}
> +
> +	tty = tty_driver_lookup_tty(driver, NULL, index);

No error check?

> +
> +	tty_driver_kref_put(driver);
> +err_lookup_driver:
> +
> +	mutex_unlock(&tty_mutex);
> +	return tty;

Can't you share a lot of this code with tty_kopen()?  It feels odd to
duplicatate _almost_ all of it here.

> +}
> +EXPORT_SYMBOL(tty_kopen_shared);

EXPORT_SYMBOL_GPL() please.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 09:17:17AM +0100, Uwe Kleine-König wrote:
> For a given struct tty_struct this yields the corresponding statistics
> about sent and received characters (and some more)

Why?

> 
> Use the function to simplify tty_tiocgicount().
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/tty_io.c | 20 ++++++++++++++++----
>  include/linux/tty.h  |  2 ++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 584025117cd3..df3942eb3468 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2497,15 +2497,27 @@ static int tty_tiocmset(struct tty_struct *tty, unsigned int cmd,
>  	return tty->ops->tiocmset(tty, set, clear);
>  }
>  
> +int tty_get_icount(struct tty_struct *tty,
> +		   struct serial_icounter_struct *icount)

kerneldoc?

> +{
> +	memset(icount, 0, sizeof(*icount));
> +
> +	if (tty->ops->get_icount)
> +		return tty->ops->get_icount(tty, icount);
> +	else
> +		return -EINVAL;
> +}
> +EXPORT_SYMBOL(tty_get_icount);

EXPORT_SYMBOL_GPL() please.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 09:17:18AM +0100, Uwe Kleine-König wrote:
> Usage is as follows:
> 
> 	myled=ledname
> 	tty=ttyS0
> 
> 	echo tty > /sys/class/led/$myled/trigger
> 	cat /sys/class/tty/$tty/dev > /sys/class/led/$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 | 146 +++++++++++++++++++++++++++++
>  3 files changed, 154 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..3f3197366700
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -0,0 +1,146 @@
> +// 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;
> +	struct serial_icounter_struct icount;
> +};
> +
> +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);
> +
> +	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 (icount.rx > trigger_data->icount.rx ||
> +	    icount.tx > trigger_data->icount.tx) {

What happens when icount.rx and/or icount.tx wraps?  It's "only" an int.

> +		unsigned long delay_on = 100, delay_off = 100;
> +
> +		led_blink_set_oneshot(trigger_data->led_cdev,
> +				      &delay_on, &delay_off, 0);
> +
> +		trigger_data->icount = icount;

Implicit memcpy of a structure?  Ick.

All you care about are the two integers, why not just track them instead
of the whole thing?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 09:17:15AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> back in November last year I sent my last approach to implement an led
> trigger for UARTs (Message-Id:
> 20181106213739.29628-1-u.kleine-koenig@pengutronix.de). This was parked
> on my todo list since then somewhere far from the top.
> 
> This is a new approach. Similar to the netdev trigger the tty trigger
> introduced here periodically checks the statistics related to a given
> tty and if that changed the LED is flashed once.
> 
> I had to introduce two functions in the tty layer to be able to get my
> hands on the needed data in the first two patches, the third patch then
> adds the trigger.

I like the idea, seems sane.  Minor code nits on the implementation
though...

thanks,

greg k-h

Re: [PATCH v2 1/3] tty: new helper function tty_kopen_shared

[Uwe Kleine-König]

Hello Greg,

all feedback I don't respond to is planned to be fixed in v3.

On Tue, Dec 17, 2019 at 09:27:33AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 17, 2019 at 09:17:16AM +0100, Uwe Kleine-König wrote:
> > +struct tty_struct *tty_kopen_shared(dev_t device)
> > +{
> > +	struct tty_struct *tty;
> > +	struct tty_driver *driver;
> > +	int index = -1;
> > +
> > +	mutex_lock(&tty_mutex);
> > +	driver = tty_lookup_driver(device, NULL, &index);
> > +	if (IS_ERR(driver)) {
> > +		tty = ERR_CAST(driver);
> > +		goto err_lookup_driver;
> > +	}
> > +
> > +	tty = tty_driver_lookup_tty(driver, NULL, index);
> 
> No error check?

Well, the caller of tty_kopen_shared is supposed to check for error
returns. Do you think an error message here would be approriate? I'd do
this in the caller similar to how tty_kopen works.

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello Greg,

On Tue, Dec 17, 2019 at 09:32:11AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 17, 2019 at 09:17:18AM +0100, Uwe Kleine-König wrote:
> > +	ret = tty_get_icount(trigger_data->tty, &icount);
> > +	if (icount.rx > trigger_data->icount.rx ||
> > +	    icount.tx > trigger_data->icount.tx) {
> 
> What happens when icount.rx and/or icount.tx wraps?  It's "only" an int.

Good catch. I wonder why this is not an unsigned quantity. Just grepping
through drivers/tty/serial most drivers just increment these counters
and don't care for overflow (which is undefined for ints) either. :-\

..ooOO(Where is the can maintainer? --- We found a can of worms :-)

> > +		unsigned long delay_on = 100, delay_off = 100;
> > +
> > +		led_blink_set_oneshot(trigger_data->led_cdev,
> > +				      &delay_on, &delay_off, 0);
> > +
> > +		trigger_data->icount = icount;
> 
> Implicit memcpy of a structure?  Ick.

I'd call that elegant ;-)

> All you care about are the two integers, why not just track them instead
> of the whole thing?

For now I only care about tx and rx, but I intend to add some bells and
whistles to trigger on other events. (But I don't care much, can add
that once I implement this support.)

Best regards
Uwe

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

Re: [PATCH v2 1/3] tty: new helper function tty_kopen_shared

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 11:51:01AM +0100, Uwe Kleine-König wrote:
> Hello Greg,
> 
> all feedback I don't respond to is planned to be fixed in v3.
> 
> On Tue, Dec 17, 2019 at 09:27:33AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 17, 2019 at 09:17:16AM +0100, Uwe Kleine-König wrote:
> > > +struct tty_struct *tty_kopen_shared(dev_t device)
> > > +{
> > > +	struct tty_struct *tty;
> > > +	struct tty_driver *driver;
> > > +	int index = -1;
> > > +
> > > +	mutex_lock(&tty_mutex);
> > > +	driver = tty_lookup_driver(device, NULL, &index);
> > > +	if (IS_ERR(driver)) {
> > > +		tty = ERR_CAST(driver);
> > > +		goto err_lookup_driver;
> > > +	}
> > > +
> > > +	tty = tty_driver_lookup_tty(driver, NULL, index);
> > 
> > No error check?
> 
> Well, the caller of tty_kopen_shared is supposed to check for error
> returns. Do you think an error message here would be approriate? I'd do
> this in the caller similar to how tty_kopen works.

Ah, you are passing it on to the caller, ok, nevermind.

greg k-h

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

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 11:58:26AM +0100, Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Tue, Dec 17, 2019 at 09:32:11AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 17, 2019 at 09:17:18AM +0100, Uwe Kleine-König wrote:
> > > +	ret = tty_get_icount(trigger_data->tty, &icount);
> > > +	if (icount.rx > trigger_data->icount.rx ||
> > > +	    icount.tx > trigger_data->icount.tx) {
> > 
> > What happens when icount.rx and/or icount.tx wraps?  It's "only" an int.
> 
> Good catch. I wonder why this is not an unsigned quantity. Just grepping
> through drivers/tty/serial most drivers just increment these counters
> and don't care for overflow (which is undefined for ints) either. :-\

It is not undefined for the kernel, I'm pretty sure we tell the compiler
to be sane for this type of thing.  It should "just wrap".  Oh wait
"int" is "signed int" here, hah, that's funny.  Surely someone has
noticed that in 20+ years by now?

> ..ooOO(Where is the can maintainer? --- We found a can of worms :-)

:)

> 
> > > +		unsigned long delay_on = 100, delay_off = 100;
> > > +
> > > +		led_blink_set_oneshot(trigger_data->led_cdev,
> > > +				      &delay_on, &delay_off, 0);
> > > +
> > > +		trigger_data->icount = icount;
> > 
> > Implicit memcpy of a structure?  Ick.
> 
> I'd call that elegant ;-)
> 
> > All you care about are the two integers, why not just track them instead
> > of the whole thing?
> 
> For now I only care about tx and rx, but I intend to add some bells and
> whistles to trigger on other events. (But I don't care much, can add
> that once I implement this support.)

Start small and add more as-needed, you can always move back to the full
structure later on if you really need it.

thanks,

greg k-h