Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Heiko Stübner]

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