| Message ID | 20180503201952.16592-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tty: implement led triggers | expand |
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
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
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
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
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
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.
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
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 */
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(+)