On Sat, 2021-07-03 at 14:12 +0200, Arnd Bergmann wrote:
> On Fri, Jul 2, 2021 at 9:42 PM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Fri, Jul 2, 2021 at 6:48 AM Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
> > > A rework for PCI I/O space access from Niklas Schnelle:
> >
> > I pulled this, but then I ended up unpulling.
> >
> > I don't absolutely _hate_ the concept, but I really find this to be
> > very unpalatable:
> >
> > #if !defined(inb) && !defined(_inb)
> > #define _inb _inb
> > static inline u8 _inb(unsigned long addr)
> > {
> > #ifdef PCI_IOBASE
> > u8 val;
> >
> > __io_pbr();
> > val = __raw_readb(PCI_IOBASE + addr);
> > __io_par(val);
> > return val;
> > #else
> > WARN_ONCE(1, "No I/O port support\n");
> > return ~0;
> > #endif
> > }
> > #endif
> >
> > because honestly, the notion of a run-time warning for a compile-time
> > "this cannot work" is just wrong.
>
> Ok, fair enough, back to the drawing board then.
Yes, hard to argue with the reasoning. I'll be here to assist with
testing etc.
>
> > If the platform doesn't have inb/outb, and you compile some driver
> > that uses them, you don't want a run-time warning. Particularly since
> > in many cases nobody will ever run it, and the main use case was to do
> > compile-testing across a wide number of platforms.
> >
> > So if the platform doesn't have inb/outb, they simply should not be
> > declared, and there should be a *compile-time* error. That is
> > literally a lot more useful, and it avoids this extra code.
>
> I tried adding a Kconfig option over a decade ago, but at the time
> gave up when I couldn't still get drivers/ide and the 8250 uart driver
> to build in a sensible way that would still allow the MMIO based
> variants to work, but leave out the PIO accessors. With drivers/ide
> gone, and the drivers/tty/serial/ having gone through many changes,
> it's probably easier now.
>
> I could imagine adding a CONFIG_LEGACY_PCI that controls
> whether we have any pre-PCIe devices or those PCIe drivers
> that need PIO accessors other than ioport_map()/pci_iomap().
>
> This can then select a CONFIG_IOPORT, which controls whether
> inb/outb etc are provided. x86 and anything that uses inb/outb for
> non-PCI devices would select it as well.
I saw your patch in the other mail and will give it a try on our
systems as well.
>
> > Extra code that not only doesn't add value, but that actually
> > *subtracts* value is not code I really want to pull.
>
> What happened here specifically is that the asm-generic version
> is definitely broken and can cause a NULL pointer dereference
> on platforms that used to fall back to NULL PCI_IOBASE.
>
> The latest clang does complain about those drivers with a
> correct warning (not an error) that shows up in s390 allmodconfig
> builds. Niklas' original version of the patch tried to shut up the
> warning but did not address the dangerous behavior, which I
> did not find sufficient either.
>
> The version we got here makes it no longer crash the kernel, but
> I see your point that the runtime warning is still wrong. I'll have
> a look at what it would take to guard all inb/outb callers with a
> Kconfig conditional, and will report back after that.
>
> Arnd
Thanks for your explanation I had already forgotten some of the details
and have nothing to add.
Except, thanks, I guess I can now strike "Got code criticiced by Linus
Torvalds" from my bucket list.
