linux-kernel - Re: [PATCH 3/6] 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: <aXPwmfqPlUkI2zuw@smile.fi.intel.com>
Date: Sat, 24 Jan 2026 00:05:13 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, linux-serial@...r.kernel.org,
	qianfan Zhao <qianfanguijin@....com>,
	Adriana Nicolae <adriana@...sta.com>, linux-kernel@...r.kernel.org,
	"Bandal, Shankar" <shankar.bandal@...el.com>,
	"Murthy, Shanth" <shanth.murthy@...el.com>
Subject: Re: [PATCH 3/6] serial: 8250_dw: Rework dw8250_handle_irq() locking
 and IIR handling

On Fri, Jan 23, 2026 at 07:27:36PM +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.
> 
> As serial8250_handle_irq() takes port's lock itself, create
> serial8250_handle_irq_locked() that allows caller to hold port's lock
> across the call. Take port's lock only once in dw8250_handle_irq() and
> call serial8250_handle_irq_locked() directly.

Sounds to me that the latter can be split to a prerequisite patch.

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

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>

+ cleanup.h

>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>

...

> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c

>  #include <linux/ioport.h>
>  #include <linux/init.h>
>  #include <linux/irq.h>

> +#include <linux/lockdep.h>

I would still keep more order.

>  #include <linux/console.h>
>  #include <linux/gpio/consumer.h>

Giving the context we have, the better place for a new inclusion is somewhere
here.

>  #include <linux/sysrq.h>

(Also perhaps sorting headers in a separate patch helps with finding better
 places for the future inclusions?)

...

> +EXPORT_SYMBOL_GPL(serial8250_handle_irq_locked);

Wondering if we can start exporting with a namespace...

-- 
With Best Regards,
Andy Shevchenko