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

[Niklas Schnelle]

On Wed, 2024-10-02 at 23:59 +0100, Maciej W. Rozycki wrote:
> On Wed, 2 Oct 2024, Arnd Bergmann wrote:
> 
> > 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.
> 
>  Can we just provide dummy prototypes with `__attribute__((error("...")))' 
> instead?  This will surely prevent us from having to bend backwards so as 
> to make sure the compiler won't see any spurious references to these 
> inexistent functions or macros.  We already have a `__compiletime_error()' 
> macro for this purpose even.

This is already done in the final patch of my series when disabling
inb()/outb() and friends.


> 
> > 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 can be worse than that.  Part of my confusion with the defxx driver 
> trying to do port I/O with my POWER9 system came from the mapping actually 
> resulting in non-NULL invalid pointers, dereferencing which caused a flood 
> of obscure messages produced to the system console by the system firmware:
> 
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> [...]
> 
> from which it was all but obvious that they were caused by an attempt to 
> use PCI port I/O with a system lacking support for it.
> 
> > 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 agree.  Enthusiastically.

I think there was also a bit of a misunderstanding. My argument that
this would be very ugly in the general case was really meant as general
case outside of drivers like 8250 that deals with both I/O port and
MMIO i.e. we can't warn/error when !HAS_IOPORT deactivates a whole
driver because seeing an I/O port BAR in common PCI code doesn't mean
that it is required for use of the device.

I'm working on a new proposal for 8250 now. Basically I think we can
put the warning/error in serial8250_pci_setup_port(). And then for
those 8250_pci.c "sub drivers" that require I/O ports instead of just
ifdeffing out their setup/init/exit we can define anything but setup to
NULL and setup to pci_default_setup() such that the latter will find
the I/O port BAR via FL_GET_BASE() and subsequently cause the error
print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
also keeps the #ifdefs to just around the code that wouldn't compile.

One thing I'm not happy with yet is the handling around
is_upf_fourport(port) in 8250_pci.c. With !HAS_IOPORT this is defined
as false. With that it makes sure that inb_p()/outb_p() aren't called
but I think this only works because the compiler usually drops the dead
if clause. I think there were previous discussions around this but I
think just like IS_ENABLED() checks this isn't quite correct.

Thanks,
Niklas