| Message ID | 20230413070342.36155-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: 8250: Clear port->pm on port specific driver unbind | expand |
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
* 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
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>
* 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
* 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
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);
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(+)