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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Nov 25, 2024 at 10:53:46AM +0100, Arnd Bergmann wrote:
> 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?.

Yes, and we have a constant for that, I'll send a patch to make it clear.

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

AFAICS it does only partial revert, so I don't see how your patch
may break that.

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

-- 
With Best Regards,
Andy Shevchenko