[PATCH] serial: 8250: Clear port->pm on port specific driver unbind

[PATCH] serial: 8250: Clear port->pm on port specific driver unbind

[Tony Lindgren]

When we unbind a serial port hardware specific 8250 driver, the generic
serial8250 driver takes over the port. After that we see an oops about 10
seconds later. This can produce the following at least on some TI SoCs:

Unhandled fault: imprecise external abort (0x1406)
Internal error: : 1406 [#1] SMP ARM

Turns out that we may still have the serial port hardware specific driver
port->pm in use, and serial8250_pm() tries to call it after the port
specific driver is gone:

serial8250_pm [8250_base] from uart_change_pm+0x54/0x8c [serial_base]
uart_change_pm [serial_base] from uart_hangup+0x154/0x198 [serial_base]
uart_hangup [serial_base] from __tty_hangup.part.0+0x328/0x37c
__tty_hangup.part.0 from disassociate_ctty+0x154/0x20c
disassociate_ctty from do_exit+0x744/0xaac
do_exit from do_group_exit+0x40/0x8c
do_group_exit from __wake_up_parent+0x0/0x1c

Let's fix the issue by clearing port->pm in serial8250_unregister_port().

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1157,6 +1157,7 @@ void serial8250_unregister_port(int line)
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
 		uart->port.type = PORT_UNKNOWN;
 		uart->port.dev = &serial8250_isa_devs->dev;
+		uart->port.pm = NULL;
 		uart->capabilities = 0;
 		serial8250_apply_quirks(uart);
 		uart_add_one_port(&serial8250_reg, &uart->port);
-- 
2.40.0

Re: [PATCH] serial: 8250: Clear port->pm on port specific driver unbind

[Andy Shevchenko]

On Thu, Apr 13, 2023 at 10:03:41AM +0300, Tony Lindgren wrote:
> When we unbind a serial port hardware specific 8250 driver, the generic
> serial8250 driver takes over the port. After that we see an oops about 10
> seconds later. This can produce the following at least on some TI SoCs:
> 
> Unhandled fault: imprecise external abort (0x1406)
> Internal error: : 1406 [#1] SMP ARM
> 
> Turns out that we may still have the serial port hardware specific driver
> port->pm in use, and serial8250_pm() tries to call it after the port
> specific driver is gone:
> 
> serial8250_pm [8250_base] from uart_change_pm+0x54/0x8c [serial_base]
> uart_change_pm [serial_base] from uart_hangup+0x154/0x198 [serial_base]
> uart_hangup [serial_base] from __tty_hangup.part.0+0x328/0x37c
> __tty_hangup.part.0 from disassociate_ctty+0x154/0x20c
> disassociate_ctty from do_exit+0x744/0xaac
> do_exit from do_group_exit+0x40/0x8c
> do_group_exit from __wake_up_parent+0x0/0x1c
> 
> Let's fix the issue by clearing port->pm in serial8250_unregister_port().

Sounds to me like a fix that needs a Fixes tag.
Code wise LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/serial/8250/8250_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1157,6 +1157,7 @@ void serial8250_unregister_port(int line)
>  		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
>  		uart->port.type = PORT_UNKNOWN;
>  		uart->port.dev = &serial8250_isa_devs->dev;
> +		uart->port.pm = NULL;
>  		uart->capabilities = 0;
>  		serial8250_apply_quirks(uart);
>  		uart_add_one_port(&serial8250_reg, &uart->port);
> -- 
> 2.40.0

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: Clear port->pm on port specific driver unbind

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230413 16:06]:
> On Thu, Apr 13, 2023 at 10:03:41AM +0300, Tony Lindgren wrote:
> > Let's fix the issue by clearing port->pm in serial8250_unregister_port().
> 
> Sounds to me like a fix that needs a Fixes tag.

Maybe commit c161afe9759d ("8250: allow platforms to override PM hook.").

That's a bit unclear though as the hardware specific functions were
available at that point as they were passed in platform data. This can
be seen with git blame c161afe9759d drivers/serial/8250.c. To me it seems
the port->pm became potentially invalid if a serial port device driver
started implementing PM runtime?

Maybe just tagging it with Cc: stable is better if no obvious Fixes tag
can be figured out.

> Code wise LGTM,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

OK thanks,

Tony

Re: [PATCH] serial: 8250: Clear port->pm on port specific driver unbind

[Ilpo Järvinen]

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

On Fri, 14 Apr 2023, Tony Lindgren wrote:

> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230413 16:06]:
> > On Thu, Apr 13, 2023 at 10:03:41AM +0300, Tony Lindgren wrote:
> > > Let's fix the issue by clearing port->pm in serial8250_unregister_port().
> > 
> > Sounds to me like a fix that needs a Fixes tag.
> 
> Maybe commit c161afe9759d ("8250: allow platforms to override PM hook.").
> 
> That's a bit unclear though as the hardware specific functions were
> available at that point as they were passed in platform data. This can
> be seen with git blame c161afe9759d drivers/serial/8250.c. To me it seems
> the port->pm became potentially invalid if a serial port device driver
> started implementing PM runtime?
> 
> Maybe just tagging it with Cc: stable is better if no obvious Fixes tag
> can be figured out.

I'd just put that c161afe9759d there. It seems quite harmless even if it 
would be unnecessary before some driver commit which is much harder to 
pinpoint (and it would likely turn out old enough to not matter anyway 
for the kernels stable cares about).

I forgot to give this earlier:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH] serial: 8250: Clear port->pm on port specific driver unbind

[Tony Lindgren]

* Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [230414 07:36]:
> On Fri, 14 Apr 2023, Tony Lindgren wrote:
> 
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230413 16:06]:
> > > On Thu, Apr 13, 2023 at 10:03:41AM +0300, Tony Lindgren wrote:
> > > > Let's fix the issue by clearing port->pm in serial8250_unregister_port().
> > > 
> > > Sounds to me like a fix that needs a Fixes tag.
> > 
> > Maybe commit c161afe9759d ("8250: allow platforms to override PM hook.").
> > 
> > That's a bit unclear though as the hardware specific functions were
> > available at that point as they were passed in platform data. This can
> > be seen with git blame c161afe9759d drivers/serial/8250.c. To me it seems
> > the port->pm became potentially invalid if a serial port device driver
> > started implementing PM runtime?
> > 
> > Maybe just tagging it with Cc: stable is better if no obvious Fixes tag
> > can be figured out.
> 
> I'd just put that c161afe9759d there. It seems quite harmless even if it 
> would be unnecessary before some driver commit which is much harder to 
> pinpoint (and it would likely turn out old enough to not matter anyway 
> for the kernels stable cares about).

OK works for me.

I'm now wondering still if we should clear all the conditional hardware
specific functions too in addition to port->pm that get set in
serial8250_register_8250_port(). Maybe best done in a separate patch
as needed.. Any suggestions?

> I forgot to give this earlier:
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks,

Tony

Re: [PATCH] serial: 8250: Clear port->pm on port specific driver unbind

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [230414 09:40]:
> I'm now wondering still if we should clear all the conditional hardware
> specific functions too in addition to port->pm that get set in
> serial8250_register_8250_port(). Maybe best done in a separate patch
> as needed.. Any suggestions?

Well we can't do memset on the port for sure at this point.. But what
we can do is call serial8250_set_defaults() instead of clearing just
port->pm. This will set the port back to serial8250 default functions,
and will set port->pm too.

I'll send v2 patch after some more testing.

Regards,

Tony