Re: [PATCH v2 2/2] serial: 8250_port: properly handle runtime PM in IRQ

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Sep 24, 2026 at 12:40:41PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] serial: 8250: missing port->dev NULL check in serial8250_default_handle_irq()
> --

Isn't this a preexisting issue since serial8250_rpm_get() is implemented this way
https://elixir.bootlin.com/linux/v7.3-rc3/source/drivers/tty/serial/8250/8250_port.c#L517
?

Yes, only for platforms with UART_CAP_RPM, but it doesn't matter, they have no
special handling for the port in this case. (And pm_runtime_get_if_active() actually
works properly on those which do not set that flag as we only check pm_status for 0
and don't care for the other cases.)

> commit 128116d52224b04215dd65b2f4f799e8c14bdec7
> Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> serial: 8250_port: properly handle runtime PM in IRQ
> 
> This commit updates the 8250 driver IRQ handler to explicitly check the
> runtime PM status before processing the interrupt. It returns immediately
> if the device is powered off, assuming it is a shared interrupt.
> 
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index c0bcc0742a60c..1e62460b19ab9 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -1878,13 +1878,22 @@ EXPORT_SYMBOL_GPL(serial8250_handle_irq);
> >  
> >  static int serial8250_default_handle_irq(struct uart_port *port)
> >  {
> [ ... ]
> > +	/* if driver suspended, return, probably shared interrupt */
> > +	pm_status = pm_runtime_get_if_active(port->dev);
> 
> [Severity: High]
> Will this trigger a kernel panic if the port has no associated device? 
> Some ports, such as legacy ISA serial ports, might have a NULL port->dev. 
> Calling pm_runtime_get_if_active() with a NULL pointer would unconditionally 
> dereference it.
> 
> > +	if (!pm_status)
> > +		return 0;
> >  
> >  	iir = serial_port_in(port, UART_IIR);
> > -	return serial8250_handle_irq(port, iir);
> > +	ret = serial8250_handle_irq(port, iir);
> > +
> > +	if (pm_status > 0)
> > +		pm_runtime_put_autosuspend(port->dev);
> 
> [Severity: High]
> Similarly, does this put operation need a check to ensure port->dev is valid
> before passing it to pm_runtime_put_autosuspend()?

Same as above. We check pm_status. It can't be positive if above case failed
already.

> > +	return ret;
> >  }

-- 
With Best Regards,
Andy Shevchenko