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
