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? -- Florian -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
