diff mbox series

[v11] leds: trigger: implement a tty trigger

Message ID 20210113173018.bq2fkea2o3yp6rf6@pengutronix.de
State New
Headers show
Series [v11] leds: trigger: implement a tty trigger | expand

Commit Message

Uwe Kleine-König Jan. 13, 2021, 5:30 p.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>
---
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.)

Best regards
Uwe

 .../ABI/testing/sysfs-class-led-trigger-tty   |   6 +
 drivers/leds/trigger/Kconfig                  |   9 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-tty.c            | 183 ++++++++++++++++++
 4 files changed, 199 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c


base-commit: 5c8fe583cce542aa0b84adc939ce85293de36e5e
prerequisite-patch-id: a2c3eea0944a3ae222a4429ba4983d24bea8e0fa
prerequisite-patch-id: 8203736a5395e3deef08add27f70f0f00b845d43

Comments

Pavel Machek Feb. 18, 2021, 1:37 p.m. UTC | #1
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
-- 
http://www.livejournal.com/~pavelmachek
Uwe Kleine-König Feb. 19, 2021, 8 a.m. UTC | #2
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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
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..d2ab6ab080ac
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,183 @@ 
+// 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_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");