On 11/22/24 11:24, 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.
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.
All four affected platforms use ttyS0, only it is mmio based,
not io port based.
Guenter
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
