On Mon, Nov 25, 2024, at 08:55, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 08:24:37PM +0100, Arnd Bergmann wrote:
>> On Fri, Nov 22, 2024, at 18:22, Guenter Roeck wrote:
>>
>> 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.
Ok.
> 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.
So I see serial8250_setup_port() setting "up->cur_iotype = 0xFF"
by first calling serial8250_init_port(), this is the "not
set" value you mean, right?.
Right after that it calls serial8250_set_defaults(),
which sets "up->cur_iotype = p->iotype;", which may or
may not be initialized here.
The possible calls chains I see leading up to
serial8250_setup_port() are:
serial8250_register_8250_port(): here we first initialize
the iotype incorrectly, then set uart->port.iotype and
call serial8250_set_defaults() again to fix it.
module_init(serial8250_init): relies on the first
serial8250_set_defaults() for the ISA ports since they
are always UPIO_PORT, but sets the other ones (pnp, acpi,
platform_data) correctly.
early_serial_setup(): called only on non-ISA platforms,
shouldn't need to call serial8250_isa_init_ports() at
all.
console_initcall(univ8250_console_init): Not completely
sure here, it seems this now only allows ports that
are registered from one of the methods above
Can you have a look at the patch below? I think this
correctly removes the broken serial8250_set_defaults()
while also still adding it in the one case that relies
on the implied version.
This does however revert f4c23a140d80 ("serial: 8250:
fix null-ptr-deref in serial8250_start_tx()") and
might bring back the bug came from opening an
uninitialized uart. On the other hand, I don't see
why that doesn't also crash from accessing the invalid
I/O port on the same architectures.
Arnd
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5f9f06911795..5b72309611cb 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -358,8 +358,6 @@ struct uart_8250_port *serial8250_setup_port(int index)
up->ops = &univ8250_driver_ops;
- serial8250_set_defaults(up);
-
return up;
}
diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 66fd6d5d4835..d3c1668add58 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -94,6 +94,10 @@ static void __init __serial8250_isa_init_ports(void)
port->regshift = old_serial_port[i].iomem_reg_shift;
port->irqflags |= irqflag;
+
+ serial8250_set_defaults(port);
+
+ /* Allow Intel CE4100 and jailhouse to override defaults */
if (serial8250_isa_config != NULL)
serial8250_isa_config(i, &up->port, &up->capabilities);
}
