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
