Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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.

> 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@arista.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.

> 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?

...

> +	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)?

...

> +	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?

...

> +	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?

> +		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?

>  }
>  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;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko