| Message ID | 20161123100106.15969-2-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > With this patch the serial core provides LED triggers for RX and TX. > > As the serial core layer does not know when the hardware actually sends > or receives characters, this needs help from the UART drivers. The > LED triggers are registered in uart_add_led_triggers() called from > the UART drivers which want to support LED triggers. All the driver > has to do then is to call uart_led_trigger_[tx|rx] to indicate > activity. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ > include/linux/serial_core.h | 10 ++++++ > 2 files changed, 83 insertions(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index f2303f3..3e8afb7 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -34,6 +34,7 @@ > #include <linux/serial_core.h> > #include <linux/delay.h> > #include <linux/mutex.h> > +#include <linux/leds.h> > > #include <asm/irq.h> > #include <asm/uaccess.h> > @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = { > .attrs = tty_dev_attrs, > }; > > +void uart_led_trigger_tx(struct uart_port *uport) > +{ > + unsigned long delay = 50; > + > + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0); > +} > + > +void uart_led_trigger_rx(struct uart_port *uport) > +{ > + unsigned long delay = 50; > + > + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0); > +} Don't these functions need an EXPORT_SYMBOL_GPL() to work properly with uart drivers being built as a module? thanks, greg k-h
On Wed, Nov 23, 2016 at 11:08:19AM +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > > With this patch the serial core provides LED triggers for RX and TX. > > > > As the serial core layer does not know when the hardware actually sends > > or receives characters, this needs help from the UART drivers. The > > LED triggers are registered in uart_add_led_triggers() called from > > the UART drivers which want to support LED triggers. All the driver > > has to do then is to call uart_led_trigger_[tx|rx] to indicate > > activity. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/serial_core.h | 10 ++++++ > > 2 files changed, 83 insertions(+) > > > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > > index f2303f3..3e8afb7 100644 > > --- a/drivers/tty/serial/serial_core.c > > +++ b/drivers/tty/serial/serial_core.c > > @@ -34,6 +34,7 @@ > > #include <linux/serial_core.h> > > #include <linux/delay.h> > > #include <linux/mutex.h> > > +#include <linux/leds.h> > > > > #include <asm/irq.h> > > #include <asm/uaccess.h> > > @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = { > > .attrs = tty_dev_attrs, > > }; > > > > +void uart_led_trigger_tx(struct uart_port *uport) > > +{ > > + unsigned long delay = 50; > > + > > + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0); > > +} > > + > > +void uart_led_trigger_rx(struct uart_port *uport) > > +{ > > + unsigned long delay = 50; > > + > > + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0); > > +} > > Don't these functions need an EXPORT_SYMBOL_GPL() to work properly with > uart drivers being built as a module? Yes, for sure. Will fix in next version. Sascha
On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > With this patch the serial core provides LED triggers for RX and TX. > > As the serial core layer does not know when the hardware actually sends > or receives characters, this needs help from the UART drivers. The > LED triggers are registered in uart_add_led_triggers() called from > the UART drivers which want to support LED triggers. All the driver > has to do then is to call uart_led_trigger_[tx|rx] to indicate > activite. Hello Sascha, > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ > include/linux/serial_core.h | 10 ++++++ > 2 files changed, 83 insertions(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index f2303f3..3e8afb7 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -34,6 +34,7 @@ > #include <linux/serial_core.h> > #include <linux/delay.h> > #include <linux/mutex.h> > +#include <linux/leds.h> > > #include <asm/irq.h> > #include <asm/uaccess.h> > @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = { > .attrs = tty_dev_attrs, > }; > > +void uart_led_trigger_tx(struct uart_port *uport) > +{ > + unsigned long delay = 50; > + > + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0); > +} > + > +void uart_led_trigger_rx(struct uart_port *uport) > +{ > + unsigned long delay = 50; > + > + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0); For both rx/tx the core constrains the delay_on/off along with the inversion. Instead of adding the led_trigger_rx/tx and led_trigger_rx/tx_name to the struct uart_port, wouldn't it be better to add a new struct led_trigger that would encapsulate those along wit the on/off delay and the inversion? That way those values could be communicated to the core at registration time instead of hard-coding things. > +} > + > +/** > + * uart_add_led_triggers - register LED triggers for a UART > + * @drv: pointer to the uart low level driver structure for this port > + * @uport: uart port structure to use for this port. > + * > + * Called by the driver to register LED triggers for a UART. It's the > + * drivers responsibility to call uart_led_trigger_rx/tx on received > + * and transmitted chars then. > + */ > +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport) > +{ > + int ret; > + > + if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS)) > + return 0; Since this is a public interface, checking for valid led_trigger_tx/rx before moving on with the rest of the initialisation is probably a good idea. Thanks, Mathieu > + > + uport->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx", > + drv->dev_name, uport->line); > + uport->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx", > + drv->dev_name, uport->line); > + if (!uport->led_trigger_tx_name || !uport->led_trigger_rx_name) { > + ret = -ENOMEM; > + goto err_alloc; > + } > + > + led_trigger_register_simple(uport->led_trigger_tx_name, > + &uport->led_trigger_tx); > + led_trigger_register_simple(uport->led_trigger_rx_name, > + &uport->led_trigger_rx); > + > + return 0; > + > +err_alloc: > + kfree(uport->led_trigger_tx_name); > + kfree(uport->led_trigger_rx_name); > + > + return ret; > +} > + > +/** > + * uart_remove_led_triggers - remove LED triggers > + * @drv: pointer to the uart low level driver structure for this port > + * @uport: uart port structure to use for this port. > + * > + * Remove LED triggers previously registered with uart_add_led_triggers > + */ > +void uart_remove_led_triggers(struct uart_port *uport) > +{ > + if (uport->led_trigger_rx) > + led_trigger_unregister_simple(uport->led_trigger_rx); > + kfree(uport->led_trigger_rx_name); > + > + if (uport->led_trigger_tx) > + led_trigger_unregister_simple(uport->led_trigger_tx); > + kfree(uport->led_trigger_tx_name); > +} > + > /** > * uart_add_one_port - attach a driver-defined port structure > * @drv: pointer to the uart low level driver structure for this port > @@ -2872,6 +2944,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport) > WARN_ON(atomic_dec_return(&state->refcount) < 0); > wait_event(state->remove_wait, !atomic_read(&state->refcount)); > state->uart_port = NULL; > + > mutex_unlock(&port->mutex); > out: > mutex_unlock(&port_mutex); > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h > index 3442014..1b0a169 100644 > --- a/include/linux/serial_core.h > +++ b/include/linux/serial_core.h > @@ -29,6 +29,7 @@ > #include <linux/tty.h> > #include <linux/mutex.h> > #include <linux/sysrq.h> > +#include <linux/leds.h> > #include <uapi/linux/serial_core.h> > > #ifdef CONFIG_SERIAL_CORE_CONSOLE > @@ -249,6 +250,10 @@ struct uart_port { > const struct attribute_group **tty_groups; /* all attributes (serial core use only) */ > struct serial_rs485 rs485; > void *private_data; /* generic platform data pointer */ > + struct led_trigger *led_trigger_rx; > + char *led_trigger_rx_name; > + struct led_trigger *led_trigger_tx; > + char *led_trigger_tx_name; > }; > > static inline int serial_port_in(struct uart_port *up, int offset) > @@ -392,6 +397,11 @@ void uart_console_write(struct uart_port *port, const char *s, > unsigned int count, > void (*putchar)(struct uart_port *, int)); > > +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport); > +void uart_remove_led_triggers(struct uart_port *uport); > +void uart_led_trigger_tx(struct uart_port *port); > +void uart_led_trigger_rx(struct uart_port *port); > + > /* > * Port/driver registration/removal > */ > -- > 2.10.2 >
On 11/23/2016 02:01 AM, Sascha Hauer wrote: > With this patch the serial core provides LED triggers for RX and TX. > > As the serial core layer does not know when the hardware actually sends > or receives characters, this needs help from the UART drivers. Looking at 8250, we call serial8250_tx_chars from __start_tx which is called form the uart_ops::start_tx, for RX I sort of agree, since this happens in interrupt handler. I suppose some drivers could actually queue work but not do it right away though? > The LED triggers are registered in uart_add_led_triggers() called from > the UART drivers which want to support LED triggers. All the driver > has to do then is to call uart_led_trigger_[tx|rx] to indicate > activity. Could we somehow remedy the lack of knowledge from the core as whether the HW sends/receives characters first before adding support for LED triggers? It would be more generic and future proof to require UART drivers to report to the core when they actually TX/RX, and then at the core level, utilize that knowledge to perform the LED trigger. Side note: are you positive using drv->dev_name is robust enough on systems with many different UART drivers, yet all of them being ttyS*? Thanks!
Hi Sascha, [auto build test WARNING on tty/tty-testing] [also build test WARNING on v4.9-rc6 next-20161123] [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/Sascha-Hauer/serial-core-Add-LED-trigger-support/20161124-010846 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing reproduce: make htmldocs All warnings (new ones prefixed by >>): make[3]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule. include/linux/init.h:1: warning: no structured comments found include/linux/workqueue.h:392: warning: No description found for parameter '...' include/linux/workqueue.h:392: warning: Excess function parameter 'args' description in 'alloc_workqueue' include/linux/workqueue.h:413: warning: No description found for parameter '...' include/linux/workqueue.h:413: warning: Excess function parameter 'args' description in 'alloc_ordered_workqueue' include/linux/kthread.h:26: warning: No description found for parameter '...' kernel/sys.c:1: warning: no structured comments found drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found include/linux/fence-array.h:61: warning: No description found for parameter 'fence' >> drivers/tty/serial/serial_core.c:2773: warning: Excess function parameter 'drv' description in 'uart_remove_led_triggers' include/sound/core.h:324: warning: No description found for parameter '...' include/sound/core.h:335: warning: No description found for parameter '...' include/sound/core.h:388: warning: No description found for parameter '...' include/media/media-entity.h:1054: warning: No description found for parameter '...' include/net/mac80211.h:2148: WARNING: Inline literal start-string without end-string. include/net/mac80211.h:2153: WARNING: Inline literal start-string without end-string. include/net/mac80211.h:3202: ERROR: Unexpected indentation. include/net/mac80211.h:3205: WARNING: Block quote ends without a blank line; unexpected unindent. include/net/mac80211.h:3207: ERROR: Unexpected indentation. include/net/mac80211.h:3208: WARNING: Block quote ends without a blank line; unexpected unindent. include/net/mac80211.h:1435: WARNING: Inline emphasis start-string without end-string. include/net/mac80211.h:1172: WARNING: Inline literal start-string without end-string. include/net/mac80211.h:1173: WARNING: Inline literal start-string without end-string. include/net/mac80211.h:814: ERROR: Unexpected indentation. include/net/mac80211.h:815: WARNING: Block quote ends without a blank line; unexpected unindent. include/net/mac80211.h:820: ERROR: Unexpected indentation. include/net/mac80211.h:821: WARNING: Block quote ends without a blank line; unexpected unindent. include/net/mac80211.h:2489: ERROR: Unexpected indentation. include/net/mac80211.h:1768: ERROR: Unexpected indentation. include/net/mac80211.h:1772: WARNING: Block quote ends without a blank line; unexpected unindent. include/net/mac80211.h:1746: WARNING: Inline emphasis start-string without end-string. kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string. kernel/time/timer.c:1240: ERROR: Unexpected indentation. kernel/time/timer.c:1242: ERROR: Unexpected indentation. kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent. include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent. include/linux/wait.h:124: ERROR: Unexpected indentation. include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent. kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent. kernel/signal.c:317: WARNING: Inline literal start-string without end-string. drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent. drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent. drivers/tty/serial/serial_core.c:1897: WARNING: Definition list ends without a blank line; unexpected unindent. include/linux/spi/spi.h:369: ERROR: Unexpected indentation. WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting vim +2773 drivers/tty/serial/serial_core.c 2757 err_alloc: 2758 kfree(uport->led_trigger_tx_name); 2759 kfree(uport->led_trigger_rx_name); 2760 2761 return ret; 2762 } 2763 2764 /** 2765 * uart_remove_led_triggers - remove LED triggers 2766 * @drv: pointer to the uart low level driver structure for this port 2767 * @uport: uart port structure to use for this port. 2768 * 2769 * Remove LED triggers previously registered with uart_add_led_triggers 2770 */ 2771 void uart_remove_led_triggers(struct uart_port *uport) 2772 { > 2773 if (uport->led_trigger_rx) 2774 led_trigger_unregister_simple(uport->led_trigger_rx); 2775 kfree(uport->led_trigger_rx_name); 2776 2777 if (uport->led_trigger_tx) 2778 led_trigger_unregister_simple(uport->led_trigger_tx); 2779 kfree(uport->led_trigger_tx_name); 2780 } 2781 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Nov 23, 2016 at 10:13:02AM -0700, Mathieu Poirier wrote: > On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > > With this patch the serial core provides LED triggers for RX and TX. > > > > As the serial core layer does not know when the hardware actually sends > > or receives characters, this needs help from the UART drivers. The > > LED triggers are registered in uart_add_led_triggers() called from > > the UART drivers which want to support LED triggers. All the driver > > has to do then is to call uart_led_trigger_[tx|rx] to indicate > > activite. > > Hello Sascha, > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/serial_core.h | 10 ++++++ > > 2 files changed, 83 insertions(+) > > > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > > index f2303f3..3e8afb7 100644 > > --- a/drivers/tty/serial/serial_core.c > > +++ b/drivers/tty/serial/serial_core.c > > @@ -34,6 +34,7 @@ > > #include <linux/serial_core.h> > > #include <linux/delay.h> > > #include <linux/mutex.h> > > +#include <linux/leds.h> > > > > #include <asm/irq.h> > > #include <asm/uaccess.h> > > @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = { > > .attrs = tty_dev_attrs, > > }; > > > > +void uart_led_trigger_tx(struct uart_port *uport) > > +{ > > + unsigned long delay = 50; > > + > > + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0); > > +} > > + > > +void uart_led_trigger_rx(struct uart_port *uport) > > +{ > > + unsigned long delay = 50; > > + > > + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0); > > For both rx/tx the core constrains the delay_on/off along with the inversion. > Instead of adding the led_trigger_rx/tx and led_trigger_rx/tx_name to the > struct uart_port, wouldn't it be better to add a new struct led_trigger that > would encapsulate those along wit the on/off delay and the inversion? > > That way those values could be communicated to the core at registration time > instead of hard-coding things. Not sure this goes into the right direction. Looking at the other callers of led_trigger_blink_oneshot() most of them use an arbitrary blink time of 30 or 50ms and the users seem to be happy with it. There doesn't seem to be the desire to make that value configurable. If we want to make it configurable, it's probably better to move the option to the led device rather than putting the burden on every user of the led triggers. I don't think the inversion flag is meant for being configurable. It's rather used for the default state of the LED. The CAN layer for example turns the LED on when the interface is up and then blinks (turns off and on again) on activity. For this it uses the inversion flag. > > > +} > > + > > +/** > > + * uart_add_led_triggers - register LED triggers for a UART > > + * @drv: pointer to the uart low level driver structure for this port > > + * @uport: uart port structure to use for this port. > > + * > > + * Called by the driver to register LED triggers for a UART. It's the > > + * drivers responsibility to call uart_led_trigger_rx/tx on received > > + * and transmitted chars then. > > + */ > > +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport) > > +{ > > + int ret; > > + > > + if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS)) > > + return 0; > > Since this is a public interface, checking for valid led_trigger_tx/rx before > moving on with the rest of the initialisation is probably a good idea. What do you mean? Should we return an error when CONFIG_LEDS_TRIGGERS is disabled? Sascha
On Wed, Nov 23, 2016 at 10:57:13AM -0800, Florian Fainelli wrote: > On 11/23/2016 02:01 AM, Sascha Hauer wrote: > > With this patch the serial core provides LED triggers for RX and TX. > > > > As the serial core layer does not know when the hardware actually sends > > or receives characters, this needs help from the UART drivers. > > Looking at 8250, we call serial8250_tx_chars from __start_tx which is > called form the uart_ops::start_tx, for RX I sort of agree, since this > happens in interrupt handler. I suppose some drivers could actually > queue work but not do it right away though? RX is not the problem. When characters are received all drivers call tty_flip_buffer_push() which could be used for LED triggering (either directly or we replace the calls to tty_flip_buffer_push() with some wrapper function). The problem comes with TX. The serial core has a FIFO which gets characters from the tty layer. There is no function which the serial drivers could call to read from this FIFO, instead they have to open code this. Continuing with the 8250 driver serial8250_tx_chars() is not only called from __start_tx(), but also from the interrupt handler. Transmission works without the serial_core ever noticing. > > > The LED triggers are registered in uart_add_led_triggers() called from > > the UART drivers which want to support LED triggers. All the driver > > has to do then is to call uart_led_trigger_[tx|rx] to indicate > > activity. > > Could we somehow remedy the lack of knowledge from the core as whether > the HW sends/receives characters first before adding support for LED > triggers? It would be more generic and future proof to require UART > drivers to report to the core when they actually TX/RX, and then at the > core level, utilize that knowledge to perform the LED trigger. Maybe we could introduce a function to read from the TX FIFO. Having open coded this in each and every driver is not nice anyway. > > Side note: are you positive using drv->dev_name is robust enough on > systems with many different UART drivers, yet all of them being ttyS*? If we have different UART drivers, only one of them provides ttyS*, no? Other drivers will have to use another namespace. Sascha
On Wed, Nov 23, 2016 at 11:08:19AM +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > > With this patch the serial core provides LED triggers for RX and TX. > > > > As the serial core layer does not know when the hardware actually sends > > or receives characters, this needs help from the UART drivers. The > > LED triggers are registered in uart_add_led_triggers() called from > > the UART drivers which want to support LED triggers. All the driver > > has to do then is to call uart_led_trigger_[tx|rx] to indicate > > activity. BTW last time LED triggers were discussed (https://patchwork.kernel.org/patch/9212885/) You and Arnd mandated the triggers should be implemented in the tty layer. By tty layer did you really mean the tty layer or did you mean serial_core? We could implement it in the tty layer, but tty doesn't know when the characters are actually sent. There could be arbitrary time passing between a tty_operations->put_char and the character being on the wire. Also I am not sure if we want to have LED triggers for each and every tty in the system Sascha
On Thu, Nov 24, 2016 at 09:26:26AM +0100, Sascha Hauer wrote: > On Wed, Nov 23, 2016 at 11:08:19AM +0100, Greg Kroah-Hartman wrote: > > On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > > > With this patch the serial core provides LED triggers for RX and TX. > > > > > > As the serial core layer does not know when the hardware actually sends > > > or receives characters, this needs help from the UART drivers. The > > > LED triggers are registered in uart_add_led_triggers() called from > > > the UART drivers which want to support LED triggers. All the driver > > > has to do then is to call uart_led_trigger_[tx|rx] to indicate > > > activity. > > BTW last time LED triggers were discussed > (https://patchwork.kernel.org/patch/9212885/) You and Arnd mandated the > triggers should be implemented in the tty layer. By tty layer did you > really mean the tty layer or did you mean serial_core? > > We could implement it in the tty layer, but tty doesn't know when the > characters are actually sent. There could be arbitrary time passing > between a tty_operations->put_char and the character being on the wire. With USB serial devices and even basic UARTs, you never really know when "the character is on the wire", you can only guess. And really, just guessing is good enough given that no one is using this type of interface to actually count when exactly the bits hit the wire. This is just for those that like blinky-lights :) > Also I am not sure if we want to have LED triggers for each and every > tty in the system Why not? It's opt-in by the user, so might as well let them do it for whatever tty they want to. thanks, greg k-h
On Thu, Nov 24, 2016 at 07:41:37AM +0100, Sascha Hauer wrote: > On Wed, Nov 23, 2016 at 10:13:02AM -0700, Mathieu Poirier wrote: > > On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > > > With this patch the serial core provides LED triggers for RX and TX. > > > > > > As the serial core layer does not know when the hardware actually sends > > > or receives characters, this needs help from the UART drivers. The > > > LED triggers are registered in uart_add_led_triggers() called from > > > the UART drivers which want to support LED triggers. All the driver > > > has to do then is to call uart_led_trigger_[tx|rx] to indicate > > > activite. > > > > Hello Sascha, > > > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ > > > include/linux/serial_core.h | 10 ++++++ > > > 2 files changed, 83 insertions(+) > > > > > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > > > index f2303f3..3e8afb7 100644 > > > --- a/drivers/tty/serial/serial_core.c > > > +++ b/drivers/tty/serial/serial_core.c > > > @@ -34,6 +34,7 @@ > > > #include <linux/serial_core.h> > > > #include <linux/delay.h> > > > #include <linux/mutex.h> > > > +#include <linux/leds.h> > > > > > > #include <asm/irq.h> > > > #include <asm/uaccess.h> > > > @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = { > > > .attrs = tty_dev_attrs, > > > }; > > > > > > +void uart_led_trigger_tx(struct uart_port *uport) > > > +{ > > > + unsigned long delay = 50; > > > + > > > + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0); > > > +} > > > + > > > +void uart_led_trigger_rx(struct uart_port *uport) > > > +{ > > > + unsigned long delay = 50; > > > + > > > + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0); > > > > For both rx/tx the core constrains the delay_on/off along with the inversion. > > Instead of adding the led_trigger_rx/tx and led_trigger_rx/tx_name to the > > struct uart_port, wouldn't it be better to add a new struct led_trigger that > > would encapsulate those along wit the on/off delay and the inversion? > > > > That way those values could be communicated to the core at registration time > > instead of hard-coding things. > > Not sure this goes into the right direction. Looking at the other > callers of led_trigger_blink_oneshot() most of them use an arbitrary > blink time of 30 or 50ms and the users seem to be happy with it. There > doesn't seem to be the desire to make that value configurable. If we > want to make it configurable, it's probably better to move the option > to the led device rather than putting the burden on every user of the > led triggers. So you did find instances where the blink time wasn't 50ms. To me that's a valid reason to not hard code the blink time and proceed differently. > > I don't think the inversion flag is meant for being configurable. It's > rather used for the default state of the LED. The CAN layer for example > turns the LED on when the interface is up and then blinks (turns off and > on again) on activity. For this it uses the inversion flag. > > > > > > +} > > > + > > > +/** > > > + * uart_add_led_triggers - register LED triggers for a UART > > > + * @drv: pointer to the uart low level driver structure for this port > > > + * @uport: uart port structure to use for this port. > > > + * > > > + * Called by the driver to register LED triggers for a UART. It's the > > > + * drivers responsibility to call uart_led_trigger_rx/tx on received > > > + * and transmitted chars then. > > > + */ > > > +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport) > > > +{ > > > + int ret; > > > + > > > + if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS)) > > > + return 0; > > > > Since this is a public interface, checking for valid led_trigger_tx/rx before > > moving on with the rest of the initialisation is probably a good idea. > > What do you mean? Should we return an error when CONFIG_LEDS_TRIGGERS is > disabled? if (!uport->led_trigger_rx || !uport->led_triggertx) return -EINVAL; > > Sascha > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Nov 24, 2016 at 08:45:43AM -0700, Mathieu Poirier wrote: > On Thu, Nov 24, 2016 at 07:41:37AM +0100, Sascha Hauer wrote: > > On Wed, Nov 23, 2016 at 10:13:02AM -0700, Mathieu Poirier wrote: > > > On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote: > > > > With this patch the serial core provides LED triggers for RX and TX. > > > > > > > > As the serial core layer does not know when the hardware actually sends > > > > or receives characters, this needs help from the UART drivers. The > > > > LED triggers are registered in uart_add_led_triggers() called from > > > > the UART drivers which want to support LED triggers. All the driver > > > > has to do then is to call uart_led_trigger_[tx|rx] to indicate > > > > activite. > > > > > > Hello Sascha, > > > > > > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > --- > > > > drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/serial_core.h | 10 ++++++ > > > > 2 files changed, 83 insertions(+) > > > > > > > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > > > > index f2303f3..3e8afb7 100644 > > > > --- a/drivers/tty/serial/serial_core.c > > > > +++ b/drivers/tty/serial/serial_core.c > > > > @@ -34,6 +34,7 @@ > > > > #include <linux/serial_core.h> > > > > #include <linux/delay.h> > > > > #include <linux/mutex.h> > > > > +#include <linux/leds.h> > > > > > > > > #include <asm/irq.h> > > > > #include <asm/uaccess.h> > > > > @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = { > > > > .attrs = tty_dev_attrs, > > > > }; > > > > > > > > +void uart_led_trigger_tx(struct uart_port *uport) > > > > +{ > > > > + unsigned long delay = 50; > > > > + > > > > + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0); > > > > +} > > > > + > > > > +void uart_led_trigger_rx(struct uart_port *uport) > > > > +{ > > > > + unsigned long delay = 50; > > > > + > > > > + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0); > > > > > > For both rx/tx the core constrains the delay_on/off along with the inversion. > > > Instead of adding the led_trigger_rx/tx and led_trigger_rx/tx_name to the > > > struct uart_port, wouldn't it be better to add a new struct led_trigger that > > > would encapsulate those along wit the on/off delay and the inversion? > > > > > > That way those values could be communicated to the core at registration time > > > instead of hard-coding things. > > > > Not sure this goes into the right direction. Looking at the other > > callers of led_trigger_blink_oneshot() most of them use an arbitrary > > blink time of 30 or 50ms and the users seem to be happy with it. There > > doesn't seem to be the desire to make that value configurable. If we > > want to make it configurable, it's probably better to move the option > > to the led device rather than putting the burden on every user of the > > led triggers. > > So you did find instances where the blink time wasn't 50ms. To me that's > a valid reason to not hard code the blink time and proceed differently. But they are hardcoded to these values. My gut feeling is that the authors had to pick a value and some used 30ms while others used 50ms. Making this configurable to me only means that it will be harder to cleanup later. I'd rather wait until someone comes along who really can present good reasons to make that configurable. > > > > > I don't think the inversion flag is meant for being configurable. It's > > rather used for the default state of the LED. The CAN layer for example > > turns the LED on when the interface is up and then blinks (turns off and > > on again) on activity. For this it uses the inversion flag. > > > > > > > > > +} > > > > + > > > > +/** > > > > + * uart_add_led_triggers - register LED triggers for a UART > > > > + * @drv: pointer to the uart low level driver structure for this port > > > > + * @uport: uart port structure to use for this port. > > > > + * > > > > + * Called by the driver to register LED triggers for a UART. It's the > > > > + * drivers responsibility to call uart_led_trigger_rx/tx on received > > > > + * and transmitted chars then. > > > > + */ > > > > +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport) > > > > +{ > > > > + int ret; > > > > + > > > > + if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS)) > > > > + return 0; > > > > > > Since this is a public interface, checking for valid led_trigger_tx/rx before > > > moving on with the rest of the initialisation is probably a good idea. > > > > What do you mean? Should we return an error when CONFIG_LEDS_TRIGGERS is > > disabled? > > if (!uport->led_trigger_rx || !uport->led_triggertx) > return -EINVAL; uport->led_trigger_rx|tx are structs allocated in this function. It's the normal case that they are NULL. Sascha
On 11/24/2016 12:17 AM, Sascha Hauer wrote: > On Wed, Nov 23, 2016 at 10:57:13AM -0800, Florian Fainelli wrote: >> On 11/23/2016 02:01 AM, Sascha Hauer wrote: >>> With this patch the serial core provides LED triggers for RX and TX. >>> >>> As the serial core layer does not know when the hardware actually sends >>> or receives characters, this needs help from the UART drivers. >> >> Looking at 8250, we call serial8250_tx_chars from __start_tx which is >> called form the uart_ops::start_tx, for RX I sort of agree, since this >> happens in interrupt handler. I suppose some drivers could actually >> queue work but not do it right away though? > > RX is not the problem. When characters are received all drivers call > tty_flip_buffer_push() which could be used for LED triggering (either > directly or we replace the calls to tty_flip_buffer_push() with some > wrapper function). > The problem comes with TX. The serial core has a FIFO which gets > characters from the tty layer. There is no function which the serial > drivers could call to read from this FIFO, instead they have to open code > this. > Continuing with the 8250 driver serial8250_tx_chars() is not only called > from __start_tx(), but also from the interrupt handler. Transmission > works without the serial_core ever noticing. > >> >>> The LED triggers are registered in uart_add_led_triggers() called from >>> the UART drivers which want to support LED triggers. All the driver >>> has to do then is to call uart_led_trigger_[tx|rx] to indicate >>> activity. >> >> Could we somehow remedy the lack of knowledge from the core as whether >> the HW sends/receives characters first before adding support for LED >> triggers? It would be more generic and future proof to require UART >> drivers to report to the core when they actually TX/RX, and then at the >> core level, utilize that knowledge to perform the LED trigger. > > Maybe we could introduce a function to read from the TX FIFO. Having > open coded this in each and every driver is not nice anyway. Something like that could be nice to have yes. > >> >> Side note: are you positive using drv->dev_name is robust enough on >> systems with many different UART drivers, yet all of them being ttyS*? > > If we have different UART drivers, only one of them provides ttyS*, no? > Other drivers will have to use another namespace. I remember this was a problem a couple of years ago last I tried, with the 8250 driver being actually preventing other drivers from using ttyS*, but if you don't have 8250 taking over the low ttyS numbers, you may run into a case where several drivers successfully register their character devices under a ttyS* numbering scheme. Whether this is hypothetical or not nowadays, it may be nicer to provide a more uniquely distinct name like what dev_name() returns, or ttyS*-driver-tx for instance?
> > If we have different UART drivers, only one of them provides ttyS*, no? > > Other drivers will have to use another namespace. > > I remember this was a problem a couple of years ago last I tried, with > the 8250 driver being actually preventing other drivers from using > ttyS*, but if you don't have 8250 taking over the low ttyS numbers, you > may run into a case where several drivers successfully register their > character devices under a ttyS* numbering scheme. > > Whether this is hypothetical or not nowadays, it may be nicer to provide > a more uniquely distinct name like what dev_name() returns, or > ttyS*-driver-tx for instance? It needs to be at the tty layer anyway or you will break low end machines. On a low end system with a 16450A UART you've got something like 160 microseconds at 57600 baud before you must have exited the IRQ handler, re-entered it and be reading the data register again. The current proposed patches touching the 8250 IRQ path are almost certainly going to cause regressions on old machines. So you need to hook receive at a point outside of the IRQ layer (eg when the workqueue starts feeding it into the ldisc), but on the tx side you need to hook the write method of the tty really, because tty_write sends data to the ldisc and what happens then is up to the ldisc. Even there though you have cases that are terribly latency dependant like some of the dumb programming tools that don't window. If you halve someones programming rate for microcontrollers you are going to get hate mail 8) For RX that can mostly be avoided by queuing the data then doing the LEDs, for TX I'm not clear you can easily hide the latency. Alan
Hi! > As the serial core layer does not know when the hardware actually sends > or receives characters, this needs help from the UART drivers. The > LED triggers are registered in uart_add_led_triggers() called from > the UART drivers which want to support LED triggers. All the driver > has to do then is to call uart_led_trigger_[tx|rx] to indicate > activity. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ > include/linux/serial_core.h | 10 ++++++ > 2 files changed, 83 insertions(+) > > + if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS)) > + return 0; > + > + uport->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx", > + drv->dev_name, uport->line); > + uport->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx", > + drv->dev_name, uport->line); Is it neccessary to have separate triggers for rx and tx? Won't most common application be "light a led for rx _or_ tx", which is something this can not do? If I have system with 200 serials, this creates 400 triggers, each trigger name will be about 10 characters AFAICT... and we'll overflow some buffer when doing "cat triggers", no? Would it be enough to have 3 triggers? (Any activity, any rx, any tx)? Thanks, Pavel
Hi! > > Could we somehow remedy the lack of knowledge from the core as whether > > the HW sends/receives characters first before adding support for LED > > triggers? It would be more generic and future proof to require UART > > drivers to report to the core when they actually TX/RX, and then at the > > core level, utilize that knowledge to perform the LED trigger. > > Maybe we could introduce a function to read from the TX FIFO. Having > open coded this in each and every driver is not nice anyway. Yes, please. Pavel
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index f2303f3..3e8afb7 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -34,6 +34,7 @@ #include <linux/serial_core.h> #include <linux/delay.h> #include <linux/mutex.h> +#include <linux/leds.h> #include <asm/irq.h> #include <asm/uaccess.h> @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = { .attrs = tty_dev_attrs, }; +void uart_led_trigger_tx(struct uart_port *uport) +{ + unsigned long delay = 50; + + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0); +} + +void uart_led_trigger_rx(struct uart_port *uport) +{ + unsigned long delay = 50; + + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0); +} + +/** + * uart_add_led_triggers - register LED triggers for a UART + * @drv: pointer to the uart low level driver structure for this port + * @uport: uart port structure to use for this port. + * + * Called by the driver to register LED triggers for a UART. It's the + * drivers responsibility to call uart_led_trigger_rx/tx on received + * and transmitted chars then. + */ +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport) +{ + int ret; + + if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS)) + return 0; + + uport->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx", + drv->dev_name, uport->line); + uport->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx", + drv->dev_name, uport->line); + if (!uport->led_trigger_tx_name || !uport->led_trigger_rx_name) { + ret = -ENOMEM; + goto err_alloc; + } + + led_trigger_register_simple(uport->led_trigger_tx_name, + &uport->led_trigger_tx); + led_trigger_register_simple(uport->led_trigger_rx_name, + &uport->led_trigger_rx); + + return 0; + +err_alloc: + kfree(uport->led_trigger_tx_name); + kfree(uport->led_trigger_rx_name); + + return ret; +} + +/** + * uart_remove_led_triggers - remove LED triggers + * @drv: pointer to the uart low level driver structure for this port + * @uport: uart port structure to use for this port. + * + * Remove LED triggers previously registered with uart_add_led_triggers + */ +void uart_remove_led_triggers(struct uart_port *uport) +{ + if (uport->led_trigger_rx) + led_trigger_unregister_simple(uport->led_trigger_rx); + kfree(uport->led_trigger_rx_name); + + if (uport->led_trigger_tx) + led_trigger_unregister_simple(uport->led_trigger_tx); + kfree(uport->led_trigger_tx_name); +} + /** * uart_add_one_port - attach a driver-defined port structure * @drv: pointer to the uart low level driver structure for this port @@ -2872,6 +2944,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport) WARN_ON(atomic_dec_return(&state->refcount) < 0); wait_event(state->remove_wait, !atomic_read(&state->refcount)); state->uart_port = NULL; + mutex_unlock(&port->mutex); out: mutex_unlock(&port_mutex); diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h index 3442014..1b0a169 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -29,6 +29,7 @@ #include <linux/tty.h> #include <linux/mutex.h> #include <linux/sysrq.h> +#include <linux/leds.h> #include <uapi/linux/serial_core.h> #ifdef CONFIG_SERIAL_CORE_CONSOLE @@ -249,6 +250,10 @@ struct uart_port { const struct attribute_group **tty_groups; /* all attributes (serial core use only) */ struct serial_rs485 rs485; void *private_data; /* generic platform data pointer */ + struct led_trigger *led_trigger_rx; + char *led_trigger_rx_name; + struct led_trigger *led_trigger_tx; + char *led_trigger_tx_name; }; static inline int serial_port_in(struct uart_port *up, int offset) @@ -392,6 +397,11 @@ void uart_console_write(struct uart_port *port, const char *s, unsigned int count, void (*putchar)(struct uart_port *, int)); +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport); +void uart_remove_led_triggers(struct uart_port *uport); +void uart_led_trigger_tx(struct uart_port *port); +void uart_led_trigger_rx(struct uart_port *port); + /* * Port/driver registration/removal */
With this patch the serial core provides LED triggers for RX and TX. As the serial core layer does not know when the hardware actually sends or receives characters, this needs help from the UART drivers. The LED triggers are registered in uart_add_led_triggers() called from the UART drivers which want to support LED triggers. All the driver has to do then is to call uart_led_trigger_[tx|rx] to indicate activity. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++ include/linux/serial_core.h | 10 ++++++ 2 files changed, 83 insertions(+)