[PATCH v7 0/3] leds: trigger: implement a tty trigger

[PATCH v7 0/3] leds: trigger: implement a tty trigger

[Uwe Kleine-König]

Hello,

this is v7 of a series adding support for tty triggers. See patch 3 for
how to use it. The first two patches provide the necessary
infrastructure in the tty subsystem to make the trigger possible.

Changes compared to v6 sent with Message-Id:
20200213091600.554-1-uwe@kleine-koenig.org on 13 Feb 2020:

 - use the tty's name instead of its major:minor
 - drop kstrtodev_t helper
 - retry opening the tty in case it only appears after it was configured
   in the trigger

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            | 192 ++++++++++++++++++
 drivers/staging/speakup/spk_ttyio.c           |   2 +-
 drivers/tty/tty_io.c                          |  87 ++++++--
 include/linux/tty.h                           |   7 +-
 7 files changed, 277 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

base-commit: dcb7fd82c75ee2d6e6f9d8cc71c52519ed52e258
-- 
2.27.0

[PATCH v7 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 9b95f77f9265..5a134718dac8 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 5a6f36b391d9..05d42f226304 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1871,22 +1871,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;
@@ -1901,7 +1886,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) {
@@ -1919,7 +1904,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 a99e9b8e4e31..c867d6fe67c4 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -413,7 +413,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);
@@ -438,7 +439,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.27.0

[PATCH v7 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 05d42f226304..6decc3ae1fb1 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2488,15 +2488,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 c867d6fe67c4..3ac812b347de 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -497,6 +497,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.27.0

[PATCH v7 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
	echo $tty > /sys/class/leds/$myled/ttyname

. 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            | 192 ++++++++++++++++++
 4 files changed, 206 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..5c53ce3ede36
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -0,0 +1,6 @@
+What:		/sys/class/leds/<led>/ttyname
+Date:		Jul 2020
+KernelVersion:	5.8
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Specifies the tty device name 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..e44e2202fa34
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/delay.h>
+#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 mutex mutex;
+	const char *ttyname;
+	struct tty_struct *tty;
+	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 ttyname_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;
+
+	mutex_lock(&trigger_data->mutex);
+
+	if (trigger_data->ttyname)
+		len = sprintf(buf, "%s\n", trigger_data->ttyname);
+
+	mutex_unlock(&trigger_data->mutex);
+
+	return len;
+}
+
+static ssize_t ttyname_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);
+	char *ttyname;
+	ssize_t ret = size;
+
+	ledtrig_tty_halt(trigger_data);
+
+	mutex_lock(&trigger_data->mutex);
+
+	if (size > 0 && buf[size - 1] == '\n')
+		size -= 1;
+
+	if (size) {
+		ttyname = kmemdup_nul(buf, size, GFP_KERNEL);
+		if (!ttyname) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+	} else {
+		ttyname = NULL;
+	}
+
+	kfree(trigger_data->ttyname);
+	tty_kref_put(trigger_data->tty);
+	trigger_data->tty = NULL;
+
+	trigger_data->ttyname = ttyname;
+
+out_unlock:
+	mutex_unlock(&trigger_data->mutex);
+
+	if (ttyname)
+		ledtrig_tty_restart(trigger_data);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(ttyname);
+
+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;
+	bool firstrun = false;
+
+	mutex_lock(&trigger_data->mutex);
+
+	BUG_ON(!trigger_data->ttyname);
+
+	/* try to get the tty corresponding to $ttyname */
+	if (!trigger_data->tty) {
+		dev_t devno;
+		struct tty_struct *tty;
+		int ret;
+
+		firstrun = true;
+
+		ret = tty_dev_name_to_number(trigger_data->ttyname, &devno);
+		if (ret < 0)
+			/*
+			 * A device with this name might appear later, so keep
+			 * retrying.
+			 */
+			goto out;
+
+		tty = tty_kopen_shared(devno);
+		if (IS_ERR(tty) || !tty)
+			/* What to do? retry or abort */
+			goto out;
+
+		trigger_data->tty = tty;
+	}
+
+	ret = tty_get_icount(trigger_data->tty, &icount);
+	if (ret)
+		return;
+
+	while (firstrun ||
+	       icount.rx != trigger_data->rx ||
+	       icount.tx != trigger_data->tx) {
+
+		led_set_brightness(trigger_data->led_cdev, LED_ON);
+
+		msleep(100);
+
+		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+
+		trigger_data->rx = icount.rx;
+		trigger_data->tx = icount.tx;
+		firstrun = false;
+
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (ret)
+			return;
+	}
+
+out:
+	mutex_unlock(&trigger_data->mutex);
+	schedule_delayed_work(&trigger_data->dwork, msecs_to_jiffies(100));
+}
+
+static struct attribute *ledtrig_tty_attrs[] = {
+	&dev_attr_ttyname.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;
+	mutex_init(&trigger_data->mutex);
+
+	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.27.0

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

[Pavel Machek]

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

Hi!

> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -0,0 +1,6 @@
> +What:		/sys/class/leds/<led>/ttyname
> +Date:		Jul 2020
> +KernelVersion:	5.8
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		Specifies the tty device name 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.

"This driver can be built as a module, resulting module will be
named.. If unsure, say N."

> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0

2.0+ is preffered.

> +
> +	while (firstrun ||
> +	       icount.rx != trigger_data->rx ||
> +	       icount.tx != trigger_data->tx) {
> +
> +		led_set_brightness(trigger_data->led_cdev, LED_ON);
> +
> +		msleep(100);
> +
> +		led_set_brightness(trigger_data->led_cdev, LED_OFF);

Is this good idea inside workqueue?

> +		trigger_data->rx = icount.rx;
> +		trigger_data->tx = icount.tx;
> +		firstrun = false;
> +
> +		ret = tty_get_icount(trigger_data->tty, &icount);
> +		if (ret)
> +			return;

Unbalanced locking.

> +	}
> +
> +out:
> +	mutex_unlock(&trigger_data->mutex);
> +	schedule_delayed_work(&trigger_data->dwork, msecs_to_jiffies(100));
> +}

