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
