Message-ID: <fc09f6fd-013f-25fd-484c-cac59b0a60b6@linux.intel.com>
Date: Tue, 27 Jan 2026 15:35:27 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
    Jiri Slaby <jirislaby@...nel.org>, 
    linux-serial <linux-serial@...r.kernel.org>, 
    qianfan Zhao <qianfanguijin@....com>, Adriana Nicolae <adriana@...sta.com>, 
    Markus Mayer <markus.mayer@...aro.org>, Tim Kryger <tim.kryger@...aro.org>, 
    Matt Porter <matt.porter@...aro.org>, 
    Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
    Jamie Iles <jamie@...ieiles.com>, LKML <linux-kernel@...r.kernel.org>, 
    stable@...r.kernel.org, "Bandal, Shankar" <shankar.bandal@...el.com>, 
    "Murthy, Shanth" <shanth.murthy@...el.com>
Subject: Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

On Sat, 24 Jan 2026, Andy Shevchenko wrote:

> On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:
> > DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
> > Existance of BUSY depends on uart_16550_compatible, if UART HW is
> > configured with 16550 compatible those registers can always be written.
> 
> with 16550 compatible --> with it
> 
> > There currently is dw8250_force_idle() which attempts to archive
> > non-BUSY state by disabling FIFO, however, the solution is unreliable
> > when Rx keeps getting more and more characters.
> > 
> > Create a sequence of operations to enforce that ensures UART cannot
> > keep BUSY asserted indefinitely. The new sequence relies on enabling
> > loopback mode temporarily to prevent incoming Rx characters keeping
> > UART BUSY.
> 
> What if UART was already in a loopback mode? I assume that Tx pause
> described below should not affect the case.
>
> The real case scenario that I am thinking of is a stress test of UART
> using loopback mode.

If you're running a stress test that transfers characters while writing to 
LCR, IMO you get to keep all the pieces yourself.

What will happen though is that LCR write would succeed still because of 
the locking that will prevent Tx'ing more to loopback, but the stress test 
might lose some pieces instead of getting to keep them. :-)

In general, I don't see sane reasons to mess with LCR while a real 
transfer is going on. How is it sane to change line settings such as # of 
bits while xferring something!?!


This is to fix scenarios where what's happening on the serial line is not 
under our control (the other end keeps sending characters). There's 
nothing we can do to stop that unlike running a loopback the stress test 
while writing to LCR which is plain stupidity.

> > Ensure no Tx in ongoing while the UART is switches into the loopback
> > mode (requires exporting serial8250_fifo_wait_for_lsr_thre() and adding
> > DMA Tx pause/resume functions).
> > 
> > According to tests performed by Adriana Nicolae <adriana@...sta.com>,
> > simply disabling FIFO or clearing FIFOs only once does not always
> > ensure BUSY is deasserted but up to two tries may be needed. This could
> > be related to ongoing Rx of a character (a guess, not known for sure).
> 
> Sounds like a plausible theory because UART has shift registers that are
> working independently on the current situation with FIFO. They are actual
> frontends for Tx and Rx data on the wire.

Yes. I just mentioned it's a guess as it's hard to verify, so if somebody 
looks at this commit from the history, they know I've not been able to 
confirm but just made an educated guess. And if they've been able to
acquire better information, they're more likely to rely on that info 
instead of my guesswork.

> > Therefore, retry FIFO clearing a few times (retry limit 4 is arbitrary
> > number but using, e.g., p->fifosize seems overly large). Tests
> > performed by others did not exhibit similar challenge but it does not
> > seem harmful to leave the FIFO clearing loop in place for all DW UARTs
> > with BUSY functionality.
> > 
> > Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
> > writes. In case of plain LCR writes, opportunistically try to update
> > LCR first and only invoke dw8250_idle_enter() if the write did not
> > succeed (it has been observed that in practice most LCR writes do
> > succeed without complications).
> > 
> > This issue was first reported by qianfan Zhao who put lots of debugging
> > effort into understanding the solution space.
> 
> ...
> 
> > +	/* Prevent triggering interrupt from RBR filling */
> > +	p->serial_out(p, UART_IER, 0);
> 
> Do we specifically use callbacks directly and not wrappers all over the change?

I guess it's just a habit, I suppose you meant using serial_port_in/out 
instead. I can try to change those.

> ...
> 
> > +	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
> > +	ndelay(p->frame_time);
> 
> Wouldn't be a problem on lowest baud rates (exempli gratia 110)?

Perhaps, but until somebody comes with an issue report related to 110, I'm 
wondering if this really is worth trying to address. Any suggestion how is 
welcome as well?

> > +	retries = 4;	/* Arbitrary limit, 2 was always enough in tests */
> > +	do {
> > +		serial8250_clear_fifos(up);
> > +		if (!(p->serial_in(p, usr_reg) & DW_UART_USR_BUSY))
> > +			break;
> > +		ndelay(p->frame_time);
> > +	} while (--retries);
> 
> read_poll_timeout_atomic() ? I assume it can't be used due to small frame time?

Frame time is in nanoseconds yes. I did consider 
read_poll_timeout_atomic() but it would have required nsec -> usec 
conversion so I left this as it is.

> > +	if (d->in_idle) {
> 
> > +		/*
> > +		 * FIXME: this deadlocks if port->lock is already held
> > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > +		 */
> 
> Hmm... That FIXME should gone since we have non-blocking consoles, no?

No, lockdep still gets angry if printing is used while holding port's 
lock.

What would be possible though, is to mark the port's lock critical section 
for print deferral (but it's outside the scope of this series). In case of 
serial, it would be justified to use deferred printing (which is only 
meant for special cases) because serial console and printing are related.

> > +		return;
> > +	}
> 
> ...
> 
> > +	ret = dw8250_idle_enter(p);
> > +	if (ret < 0) {
> > +		/*
> > +		 * FIXME: this deadlocks if port->lock is already held
> > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > +		 */
> > +		goto idle_failed;
> >  	}
> > -	/*
> > -	 * FIXME: this deadlocks if port->lock is already held
> > -	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > -	 */
> 
> Ditto.
> 
> >  }
> 
> ...
> 
> >  	p->dev		= dev;
> 
> Maybe put an added line here?
> 
> >  	p->set_ldisc	= dw8250_set_ldisc;
> >  	p->set_termios	= dw8250_set_termios;
> > +	p->set_divisor	= dw8250_set_divisor;
> 
> ...
> 
> > +EXPORT_SYMBOL_GPL(serial8250_clear_fifos);
> 
> Same Q, perhaps start exporting with a namespace?

Yes, I'll put this and the wait func into NS.

> >  }
> >  EXPORT_SYMBOL_GPL(serial8250_set_defaults);
> 
> ...
> 
> > +void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < count; i++) {
> 
> 	while (count--) ?
> 
> Ah, it's existing code... OK then.
>
> > +		if (wait_for_lsr(up, UART_LSR_THRE))
> > +			return;
> > +	}
> > +}
> 
> 

-- 
 i.

Powered by blists - more mailing lists