-- 
(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 v7 3/3] leds: trigger: implement a tty trigger

[Greg Kroah-Hartman]

On Sun, Jul 12, 2020 at 10:24:53AM +0200, Pavel Machek wrote:
> > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> 2.0+ is preffered.

No it is not, that's up to the developer.

thanks,

greg k-h

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

[Pavel Machek]

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

On Sun 2020-07-12 10:43:52, Greg Kroah-Hartman wrote:
> On Sun, Jul 12, 2020 at 10:24:53AM +0200, Pavel Machek wrote:
> > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > @@ -0,0 +1,192 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > 
> > 2.0+ is preffered.
> 
> No it is not, that's up to the developer.

For code I maintain, yes it is.
								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 --]

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

[Greg Kroah-Hartman]

On Sun, Jul 12, 2020 at 10:50:59AM +0200, Pavel Machek wrote:
> On Sun 2020-07-12 10:43:52, Greg Kroah-Hartman wrote:
> > On Sun, Jul 12, 2020 at 10:24:53AM +0200, Pavel Machek wrote:
> > > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > > @@ -0,0 +1,192 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > 
> > > 2.0+ is preffered.
> > 
> > No it is not, that's up to the developer.
> 
> For code I maintain, yes it is.

That's up to the developer of the code, not the maintainer, as the
maintainer is not the copyright holder of it.  For new files, it is up
to the author of that code.  No maintainer should impose a license rule
like this on their subsystem, that's just not ok at all.  The only
"rule" is that it is compatible with GPLv2, nothing else.

thanks,

greg k-h

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

[Pavel Machek]

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

On Sun 2020-07-12 11:02:17, Greg Kroah-Hartman wrote:
> On Sun, Jul 12, 2020 at 10:50:59AM +0200, Pavel Machek wrote:
> > On Sun 2020-07-12 10:43:52, Greg Kroah-Hartman wrote:
> > > On Sun, Jul 12, 2020 at 10:24:53AM +0200, Pavel Machek wrote:
> > > > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > > > @@ -0,0 +1,192 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > 
> > > > 2.0+ is preffered.
> > > 
> > > No it is not, that's up to the developer.
> > 
> > For code I maintain, yes it is.
> 
> That's up to the developer of the code, not the maintainer, as the
> maintainer is not the copyright holder of it.  For new files, it is up
> to the author of that code.  No maintainer should impose a license rule
> like this on their subsystem, that's just not ok at all.  The only
> "rule" is that it is compatible with GPLv2, nothing else.

No, see for example device tree rules.

Plus, IIRC it was you who asked the developer to "doublecheck with
their legal" when you seen GPL-2.0+. You can't really prevent me from
doing the same.

								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 --]

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

[Greg Kroah-Hartman]

