Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access

[Niklas Schnelle]

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.