[PATCH] tty: implement a rx led trigger

[PATCH] tty: implement a rx led trigger

[Uwe Kleine-König]

The trigger fires when data is pushed to the ldisc. 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.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

this is actually v2 of the series "serial: Add LED trigger support" that
Sascha sent in November 2016.

For now this is only the RX part however that I'd like to use as a base
to get feedback. Once we got to an acceptable change the TX part should
be easy (once the right place to place the trigger is clear).

Other than that the trigger moved from a dedicated function in
serial_core that each driver is supposed to call on reception of data to
flush_to_ldisc().

Best regards
Uwe

 drivers/tty/tty_buffer.c |  4 ++++
 drivers/tty/tty_port.c   | 16 ++++++++++++++++
 include/linux/tty.h      |  3 +++
 3 files changed, 23 insertions(+)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index c996b6859c5e..4d364b77b1a7 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -17,6 +17,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/ratelimit.h>
+#include <linux/leds.h>
 
 
 #define MIN_TTYB_SIZE	256
@@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
 		struct tty_buffer *head = buf->head;
 		struct tty_buffer *next;
 		int count;
+		unsigned long delay = 50 /* ms */;
 
 		/* Ldisc or user is trying to gain exclusive access */
 		if (atomic_read(&buf->priority))
@@ -521,6 +523,8 @@ static void flush_to_ldisc(struct work_struct *work)
 			continue;
 		}
 
+		led_trigger_blink_oneshot(port->led_trigger_rx, &delay, &delay, 0);
+
 		count = receive_buf(port, head, count);
 		if (!count)
 			break;
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 25d736880013..51b78a585417 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -16,6 +16,7 @@
 #include <linux/wait.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/serdev.h>
 
@@ -157,6 +158,18 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
 
 	tty_port_link_device(port, driver, index);
 
+	port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
+					      driver->name, index);
+	if (!port->led_trigger_rx_name) {
+		pr_err("Failed to allocate trigger name for %s%d\n",
+		       driver->name, index);
+		goto skip_trigger;
+	}
+
+	led_trigger_register_simple(port->led_trigger_rx_name,
+				    &port->led_trigger_rx);
+
+skip_trigger:
 	dev = serdev_tty_port_register(port, device, driver, index);
 	if (PTR_ERR(dev) != -ENODEV) {
 		/* Skip creating cdev if we registered a serdev device */
@@ -206,6 +219,9 @@ void tty_port_unregister_device(struct tty_port *port,
 	if (ret == 0)
 		return;
 
+	led_trigger_unregister_simple(port->led_trigger_rx);
+	kfree(port->led_trigger_rx_name);
+
 	tty_unregister_device(driver, index);
 }
 EXPORT_SYMBOL_GPL(tty_port_unregister_device);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1dd587ba6d88..7e36ee1d2056 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -249,6 +249,9 @@ struct tty_port {
 						   set to size of fifo */
 	struct kref		kref;		/* Ref counter */
 	void 			*client_data;
+
+	struct led_trigger	*led_trigger_rx;
+	char			*led_trigger_rx_name;
 };
 
 /* tty_port::iflags bits -- use atomic bit ops */
-- 
2.17.0

Re: [PATCH] tty: implement a rx led trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]

Hi!

> The trigger fires when data is pushed to the ldisc. 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.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Well, why not, but there certainly should be config option? Because I
really don't see many people wanting to use this trigger.

> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index c996b6859c5e..4d364b77b1a7 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -17,6 +17,7 @@
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/ratelimit.h>
> +#include <linux/leds.h>
>  
>  
>  #define MIN_TTYB_SIZE	256
> @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
>  		struct tty_buffer *head = buf->head;
>  		struct tty_buffer *next;
>  		int count;
> +		unsigned long delay = 50 /* ms */;
>  
>  		/* Ldisc or user is trying to gain exclusive access */
>  		if (atomic_read(&buf->priority))
> @@ -521,6 +523,8 @@ static void flush_to_ldisc(struct work_struct *work)
>  			continue;
>  		}
>  
> +		led_trigger_blink_oneshot(port->led_trigger_rx, &delay, &delay, 0);
> +
>  		count = receive_buf(port, head, count);
>  		if (!count)
>  			break;
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 25d736880013..51b78a585417 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -16,6 +16,7 @@
>  #include <linux/wait.h>
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
> +#include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/serdev.h>
>  
> @@ -157,6 +158,18 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
>  
>  	tty_port_link_device(port, driver, index);
>  
> +	port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> +					      driver->name, index);
> +	if (!port->led_trigger_rx_name) {
> +		pr_err("Failed to allocate trigger name for %s%d\n",
> +		       driver->name, index);
> +		goto skip_trigger;
> +	}
> +
> +	led_trigger_register_simple(port->led_trigger_rx_name,
> +				    &port->led_trigger_rx);
> +
> +skip_trigger:
>  	dev = serdev_tty_port_register(port, device, driver, index);
>  	if (PTR_ERR(dev) != -ENODEV) {
>  		/* Skip creating cdev if we registered a serdev device */
> @@ -206,6 +219,9 @@ void tty_port_unregister_device(struct tty_port *port,
>  	if (ret == 0)
>  		return;
>  
> +	led_trigger_unregister_simple(port->led_trigger_rx);

Is it ok to unregister if it was not registered in the first place?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] tty: implement a rx led trigger

[Uwe Kleine-König]

On Thu, May 03, 2018 at 12:10:47PM +0200, Pavel Machek wrote:
> Hi!
> 
> > The trigger fires when data is pushed to the ldisc. 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.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Well, why not, but there certainly should be config option? Because I
> really don't see many people wanting to use this trigger.

Yeah, can be done. I'll implement this for v2.

> > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> > index c996b6859c5e..4d364b77b1a7 100644
> > --- a/drivers/tty/tty_buffer.c
> > +++ b/drivers/tty/tty_buffer.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/module.h>
> >  #include <linux/ratelimit.h>
> > +#include <linux/leds.h>
> >  
> >  
> >  #define MIN_TTYB_SIZE	256
> > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> >  		struct tty_buffer *head = buf->head;
> >  		struct tty_buffer *next;
> >  		int count;
> > +		unsigned long delay = 50 /* ms */;
> >  
> >  		/* Ldisc or user is trying to gain exclusive access */
> >  		if (atomic_read(&buf->priority))
> > @@ -521,6 +523,8 @@ static void flush_to_ldisc(struct work_struct *work)
> >  			continue;
> >  		}
> >  
> > +		led_trigger_blink_oneshot(port->led_trigger_rx, &delay, &delay, 0);
> > +
> >  		count = receive_buf(port, head, count);
> >  		if (!count)
> >  			break;
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index 25d736880013..51b78a585417 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/wait.h>
> >  #include <linux/bitops.h>
> >  #include <linux/delay.h>
> > +#include <linux/leds.h>
> >  #include <linux/module.h>
> >  #include <linux/serdev.h>
> >  
> > @@ -157,6 +158,18 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
> >  
> >  	tty_port_link_device(port, driver, index);
> >  
> > +	port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> > +					      driver->name, index);
> > +	if (!port->led_trigger_rx_name) {
> > +		pr_err("Failed to allocate trigger name for %s%d\n",
> > +		       driver->name, index);
> > +		goto skip_trigger;
> > +	}
> > +
> > +	led_trigger_register_simple(port->led_trigger_rx_name,
> > +				    &port->led_trigger_rx);
> > +
> > +skip_trigger:
> >  	dev = serdev_tty_port_register(port, device, driver, index);
> >  	if (PTR_ERR(dev) != -ENODEV) {
> >  		/* Skip creating cdev if we registered a serdev device */
> > @@ -206,6 +219,9 @@ void tty_port_unregister_device(struct tty_port *port,
> >  	if (ret == 0)
> >  		return;
> >  
> > +	led_trigger_unregister_simple(port->led_trigger_rx);
> 
> Is it ok to unregister if it was not registered in the first place?

led_trigger_unregister_simple() looks as follows:

	if (trig)
		led_trigger_unregister(trig);
	kfree(trig);

So yes, it does the right thing when it wasn't registered. Assuming it
is NULL then, which doesn't seem to be guaranteed. Probably I'd need

	port->led_trigger_rx = NULL
	
if kasprintf failed.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] tty: implement a rx led trigger

[Robin Murphy]

On 03/05/18 11:04, Uwe Kleine-König wrote:
[...]
> @@ -157,6 +158,18 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
>   
>   	tty_port_link_device(port, driver, index);
>   
> +	port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> +					      driver->name, index);
> +	if (!port->led_trigger_rx_name) {
> +		pr_err("Failed to allocate trigger name for %s%d\n",
> +		       driver->name, index);
> +		goto skip_trigger;

Hmm, isn't that a rather awkward way to spell "else"? ;)

Robin.

> +	}
> +
> +	led_trigger_register_simple(port->led_trigger_rx_name,
> +				    &port->led_trigger_rx);
> +
> +skip_trigger:
>   	dev = serdev_tty_port_register(port, device, driver, index);
>   	if (PTR_ERR(dev) != -ENODEV) {
>   		/* Skip creating cdev if we registered a serdev device */

[PATCH v2] tty: implement led triggers

[Uwe Kleine-König]

The rx trigger fires when data is pushed to the ldisc. 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.

Similarily the tx trigger fires when data taken from the ldisc.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Changes since v1, sent with Message-Id: 
20180503100448.1350-1-u.kleine-koenig@pengutronix.de:

 - implement tx trigger;
 - introduce Kconfig symbol for conditional compilation;
 - set trigger to NULL if allocating the name failed to not free random
   pointers in case the port struct wasn't zeroed;
 - use if/else instead of goto

 drivers/tty/Kconfig      |  7 +++++++
 drivers/tty/tty_buffer.c |  4 ++++
 drivers/tty/tty_io.c     |  6 ++++++
 drivers/tty/tty_port.c   | 32 ++++++++++++++++++++++++++++++++
 include/linux/tty.h      |  7 +++++++
 5 files changed, 56 insertions(+)

diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 0840d27381ea..07a2fb05439f 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_TRIGGER
+	bool "Enable support for TTY actions making LEDs blink"
+	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..4d364b77b1a7 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -17,6 +17,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/ratelimit.h>
+#include <linux/leds.h>
 
 
 #define MIN_TTYB_SIZE	256
@@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
 		struct tty_buffer *head = buf->head;
 		struct tty_buffer *next;
 		int count;
+		unsigned long delay = 50 /* ms */;
 
 		/* Ldisc or user is trying to gain exclusive access */
 		if (atomic_read(&buf->priority))
@@ -521,6 +523,8 @@ static void flush_to_ldisc(struct work_struct *work)
 			continue;
 		}
 
