| Message ID | 20201218104246.591315-4-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | leds: trigger: implement a tty trigger | expand |
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
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
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
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
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/ |
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
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 --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");
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