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