+		led_trigger_blink_oneshot(port->led_trigger_rx, &delay, &delay, 0);
+
 		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..2c840f1e1e82 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -83,6 +83,7 @@
 #include <linux/timer.h>
 #include <linux/ctype.h>
 #include <linux/kd.h>
+#include <linux/leds.h>
 #include <linux/mm.h>
 #include <linux/string.h>
 #include <linux/slab.h>
@@ -950,11 +951,16 @@ static inline ssize_t do_tty_write(
 	/* Do the write .. */
 	for (;;) {
 		size_t size = count;
+		unsigned long delay = 50 /* ms */;
+
 		if (size > chunk)
 			size = chunk;
 		ret = -EFAULT;
 		if (copy_from_user(tty->write_buf, buf, size))
 			break;
+
+		led_trigger_blink_oneshot(tty->port->led_trigger_tx, &delay, &delay, 0);
+
 		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..f042879a597c 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -16,6 +16,7 @@
 #include <linux/wait.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/serdev.h>
 
@@ -157,6 +158,30 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
 
 	tty_port_link_device(port, driver, index);
 
+#ifdef CONFIG_TTY_LEDS_TRIGGER
+	port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
+					      driver->name, index);
+	if (port->led_trigger_rx_name) {
+		led_trigger_register_simple(port->led_trigger_rx_name,
+					    &port->led_trigger_rx);
+	} else {
+		port->led_trigger_rx = NULL;
+		pr_err("Failed to allocate trigger name for %s%d\n",
+		       driver->name, index);
+	}
+
+	port->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
+					      driver->name, index);
+	if (port->led_trigger_tx_name) {
+		led_trigger_register_simple(port->led_trigger_tx_name,
+					    &port->led_trigger_tx);
+	} else {
+		port->led_trigger_tx = NULL;
+		pr_err("Failed to allocate trigger name for %s%d\n",
+		       driver->name, index);
+	}
+#endif
+
 	dev = serdev_tty_port_register(port, device, driver, index);
 	if (PTR_ERR(dev) != -ENODEV) {
 		/* Skip creating cdev if we registered a serdev device */
@@ -206,6 +231,13 @@ void tty_port_unregister_device(struct tty_port *port,
 	if (ret == 0)
 		return;
 
+#ifdef CONFIG_TTY_LEDS_TRIGGER
+	led_trigger_unregister_simple(port->led_trigger_rx);
+	kfree(port->led_trigger_rx_name);
+	led_trigger_unregister_simple(port->led_trigger_tx);
+	kfree(port->led_trigger_tx_name);
+#endif
+
 	tty_unregister_device(driver, index);
 }
 EXPORT_SYMBOL_GPL(tty_port_unregister_device);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1dd587ba6d88..b7dc957365b6 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -249,6 +249,13 @@ struct tty_port {
 						   set to size of fifo */
 	struct kref		kref;		/* Ref counter */
 	void 			*client_data;
+
+#ifdef CONFIG_TTY_LEDS_TRIGGER
+	struct led_trigger	*led_trigger_rx;
+	char			*led_trigger_rx_name;
+	struct led_trigger	*led_trigger_tx;
+	char			*led_trigger_tx_name;
+#endif
 };
 
 /* tty_port::iflags bits -- use atomic bit ops */
-- 
2.17.0

Re: [PATCH v2] tty: implement led triggers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4156 bytes --]