On Sun, Jul 12, 2020 at 11:07:31AM +0200, Pavel Machek wrote:
> On Sun 2020-07-12 11:02:17, Greg Kroah-Hartman wrote:
> > On Sun, Jul 12, 2020 at 10:50:59AM +0200, Pavel Machek wrote:
> > > On Sun 2020-07-12 10:43:52, Greg Kroah-Hartman wrote:
> > > > On Sun, Jul 12, 2020 at 10:24:53AM +0200, Pavel Machek wrote:
> > > > > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > > > > @@ -0,0 +1,192 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > 
> > > > > 2.0+ is preffered.
> > > > 
> > > > No it is not, that's up to the developer.
> > > 
> > > For code I maintain, yes it is.
> > 
> > That's up to the developer of the code, not the maintainer, as the
> > maintainer is not the copyright holder of it.  For new files, it is up
> > to the author of that code.  No maintainer should impose a license rule
> > like this on their subsystem, that's just not ok at all.  The only
> > "rule" is that it is compatible with GPLv2, nothing else.
> 
> No, see for example device tree rules.

Note, I don't agree with that rule, and if you have noticed, it's not
really enforced.

> Plus, IIRC it was you who asked the developer to "doublecheck with
> their legal" when you seen GPL-2.0+.  You can't really prevent me from
> doing the same.

Asking to verify that a specific license is what they really want it to
be and they know the ramifications of it is NOT the same as saying "For
code in the subsystem I maintain it has to be GPLv2+".

thanks,

greg k-h

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

[Uwe Kleine-König]

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

Hello Pavel,

On Sun, Jul 12, 2020 at 10:24:53AM +0200, Pavel Machek wrote:
> > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> 2.0+ is preffered.

My employer requests GPL-2.0-only for kernel code.

> > +	while (firstrun ||
> > +	       icount.rx != trigger_data->rx ||
> > +	       icount.tx != trigger_data->tx) {
> > +
> > +		led_set_brightness(trigger_data->led_cdev, LED_ON);
> > +
> > +		msleep(100);
> > +
> > +		led_set_brightness(trigger_data->led_cdev, LED_OFF);
> 
> Is this good idea inside workqueue?

What is "this"? The msleep? Calling led_set_brightness? What would you
recommend instead? Maybe led_set_brightness_nosleep()?

> > +		trigger_data->rx = icount.rx;
> > +		trigger_data->tx = icount.tx;
> > +		firstrun = false;
> > +
> > +		ret = tty_get_icount(trigger_data->tty, &icount);
> > +		if (ret)
> > +			return;
> 
> Unbalanced locking.

indeed, will fix and resend after the above issues are resolved.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Johan Hovold]

On Tue, Jul 07, 2020 at 06:59:58PM +0200, Uwe Kleine-König wrote:
> Usage is as follows:
> 
> 	myled=ledname
> 	tty=ttyS0
> 
> 	echo tty > /sys/class/leds/$myled/trigger
> 	echo $tty > /sys/class/leds/$myled/ttyname
> 
> . 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            | 192 ++++++++++++++++++
>  4 files changed, 206 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..5c53ce3ede36
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -0,0 +1,6 @@
> +What:		/sys/class/leds/<led>/ttyname
> +Date:		Jul 2020
> +KernelVersion:	5.8
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		Specifies the tty device name 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..e44e2202fa34
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/delay.h>
> +#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 mutex mutex;
> +	const char *ttyname;
> +	struct tty_struct *tty;
> +	int rx, tx;
> +};
> +
> +static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)
> +{
> +	cancel_delayed_work_sync(&trigger_data->dwork);
> +}

> +static ssize_t ttyname_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);
> +	char *ttyname;
> +	ssize_t ret = size;
> +
> +	ledtrig_tty_halt(trigger_data);
> +
> +	mutex_lock(&trigger_data->mutex);
> +
> +	if (size > 0 && buf[size - 1] == '\n')
> +		size -= 1;
> +
> +	if (size) {
> +		ttyname = kmemdup_nul(buf, size, GFP_KERNEL);
> +		if (!ttyname) {
> +			ret = -ENOMEM;
> +			goto out_unlock;
> +		}
> +	} else {
> +		ttyname = NULL;
> +	}
> +
> +	kfree(trigger_data->ttyname);
> +	tty_kref_put(trigger_data->tty);
> +	trigger_data->tty = NULL;
> +
> +	trigger_data->ttyname = ttyname;
> +
> +out_unlock:
> +	mutex_unlock(&trigger_data->mutex);
> +
> +	if (ttyname)
> +		ledtrig_tty_restart(trigger_data);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(ttyname);
> +
> +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;
> +	bool firstrun = false;
> +
> +	mutex_lock(&trigger_data->mutex);
> +
> +	BUG_ON(!trigger_data->ttyname);
> +
> +	/* try to get the tty corresponding to $ttyname */
> +	if (!trigger_data->tty) {
> +		dev_t devno;
> +		struct tty_struct *tty;
> +		int ret;
> +
> +		firstrun = true;
> +
> +		ret = tty_dev_name_to_number(trigger_data->ttyname, &devno);
> +		if (ret < 0)
> +			/*
> +			 * A device with this name might appear later, so keep
> +			 * retrying.
> +			 */
> +			goto out;
> +
> +		tty = tty_kopen_shared(devno);
> +		if (IS_ERR(tty) || !tty)
> +			/* What to do? retry or abort */
> +			goto out;
> +
> +		trigger_data->tty = tty;
> +	}
> +
> +	ret = tty_get_icount(trigger_data->tty, &icount);
> +	if (ret)
> +		return;
> +
> +	while (firstrun ||
> +	       icount.rx != trigger_data->rx ||
> +	       icount.tx != trigger_data->tx) {
> +
> +		led_set_brightness(trigger_data->led_cdev, LED_ON);
> +
> +		msleep(100);
> +
> +		led_set_brightness(trigger_data->led_cdev, LED_OFF);
> +
> +		trigger_data->rx = icount.rx;
> +		trigger_data->tx = icount.tx;
> +		firstrun = false;
> +
> +		ret = tty_get_icount(trigger_data->tty, &icount);
> +		if (ret)
> +			return;
> +	}

Haven't looked at the latest proposal in detail, but this looks broken
as you can potentially loop indefinitely in a worker thread, and with no
way to stop the trigger (delayed work).

> +
> +out:
> +	mutex_unlock(&trigger_data->mutex);
> +	schedule_delayed_work(&trigger_data->dwork, msecs_to_jiffies(100));
> +}

Johan

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

[Uwe Kleine-König]

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

Hello Johan,

On Tue, Jul 14, 2020 at 09:13:55AM +0200, Johan Hovold wrote:
> On Tue, Jul 07, 2020 at 06:59:58PM +0200, Uwe Kleine-König wrote:
> > +	while (firstrun ||
> > +	       icount.rx != trigger_data->rx ||
> > +	       icount.tx != trigger_data->tx) {
> > +
> > +		led_set_brightness(trigger_data->led_cdev, LED_ON);
> > +
> > +		msleep(100);
> > +
> > +		led_set_brightness(trigger_data->led_cdev, LED_OFF);
> > +
> > +		trigger_data->rx = icount.rx;
> > +		trigger_data->tx = icount.tx;
> > +		firstrun = false;
> > +
> > +		ret = tty_get_icount(trigger_data->tty, &icount);
> > +		if (ret)
> > +			return;
> > +	}
> 
> Haven't looked at the latest proposal in detail, but this looks broken
> as you can potentially loop indefinitely in a worker thread, and with no
> way to stop the trigger (delayed work).

I don't think that potentially looping indefinitely is a problem, but
indeed it should drop the lock during each iteration. Will think about
how to adapt.

Thanks
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Johan Hovold]

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

On Tue, Jul 21, 2020 at 09:48:15PM +0200, Uwe Kleine-König wrote:
> Hello Johan,
> 
> On Tue, Jul 14, 2020 at 09:13:55AM +0200, Johan Hovold wrote:
> > On Tue, Jul 07, 2020 at 06:59:58PM +0200, Uwe Kleine-König wrote:
> > > +	while (firstrun ||
> > > +	       icount.rx != trigger_data->rx ||
> > > +	       icount.tx != trigger_data->tx) {
> > > +
> > > +		led_set_brightness(trigger_data->led_cdev, LED_ON);
> > > +
> > > +		msleep(100);
> > > +
> > > +		led_set_brightness(trigger_data->led_cdev, LED_OFF);
> > > +
> > > +		trigger_data->rx = icount.rx;
> > > +		trigger_data->tx = icount.tx;
> > > +		firstrun = false;
> > > +
> > > +		ret = tty_get_icount(trigger_data->tty, &icount);
> > > +		if (ret)
> > > +			return;
> > > +	}
> > 
> > Haven't looked at the latest proposal in detail, but this looks broken
> > as you can potentially loop indefinitely in a worker thread, and with no
> > way to stop the trigger (delayed work).
> 
> I don't think that potentially looping indefinitely is a problem, but
> indeed it should drop the lock during each iteration. Will think about
> how to adapt.

You musn't queue work that can run for long on the global shared
workqueue as it affects flushing:

	* system_wq is the one used by schedule[_delayed]_work[_on]().
	* Multi-CPU multi-threaded.  There are users which expect relatively
	* short queue flush time.  Don't queue works which can run for too
	* long.

Work that potentially run indefinitely is an absolute no-no. :)

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]