On 11/24/2016 12:17 AM, Sascha Hauer wrote:

On Wed, Nov 23, 2016 at 10:57:13AM -0800, Florian Fainelli wrote:

quoted

On 11/23/2016 02:01 AM, Sascha Hauer wrote:

quoted

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.

quoted

quoted

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.

quoted

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