Re: [PATCH 3/6] serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2655 bytes --]

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.