Hi Uwe,

I love your patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.17-rc3 next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/tty-implement-led-triggers/20180504-075232
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: i386-randconfig-x014-201817 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/tty/tty_io.c: In function 'do_tty_write':
>> drivers/tty/tty_io.c:962:38: error: 'struct tty_port' has no member named 'led_trigger_tx'
      led_trigger_blink_oneshot(tty->port->led_trigger_tx, &delay, &delay, 0);
                                         ^~
--
   drivers/tty/tty_buffer.c: In function 'flush_to_ldisc':
>> drivers/tty/tty_buffer.c:526:33: error: 'struct tty_port' has no member named 'led_trigger_rx'
      led_trigger_blink_oneshot(port->led_trigger_rx, &delay, &delay, 0);
                                    ^~

vim +962 drivers/tty/tty_io.c

   893	
   894	/*
   895	 * Split writes up in sane blocksizes to avoid
   896	 * denial-of-service type attacks
   897	 */
   898	static inline ssize_t do_tty_write(
   899		ssize_t (*write)(struct tty_struct *, struct file *, const unsigned char *, size_t),
   900		struct tty_struct *tty,
   901		struct file *file,
   902		const char __user *buf,
   903		size_t count)
   904	{
   905		ssize_t ret, written = 0;
   906		unsigned int chunk;
   907	
   908		ret = tty_write_lock(tty, file->f_flags & O_NDELAY);
   909		if (ret < 0)
   910			return ret;
   911	
   912		/*
   913		 * We chunk up writes into a temporary buffer. This
   914		 * simplifies low-level drivers immensely, since they
   915		 * don't have locking issues and user mode accesses.
   916		 *
   917		 * But if TTY_NO_WRITE_SPLIT is set, we should use a
   918		 * big chunk-size..
   919		 *
   920		 * The default chunk-size is 2kB, because the NTTY
   921		 * layer has problems with bigger chunks. It will
   922		 * claim to be able to handle more characters than
   923		 * it actually does.
   924		 *
   925		 * FIXME: This can probably go away now except that 64K chunks
   926		 * are too likely to fail unless switched to vmalloc...
   927		 */
   928		chunk = 2048;
   929		if (test_bit(TTY_NO_WRITE_SPLIT, &tty->flags))
   930			chunk = 65536;
   931		if (count < chunk)
   932			chunk = count;
   933	
   934		/* write_buf/write_cnt is protected by the atomic_write_lock mutex */
   935		if (tty->write_cnt < chunk) {
   936			unsigned char *buf_chunk;
   937	
   938			if (chunk < 1024)
   939				chunk = 1024;
   940	
   941			buf_chunk = kmalloc(chunk, GFP_KERNEL);
   942			if (!buf_chunk) {
   943				ret = -ENOMEM;
   944				goto out;
   945			}
   946			kfree(tty->write_buf);
   947			tty->write_cnt = chunk;
   948			tty->write_buf = buf_chunk;
   949		}
   950	
   951		/* Do the write .. */
   952		for (;;) {
   953			size_t size = count;
   954			unsigned long delay = 50 /* ms */;
   955	
   956			if (size > chunk)
   957				size = chunk;
   958			ret = -EFAULT;
   959			if (copy_from_user(tty->write_buf, buf, size))
   960				break;
   961	
 > 962			led_trigger_blink_oneshot(tty->port->led_trigger_tx, &delay, &delay, 0);
   963	
   964			ret = write(tty, file, tty->write_buf, size);
   965			if (ret <= 0)
   966				break;
   967			written += ret;
   968			buf += ret;
   969			count -= ret;
   970			if (!count)
   971				break;
   972			ret = -ERESTARTSYS;
   973			if (signal_pending(current))
   974				break;
   975			cond_resched();
   976		}
   977		if (written) {
   978			tty_update_time(&file_inode(file)->i_mtime);
   979			ret = written;
   980		}
   981	out:
   982		tty_write_unlock(tty);
   983		return ret;
   984	}
   985	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28533 bytes --]

Re: [PATCH v2] tty: implement led triggers

[Johan Hovold]

On Thu, May 03, 2018 at 10:19:52PM +0200, Uwe Kleine-König wrote:
> The rx trigger fires when data is pushed to the ldisc. 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.
> 
> Similarily the tx trigger fires when data taken from the ldisc.

You meant copied from user space, or written to the ldisc, here.

Note that the rx-path is shared with serdev, but the write path is not.
So with this series, serdev devices would only trigger the rx-led.

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since v1, sent with Message-Id: 
> 20180503100448.1350-1-u.kleine-koenig@pengutronix.de:
> 
>  - implement tx trigger;
>  - introduce Kconfig symbol for conditional compilation;
>  - set trigger to NULL if allocating the name failed to not free random
>    pointers in case the port struct wasn't zeroed;
>  - use if/else instead of goto

> @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
>  		struct tty_buffer *head = buf->head;
>  		struct tty_buffer *next;
>  		int count;
> +		unsigned long delay = 50 /* ms */;

Comment after the semicolon?

>  
>  		/* Ldisc or user is trying to gain exclusive access */
>  		if (atomic_read(&buf->priority))

> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 25d736880013..f042879a597c 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -16,6 +16,7 @@
>  #include <linux/wait.h>
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
> +#include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/serdev.h>
>  
> @@ -157,6 +158,30 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
>  
>  	tty_port_link_device(port, driver, index);
>  
> +#ifdef CONFIG_TTY_LEDS_TRIGGER
> +	port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> +					      driver->name, index);
> +	if (port->led_trigger_rx_name) {
> +		led_trigger_register_simple(port->led_trigger_rx_name,
> +					    &port->led_trigger_rx);
> +	} else {
> +		port->led_trigger_rx = NULL;
> +		pr_err("Failed to allocate trigger name for %s%d\n",
> +		       driver->name, index);
> +	}
> +
> +	port->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
> +					      driver->name, index);
> +	if (port->led_trigger_tx_name) {
> +		led_trigger_register_simple(port->led_trigger_tx_name,
> +					    &port->led_trigger_tx);
> +	} else {
> +		port->led_trigger_tx = NULL;
> +		pr_err("Failed to allocate trigger name for %s%d\n",
> +		       driver->name, index);
> +	}
> +#endif

Besides the ugly ifdefs here, you're leaking the above LED resources in
the error paths below (e.g. on probe deferrals).

