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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 18 May 2020 00:01:20 +0200
From:   Heiko Stübner <heiko@...ech.de>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     gregkh@...uxfoundation.org, jslaby@...e.com,
        andriy.shevchenko@...ux.intel.com, matwey.kornilov@...il.com,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        christoph.muellner@...obroma-systems.com,
        giulio.benetti@...ronovasrl.com
Subject: Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485

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