On Wed, 2023-03-08 at 13:21 +0100, Arnd Bergmann wrote:
> 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;
Sadly the IS_ENABLED() plus ternary still gives me an undeclared
identifier error for io_serial_in(). So I think we need the more ugly
#ifdef. With that I hope it would then not crash even if one might be
left without any console at all.
>
> 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.
So in the above case .iotype is implicitly set to 0 which is UPIO_PORT
so I think one could argue it is selected, no? Not sure how picking
UPIO_MEM as default would look like then. One thing we could do though
is make the switch/case more regular like so:
...
#ifdef CONFIG_HAS_IOPORT
case UPIO_PORT:
p->serial_in = io_serial_in;
p->serial_out = io_serial_out;
break;
#endif
default:
WARN(1, "Unsupported UART type %x\n", p->iotype);
p->serial_in = no_serial_in;
p->serial_out = no_serial_out;
}
...
That way we would have to always define no_serial_in() /
no_serial_out() but would also gracefully handle when p->iotype is set
to some other value and it looks relatively clean.
