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

["Arnd Bergmann" <arnd@kernel.org>]

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);
 	}