>  	dev = serdev_tty_port_register(port, device, driver, index);
>  	if (PTR_ERR(dev) != -ENODEV) {
>  		/* Skip creating cdev if we registered a serdev device */

> @@ -249,6 +249,13 @@ struct tty_port {
>  						   set to size of fifo */
>  	struct kref		kref;		/* Ref counter */
>  	void 			*client_data;
> +
> +#ifdef CONFIG_TTY_LEDS_TRIGGER
> +	struct led_trigger	*led_trigger_rx;
> +	char			*led_trigger_rx_name;

const?

> +	struct led_trigger	*led_trigger_tx;
> +	char			*led_trigger_tx_name;

ditto.

> +#endif

Johan

Re: [PATCH v2] tty: implement led triggers

[Uwe Kleine-König]

Hello Johan,

thanks for your feedback.

On Mon, May 07, 2018 at 10:02:52AM +0200, Johan Hovold wrote:
> On Thu, May 03, 2018 at 10:19:52PM +0200, Uwe Kleine-König wrote:
> > The rx trigger fires when data is pushed to the ldisc. 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.
> > 
> > Similarily the tx trigger fires when data taken from the ldisc.
> 
> You meant copied from user space, or written to the ldisc, here.

ack.

> Note that the rx-path is shared with serdev, but the write path is not.
> So with this series, serdev devices would only trigger the rx-led.

Where would be the right place to put the tx trigger to catch serdev,
too?

> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Changes since v1, sent with Message-Id: 
> > 20180503100448.1350-1-u.kleine-koenig@pengutronix.de:
> > 
> >  - implement tx trigger;
> >  - introduce Kconfig symbol for conditional compilation;
> >  - set trigger to NULL if allocating the name failed to not free random
> >    pointers in case the port struct wasn't zeroed;
> >  - use if/else instead of goto
> 
> > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> >  		struct tty_buffer *head = buf->head;
> >  		struct tty_buffer *next;
> >  		int count;
> > +		unsigned long delay = 50 /* ms */;
> 
> Comment after the semicolon?

Given that this comment is about the 50 and not the delay member, I
prefer it before the ;.

> >  
> >  		/* Ldisc or user is trying to gain exclusive access */
> >  		if (atomic_read(&buf->priority))
> 
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index 25d736880013..f042879a597c 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/wait.h>
> >  #include <linux/bitops.h>
> >  #include <linux/delay.h>
> > +#include <linux/leds.h>
> >  #include <linux/module.h>
> >  #include <linux/serdev.h>
> >  
> > @@ -157,6 +158,30 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
> >  
> >  	tty_port_link_device(port, driver, index);
> >  
> > +#ifdef CONFIG_TTY_LEDS_TRIGGER
> > +	port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> > +					      driver->name, index);
> > +	if (port->led_trigger_rx_name) {
> > +		led_trigger_register_simple(port->led_trigger_rx_name,
> > +					    &port->led_trigger_rx);
> > +	} else {
> > +		port->led_trigger_rx = NULL;
> > +		pr_err("Failed to allocate trigger name for %s%d\n",
> > +		       driver->name, index);
> > +	}
> > +
> > +	port->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
> > +					      driver->name, index);
> > +	if (port->led_trigger_tx_name) {
> > +		led_trigger_register_simple(port->led_trigger_tx_name,
> > +					    &port->led_trigger_tx);
> > +	} else {
> > +		port->led_trigger_tx = NULL;
> > +		pr_err("Failed to allocate trigger name for %s%d\n",
> > +		       driver->name, index);
> > +	}
> > +#endif
> 
> Besides the ugly ifdefs here, you're leaking the above LED resources in
> the error paths below (e.g. on probe deferrals).

ack, the ifdevs are ugly. I'm working on an idea to get rid of them.
 
Will think about how to plug the leak.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2] tty: implement led triggers

[Johan Hovold]

On Mon, May 07, 2018 at 10:41:27AM +0200, Uwe Kleine-König wrote:
> Hello Johan,
> 
> thanks for your feedback.
> 
> On Mon, May 07, 2018 at 10:02:52AM +0200, Johan Hovold wrote:
> > On Thu, May 03, 2018 at 10:19:52PM +0200, Uwe Kleine-König wrote:
> > > The rx trigger fires when data is pushed to the ldisc. 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.
> > > 
> > > Similarily the tx trigger fires when data taken from the ldisc.
> > 
> > You meant copied from user space, or written to the ldisc, here.
> 
> ack.
> 
> > Note that the rx-path is shared with serdev, but the write path is not.
> > So with this series, serdev devices would only trigger the rx-led.
> 
> Where would be the right place to put the tx trigger to catch serdev,
> too?

I haven't given this much thought, but do we really want this for serdev
at all? I'm thinking whatever driver or subsystem is using serdev should
have their own triggers (e.g. bluetooth or net).

