Hi Lukas,
Am Samstag, 2. Mai 2020, 15:49:27 CEST schrieb Lukas Wunner:
> On Thu, Mar 26, 2020 at 12:14:19AM +0100, Heiko Stuebner wrote:
> > @@ -1529,11 +1535,22 @@ static inline void __stop_tx(struct uart_8250_port *p)
> > /*
> > * To provide required timeing and allow FIFO transfer,
> > * __stop_tx_rs485() must be called only when both FIFO and
> > - * shift register are empty. It is for device driver to enable
> > - * interrupt on TEMT.
> > + * shift register are empty. If 8250 port supports it,
> > + * it is for device driver to enable interrupt on TEMT.
> > + * Otherwise must loop-read until TEMT and THRE flags are set.
> > */
> > - if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> > - return;
> > + if (p->capabilities & UART_CAP_TEMT) {
> > + if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> > + return;
> > + } else {
> > + int lsr;
> > +
> > + if (readx_poll_timeout(__get_lsr, p, lsr,
> > + (lsr & BOTH_EMPTY) == BOTH_EMPTY,
> > + 0, 10000) < 0)
> > + pr_warn("%s: timeout waiting for fifos to empty\n",
> > + p->port.name);
> > + }
>
> Do you actually need to check for the timeout? How could this happen?
> Only if some other part of the driver would disable the transmitter
> I guess, which would be a bug.
Checking for a timeout was strongly suggested in v1 ;-)
> Also, note that __stop_tx() may be called from hardirq context via
> serial8250_tx_chars(). If the baudrate is low, you may spin for a
> fairly long time in IRQ context. E.g. with 9600 8N1, it takes about
> 1 msec for one char to transmit.
I did play around with different baud rates and data amounts today
and even ran into the timeout with the current 10ms when doing a
"dmesg > /dev/ttyS3" ... combined with the hardirq issue you mentioned
I think I found a slightly better variant to do this ... by catching the first
100us in the interrupt handler and otherwise re-using the existing
stop-timer infrastructure to move this out of the actual __stop_tx function.
I've sent a v3 based on your new series just now ... if you find time
please have a look :-)
Thanks
Heiko
