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

[Niklas Schnelle]

On Fri, 2024-10-04 at 12:09 +0200, Niklas Schnelle wrote:
> On Wed, 2024-10-02 at 23:59 +0100, Maciej W. Rozycki wrote:
> > On Wed, 2 Oct 2024, Arnd Bergmann wrote:
> > 
---8<---
> > > 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.

I've now got this to compile on s390x with the !S390 check removed in
Kconfig. Instead of "misusing" pci_default_setup() I instead added a
pci_fail_io_port_setup() helper that prints the error and returns -
EINVAL.

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

It's not the nicest but I added the necessary #ifdefs in 8250_port.c
and at least they are small sections. As Arnd said we will go back to a
single series. So I've also switched to a b4 prep workflow which I
usually use nowadays and so the current code can be found here:

https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=b4/has_ioport

I plan to resend on Monday based on v6.12-rc2 which will also include
the bcachefs fix to fix building on Big Endian which I was previously
carrying for my s390x development convenience.

Thanks,
Niklas