So it might be better to move the rx-blinking to the default tty-port
client receive_buf callback instead (i.e.
tty_port_default_receive_buf()).

And then the resource allocations would need to go after the serdev
registration in tty_port_register_device_attr_serdev().

> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Changes since v1, sent with Message-Id: 
> > > 20180503100448.1350-1-u.kleine-koenig@pengutronix.de:
> > > 
> > >  - implement tx trigger;
> > >  - introduce Kconfig symbol for conditional compilation;
> > >  - set trigger to NULL if allocating the name failed to not free random
> > >    pointers in case the port struct wasn't zeroed;
> > >  - use if/else instead of goto
> > 
> > > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> > >  		struct tty_buffer *head = buf->head;
> > >  		struct tty_buffer *next;
> > >  		int count;
> > > +		unsigned long delay = 50 /* ms */;
> > 
> > Comment after the semicolon?
> 
> Given that this comment is about the 50 and not the delay member, I
> prefer it before the ;.

Hmm. I personally find it hard to read and can only find about 30
instances of this comment style (for assignments) in the kernel. And
arguably the comment applies equally well to the delay variable in this
case too.

> > Besides the ugly ifdefs here, you're leaking the above LED resources in
> > the error paths below (e.g. on probe deferrals).

> ack, the ifdevs are ugly. I'm working on an idea to get rid of them.

Sounds good.

Thanks,
Johan

Re: [PATCH v2] tty: implement led triggers

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

Hi!

> > > > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> > > >  		struct tty_buffer *head = buf->head;
> > > >  		struct tty_buffer *next;
> > > >  		int count;
> > > > +		unsigned long delay = 50 /* ms */;
> > > 
> > > Comment after the semicolon?
> > 
> > Given that this comment is about the 50 and not the delay member, I
> > prefer it before the ;.
> 
> Hmm. I personally find it hard to read and can only find about 30
> instances of this comment style (for assignments) in the kernel. And
> arguably the comment applies equally well to the delay variable in this
> case too.

It is not too traditional, but I believe it makes sense....

(and yes, I wish we had kernel in Rust, so we could have real units
attached to our variables...)

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2] tty: implement led triggers

[Robin Murphy]

On 10/05/18 12:14, Pavel Machek wrote:
> Hi!
> 
>>>>> @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
>>>>>   		struct tty_buffer *head = buf->head;
>>>>>   		struct tty_buffer *next;
>>>>>   		int count;
>>>>> +		unsigned long delay = 50 /* ms */;
>>>>
>>>> Comment after the semicolon?
>>>
>>> Given that this comment is about the 50 and not the delay member, I
>>> prefer it before the ;.
>>
>> Hmm. I personally find it hard to read and can only find about 30
>> instances of this comment style (for assignments) in the kernel. And
>> arguably the comment applies equally well to the delay variable in this
>> case too.
> 
> It is not too traditional, but I believe it makes sense....
> 
> (and yes, I wish we had kernel in Rust, so we could have real units
> attached to our variables...)

Well, the variable itself could always be named "delay_ms" if it's 
really that important.

Robin.

Re: [PATCH v2] tty: implement led triggers

[Uwe Kleine-König]

On Thu, May 10, 2018 at 12:25:21PM +0100, Robin Murphy wrote:
> On 10/05/18 12:14, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> > > > > >   		struct tty_buffer *head = buf->head;
> > > > > >   		struct tty_buffer *next;
> > > > > >   		int count;
> > > > > > +		unsigned long delay = 50 /* ms */;
> > > > > 
> > > > > Comment after the semicolon?
> > > > 
> > > > Given that this comment is about the 50 and not the delay member, I
> > > > prefer it before the ;.
> > > 
> > > Hmm. I personally find it hard to read and can only find about 30
> > > instances of this comment style (for assignments) in the kernel. And
> > > arguably the comment applies equally well to the delay variable in this
> > > case too.
> > 
> > It is not too traditional, but I believe it makes sense....
> > 
> > (and yes, I wish we had kernel in Rust, so we could have real units
> > attached to our variables...)
> 
> Well, the variable itself could always be named "delay_ms" if it's really
> that important.

FTR: That's what I did for v3.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |