diff mbox series

[v10,3/3] leds: trigger: implement a tty trigger

Message ID 20201218104246.591315-4-u.kleine-koenig@pengutronix.de
State New
Headers show
Series leds: trigger: implement a tty trigger | expand

Commit Message

Uwe Kleine-König Dec. 18, 2020, 10:42 a.m. UTC
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                  |   9 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-tty.c            | 188 ++++++++++++++++++
 4 files changed, 204 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

Comments

Greg KH Jan. 13, 2021, 4:16 p.m. UTC | #1
On Fri, Dec 18, 2020 at 11:42:46AM +0100, 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                  |   9 +

>  drivers/leds/trigger/Makefile                 |   1 +

>  drivers/leds/trigger/ledtrig-tty.c            | 188 ++++++++++++++++++

>  4 files changed, 204 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..2bf6b24e781b

> --- /dev/null

> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty

> @@ -0,0 +1,6 @@

> +What:		/sys/class/leds/<led>/ttyname

> +Date:		Dec 2020

> +KernelVersion:	5.10

> +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..b77a01bd27f4 100644

> --- a/drivers/leds/trigger/Kconfig

> +++ b/drivers/leds/trigger/Kconfig

> @@ -144,4 +144,13 @@ 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.

> +

> +	  When build as a module this driver will be called ledtrig-tty.

> +

>  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..c1e87c0d23c3

> --- /dev/null

> +++ b/drivers/leds/trigger/ledtrig-tty.c

> @@ -0,0 +1,188 @@

> +// 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);

> +}


This causes the following build warning with the patch applied:

drivers/leds/trigger/ledtrig-tty.c:19:13: warning: ‘ledtrig_tty_halt’ defined but not used [-Wunused-function]
   19 | static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)
      |             ^~~~~~~~~~~~~~~~

Can you fix this up and just resend this patch (the other 2 are already
in my tree), so that I can queue it up?

thanks,

greg k-h
Greg KH Jan. 13, 2021, 6:34 p.m. UTC | #2
On Wed, Jan 13, 2021 at 06:30:18PM +0100, 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>

> ---

> Hello,

> 

> On Wed, Jan 13, 2021 at 05:16:00PM +0100, Greg Kroah-Hartman wrote:

> > This causes the following build warning with the patch applied:

> > 

> > drivers/leds/trigger/ledtrig-tty.c:19:13: warning: ‘ledtrig_tty_halt’ defined but not used [-Wunused-function]

> >    19 | static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)

> >       |             ^~~~~~~~~~~~~~~~

> > 

> > Can you fix this up and just resend this patch (the other 2 are already

> > in my tree),

> 

> oh indeed. Thanks for catching this and so preventing me still more

> shame when I have to receive a dozen or so patches that fix this :-)

> Droping this function is the only change since v10.

> 

> > so that I can queue it up?

> 

> Oh, so you are LED maintainer now? My congratulations.

> (Honestly, do you plan to apply this without their ack? Not that I'm

> against you doing that, I'm happy if I can archive this patch series as

> done, but I'm a bit surprised.)


It's drug on for so long now, the infrastructure that this driver needs
has now bee merged, so I see no reason why this driver can't be taken
now.  I offered up a "any objections?" in the past, and have gotten
none, so I will take that for quiet acceptance :)

thanks,

greg k-h
Pavel Machek Feb. 18, 2021, 1:33 p.m. UTC | #3
Hi!

> > > so that I can queue it up?
> > 
> > Oh, so you are LED maintainer now? My congratulations.
> > (Honestly, do you plan to apply this without their ack? Not that I'm
> > against you doing that, I'm happy if I can archive this patch series as
> > done, but I'm a bit surprised.)
> 
> It's drug on for so long now, the infrastructure that this driver needs
> has now bee merged, so I see no reason why this driver can't be taken
> now.  I offered up a "any objections?" in the past, and have gotten
> none, so I will take that for quiet acceptance :)

Thanks for taking the infrastructure patches, but please drop this
one. Its buggy, as were previous versions. I'll handle it.

Best regards,
								Pavel
Pavel Machek Feb. 18, 2021, 1:37 p.m. UTC | #4
Hi!

Close, but see below:

> +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;
> +	bool running;
> +
> +	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;

Unlock without a lock:

> +out_unlock:
> +	mutex_unlock(&trigger_data->mutex);
> +
> +	if (ttyname && !running)
> +		ledtrig_tty_restart(trigger_data);
> +
> +	return ret;
> +}

> +
> +		tty = tty_kopen_shared(devno);
> +		if (IS_ERR(tty) || !tty)
> +			/* What to do? retry or abort */
> +			goto out;

Abort would make sense to me.

