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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2aa4aaa4-07b5-2003-eba7-6e1e6fa89611@linux.intel.com>
Date: Tue, 27 Jan 2026 14:48:30 +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>
Subject: Re: [PATCH 3/6] serial: 8250_dw: Rework dw8250_handle_irq() locking
 and IIR handling

On Sat, 24 Jan 2026, Andy Shevchenko wrote:

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

It's not easy to split this DW-side IIR rework and locking changes. What I 
can do is to make 8250_port change separately. I guess I'll do just that 
and only the 8250_dw change in this 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.

Feels to me something that is in the eye of the beholder, but whatever, I 
can move it from one's "correct" place to somebody elses "correct"
place. :-)

> >  #include <linux/sysrq.h>
> 
> (Also perhaps sorting headers in a separate patch helps with finding better
>  places for the future inclusions?)

Yes, later (not in this series).

> ...
> 
> > +EXPORT_SYMBOL_GPL(serial8250_handle_irq_locked);
> 
> Wondering if we can start exporting with a namespace...

I'll do that. I picked "SERIAL_8250", is that fine or should I use e.g. 
"8250" instead?

-- 
 i.