Message-ID: <alpine.DEB.2.21.2410021632150.45128@angie.orcam.me.uk>
Date: Wed, 2 Oct 2024 19:12:41 +0100 (BST)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: 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, Arnd Bergmann <arnd@...nel.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, 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.

 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.

 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.

 A message such as:

serial 0001:01:00.0: cannot handle, no port I/O support in the system

would definitely help.

> As an example for the ugliness, for 8250 one could get something to
> work as it supports both I/O port and MMIO devices. First one would
> need to not #ifdef the setup routines and keep the quirk entry for
> devices that use UPIO_PORT and then in pciserial_init_ports() one could
> check with !IS_ENABLED(CONFIG_HAS_IOPORT) && uart.port.iotype ==
> UPIO_PORT. But then for moxa one would have to keep pci_moxa_setup() to
> set iotype = UPIO_PORT but would have to #ifdef in pci_moxa_init()
> because the initialization already uses I/O ports and init is part of
> the quirk.

 I don't think it has to be as complex as that.  The OxSemi Tornado driver 
which I care about a lot is an example of one that handles hardware that 
can be strapped for either access type (and I have cards with actual pin 
header jumpers and associated documentation for the user to configure 
that), so you only know at run time from the type of BAR 0 whether you 
need port I/O or MMIO.  So it falls into #2 above, and all you need is to 
handle this in `serial8250_pci_setup_port', which I can see your change 
doesn't take into account anyway, whether silently or aloud.

> I think allowing for such custom configs is a possible second step and
> I agree it would be nice and probably becomes more useful as more and
> more platforms lose I/O port support. First we need to be able to build
> without HAS_IOPORT on architectures that just can't do I/O port access
> though, and I'd like not delay this any more.

 I agree it will best be done in steps, no worry.

  Maciej

Powered by blists - more mailing lists