On Mon, May 14, 2018 at 12:15:05PM +0200, Uwe Kleine-König wrote:
> The rx trigger fires when data is pushed to the ldisc by the driver. This
> is a bit later than the actual receiving of data but has the nice benefit
> that it doesn't need adaption for each driver and isn't in the hot path.
>
> Similarly the tx trigger fires when data was copied from userspace and is
> given to the ldisc.
This also has the negative effect of having the blink rate (and
pattern) be dependent on the size of the buffers provided by userspace
however.
With buffers of 2048 bytes or more and at 115200 baud you get a 178 ms
period (rather than the "default" 100 ms used for smaller buffers and
high-enough baudrates) and this drops to 2 seconds and 68 second periods
for 9600 and 300 baud respectively.
Also note that by hooking into do_tty_write, for UARTs used for consoles
there will be no blink on console output, only for console input...
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> ---
Please provide a full changelog here when resending next time.
> drivers/tty/Kconfig | 7 +++++++
> drivers/tty/tty_buffer.c | 2 ++
> drivers/tty/tty_io.c | 3 +++
> drivers/tty/tty_port.c | 32 ++++++++++++++++++++++++++++++--
> include/linux/tty.h | 22 ++++++++++++++++++++++
> 5 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 0840d27381ea..b119c0fa1f5a 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -41,6 +41,13 @@ config VT
> If unsure, say Y, or else you won't be able to do much with your new
> shiny Linux system :-)
>
> +config TTY_LEDS_TRIGGERS
> + bool "Enable support for TTY actions making LEDs blink"
This should just be something shorter like "TTY LED Triggers" (cf.
USB_LED_TRIG and most other led trigger descriptions).
> + depends on LEDS_TRIGGERS
> + ---help---
> + This enable support for tty triggers. It provides two LED triggers
> + (rx and tx) for each TTY.
> +
> config CONSOLE_TRANSLATIONS
> depends on VT
> default y
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index c996b6859c5e..364080ce8e91 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -521,6 +521,8 @@ static void flush_to_ldisc(struct work_struct *work)
> continue;
> }
>
> + tty_led_trigger_rx(port);
> +
This does not belong here now that you added the led-hook call to the
default receive_buf callback as I suggested.
> count = receive_buf(port, head, count);
> if (!count)
> break;
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 7c838b90a31d..8ef597dc0c3d 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -955,6 +955,9 @@ static inline ssize_t do_tty_write(
> ret = -EFAULT;
> if (copy_from_user(tty->write_buf, buf, size))
> break;
> +
> + tty_led_trigger_tx(tty->port);
> +
> ret = write(tty, file, tty->write_buf, size);
> if (ret <= 0)
> break;
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 25d736880013..72dcde234a7d 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -37,6 +37,8 @@ static int tty_port_default_receive_buf(struct tty_port *port,
>
> ret = tty_ldisc_receive_buf(disc, p, (char *)f, count);
>
> + tty_led_trigger_rx(port);
> +
> tty_ldisc_deref(disc);
>
> return ret;
> @@ -163,8 +165,31 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
> return dev;
> }
>
> - return tty_register_device_attr(driver, index, device, drvdata,
> - attr_grp);
> + if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS)) {
This doesn't really improve readability much. Use compile guards in the
header file to provide dummy implementations for setup/teardown
functions when the trigger isn't selected. And yes, the actual
implementation may require ifdefs around them in the c-file as well.
Also note that you need to add this to tty_port_register_device() as
well... Serdev is currently opt-in and only enabled for serial core (due
to some lifetime issues and issues related to hotplug), but there's no
reason to limit these triggers to serial core.
> + int ret;
> +
> + ret = led_trigger_register_format(&port->led_trigger_rx,
> + "%s%d-rx", driver->name, index);
Note that the pattern you currently use for the trigger name (and in the
error message below) may not match the tty name which for example may be
offset by driver->name_base (see tty_line_name()).
> + if (ret < 0)
> + pr_warn("Failed to register rx trigger for %s%d (%d)\n",
> + driver->name, index, ret);
> +
> + ret = led_trigger_register_format(&port->led_trigger_tx,
> + "%s%d-tx", driver->name, index);
> + if (ret < 0)
> + pr_warn("Failed to register tx trigger for %s%d (%d)\n",
> + driver->name, index, ret);
> + }
> +
> + dev = tty_register_device_attr(driver, index,
> + device, drvdata, attr_grp);
> +
> + if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS) && IS_ERR(dev)) {
> + led_trigger_unregister_simple(port->led_trigger_tx);
> + led_trigger_unregister_simple(port->led_trigger_rx);
> + }
> +
> + return dev;
Thanks,
Johan
