Messages in this thread | | | Date | Wed, 08 Mar 2023 13:21:48 +0100 | From | "Arnd Bergmann" <> | Subject | Re: [niks:has_ioport_v3] [tty] aa0652d7f1: BUG:kernel_NULL_pointer_dereference,address |
| |
On Wed, Mar 8, 2023, at 12:24, Niklas Schnelle wrote: > On Thu, 2023-01-05 at 09:03 +0100, Arnd Bergmann wrote: > > Yes that makes sense, it's clearly not correct to put the default case > inside CONFIG_SERIAL_8250_RT288X. What do you think about going with > something like: > > @@ -519,9 +534,14 @@ static void set_io_from_upio(struct uart_port *p) > #endif > > default: > +#ifdef CONFIG_HAS_IOPORT > p->serial_in = io_serial_in; > p->serial_out = io_serial_out; > break; > +#else > + WARN(1, "Unsupported UART type \"io\"\n"); > + return; > +#endif > }
I think we have to ensure that ->serial_in() always points to some function that doesn't immediately panic, though that could be an empty dummy like
default: p->serial_in = IS_ENABLED(CONFIG_HAS_IOPORT) ? io_serial_in : no_serial_in; p->serial_out = IS_ENABLED(CONFIG_HAS_IOPORT) ? io_serial_out : no_serial_out;
Ideally we'd make mem_serial_in() the default function and only use io_serial_in() when UPIO_PORT is selected, but that still causes a NULL pointer dereference when a platform initializes a 8250 like
static struct plat_serial8250_port serial_platform_data[] = { { .iobase = 0x3f8, /* NULL pointer */ .irq = IRQ_ISA_UART, .uartclk = 1843200, /* default .iotype = UPIO_PORT, */ },
so I think an empty function plus a warning is best here.
> I've pushed a version with the above change rebased on v6.3-rc1 to my > git.kernel.org repository and will do some more testing before I can > hopefully send this out for review and make some progress on this. > Meanwhile the original problem is now the only thing preventing clean > Werror builds on clang for s390 as far as I understand.
Thanks a lot! Let's hope we can manage to get this merged at last.
Arnd
| |