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

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

On Fri, Nov 22, 2024, at 16:35, Niklas Schnelle wrote:
> On Fri, 2024-11-22 at 07:18 -0800, Guenter Roeck wrote:
>> On Fri, Apr 05, 2024 at 05:29:24PM +0200, Niklas Schnelle wrote:
>> > In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
>> > compile time. We thus need to add HAS_IOPORT as dependency for those
>> > drivers using them unconditionally. For 8250 based drivers some support
>> > MMIO only use so fence only the parts requiring I/O ports.
>> > 
>> > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
>> > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
>> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>> ...
>> > @@ -422,10 +443,12 @@ static void set_io_from_upio(struct uart_port *p)
>> >  	up->dl_write = default_serial_dl_write;
>> >  
>> > +	default:
>> > +		WARN(1, "Unsupported UART type %x\n", p->iotype);
>> 
>> So, according to this patch, the serial uart on microblaze, nios2,
>> openrisc, xtensa, and possibly others is not or no longer supported.
>> 
>> WARNING: CPU: 0 PID: 0 at drivers/tty/serial/8250/8250_port.c:470 serial8250_set_defaults+0x1a8/0x22c
>> Unsupported UART type 0
>> 
>> Any special reason ?
>
> So according to the warning the p->iotype is 0 which is UPIO_PORT.
> For UPIO_PORT the switch above the WARN would pick io_serial_in() and
> io_serial_out() as handlers. These use inb() respectively outb() to
> access the serial so I don't see how they would work with !HAS_IOPORT
> and it most definitely won't work for s390.
>
> Now for Microblaze Kconfig says to select HAS_IOPORT if PCI so I'd
> assume that it can use inb()/outb() and maybe the PCI requirement is
> not correct if this isn't a PCI device and it used to work with
> inb()/outb()? For nios2, openrisc, and xtensa they don't select
> HAS_IOPORT so either it really won't work anyway or they should select
> it. Can you tell us more about the devices involved where you saw this?

I've tried to have a quick look at the four architectures, here
is what I see in the sources:

- on microblaze, the default uart is xlnx,xps-uartlite-1.00.a,
  and there is no 8250. The PCI code that theoretically supports
  I/O port access through an ISA bridge (copied from powerpc),
  but there is currently no code to set up the I/O space window,
  so in practice the port I/O is just a NULL pointer dereference.

- nios2 has a 16550 compatible UART listed in the devicetree
  file, but this uses normal UPIO_MEM registers, not ISA-style
  I/O ports. There is no support for ISA or PCI.

- openrisc has no support for port I/O, like on nios2 the
  16550 style uart is on UPIO_MEM according to the devicetree

- xtensa manually sets up a UPIO_MEM uart in its board files
  and in its dts files. The PCI support does have code to
  hand port I/O, but it looks incorrect to me and either broke
  at some point or never worked. Most likely this was copied
  incorrectly from old powerpc or sparc code where it worked.

So in all four cases, the normal uart should keep working,
and if something tried to start up an ISA style 8250, I
would expect to see the new version produce the WARN()
in place of what was a NULL pointer dereference (reading
from (u8 *)0x2f8) before.

Since there are so many ways to set up a broken 8250,
it is still possible that something tries to add another
UPIO_PORT uart, and that this causes the WARN() to trigger,
if we find any of those, the fix is to no stop registering
broken ports.

     Arnd