Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

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

On Fri, Nov 22, 2024 at 08:24:37PM +0100, Arnd Bergmann wrote:
> On Fri, Nov 22, 2024, at 18:22, Guenter Roeck wrote:
> > On 11/22/24 08:31, Arnd Bergmann wrote:
> >> On Fri, Nov 22, 2024, at 16:35, Niklas Schnelle wrote:
> >>> On Fri, 2024-11-22 at 07:18 -0800, Guenter Roeck wrote:

...

> >> So in all four cases, the normal uart should keep working,
> >> and if something tried to start up an ISA style 8250, I
> >> would expect to see the new version produce the WARN()
> >> in place of what was a NULL pointer dereference (reading
> >> from (u8 *)0x2f8) before.
> >> 
> >> Since there are so many ways to set up a broken 8250,
> >> it is still possible that something tries to add another
> >> UPIO_PORT uart, and that this causes the WARN() to trigger,
> >> if we find any of those, the fix is to no stop registering
> >> broken ports.
> >
> > The call chain in all cases is
> >
> > [    0.013596] Call Trace:
> > [    0.013796]  [<d06eb249>] dump_stack+0x9/0xc
> > [    0.014115]  [<d000c12c>] __warn+0x94/0xbc
> > [    0.014332]  [<d000c29c>] warn_slowpath_fmt+0x148/0x184
> > [    0.014560]  [<d04f03d8>] set_io_from_upio+0x70/0x98
> > [    0.014781]  [<d04f0459>] serial8250_set_defaults+0x59/0x8c
> > [    0.015013]  [<d04efa6a>] serial8250_setup_port+0x6e/0x90
> > [    0.015240]  [<d0ae2a12>] serial8250_isa_init_ports+0x32/0x5c
> > [    0.015473]  [<d0ae28a1>] univ8250_console_init+0x15/0x24
> > [    0.015698]  [<d0ad0684>] console_init+0x18/0x20
> > [    0.015926]  [<d0acbf43>] start_kernel+0x3db/0x4cc
> > [    0.016145]  [<d06ebc37>] _startup+0x13b/0x13b
> >
> > That seems unconditional. What is the architecture expected to do to
> > prevent this call chain from being executed ?
> 
> This looks like a bug in drivers/tty/serial/8250/8250_platform.c
> to me, not something the architectures did wrong. My impression
> from __serial8250_isa_init_ports() is that this is supposed
> to initialize exactly the ports in the old_serial_port[]
> array, which is filled only on alpha, m68k and x86 but not
> on the other ones.

> Andy moved this code in ffd8e8bd26e9 ("serial: 8250: Extract
> platform driver"), but I don't think this move by itself
> changed anything.

Yep. the idea was to purely split this code out of the core
library parts. I was not intending any functional changes.

I believe it's a preexisted bug, but if you can point out to
the breakage I made, please do it, so I would be able to fix
ASAP.

> serial8250_setup_port() is where it ends up using the
> uninitialized serial8250_ports[index] contents. Since 
> port->iotype is not set to anything here, it defaults to
> '0', which happens to be UPIO_PORT.

Btw, we have a new constant to tell the 8250 core that IO type is not set,
but that one is not used here.

> The reason it doesn't immediately crash and burn is that
> this is still only setting up the structures for later
> use, but I assume that trying to use console=ttyS0, or
> opening /dev/ttyS0 on the uninitialized port would
> then cause an oops.
> 
> The bit I'm less sure about is why the
> serial8250_setup_port() function is called here in
> the first place. I assume it does something for
> /some/ architecture, but it's clearly wrong for
> most of them.

-- 
With Best Regards,
Andy Shevchenko