linux-kernel - Re: [PATCH v2 7/7] serial: 8250

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]
Message-ID: <407db45d-c4d9-e6b1-8e35-e398da89d40e@linux.intel.com>
Date: Fri, 30 Jan 2026 14:44:38 +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>, 
    Tim Kryger <tim.kryger@...aro.org>, Matt Porter <matt.porter@...aro.org>, 
    Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
    Markus Mayer <markus.mayer@...aro.org>, Jamie Iles <jamie@...ieiles.com>, 
    LKML <linux-kernel@...r.kernel.org>, 
    "Bandal, Shankar" <shankar.bandal@...el.com>, 
    "Murthy, Shanth" <shanth.murthy@...el.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2 7/7] serial: 8250_dw: Ensure BUSY is deasserted

On Wed, 28 Jan 2026, Andy Shevchenko wrote:

> On Wed, Jan 28, 2026 at 12:53:01PM +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 it those registers can always be written.
> > 
> > There currently is dw8250_force_idle() which attempts to achieve
> > non-BUSY state by disabling FIFO, however, the solution is unreliable
> > when Rx keeps getting more and more characters.
> > 
> > Create a sequence of operations 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.
> > 
> > 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).
> > 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.
> 
> Some nit-picks below, otherwise seems good to go
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> 
> ...
> 
> > Reported-by: qianfan Zhao <qianfanguijin@....com>
> > Link: https://lore.kernel.org/linux-serial/289bb78a-7509-1c5c-2923-a04ed3b6487d@163.com/
> > Reported-by: Adriana Nicolae <adriana@...sta.com>
> > Link: https://lore.kernel.org/linux-serial/20250819182322.3451959-1-adriana@arista.com/
> 
> Shouldn't these Link:s be Closes: tags?

To not possibly give wrong signals, until they confirm their cases are 
indeed solved by this patch, I'd like to keep these as Link tag only.

> > +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> >  	struct uart_8250_port *up = up_to_u8250p(p);
> > +	unsigned int usr_reg = DW_UART_USR;
> > +	int retries;
> > +	u32 lsr;
> 
> 
> > +	if (d->pdata)
> > +		usr_reg = d->pdata->usr_reg;
> 
> I would unite this with definition above:
> 
> 	unsigned int usr_reg = d->pdata ? d->pdata->usr_reg : DW_UART_USR;
>
> > +	lsr = serial_lsr_in(up);
> 
> > +	if (lsr & UART_LSR_DR) {
> > +		serial_port_in(p, UART_RX);
> > +		up->lsr_saved_flags = 0;
> >  	}
> 
> This seems repeating a top of serial8250_read_char(). Perhaps we can do it
> in a helper at some point?

I don't see enough similarity as I'd need to deal with lsr_saved_flags 
somehow still here.

> > +	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);
> > +		 */
> 
> Does it make sense to print an error here (assuming it will work with nbcon)?
> If so, maybe leave it at the end of the function, after dw8250_idle_exit()
> and goto there?

I think the print would be useful. I'll leave the FIXME+commented out 
print into the end of the function in v3.

I also realized that on error, dw8250_idle_enter() should undo what it 
changed, that is, call dw8250_idle_exit() within which will simplify 
caller-side error handling slightly.

-- 
 i.