Message-Id: <84bbda13-ded1-4ada-a765-9d012d3f4abd@app.fastmail.com>
Date: Wed, 02 Oct 2024 22:00:08 +0000
From: "Arnd Bergmann" <arnd@...nel.org>
To: "Maciej W. Rozycki" <macro@...am.me.uk>,
 "Niklas Schnelle" <schnelle@...ux.ibm.com>
Cc: "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Jiri Slaby" <jirislaby@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 linux-serial@...r.kernel.org, "Heiko Carstens" <hca@...ux.ibm.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

On Wed, Oct 2, 2024, at 18:12, Maciej W. Rozycki wrote:
> On Wed, 2 Oct 2024, Niklas Schnelle wrote:
>
>> >  Ideally we could come with a slightly user-friendlier change that would 
>> > report the inability to handle port I/O devices as they are discovered 
>> > rather than just silently ignoring them.
>> 
>> I think this would generally get quite ugly as one would have to keep
>> around enough of the drivers which can't possibly work in that
>> !HAS_IOPORT kernel to identify the device and print some error. It's
>> also not what happens when anything else isn't supported by your kernel
>> build. And I don't think we can just look for any I/O ports either
>> because they could be an alternative access method that isn't required.
>
>  There might be corner cases, but offhand I think it's simpler than you 
> outline.  There are two cases to handle here:
>
> 1. Code you've #ifdef'd out that explicitly refers port I/O resources.
>    So rather than having struct entries referring to problematic `*_init' 
>    handlers #ifdef'd out we can keep them and make them call an error 
>    reporting function if (!IS_ENABLED(CONFIG_HAS_IOPORT)).  As a side 
>    effect the structure of code will improve as we don't really like 
>    #ifdefs sprinkled throughout.
>
> 2. Code that infers the access type required from BARs.  It has to handle 
>    the unsupported case anyway, so rather than doing it silently it can 
>    call the same error reporting function.
>
> Yes, there's some work to be done here, but nothing exceedingly tough I 
> believe.

I agree that this shouldn't be hard to finish. The IS_ENABLED()
check is not that easy to do as I think we need to keep calling
inb()/outb() outside of an #ifdef a compile-time error.

However, I think most of the inb/outb usage in 8250_pci.c can
just be converted to either serial_port_in()/serial_port_out(),
using the 8250 specific wrappers, or to ioread8()/iowrite8()
in combination with pci_iomap().

It might help to add a UPIO_IOMAP type to replace UPIO_PORT
for the PCI drivers and just use pci_iomap() exclusively in that
driver.

>  Also I think this case is a bit special, because it's different from a 
> missing driver.  The driver is there and the hardware is there visible in 
> the PCI hierarchy, there's nothing reported and other serial ports work, 
> or a similar serial port works elsewhere, so why doesn't this one?  The 
> user may not necessarily be aware of the peculiarity that the lack of 
> support for port I/O is.

Part of the problem that Niklas is trying to solve with the
CONFIG_HAS_IOPORT annotations is to prevent an invalid inb()/outb()
from turning into a NULL pointer dereference as it currently does
on architectures that have no way to support PIO but get the
default implementation from asm-generic/io.h.

It's not clear if having a silently non-working driver or one
that crashes makes it easier to debug for users. Having a clear
warning message in the PCI probe code is probably the best
we can hope for.

>  I was not and discovered it the hard way in the course of installing my 
> POWER9 system and trying to make the defxx driver work as supplied by the 
> distribution.  It took me a few days to conclude there is no bug anywhere 
> except for the system lacking support for port I/O and the driver having 
> been configured by the packager via a Kconfig option to use that access 
> type.  Also I had PHB4 documentation to hand to refer to and track down 
> the relevant bits.
>
>  I ended up updating the driver to choose the access type automatically 
> (as the board resources are dual-mapped, via both a port I/O and an MMIO 
> BAR), and would have done so long before if I was aware of the existence 
> of such systems.
>
>  Now I consider myself a reasonably seasoned systems software developer, 
> so what can an ordinary user say?  They might be utterly confused and 
> either report it as a system bug (if they were so determined) or just 
> conclude Linux is junk.

I think that anyone using hardware that relies on port I/O on
non-x86 is at this point going to have a reasonable understanding
of the system, so I'm not too worried here. ;-)

>  A message such as:
>
> serial 0001:01:00.0: cannot handle, no port I/O support in the system
>
> would definitely help.

Right.

       Arnd

Powered by blists - more mailing lists