linux-kernel - Re: [PATCH v2 4/7] serial: 8250_dw: Rework dw8250_handle

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f050a630-8732-346e-0f5f-0186e5640c30@linux.intel.com>
Date: Wed, 28 Jan 2026 16:00:14 +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>, 
    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 4/7] serial: 8250_dw: Rework dw8250_handle_irq()
 locking and IIR handling

On Wed, 28 Jan 2026, Andy Shevchenko wrote:

> On Wed, Jan 28, 2026 at 12:52:58PM +0200, Ilpo Järvinen wrote:
> > dw8250_handle_irq() takes port's lock multiple times with no good
> > reason to release it in between and calls serial8250_handle_irq()
> > that also takes port's lock.
> > 
> > Take port's lock only once in dw8250_handle_irq() and use
> > serial8250_handle_irq_locked() to avoid releasing port's lock in
> > between.
> > 
> > As IIR_NO_INT check in serial8250_handle_irq() was outside of port's
> > lock, it has to be done already in dw8250_handle_irq().
> > 
> > DW UART can, in addition to IIR_NO_INT, report BUSY_DETECT (0x7) which
> > collided with the IIR_NO_INT (0x1) check in serial8250_handle_irq()
> > (because & is used instead of ==) meaning that no other work is done by
> > serial8250_handle_irq() during an BUSY_DETECT interrupt.
> > 
> > This allows reorganizing code in dw8250_handle_irq() to do both
> > IIR_NO_INT and BUSY_DETECT handling right at the start simplifying
> > the logic.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> 
> ...
> 
> >  	if (!up->dma && rx_timeout) {
> >  		status = serial_lsr_in(up);
> >  
> >  		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> >  			serial_port_in(p, UART_RX);
> >  	}
> >  
> >  	/* Manually stop the Rx DMA transfer when acting as flow controller */
> >  	if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
> >  		status = serial_lsr_in(up);
> >  
> >  		if (status & (UART_LSR_DR | UART_LSR_BI)) {
> >  			dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
> 			...
> >  		}
> >  	}
> 
> Looks like now (perhaps in a separate change) this may be refactored even more.

Definitely, as was noted in the coverletter. ;-)

--
 i.

> 
> 	if (rx_timeout) {
> 		status = serial_lsr_in(up);
> 
> // Although not sure about moving the above read out from the specific conditions.
> 
> 		if (up->dma && (status & (UART_LSR_DR | UART_LSR_BI))) {
> 			/* Manually stop the Rx DMA transfer when acting as flow controller */
> 			if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma->rx_running) {
> 				dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
> 				dw8250_writel_ext(p, DW_UART_DMASA, 1);
> 		} else if (!up->dma && !(status & (UART_LSR_DR | UART_LSR_BI)))
> 			/* ... */
> 			serial_port_in(p, UART_RX);
> 		}
> 	}
> 
>