> +	if (icount.rx != trigger_data->rx ||
> +	    icount.tx != trigger_data->tx) {
> +		led_set_brightness(trigger_data->led_cdev, LED_ON);

Please use _sync version.

Best regards,
								Pavel
Uwe Kleine-König Feb. 18, 2021, 9:19 p.m. UTC | #5
Hello Pavel,

On Thu, Feb 18, 2021 at 02:33:52PM +0100, Pavel Machek wrote:
> > > > so that I can queue it up?

> > > 

> > > Oh, so you are LED maintainer now? My congratulations.

> > > (Honestly, do you plan to apply this without their ack? Not that I'm

> > > against you doing that, I'm happy if I can archive this patch series as

> > > done, but I'm a bit surprised.)

> > 

> > It's drug on for so long now, the infrastructure that this driver needs

> > has now bee merged, so I see no reason why this driver can't be taken

> > now.  I offered up a "any objections?" in the past, and have gotten

> > none, so I will take that for quiet acceptance :)

> 

> Thanks for taking the infrastructure patches, but please drop this

> one.


Given it is already part of Greg's pull request I wonder if we need an
incremental patch instead?

> Its buggy, as were previous versions. I'll handle it.


*sigh*, you're right. I will prepare a fixed version tomorrow.
Maybe I know until then if I have to prepare a v12 or an incremental
patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Greg KH Feb. 18, 2021, 9:21 p.m. UTC | #6
On Thu, Feb 18, 2021 at 10:19:48PM +0100, Uwe Kleine-König wrote:
> Hello Pavel,
> 
> On Thu, Feb 18, 2021 at 02:33:52PM +0100, Pavel Machek wrote:
> > > > > so that I can queue it up?
> > > > 
> > > > Oh, so you are LED maintainer now? My congratulations.
> > > > (Honestly, do you plan to apply this without their ack? Not that I'm
> > > > against you doing that, I'm happy if I can archive this patch series as
> > > > done, but I'm a bit surprised.)
> > > 
> > > It's drug on for so long now, the infrastructure that this driver needs
> > > has now bee merged, so I see no reason why this driver can't be taken
> > > now.  I offered up a "any objections?" in the past, and have gotten
> > > none, so I will take that for quiet acceptance :)
> > 
> > Thanks for taking the infrastructure patches, but please drop this
> > one.
> 
> Given it is already part of Greg's pull request I wonder if we need an
> incremental patch instead?

An incremental patch is easier, thanks, I can't "drop" a patch already
in my public tree.

greg k-h
Uwe Kleine-König Feb. 19, 2021, 8 a.m. UTC | #7
Hi Pavel,

On Thu, Feb 18, 2021 at 02:37:33PM +0100, Pavel Machek wrote:
> Close, but see below:
> 
> > +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;
> > +	bool running;
> > +
> > +	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;
> 
> Unlock without a lock:
> 
> > +out_unlock:
> > +	mutex_unlock(&trigger_data->mutex);

Indeed, I prepare an incremental patch that does return -ENOMEM instead
of goto out_unlock.

> > +
> > +	if (ttyname && !running)
> > +		ledtrig_tty_restart(trigger_data);
> > +
> > +	return ret;
> > +}
> 
> > +
> > +		tty = tty_kopen_shared(devno);
> > +		if (IS_ERR(tty) || !tty)
> > +			/* What to do? retry or abort */
> > +			goto out;
> 
> Abort would make sense to me.

In this case it would IMHO be sensible to already try the
tty_kopen_shared() in ttyname_store() and let that one fail if the tty
doesn't exist. I'll have to go through the history of this patch set, if
I remember correctly it was like that at one point.

> > +	if (icount.rx != trigger_data->rx ||
> > +	    icount.tx != trigger_data->tx) {
> > +		led_set_brightness(trigger_data->led_cdev, LED_ON);
> 
> Please use _sync version.

OK, there are too many variants for me. I'll just believe you that
led_set_brightness_sync is the right one. (Hmm, on the other hand I'll
have to understand the difference for a good commit log. I'll dig into
that. "Pavel said so" probably isn't good enough :-))

Best regards and thanks for your review,
Uwe
diff mbox series

Patch

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..2bf6b24e781b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -0,0 +1,6 @@ 
+What:		/sys/class/leds/<led>/ttyname
+Date:		Dec 2020
+KernelVersion:	5.10
+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..b77a01bd27f4 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -144,4 +144,13 @@  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.
+
+	  When build as a module this driver will be called ledtrig-tty.
+
 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..c1e87c0d23c3
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,188 @@ 
+// 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;
+	bool running;
+
+	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;
+	}
+
+	mutex_lock(&trigger_data->mutex);
+
+	running = trigger_data->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 && !running)
+		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;
+
+	mutex_lock(&trigger_data->mutex);
+
+	if (!trigger_data->ttyname) {
+		/* exit without rescheduling */
+		mutex_unlock(&trigger_data->mutex);
+		return;
+	}
+
+	/* try to get the tty corresponding to $ttyname */
+	if (!trigger_data->tty) {
+		dev_t devno;
+		struct tty_struct *tty;
+		int ret;
+
+		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) {
+		dev_info(trigger_data->tty->dev, "Failed to get icount, stopped polling\n");
+		mutex_unlock(&trigger_data->mutex);
+		return;
+	}
+
+	if (icount.rx != trigger_data->rx ||
+	    icount.tx != trigger_data->tx) {
+		led_set_brightness(trigger_data->led_cdev, LED_ON);
+
+		trigger_data->rx = icount.rx;
+		trigger_data->tx = icount.tx;
+	} else {
+		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+	}
+
+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);
+}
+
+static 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");