mbox series

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

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

Message

Uwe Kleine-König Dec. 18, 2020, 10:42 a.m. UTC
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

Comments

Greg KH Dec. 28, 2020, 3:10 p.m. UTC | #1
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
Greg KH Jan. 13, 2021, 4:16 p.m. UTC | #2
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 | #3
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 | #4
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
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
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