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.
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.
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.
Arnd
