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

[Niklas Schnelle]

On Tue, 2024-10-01 at 17:41 +0100, Maciej W. Rozycki wrote:
> Hi Niklas,
> 
> > With 2 more HAS_IOPORT patches having gone into v6.12-rc1 I'm looking
> > at what's left and we're down to 4 prerequisite patches[0] before being
> > able to compile-time disable inb()/outb()/…. This one being by far the
> > largest of these. Looking at your suggestion it seems that to compile
> > 8250_pci.c without HAS_IOPORT I'll have to add #ifdef CONFIG_HAS_IOPORT
> > around the MOXI section as that uses I/O ports unconditionally. The
> > rest seems fine and I guess would theoretically work on a system with
> > !HAS_IOPORT. I'll send a v2 with that included. 
> 
>  I've skimmed over and I agree, though I'd place some of the #ifdefs 
> differently, e.g. above #define QPCR_TEST_FOR1.  Overall I think it would 
> make sense to reorder code and group stuff that depends on port I/O such 
> as to minimise the number of #ifdefs.

I just noticed that while not causing compile errors pci_fintek_setup()
also sets iotype = UPIO_PORT so the devices using this won't work
without HAS_IOPORT either. 

> 
>  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.

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.

> 
> > Note however that even though your POWER9 system does not have I/O port
> > support in hardware we still have HAS_IOPORT enabled for arch/powerpc
> > if PCI is enabed so even with this patch as is your POWER9 system
> > should not be affected.
> 
>  I think we need to get this right regardless.  And also while I run a 
> generic distribution kernel on my POWER9 as a production system, I'd love 
> to see an option to build a tailored configuration that would indeed 
> remove support for port I/O from the kernel side for systems like mine 
> that have no provision for port I/O in hardware, knowing that such a 
> kernel will only ever run on such hardware and discarding compiled code 
> that won't ever be used.
> 
>   Maciej

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.

Thanks,
Niklas