| Message ID | 20201218104246.591315-1-u.kleine-koenig@pengutronix.de |
|---|---|
| Headers | show |
| Series | leds: trigger: implement a tty trigger | expand |
On Fri, Dec 18, 2020 at 11:42:43AM +0100, Uwe Kleine-König wrote: > From: Uwe Kleine-König <uwe@kleine-koenig.org> > > Hello, > > here comes v10 of this series. Changes compared to v9 sent with > Message-Id: 20201018204022.910815-1-u.kleine-koenig@pengutronix.de in > October: First 2 patches now applied to my tree. If I get an ack from the LED maintainer(s), I'll be glad to also queue up the third one too. thanks, greg k-h
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 -- http://www.livejournal.com/~pavelmachek
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
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
From: Uwe Kleine-König <uwe@kleine-koenig.org> Hello, here comes v10 of this series. Changes compared to v9 sent with Message-Id: 20201018204022.910815-1-u.kleine-koenig@pengutronix.de in October: - Bump date and kernel version in ABI doc - Fix double unlock in error path; found by Pavel - Don't stop the workqueue in ttyname_store() to fix error behaviour on memory allocation failure. Now it continues with the previous configuration instead of stopping. Also found by Pavel. Unaddressed review comments by Pavel are: - Unused assignment to len in ttyname_show This is wrong - "Poll every 100 msec... Hmm.... Okay, I guess?" Yes, I think there is no way around this given the trigger uses polling. There is no easy way to get notified instead. - "Are you sure about LED_ON [in the worker]? It should use current brightness selected by brightness file..." I found no consistent behaviour in other triggers. ledtrig-gpio implements a dedicated "desired_brightness" sysfs file, several use led_cdev->blink_brightness (via led_trigger_blink_oneshot), ledtrig-cpu uses led_trigger_event with LED_FULL. - "How is [the data initialized in ledtrig_tty_activate()] protected from concurrent access from sysfs?" I think there is no need to protect this. But I'm not sure I understood the question correctly, so please recheck and if needed point out the problem you see in more detail. 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/accessibility/speakup/spk_ttyio.c | 2 +- drivers/leds/trigger/Kconfig | 9 + drivers/leds/trigger/Makefile | 1 + drivers/leds/trigger/ledtrig-tty.c | 188 ++++++++++++++++++ drivers/tty/tty_io.c | 85 ++++++-- include/linux/tty.h | 7 +- 7 files changed, 273 insertions(+), 25 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty create mode 100644 drivers/leds/trigger/ledtrig-tty.c