linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arnd Bergmann <arnd@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-arch <linux-arch@vger.kernel.org>,
	linux-pci <linux-pci@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Niklas Schnelle <schnelle@linux.ibm.com>
Subject: Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
Date: Sat, 3 Jul 2021 14:12:26 +0200	[thread overview]
Message-ID: <CAK8P3a1D5DzmNGsEPQomkyMCmMrtD6pQ11JRMh78vbY53edp-Q@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com>

On Fri, Jul 2, 2021 at 9:42 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Jul 2, 2021 at 6:48 AM Arnd Bergmann <arnd@kernel.org> 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.

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

> 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

  reply	other threads:[~2021-07-03 12:12 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-02 13:47 [GIT PULL 1/2] asm-generic: rework PCI I/O space access Arnd Bergmann
2021-07-02 13:49 ` [GIT PULL 2/2] asm-generic: Unify asm/unaligned.h around struct helper Arnd Bergmann
2021-07-02 20:13   ` pr-tracker-bot
2021-07-02 19:42 ` [GIT PULL 1/2] asm-generic: rework PCI I/O space access Linus Torvalds
2021-07-03 12:12   ` Arnd Bergmann [this message]
2021-07-05 10:06     ` Arnd Bergmann
2021-08-03  9:46       ` John Garry
2021-08-03 10:06         ` Arnd Bergmann
2021-08-03 11:23           ` John Garry
2021-08-03 12:15             ` Arnd Bergmann
2021-08-04  7:55               ` John Garry
2021-08-04  8:52                 ` Arnd Bergmann
2021-08-10  9:19                   ` John Garry
2021-08-10 11:33                     ` Arnd Bergmann
2021-09-03  8:31                       ` Niklas Schnelle
2021-12-17 13:19                       ` Niklas Schnelle
2021-12-17 13:32                         ` Arnd Bergmann
2021-12-17 13:52                           ` Niklas Schnelle
2021-12-17 14:05                             ` Arnd Bergmann
2021-12-17 14:27                             ` John Garry
2021-12-17 14:32                               ` Arnd Bergmann
2021-12-17 15:27                                 ` John Garry
2021-12-17 15:55                                   ` Arnd Bergmann
2021-12-17 16:30                                     ` John Garry
2021-12-20  9:27                                       ` Niklas Schnelle
2021-12-21 16:48                                         ` John Garry
2021-12-21 16:57                                           ` Niklas Schnelle
2021-12-19 14:23                               ` David Laight
2021-12-21 16:21                                 ` John Garry
2021-07-05 12:40     ` Niklas Schnelle

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAK8P3a1D5DzmNGsEPQomkyMCmMrtD6pQ11JRMh78vbY53edp-Q@mail.gmail.com \
    --to=arnd@kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=schnelle@linux.ibm.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).