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

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

[Arnd Bergmann]

The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:

  Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git
tags/asm-generic-pci-ioport-5.14

for you to fetch changes up to 5ae6eadfdaf431f47adbdf1754f3b5a5fd638de2:

  asm-generic/io.h: warn in inb() and friends with undefined
PCI_IOBASE (2021-05-10 17:37:55 +0200)

----------------------------------------------------------------
asm-generic: rework PCI I/O space access

A rework for PCI I/O space access from Niklas Schnelle:

  "This is version 5 of my attempt to get rid of a clang
  -Wnull-pointer-arithmetic warning for the use of PCI_IOBASE in
  asm-generic/io.h. This was originally found on s390 but should apply to
  all platforms leaving PCI_IOBASE undefined while making use of the inb()
  and friends helpers from asm-generic/io.h.

  This applies cleanly and was compile tested on top of v5.12 for the
  previously broken ARC, nds32, h8300 and risc-v architecture. It also
  applies cleanly on v5.13-rc1 for which I boot tested it on s390.

  I did boot test this only on x86_64 and s390x the former implements
  inb() itself while the latter would emit a WARN_ONCE() but no drivers
  use inb().

Link: https://lore.kernel.org/lkml/20210510145234.594814-1-schnelle@linux.ibm.com/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

----------------------------------------------------------------
Niklas Schnelle (3):
      sparc: explicitly set PCI_IOBASE to 0
      risc-v: Use generic io.h helpers for nommu
      asm-generic/io.h: warn in inb() and friends with undefined PCI_IOBASE

 arch/riscv/include/asm/io.h |  5 +++--
 arch/sparc/include/asm/io.h |  8 ++++++++
 include/asm-generic/io.h    | 68
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 75 insertions(+), 6 deletions(-)

[GIT PULL 2/2] asm-generic: Unify asm/unaligned.h around struct helper

[Arnd Bergmann]

The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:

  Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git
tags/asm-generic-unaligned-5.14

for you to fetch changes up to 803f4e1eab7a8938ba3a3c30dd4eb5e9eeef5e63:

  asm-generic: simplify asm/unaligned.h (2021-05-17 13:30:29 +0200)

----------------------------------------------------------------
asm-generic/unaligned: Unify asm/unaligned.h around struct helper

The get_unaligned()/put_unaligned() helpers are traditionally architecture
specific, with the two main variants being the "access-ok.h" version
that assumes unaligned pointer accesses always work on a particular
architecture, and the "le-struct.h" version that casts the data to a
byte aligned type before dereferencing, for architectures that cannot
always do unaligned accesses in hardware.

Based on the discussion linked below, it appears that the access-ok
version is not realiable on any architecture, but the struct version
probably has no downsides. This series changes the code to use the
same implementation on all architectures, addressing the few exceptions
separately.

Link: https://lore.kernel.org/lkml/75d07691-1e4f-741f-9852-38c0b4f520bc@synopsys.com/
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100363
Link: https://lore.kernel.org/lkml/20210507220813.365382-14-arnd@kernel.org/
Link: git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git
unaligned-rework-v2
Link: https://lore.kernel.org/lkml/CAHk-=whGObOKruA_bU3aPGZfoDqZM1_9wBkwREp0H0FgR-90uQ@mail.gmail.com/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

----------------------------------------------------------------
Arnd Bergmann (13):
      asm-generic: use asm-generic/unaligned.h for most architectures
      openrisc: always use unaligned-struct header
      sh: remove unaligned access for sh4a
      m68k: select CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
      powerpc: use linux/unaligned/le_struct.h on LE power7
      asm-generic: unaligned: remove byteshift helpers
      asm-generic: unaligned always use struct helpers
      partitions: msdos: fix one-byte get_unaligned()
      apparmor: use get_unaligned() only for multi-byte words
      mwifiex: re-fix for unaligned accesses
      netpoll: avoid put_unaligned() on single character
      asm-generic: uaccess: 1-byte access is always aligned
      asm-generic: simplify asm/unaligned.h

 arch/alpha/include/asm/unaligned.h          |  12 --
 arch/arm/include/asm/unaligned.h            |  27 ----
 arch/ia64/include/asm/unaligned.h           |  12 --
 arch/m68k/Kconfig                           |   1 +
 arch/m68k/include/asm/unaligned.h           |  26 ----
 arch/microblaze/include/asm/unaligned.h     |  27 ----
 arch/mips/crypto/crc32-mips.c               |   2 +-
 arch/openrisc/include/asm/unaligned.h       |  47 -------
 arch/parisc/include/asm/unaligned.h         |   6 +-
 arch/powerpc/include/asm/unaligned.h        |  22 ---
 arch/sh/include/asm/unaligned-sh4a.h        | 199 ----------------------------
 arch/sh/include/asm/unaligned.h             |  13 --
 arch/sparc/include/asm/unaligned.h          |  11 --
 arch/x86/include/asm/unaligned.h            |  15 ---
 arch/xtensa/include/asm/unaligned.h         |  29 ----
 block/partitions/ldm.c                      |   2 +-
 block/partitions/ldm.h                      |   3 -
 block/partitions/msdos.c                    |  24 ++--
 drivers/net/wireless/marvell/mwifiex/pcie.c |  10 +-
 include/asm-generic/uaccess.h               |   4 +-
 include/asm-generic/unaligned.h             | 141 ++++++++++++++++----
 include/linux/unaligned/access_ok.h         |  68 ----------
 include/linux/unaligned/be_byteshift.h      |  71 ----------
 include/linux/unaligned/be_memmove.h        |  37 ------
 include/linux/unaligned/be_struct.h         |  37 ------
 include/linux/unaligned/generic.h           | 115 ----------------
 include/linux/unaligned/le_byteshift.h      |  71 ----------
 include/linux/unaligned/le_memmove.h        |  37 ------
 include/linux/unaligned/le_struct.h         |  37 ------
 include/linux/unaligned/memmove.h           |  46 -------
 net/core/netpoll.c                          |   4 +-
 security/apparmor/policy_unpack.c           |   2 +-
 32 files changed, 141 insertions(+), 1017 deletions(-)
 delete mode 100644 arch/alpha/include/asm/unaligned.h
 delete mode 100644 arch/arm/include/asm/unaligned.h
 delete mode 100644 arch/ia64/include/asm/unaligned.h
 delete mode 100644 arch/m68k/include/asm/unaligned.h
 delete mode 100644 arch/microblaze/include/asm/unaligned.h
 delete mode 100644 arch/openrisc/include/asm/unaligned.h
 delete mode 100644 arch/powerpc/include/asm/unaligned.h
 delete mode 100644 arch/sh/include/asm/unaligned-sh4a.h
 delete mode 100644 arch/sh/include/asm/unaligned.h
 delete mode 100644 arch/sparc/include/asm/unaligned.h
 delete mode 100644 arch/x86/include/asm/unaligned.h
 delete mode 100644 arch/xtensa/include/asm/unaligned.h
 delete mode 100644 include/linux/unaligned/access_ok.h
 delete mode 100644 include/linux/unaligned/be_byteshift.h
 delete mode 100644 include/linux/unaligned/be_memmove.h
 delete mode 100644 include/linux/unaligned/be_struct.h
 delete mode 100644 include/linux/unaligned/generic.h
 delete mode 100644 include/linux/unaligned/le_byteshift.h
 delete mode 100644 include/linux/unaligned/le_memmove.h
 delete mode 100644 include/linux/unaligned/le_struct.h
 delete mode 100644 include/linux/unaligned/memmove.h

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

[Linus Torvalds]

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.

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.

Extra code that not only doesn't add value, but that actually
*subtracts* value is not code I really want to pull.

                     Linus

Re: [GIT PULL 2/2] asm-generic: Unify asm/unaligned.h around struct helper

[pr-tracker-bot]

The pull request you sent on Fri, 2 Jul 2021 15:49:30 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git tags/asm-generic-unaligned-5.14

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/4cad67197989c81417810b89f09a3549b75a2441

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[Arnd Bergmann]

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

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

[Arnd Bergmann]

On Sat, Jul 3, 2021 at 2:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
> 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.

I created a preliminary patch and got it to cleanly build on my randconfig box,
here is what that involved:

- added 89 Kconfig dependencies on HAS_IOPORT for PC-style devices
  that are not on a PCI card.
- added 188 Kconfig dependencies on LEGACY_PCI for PCI drivers that
  require port I/O. The idea is to have that control drivers for both pre-PCIe
  devices and and PCIe devices that require long-deprecated features like
  I/O resources, but possibly other features as well.
- The ACPI subsystem needs access to I/O ports, so that also gets a
  dependency.
- CONFIG_INDIRECT_PIO requires HAS_IOPORT
-  /dev/ioport needs an #ifdef around it
- several graphics drivers need workarounds instead of a 'depends on'
  because they are used in virtual machines: vgaconsole, bochs, qxl,
  cirrus. They work with or without port I/O
- A usb-uhci rework to split pci from non-pci support
- Minor workarounds for optional I/O port usage in libata, ipmi, tpm,
  dmi-firmware, altera-stapl, parport, vga
- lots of #ifdefs in 8250
- some drivers/pci/ quirks are #ifdef'd
- drivers using ioport_map()/pci_iomap() to access ports could be
  kept working when I/O ports are memory mapped

I tested the patch on a 5.13-rc4 snapshot that already has other
patches applied as a baseline for randconfig testing, so it doesn't
apply as-is.

Linus, if you like this approach, then I can work on splitting it up into
meaningful patches and submit it for a future release. I think the
CONFIG_LEGACY_PCI option has value on its own, but the others
do introduce some churn.

Full patch (120KB): https://pastebin.com/yaFSmAuY

diffstat:
 drivers/accessibility/speakup/Kconfig        |   1 +
 drivers/acpi/Kconfig                         |   1 +
 drivers/ata/Kconfig                          |  34 ++++++++++-----------
 drivers/ata/ata_generic.c                    |   3 +-
 drivers/ata/libata-sff.c                     |   2 ++
 drivers/bus/Kconfig                          |   2 +-
 drivers/char/Kconfig                         |   3 +-
 drivers/char/ipmi/Makefile                   |  11 +++----
 drivers/char/ipmi/ipmi_si_intf.c             |   3 +-
 drivers/char/ipmi/ipmi_si_pci.c              |   3 ++
 drivers/char/mem.c                           |   6 +++-
 drivers/char/tpm/Kconfig                     |   1 +
 drivers/char/tpm/tpm_infineon.c              |  14 ++++++---
 drivers/char/tpm/tpm_tis_core.c              |  19 +++++-------
 drivers/comedi/Kconfig                       |  53
+++++++++++++++++++++++++++++++++
 drivers/firmware/dmi-sysfs.c                 |   4 +++
 drivers/gpio/Kconfig                         |   2 +-
 drivers/gpu/drm/bochs/Kconfig                |   1 +
 drivers/gpu/drm/bochs/bochs_hw.c             |  24 ++++++++-------
 drivers/gpu/drm/qxl/Kconfig                  |   1 +
 drivers/gpu/drm/tiny/cirrus.c                |   2 ++
 drivers/hwmon/Kconfig                        |  23 +++++++++++++--
 drivers/i2c/busses/Kconfig                   |  31 ++++++++++---------
 drivers/ide/Kconfig                          |   1 +
 drivers/iio/adc/Kconfig                      |   2 +-
 drivers/input/gameport/Kconfig               |   6 ++--
 drivers/input/serio/Kconfig                  |   2 ++
 drivers/input/touchscreen/Kconfig            |   1 +
 drivers/isdn/hardware/mISDN/Kconfig          |  14 ++++-----
 drivers/leds/Kconfig                         |   2 +-
 drivers/media/cec/platform/Kconfig           |   2 +-
 drivers/media/pci/dm1105/Kconfig             |   2 +-
 drivers/media/radio/Kconfig                  |  15 +++++++++-
 drivers/media/rc/Kconfig                     |   9 +++++-
 drivers/message/fusion/Kconfig               |   8 ++---
 drivers/misc/altera-stapl/Makefile           |   3 +-
 drivers/misc/altera-stapl/altera.c           |   6 +++-
 drivers/net/Kconfig                          |   2 +-
 drivers/net/arcnet/Kconfig                   |   2 +-
 drivers/net/can/cc770/Kconfig                |   1 +
 drivers/net/can/sja1000/Kconfig              |   1 +
 drivers/net/ethernet/8390/Kconfig            |   2 +-
 drivers/net/ethernet/amd/Kconfig             |   2 +-
 drivers/net/ethernet/intel/Kconfig           |   4 +--
 drivers/net/ethernet/sis/Kconfig             |   6 ++--
 drivers/net/ethernet/ti/Kconfig              |   4 +--
 drivers/net/ethernet/via/Kconfig             |   5 ++--
 drivers/net/fddi/Kconfig                     |   4 +--
 drivers/net/hamradio/Kconfig                 |   6 ++--
 drivers/net/wan/Kconfig                      |   2 +-
 drivers/net/wireless/atmel/Kconfig           |   4 +--
 drivers/net/wireless/intersil/hostap/Kconfig |   4 +--
 drivers/parport/Kconfig                      |   2 +-
 drivers/pci/pci-sysfs.c                      |  16 ++++++++++
 drivers/pci/quirks.c                         |   2 ++
 drivers/pcmcia/Kconfig                       |   2 +-
 drivers/platform/chrome/Kconfig              |   1 +
 drivers/platform/chrome/wilco_ec/Kconfig     |   1 +
 drivers/pnp/isapnp/Kconfig                   |   2 +-
 drivers/power/reset/Kconfig                  |   1 +
 drivers/rtc/Kconfig                          |   4 ++-
 drivers/scsi/Kconfig                         |  21 ++++++-------
 drivers/scsi/aic7xxx/Kconfig.aic79xx         |   2 +-
 drivers/scsi/aic7xxx/Kconfig.aic7xxx         |   2 +-
 drivers/scsi/aic94xx/Kconfig                 |   2 +-
 drivers/scsi/megaraid/Kconfig.megaraid       |   2 +-
 drivers/scsi/mvsas/Kconfig                   |   2 +-
 drivers/scsi/qla2xxx/Kconfig                 |   2 +-
 drivers/spi/Kconfig                          |   1 +
 drivers/staging/kpc2000/Kconfig              |   2 +-
 drivers/staging/sm750fb/Kconfig              |   2 +-
 drivers/staging/vt6655/Kconfig               |   2 +-
 drivers/tty/Kconfig                          |   2 +-
 drivers/tty/serial/8250/8250_early.c         |   4 +++
 drivers/tty/serial/8250/8250_pci.c           |  19 ++++++++++--
 drivers/tty/serial/8250/8250_port.c          |  22 ++++++++++++--
 drivers/tty/serial/8250/Kconfig              |   1 +
 drivers/tty/serial/Kconfig                   |   2 +-
 drivers/usb/core/hcd-pci.c                   |   4 +--
 drivers/usb/host/Kconfig                     |   4 +--
 drivers/usb/host/pci-quirks.c                | 128
+++++++++++++++++++++++++++++++++++++++++--------------------------------------
 drivers/usb/host/pci-quirks.h                |  33 ++++++++++++++++-----
 drivers/usb/host/uhci-hcd.c                  |   2 +-
 drivers/usb/host/uhci-hcd.h                  |  77
+++++++++++++++++++++++++++++++----------------
 drivers/video/console/Kconfig                |   4 ++-
 drivers/video/fbdev/Kconfig                  |  24 +++++++--------
 drivers/watchdog/Kconfig                     |   6 ++--
 include/asm-generic/io.h                     |   6 ++++
 include/linux/gameport.h                     |   9 ++++--
 include/linux/parport.h                      |   2 +-
 include/video/vga.h                          |   8 +++++
 lib/Kconfig                                  |   4 +++
 lib/Kconfig.kgdb                             |   1 +
 sound/drivers/Kconfig                        |   3 ++
 sound/isa/Kconfig                            |   1 +
 sound/pci/Kconfig                            |  44 ++++++++++++++++++++++-----
 96 files changed, 575 insertions(+), 272 deletions(-)

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

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.

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

[John Garry]

On 05/07/2021 11:06, Arnd Bergmann wrote:
> On Sat, Jul 3, 2021 at 2:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
>> 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.
> 
> I created a preliminary patch and got it to cleanly build on my randconfig box,
> here is what that involved:
> 
> - added 89 Kconfig dependencies on HAS_IOPORT for PC-style devices
>    that are not on a PCI card.
> - added 188 Kconfig dependencies on LEGACY_PCI for PCI drivers that
>    require port I/O. The idea is to have that control drivers for both pre-PCIe
>    devices and and PCIe devices that require long-deprecated features like
>    I/O resources, but possibly other features as well.
> - The ACPI subsystem needs access to I/O ports, so that also gets a
>    dependency.
> - CONFIG_INDIRECT_PIO requires HAS_IOPORT
> -  /dev/ioport needs an #ifdef around it
> - several graphics drivers need workarounds instead of a 'depends on'
>    because they are used in virtual machines: vgaconsole, bochs, qxl,
>    cirrus. They work with or without port I/O
> - A usb-uhci rework to split pci from non-pci support
> - Minor workarounds for optional I/O port usage in libata, ipmi, tpm,
>    dmi-firmware, altera-stapl, parport, vga
> - lots of #ifdefs in 8250
> - some drivers/pci/ quirks are #ifdef'd
> - drivers using ioport_map()/pci_iomap() to access ports could be
>    kept working when I/O ports are memory mapped
> 
> I tested the patch on a 5.13-rc4 snapshot that already has other
> patches applied as a baseline for randconfig testing, so it doesn't
> apply as-is.
> 
> Linus, if you like this approach, then I can work on splitting it up into
> meaningful patches and submit it for a future release. I think the
> CONFIG_LEGACY_PCI option has value on its own, but the others
> do introduce some churn.
> 
> Full patch (120KB): https://pastebin.com/yaFSmAuY
> 

Hi Arnd,

I am not sure if anything is happening here.

Anyway, one thing I mentioned earlier was that we could solve the 
problem of drivers accessing unmapped IO ports and crashing systems on 
archs which define PCI_IOBASE by building them under some "native port 
IO support" flag.

One example of such a driver was F71805F sensor. You put that under 
HAS_IOPORT, which would be available for all archs, I think. But I could 
not see where config LEGACY_PCI is introduced. Could we further refine 
that config to not build for such archs as arm64?

BTW, I think that the PPC dependency was added there to stop building 
for power for that same reason, so hopefully we get rid of that.

Thanks,
John

> diffstat:
>   drivers/accessibility/speakup/Kconfig        |   1 +
>   drivers/acpi/Kconfig                         |   1 +
>   drivers/ata/Kconfig                          |  34 ++++++++++-----------
>   drivers/ata/ata_generic.c                    |   3 +-
>   drivers/ata/libata-sff.c                     |   2 ++
>   drivers/bus/Kconfig                          |   2 +-
>   drivers/char/Kconfig                         |   3 +-
>   drivers/char/ipmi/Makefile                   |  11 +++----
>   drivers/char/ipmi/ipmi_si_intf.c             |   3 +-
>   drivers/char/ipmi/ipmi_si_pci.c              |   3 ++
>   drivers/char/mem.c                           |   6 +++-
>   drivers/char/tpm/Kconfig                     |   1 +
>   drivers/char/tpm/tpm_infineon.c              |  14 ++++++---
>   drivers/char/tpm/tpm_tis_core.c              |  19 +++++-------
>   drivers/comedi/Kconfig                       |  53
> +++++++++++++++++++++++++++++++++
>   drivers/firmware/dmi-sysfs.c                 |   4 +++
>   drivers/gpio/Kconfig                         |   2 +-
>   drivers/gpu/drm/bochs/Kconfig                |   1 +
>   drivers/gpu/drm/bochs/bochs_hw.c             |  24 ++++++++-------
>   drivers/gpu/drm/qxl/Kconfig                  |   1 +
>   drivers/gpu/drm/tiny/cirrus.c                |   2 ++
>   drivers/hwmon/Kconfig                        |  23 +++++++++++++--
>   drivers/i2c/busses/Kconfig                   |  31 ++++++++++---------
>   drivers/ide/Kconfig                          |   1 +
>   drivers/iio/adc/Kconfig                      |   2 +-
>   drivers/input/gameport/Kconfig               |   6 ++--
>   drivers/input/serio/Kconfig                  |   2 ++
>   drivers/input/touchscreen/Kconfig            |   1 +
>   drivers/isdn/hardware/mISDN/Kconfig          |  14 ++++-----
>   drivers/leds/Kconfig                         |   2 +-
>   drivers/media/cec/platform/Kconfig           |   2 +-
>   drivers/media/pci/dm1105/Kconfig             |   2 +-
>   drivers/media/radio/Kconfig                  |  15 +++++++++-
>   drivers/media/rc/Kconfig                     |   9 +++++-
>   drivers/message/fusion/Kconfig               |   8 ++---
>   drivers/misc/altera-stapl/Makefile           |   3 +-
>   drivers/misc/altera-stapl/altera.c           |   6 +++-
>   drivers/net/Kconfig                          |   2 +-
>   drivers/net/arcnet/Kconfig                   |   2 +-
>   drivers/net/can/cc770/Kconfig                |   1 +
>   drivers/net/can/sja1000/Kconfig              |   1 +
>   drivers/net/ethernet/8390/Kconfig            |   2 +-
>   drivers/net/ethernet/amd/Kconfig             |   2 +-
>   drivers/net/ethernet/intel/Kconfig           |   4 +--
>   drivers/net/ethernet/sis/Kconfig             |   6 ++--
>   driv

ers/net/ethernet/ti/Kconfig              |   4 +--
>   drivers/net/ethernet/via/Kconfig             |   5 ++--
>   drivers/net/fddi/Kconfig                     |   4 +--
>   drivers/net/hamradio/Kconfig                 |   6 ++--
>   drivers/net/wan/Kconfig                      |   2 +-
>   drivers/net/wireless/atmel/Kconfig           |   4 +--
>   drivers/net/wireless/intersil/hostap/Kconfig |   4 +--
>   drivers/parport/Kconfig                      |   2 +-
>   drivers/pci/pci-sysfs.c                      |  16 ++++++++++
>   drivers/pci/quirks.c                         |   2 ++
>   drivers/pcmcia/Kconfig                       |   2 +-
>   drivers/platform/chrome/Kconfig              |   1 +
>   drivers/platform/chrome/wilco_ec/Kconfig     |   1 +
>   drivers/pnp/isapnp/Kconfig                   |   2 +-
>   drivers/power/reset/Kconfig                  |   1 +
>   drivers/rtc/Kconfig                          |   4 ++-
>   drivers/scsi/Kconfig                         |  21 ++++++-------
>   drivers/scsi/aic7xxx/Kconfig.aic79xx         |   2 +-
>   drivers/scsi/aic7xxx/Kconfig.aic7xxx         |   2 +-
>   drivers/scsi/aic94xx/Kconfig                 |   2 +-
>   drivers/scsi/megaraid/Kconfig.megaraid       |   2 +-
>   drivers/scsi/mvsas/Kconfig                   |   2 +-
>   drivers/scsi/qla2xxx/Kconfig                 |   2 +-
>   drivers/spi/Kconfig                          |   1 +
>   drivers/staging/kpc2000/Kconfig              |   2 +-
>   drivers/staging/sm750fb/Kconfig              |   2 +-
>   drivers/staging/vt6655/Kconfig               |   2 +-
>   drivers/tty/Kconfig                          |   2 +-
>   drivers/tty/serial/8250/8250_early.c         |   4 +++
>   drivers/tty/serial/8250/8250_pci.c           |  19 ++++++++++--
>   drivers/tty/serial/8250/8250_port.c          |  22 ++++++++++++--
>   drivers/tty/serial/8250/Kconfig              |   1 +
>   drivers/tty/serial/Kconfig                   |   2 +-
>   drivers/usb/core/hcd-pci.c                   |   4 +--
>   drivers/usb/host/Kconfig                     |   4 +--
>   drivers/usb/host/pci-quirks.c                | 128
> +++++++++++++++++++++++++++++++++++++++++--------------------------------------
>   drivers/usb/host/pci-quirks.h                |  33 ++++++++++++++++-----
>   drivers/usb/host/uhci-hcd.c                  |   2 +-
>   drivers/usb/host/uhci-hcd.h                  |  77
> +++++++++++++++++++++++++++++++----------------
>   drivers/video/console/Kconfig                |   4 ++-
>   drivers/video/fbdev/Kconfig                  |  24 +++++++--------
>   drivers/watchdog/Kconfig                     |   6 ++--
>   include/asm-generic/io.h                     |   6 ++++
>   include/linux/gameport.h                     |   9 ++++--
>   include/linux/parport.h                      |   2 +-
>   include/video/vga.h                          |   8 +++++
>   lib/Kconfig                                  |   4 +++
>   lib/Kconfig.kgdb                             |   1 +
>   sound/drivers/Kconfig                        |   3 ++
>   sound/isa/Kconfig                            |   1 +
>   sound/pci/Kconfig                            |  44 ++++++++++++++++++++++-----
>   96 files changed, 575 insertions(+), 272 deletions(-)
> 

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

[Arnd Bergmann]

On Tue, Aug 3, 2021 at 11:46 AM John Garry <john.garry@huawei.com> wrote:
> On 05/07/2021 11:06, Arnd Bergmann wrote:

> >
> > Linus, if you like this approach, then I can work on splitting it up into
> > meaningful patches and submit it for a future release. I think the
> > CONFIG_LEGACY_PCI option has value on its own, but the others
> > do introduce some churn.
> >
> > Full patch (120KB): https://pastebin.com/yaFSmAuY
> >
>
> Hi Arnd,
>
> I am not sure if anything is happening here.

No, I'm not currently working on this, though I have it applied to
my randconfig tree.

> Anyway, one thing I mentioned earlier was that we could solve the
> problem of drivers accessing unmapped IO ports and crashing systems on
> archs which define PCI_IOBASE by building them under some "native port
> IO support" flag.

Right, that was part of the goal here.

> One example of such a driver was F71805F sensor. You put that under
> HAS_IOPORT, which would be available for all archs, I think. But I could
> not see where config LEGACY_PCI is introduced. Could we further refine
> that config to not build for such archs as arm64?
>
> BTW, I think that the PPC dependency was added there to stop building
> for power for that same reason, so hopefully we get rid of that.

Good point. It seems that I actually never added the LEGACY_PCI option
to my patch, so I'm just not building those drivers any more, and not
defining the inb()/outb() helpers either, causing a build failure when I'm
missing an option.

However it sounds like you are interested in a third option here, which
brings us to:

LEGACY_PCI: any PCI driver that uses inb()/outb() or is only available
    on old-style PCI but not PCIe hardware without a bridge.
    To be disabled for most architectures and possibly distros but can
    be enabled for kernels that want to use those devices, as long as
    CONFIG_HAS_IOPORT is set by the architecture.

HAS_IOPORT: not a legacy PCI device, but can only be built on
    architectures that define inb()/outb(). To be disabled for s390
    and any other machine that has no useful definition of those
    functions.

HARDCODED_IOPORT: (or another name you might think of,) Used by
   drivers that unconditionally do inb()/outb() without checking the
   validity of the address using firmware or other methods first.
   depends on HAS_IOPORT and possibly architecture specific
   settings.

        Arnd

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

[John Garry]

>> Anyway, one thing I mentioned earlier was that we could solve the
>> problem of drivers accessing unmapped IO ports and crashing systems on
>> archs which define PCI_IOBASE by building them under some "native port
>> IO support" flag.
> Right, that was part of the goal here.

Great

> 
>> One example of such a driver was F71805F sensor. You put that under
>> HAS_IOPORT, which would be available for all archs, I think. But I could
>> not see where config LEGACY_PCI is introduced. Could we further refine
>> that config to not build for such archs as arm64?
>>
>> BTW, I think that the PPC dependency was added there to stop building
>> for power for that same reason, so hopefully we get rid of that.
> Good point. It seems that I actually never added the LEGACY_PCI option
> to my patch,

ok, it would be nice to see that.

> so I'm just not building those drivers any more, and not
> defining the inb()/outb() helpers either, causing a build failure when I'm
> missing an option.
> 
> However it sounds like you are interested in a third option here, which
> brings us to:
> 
> LEGACY_PCI: any PCI driver that uses inb()/outb() or is only available
>      on old-style PCI but not PCIe hardware without a bridge.
>      To be disabled for most architectures and possibly distros but can
>      be enabled for kernels that want to use those devices, as long as
>      CONFIG_HAS_IOPORT is set by the architecture.
> 
> HAS_IOPORT: not a legacy PCI device, but can only be built on
>      architectures that define inb()/outb(). To be disabled for s390
>      and any other machine that has no useful definition of those
>      functions.

That seems reasonable. And asm-generic io.h should be ifdef'ed by 
HAS_IOPORT. In your patch you had it under CONFIG_IOPORT - was that 
intentional?

On another point, I noticed SCSI driver AHA152x depends on ISA, but is 
not an isa driver - however it does use port IO. Would such dependencies 
need to be changed to depend on HAS_IOPORT?

I did notice that arm32 support CONFIG_ISA - not sure why.

> 
> HARDCODED_IOPORT: (or another name you might think of,) Used by
>     drivers that unconditionally do inb()/outb() without checking the
>     validity of the address using firmware or other methods first.
>     depends on HAS_IOPORT and possibly architecture specific
>     settings.

Yeah, that sounds the same as what I was thinking. Maybe IOPORT_NATIVE 
could work as a name. I would think that only x86/ia64 would define it. 
A concern though is that someone could argue that is a functional 
dependency, rather than just a build dependency.

Thanks,
John

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

[Arnd Bergmann]

On Tue, Aug 3, 2021 at 1:23 PM John Garry <john.garry@huawei.com> wrote:
> > so I'm just not building those drivers any more, and not
> > defining the inb()/outb() helpers either, causing a build failure when I'm
> > missing an option.
> >
> > However it sounds like you are interested in a third option here, which
> > brings us to:
> >
> > LEGACY_PCI: any PCI driver that uses inb()/outb() or is only available
> >      on old-style PCI but not PCIe hardware without a bridge.
> >      To be disabled for most architectures and possibly distros but can
> >      be enabled for kernels that want to use those devices, as long as
> >      CONFIG_HAS_IOPORT is set by the architecture.
> >
> > HAS_IOPORT: not a legacy PCI device, but can only be built on
> >      architectures that define inb()/outb(). To be disabled for s390
> >      and any other machine that has no useful definition of those
> >      functions.
>
> That seems reasonable. And asm-generic io.h should be ifdef'ed by
> HAS_IOPORT. In your patch you had it under CONFIG_IOPORT - was that
> intentional?

No, that was a typo. Thanks for pointing this out.

> On another point, I noticed SCSI driver AHA152x depends on ISA, but is
> not an isa driver - however it does use port IO. Would such dependencies
> need to be changed to depend on HAS_IOPORT?

I'm not sure what you mean here. As far as I can tell, AHA152x is an ISA
driver in the sense that it is a driver for ISA add-on cards. However, it
is not a 'struct isa_driver' in the sense that AHA1542 is, AHA152x  is even
older and uses the linux-2.4 style initialization using a module_init()
function that does the probing.

> I did notice that arm32 support CONFIG_ISA - not sure why.

This is for some of the earlier machines we support:
mach-footbridge has some on-board ISA components, while
SA1100, PXA25x and S3C2410 each have at least one machine
with a PC/104 connector using ISA signaling for add-on cards.

There are also a couple of platforms with PCMCIA or CF slots
using the same ISA style I/O signals, but those have separate
drivers.

> > HARDCODED_IOPORT: (or another name you might think of,) Used by
> >     drivers that unconditionally do inb()/outb() without checking the
> >     validity of the address using firmware or other methods first.
> >     depends on HAS_IOPORT and possibly architecture specific
> >     settings.
>
> Yeah, that sounds the same as what I was thinking. Maybe IOPORT_NATIVE
> could work as a name. I would think that only x86/ia64 would define it.
> A concern though is that someone could argue that is a functional
> dependency, rather than just a build dependency.

You can have those on a number of platforms, such as early
PowerPC CHRP or pSeries systems, a number of MIPS workstations
including recent Loongson machines, and many Alpha platforms.

Maybe the name should reflect that these all use PC-style ISA/LPC
port numbers without the ISA connectors.

       Arnd

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

[John Garry]

On 03/08/2021 13:15, Arnd Bergmann wrote:
>> That seems reasonable. And asm-generic io.h should be ifdef'ed by
>> HAS_IOPORT. In your patch you had it under CONFIG_IOPORT - was that
>> intentional?
> No, that was a typo. Thanks for pointing this out.
> 
>> On another point, I noticed SCSI driver AHA152x depends on ISA, but is
>> not an isa driver - however it does use port IO. Would such dependencies
>> need to be changed to depend on HAS_IOPORT?
> I'm not sure what you mean here. As far as I can tell, AHA152x is an ISA
> driver in the sense that it is a driver for ISA add-on cards. However, it
> is not a 'struct isa_driver' in the sense that AHA1542 is, AHA152x  is even
> older and uses the linux-2.4 style initialization using a module_init()
> function that does the probing.

ok, fine. So I just wonder what the ISA kconfig dependency gets us for 
aha152x. I experimented by removing the kconfig dependency and enabling 
for the arm64 (which does not have CONFIG_ISA) std defconfig and it 
built fine.

> 
>> I did notice that arm32 support CONFIG_ISA - not sure why.
> This is for some of the earlier machines we support:
> mach-footbridge has some on-board ISA components, while
> SA1100, PXA25x and S3C2410 each have at least one machine
> with a PC/104 connector using ISA signaling for add-on cards.
> 
> There are also a couple of platforms with PCMCIA or CF slots
> using the same ISA style I/O signals, but those have separate
> drivers.
> 
>>> HARDCODED_IOPORT: (or another name you might think of,) Used by
>>>      drivers that unconditionally do inb()/outb() without checking the
>>>      validity of the address using firmware or other methods first.
>>>      depends on HAS_IOPORT and possibly architecture specific
>>>      settings.
>> Yeah, that sounds the same as what I was thinking. Maybe IOPORT_NATIVE
>> could work as a name. I would think that only x86/ia64 would define it.
>> A concern though is that someone could argue that is a functional
>> dependency, rather than just a build dependency.
> You can have those on a number of platforms, such as early
> PowerPC CHRP or pSeries systems, a number of MIPS workstations
> including recent Loongson machines, and many Alpha platforms.
> 

hmmm... if some machines under an arch support "native" port IO and some 
don't, then if we use a common multi-platform defconfig which defines 
HARDCODED_IOPORT, then we still build for platforms without "native" 
port IO, which is not ideal.

> Maybe the name should reflect that these all use PC-style ISA/LPC
> port numbers without the ISA connectors.

Thanks,
john

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

[Arnd Bergmann]

On Wed, Aug 4, 2021 at 9:55 AM John Garry <john.garry@huawei.com> wrote:
>
> On 03/08/2021 13:15, Arnd Bergmann wrote:
> >> That seems reasonable. And asm-generic io.h should be ifdef'ed by
> >> HAS_IOPORT. In your patch you had it under CONFIG_IOPORT - was that
> >> intentional?
> > No, that was a typo. Thanks for pointing this out.
> >
> >> On another point, I noticed SCSI driver AHA152x depends on ISA, but is
> >> not an isa driver - however it does use port IO. Would such dependencies
> >> need to be changed to depend on HAS_IOPORT?
> > I'm not sure what you mean here. As far as I can tell, AHA152x is an ISA
> > driver in the sense that it is a driver for ISA add-on cards. However, it
> > is not a 'struct isa_driver' in the sense that AHA1542 is, AHA152x  is even
> > older and uses the linux-2.4 style initialization using a module_init()
> > function that does the probing.
>
> ok, fine. So I just wonder what the ISA kconfig dependency gets us for
> aha152x. I experimented by removing the kconfig dependency and enabling
> for the arm64 (which does not have CONFIG_ISA) std defconfig and it
> built fine.

The point of CONFIG_ISA is to only build drivers for ISA add-on cards
on architectures that can have such slots. For ISA drivers in particular,
we don't want them to be loaded on machines that don't have them
because of the various ways this can cause trouble with hardwired
port and irq numbers.

> >> Yeah, that sounds the same as what I was thinking. Maybe IOPORT_NATIVE
> >> could work as a name. I would think that only x86/ia64 would define it.
> >> A concern though is that someone could argue that is a functional
> >> dependency, rather than just a build dependency.
> > You can have those on a number of platforms, such as early
> > PowerPC CHRP or pSeries systems, a number of MIPS workstations
> > including recent Loongson machines, and many Alpha platforms.
> >
>
> hmmm... if some machines under an arch support "native" port IO and some
> don't, then if we use a common multi-platform defconfig which defines
> HARDCODED_IOPORT, then we still build for platforms without "native"
> port IO, which is not ideal.

Correct, but that's not a problem I'm trying to solve at this point. The
machines that have those are extremely rare, so almost all configurations
that one would encounter in practice do not suffer from it, and solving it
reliably would be really hard.

       Arnd

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

[John Garry]

On 04/08/2021 09:52, Arnd Bergmann wrote:
>>>> On another point, I noticed SCSI driver AHA152x depends on ISA, but is
>>>> not an isa driver - however it does use port IO. Would such dependencies
>>>> need to be changed to depend on HAS_IOPORT?
>>> I'm not sure what you mean here. As far as I can tell, AHA152x is an ISA
>>> driver in the sense that it is a driver for ISA add-on cards. However, it
>>> is not a 'struct isa_driver' in the sense that AHA1542 is, AHA152x  is even
>>> older and uses the linux-2.4 style initialization using a module_init()
>>> function that does the probing.
>> ok, fine. So I just wonder what the ISA kconfig dependency gets us for
>> aha152x. I experimented by removing the kconfig dependency and enabling
>> for the arm64 (which does not have CONFIG_ISA) std defconfig and it
>> built fine.
> The point of CONFIG_ISA is to only build drivers for ISA add-on cards
> on architectures that can have such slots. For ISA drivers in particular,
> we don't want them to be loaded on machines that don't have them
> because of the various ways this can cause trouble with hardwired
> port and irq numbers.
> 
>>>> Yeah, that sounds the same as what I was thinking. Maybe IOPORT_NATIVE
>>>> could work as a name. I would think that only x86/ia64 would define it.
>>>> A concern though is that someone could argue that is a functional
>>>> dependency, rather than just a build dependency.
>>> You can have those on a number of platforms, such as early
>>> PowerPC CHRP or pSeries systems, a number of MIPS workstations
>>> including recent Loongson machines, and many Alpha platforms.
>>>
>> hmmm... if some machines under an arch support "native" port IO and some
>> don't, then if we use a common multi-platform defconfig which defines
>> HARDCODED_IOPORT, then we still build for platforms without "native"
>> port IO, which is not ideal.
> Correct, but that's not a problem I'm trying to solve at this point. The
> machines that have those are extremely rare, so almost all configurations
> that one would encounter in practice do not suffer from it, and solving it
> reliably would be really hard.

Hi Arnd,

This seems a reasonable approach. Do you have a plan for this work? Or 
still waiting for the green light?

I have noticed the kernel test robot reporting the following to me, 
which seems to be the same issue which was addressed in this series 
originally:

config: s390-randconfig-r032-20210802 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 
4f71f59bf3d9914188a11d0c41bedbb339d36ff5)
...
All errors (new ones prefixed by >>):

    In file included from drivers/block/null_blk/main.c:12:
    In file included from drivers/block/null_blk/null_blk.h:8:
    In file included from include/linux/blkdev.h:25:
    In file included from include/linux/scatterlist.h:9:
    In file included from arch/s390/include/asm/io.h:75:
    include/asm-generic/io.h:464:31: warning: performing pointer 
arithmetic on a null pointer has undefined behavior 
[-Wnull-pointer-arithmetic]
            val = __raw_readb(PCI_IOBASE + addr);

So I imagine lots of people are seeing these.

Thanks,
john

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

[Arnd Bergmann]

On Tue, Aug 10, 2021 at 11:19 AM John Garry <john.garry@huawei.com> wrote:
> On 04/08/2021 09:52, Arnd Bergmann wrote:
>
> This seems a reasonable approach. Do you have a plan for this work? Or
> still waiting for the green light?

I'm rather busy with other work at the moment, so no particular plans
for any time soon.

> I have noticed the kernel test robot reporting the following to me,
> which seems to be the same issue which was addressed in this series
> originally:
>
> config: s390-randconfig-r032-20210802 (attached as .config)
> compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
> 4f71f59bf3d9914188a11d0c41bedbb339d36ff5)
> ...
> All errors (new ones prefixed by >>):
>
>     In file included from drivers/block/null_blk/main.c:12:
>     In file included from drivers/block/null_blk/null_blk.h:8:
>     In file included from include/linux/blkdev.h:25:
>     In file included from include/linux/scatterlist.h:9:
>     In file included from arch/s390/include/asm/io.h:75:
>     include/asm-generic/io.h:464:31: warning: performing pointer
> arithmetic on a null pointer has undefined behavior
> [-Wnull-pointer-arithmetic]
>             val = __raw_readb(PCI_IOBASE + addr);
>
> So I imagine lots of people are seeing these.

Right, this is the original problem that Niklas was trying to solve.

If Niklas has time to get this fixed, I can probably find a way to work
with him on finishing up my proposed patch with the changes you
suggested.

       Arnd

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

[Niklas Schnelle]

On Tue, 2021-08-10 at 13:33 +0200, Arnd Bergmann wrote:
> On Tue, Aug 10, 2021 at 11:19 AM John Garry <john.garry@huawei.com> wrote:
> > On 04/08/2021 09:52, Arnd Bergmann wrote:
> > 
> > This seems a reasonable approach. Do you have a plan for this work? Or
> > still waiting for the green light?
> 
> I'm rather busy with other work at the moment, so no particular plans
> for any time soon.
> 
> > I have noticed the kernel test robot reporting the following to me,
> > which seems to be the same issue which was addressed in this series
> > originally:
> > 
> > config: s390-randconfig-r032-20210802 (attached as .config)
> > compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
> > 4f71f59bf3d9914188a11d0c41bedbb339d36ff5)
> > ...
> > All errors (new ones prefixed by >>):
> > 
> >     In file included from drivers/block/null_blk/main.c:12:
> >     In file included from drivers/block/null_blk/null_blk.h:8:
> >     In file included from include/linux/blkdev.h:25:
> >     In file included from include/linux/scatterlist.h:9:
> >     In file included from arch/s390/include/asm/io.h:75:
> >     include/asm-generic/io.h:464:31: warning: performing pointer
> > arithmetic on a null pointer has undefined behavior
> > [-Wnull-pointer-arithmetic]
> >             val = __raw_readb(PCI_IOBASE + addr);
> > 
> > So I imagine lots of people are seeing these.
> 
> Right, this is the original problem that Niklas was trying to solve.
> 
> If Niklas has time to get this fixed, I can probably find a way to work
> with him on finishing up my proposed patch with the changes you
> suggested.
> 
>        Arnd

Sorry for the late reply, this got lost in my inbox. I could spare some
cycles on this but I'm not sure how I can help.

The series you sent after Linus' nacked the previous approach looks
quite broad touching lots of areas I have little experience with. I'd
be willing to test things and look over patches the best I can of
course.

Thanks,
Niklas

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

[Niklas Schnelle]

On Tue, 2021-08-10 at 13:33 +0200, Arnd Bergmann wrote:
> On Tue, Aug 10, 2021 at 11:19 AM John Garry <john.garry@huawei.com> wrote:
> > On 04/08/2021 09:52, Arnd Bergmann wrote:
> > 
> > This seems a reasonable approach. Do you have a plan for this work? Or
> > still waiting for the green light?
> 
> I'm rather busy with other work at the moment, so no particular plans
> for any time soon.
> 
> > I have noticed the kernel test robot reporting the following to me,
> > which seems to be the same issue which was addressed in this series
> > originally:
> > 
> > config: s390-randconfig-r032-20210802 (attached as .config)
> > compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
> > 4f71f59bf3d9914188a11d0c41bedbb339d36ff5)
> > ...
> > All errors (new ones prefixed by >>):
> > 
> >     In file included from drivers/block/null_blk/main.c:12:
> >     In file included from drivers/block/null_blk/null_blk.h:8:
> >     In file included from include/linux/blkdev.h:25:
> >     In file included from include/linux/scatterlist.h:9:
> >     In file included from arch/s390/include/asm/io.h:75:
> >     include/asm-generic/io.h:464:31: warning: performing pointer
> > arithmetic on a null pointer has undefined behavior
> > [-Wnull-pointer-arithmetic]
> >             val = __raw_readb(PCI_IOBASE + addr);
> > 
> > So I imagine lots of people are seeing these.
> 
> Right, this is the original problem that Niklas was trying to solve.
> 
> If Niklas has time to get this fixed, I can probably find a way to work
> with him on finishing up my proposed patch with the changes you
> suggested.
> 
>        Arnd

Hi Arnd, Hi John,

I've had some time to look into this a bit. As a refreshed starting
point I have rebased Arnd's patch to v5.16-rc5. Since I'm not sure how
to handle authorship and it's very early I haven't sent it as RFC but
it's available as a patch from my GitHub here: 
https://gist.github.com/niklas88/a08fe76bdf9f5798500fccea6583e275

I have incorporated the following findings from this thread already:

- Added HAS_IOPORT to arch Kconfigs
- Added "config LEGACY_PCI" to drivers/pci/Kconfig
- Fixed CONFIG_HAS_IOPORT typo in asm-generic/io.h
- Removed LEGACY_PCI dependency of i2c-i801.
  Which is also used in current gen Intel platforms
  and depends on x86 anyway.

I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
as running it with defconfig. I've also been using it on my Ryzen 3990X
workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
fewer modules compared with a similar config of v5.15.8. Hard to say
which other systems might miss things of course.

I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
wondering two things. For one it feels like that could be a separate
change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
Secondly I'm wondering about good ways of identifying such drivers and
how much this overlaps with the ISA config flag.

I'd of course appreciate feedback. If you agree this is still
worthwhile to persue I'd think the next step would be trying to
refactor this into more manageable patches.

Thanks,
Niklas

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

[Arnd Bergmann]

On Fri, Dec 17, 2021 at 2:19 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
>
> I've had some time to look into this a bit. As a refreshed starting
> point I have rebased Arnd's patch to v5.16-rc5. Since I'm not sure how
> to handle authorship and it's very early I haven't sent it as RFC but
> it's available as a patch from my GitHub here:
> https://gist.github.com/niklas88/a08fe76bdf9f5798500fccea6583e275
>
> I have incorporated the following findings from this thread already:
>
> - Added HAS_IOPORT to arch Kconfigs
> - Added "config LEGACY_PCI" to drivers/pci/Kconfig
> - Fixed CONFIG_HAS_IOPORT typo in asm-generic/io.h
> - Removed LEGACY_PCI dependency of i2c-i801.
>   Which is also used in current gen Intel platforms
>   and depends on x86 anyway.
>
> I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
> as running it with defconfig. I've also been using it on my Ryzen 3990X
> workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
> fewer modules compared with a similar config of v5.15.8. Hard to say
> which other systems might miss things of course.
>
> I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
> wondering two things. For one it feels like that could be a separate
> change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
> Secondly I'm wondering about good ways of identifying such drivers and
> how much this overlaps with the ISA config flag.
>
> I'd of course appreciate feedback. If you agree this is still
> worthwhile to persue I'd think the next step would be trying to
> refactor this into more manageable patches.

Thanks a lot for restarting this work! I think this all looks reasonable
(a lot was my original patch anyway, so of course I think that ;)), but
it would be good to split it up into multiple patches.

The CONFIG_LEGACY_PCI should take care of a lot of it, and I
think that can be a single patch. I'd expand the Kconfig description
to explain that this also covers PCIe devices that use the legacy
I/O space even if they do not have a PCIe-to-PCI bridge in them.

The introduction of CONFIG_HAS_IOPORT, plus selecting it from
the respective architectures makes sense as another patch, but
I would make that separate from the #ifdef and 'depends on'
changes to individual subsystems or drivers, as they are
better reviewed separately.

        Arnd

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

[Niklas Schnelle]

On Fri, 2021-12-17 at 14:32 +0100, Arnd Bergmann wrote:
> On Fri, Dec 17, 2021 at 2:19 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> > I've had some time to look into this a bit. As a refreshed starting
> > point I have rebased Arnd's patch to v5.16-rc5. Since I'm not sure how
> > to handle authorship and it's very early I haven't sent it as RFC but
> > it's available as a patch from my GitHub here:
> > https://gist.github.com/niklas88/a08fe76bdf9f5798500fccea6583e275
> > 
> > I have incorporated the following findings from this thread already:
> > 
> > - Added HAS_IOPORT to arch Kconfigs
> > - Added "config LEGACY_PCI" to drivers/pci/Kconfig
> > - Fixed CONFIG_HAS_IOPORT typo in asm-generic/io.h
> > - Removed LEGACY_PCI dependency of i2c-i801.
> >   Which is also used in current gen Intel platforms
> >   and depends on x86 anyway.
> > 
> > I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
> > as running it with defconfig. I've also been using it on my Ryzen 3990X
> > workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
> > fewer modules compared with a similar config of v5.15.8. Hard to say
> > which other systems might miss things of course.
> > 
> > I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
> > wondering two things. For one it feels like that could be a separate
> > change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
> > Secondly I'm wondering about good ways of identifying such drivers and
> > how much this overlaps with the ISA config flag.
> > 
> > I'd of course appreciate feedback. If you agree this is still
> > worthwhile to persue I'd think the next step would be trying to
> > refactor this into more manageable patches.
> 
> Thanks a lot for restarting this work! I think this all looks reasonable
> (a lot was my original patch anyway, so of course I think that ;)), but
> it would be good to split it up into multiple patches.

Yes definitely.

> 
> The CONFIG_LEGACY_PCI should take care of a lot of it, and I
> think that can be a single patch. I'd expand the Kconfig description
> to explain that this also covers PCIe devices that use the legacy
> I/O space even if they do not have a PCIe-to-PCI bridge in them.
> 
> The introduction of CONFIG_HAS_IOPORT, plus selecting it from
> the respective architectures makes sense as another patch, but
> I would make that separate from the #ifdef and 'depends on'
> changes to individual subsystems or drivers, as they are
> better reviewed separately.
> 
>         Arnd

Sounds like a plan. How should I mark authorship in the split up
patches. I definitely still see you as the main author to all of this
but of course I'll have to do quite a bit of editing and you shouldn't
get blamed for any mistakes I make. So not sure how to handle Sign-off-
bys and git's author property.

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

[Arnd Bergmann]

On Fri, Dec 17, 2021 at 2:52 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> > The CONFIG_LEGACY_PCI should take care of a lot of it, and I
> > think that can be a single patch. I'd expand the Kconfig description
> > to explain that this also covers PCIe devices that use the legacy
> > I/O space even if they do not have a PCIe-to-PCI bridge in them.
> >
> > The introduction of CONFIG_HAS_IOPORT, plus selecting it from
> > the respective architectures makes sense as another patch, but
> > I would make that separate from the #ifdef and 'depends on'
> > changes to individual subsystems or drivers, as they are
> > better reviewed separately.
>
> Sounds like a plan. How should I mark authorship in the split up
> patches. I definitely still see you as the main author to all of this
> but of course I'll have to do quite a bit of editing and you shouldn't
> get blamed for any mistakes I make. So not sure how to handle Sign-off-
> bys and git's author property.

I don't care much either way. The two options are:

a) leave me as patch author, with my Signed-off-by, and list
    in the changelog what you have changed that wasn't in
    my version

b) list me as 'Co-developed-by' and have yourself as the patch
    author.

        Arnd

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

[John Garry]

On 17/12/2021 13:52, Niklas Schnelle wrote:

Thanks for looking at this again.

>>> I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
>>> as running it with defconfig. I've also been using it on my Ryzen 3990X
>>> workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
>>> fewer modules compared with a similar config of v5.15.8. Hard to say
>>> which other systems might miss things of course.
>>>
>>> I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
>>> wondering two things. For one it feels like that could be a separate
>>> change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
>>> Secondly I'm wondering about good ways of identifying such drivers and
>>> how much this overlaps with the ISA config flag.

I was interesting in the IOPORT_NATIVE flag (or whatever we call it) as 
it solves the problem of drivers which "unconditionally do inb()/outb() 
without checking the validity of the address using firmware or other 
methods first" being built for (and loaded on and crashing) unsuitable 
systems. Such a problem is in [0]

So if we want to support that later, then it seems that someone would 
need to go back and re-edit many same driver Kconfigs – like hwon, for 
example. I think it's better to avoid that and do it now.

Arnd, any opinion on that?

I'm happy to help with that effort.

[0] 
https://lore.kernel.org/lkml/1610729929-188490-1-git-send-email-john.garry@huawei.com/

Thanks,
John

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

[Arnd Bergmann]

On Fri, Dec 17, 2021 at 3:27 PM John Garry <john.garry@huawei.com> wrote:
> On 17/12/2021 13:52, Niklas Schnelle wrote:
> >>> I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
> >>> as running it with defconfig. I've also been using it on my Ryzen 3990X
> >>> workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
> >>> fewer modules compared with a similar config of v5.15.8. Hard to say
> >>> which other systems might miss things of course.
> >>>
> >>> I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
> >>> wondering two things. For one it feels like that could be a separate
> >>> change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
> >>> Secondly I'm wondering about good ways of identifying such drivers and
> >>> how much this overlaps with the ISA config flag.
>
> I was interesting in the IOPORT_NATIVE flag (or whatever we call it) as
> it solves the problem of drivers which "unconditionally do inb()/outb()
> without checking the validity of the address using firmware or other
> methods first" being built for (and loaded on and crashing) unsuitable
> systems. Such a problem is in [0]
>
> So if we want to support that later, then it seems that someone would
> need to go back and re-edit many same driver Kconfigs – like hwon, for
> example. I think it's better to avoid that and do it now.
>
> Arnd, any opinion on that?
>
> I'm happy to help with that effort.

I looked at the options the other day and couldn't really find any that
fell into this category, so I suggested that Niklas would skip that for the
moment. If you have a better way of finding the affected drivers,
that would be great.

       Arnd

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

[John Garry]

On 17/12/2021 14:32, Arnd Bergmann wrote:
>> I was interesting in the IOPORT_NATIVE flag (or whatever we call it) as
>> it solves the problem of drivers which "unconditionally do inb()/outb()
>> without checking the validity of the address using firmware or other
>> methods first" being built for (and loaded on and crashing) unsuitable
>> systems. Such a problem is in [0]
>>
>> So if we want to support that later, then it seems that someone would
>> need to go back and re-edit many same driver Kconfigs – like hwmon, for
>> example. I think it's better to avoid that and do it now.
>>
>> Arnd, any opinion on that?
>>
>> I'm happy to help with that effort.
> I looked at the options the other day and couldn't really find any that
> fell into this category, so I suggested that Niklas would skip that for the
> moment. 

 From looking at the patch Niklas directed us at, as I understand, 
HAS_IOPORT is to decide whether the arch/platform may support PIO 
accessors - inb et al. And on that basis I am confused why it is not 
selected for arm64. And further compounded by:

  config INDIRECT_PIO
	 	bool "Access I/O in non-MMIO mode"
	 	depends on ARM64
	+	depends on HAS_IOPORT

If arm64 does not select, then why depend on it?

 > If you have a better way of finding the affected drivers,
 > that would be great.

Assuming arm64 should select HAS_IOPORT, I am talking about f71805f as 
an example. According to that patch, this driver additionally depends on 
HAS_IOPORT; however I would rather arm64, like powerpc, should not allow 
that driver to be built at all.

Thanks,
John

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

[Arnd Bergmann]

On Fri, Dec 17, 2021 at 4:27 PM John Garry <john.garry@huawei.com> wrote:
> On 17/12/2021 14:32, Arnd Bergmann wrote:
>  From looking at the patch Niklas directed us at, as I understand,
> HAS_IOPORT is to decide whether the arch/platform may support PIO
> accessors - inb et al. And on that basis I am confused why it is not
> selected for arm64. And further compounded by:
>
>   config INDIRECT_PIO
>                 bool "Access I/O in non-MMIO mode"
>                 depends on ARM64
>         +       depends on HAS_IOPORT
>
> If arm64 does not select, then why depend on it?

Right, both arm32 and arm64 need to select HAS_IOPORT.

>  > If you have a better way of finding the affected drivers,
>  > that would be great.
>
> Assuming arm64 should select HAS_IOPORT, I am talking about f71805f as
> an example. According to that patch, this driver additionally depends on
> HAS_IOPORT; however I would rather arm64, like powerpc, should not allow
> that driver to be built at all.

Agreed, I missed these when I looked through the HAS_IOPORT users,
that's why I suggested to split up that part of the patch per subsystem
so they can be inspected more carefully.

My feeling is that in this case we want some other dependency, e.g. a
new CONFIG_LPC. It should actually be possible to use this driver on
any machine with an LPC bus, which would by definition be the primary
I/O space, so it should be possible to load it on Arm64.

         Arnd

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

[John Garry]

On 17/12/2021 15:55, Arnd Bergmann wrote:
>> > If you have a better way of finding the affected drivers,
>>   > that would be great.
>>
>> Assuming arm64 should select HAS_IOPORT, I am talking about f71805f as
>> an example. According to that patch, this driver additionally depends on
>> HAS_IOPORT; however I would rather arm64, like powerpc, should not allow
>> that driver to be built at all.
> Agreed, I missed these when I looked through the HAS_IOPORT users,
> that's why I suggested to split up that part of the patch per subsystem
> so they can be inspected more carefully.

ok

 >
 > My feeling is that in this case we want some other dependency, e.g. a
 > new CONFIG_LPC. It should actually be possible to use this driver on
 > any machine with an LPC bus, which would by definition be the primary
 > I/O space, so it should be possible to load it on Arm64.

You did suggest HARDCODED_IOPORT earlier in this thread, and the 
definition/premise there seemed sensible to me.

Anyway it seems practical to make all these changes in a single series, 
so need a way forward as Niklas has no such changes for this additional 
kconfig option.

As a start, may I suggest we at least have Niklas' patch committed to a 
dev branch based on -next or latest mainline release for further analysis?

Thanks,
John

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

[David Laight]

From: John Garry
> Sent: 17 December 2021 14:28
> 
> On 17/12/2021 13:52, Niklas Schnelle wrote:
> 
> Thanks for looking at this again.
> 
> >>> I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
> >>> as running it with defconfig. I've also been using it on my Ryzen 3990X
> >>> workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
> >>> fewer modules compared with a similar config of v5.15.8. Hard to say
> >>> which other systems might miss things of course.
> >>>
> >>> I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
> >>> wondering two things. For one it feels like that could be a separate
> >>> change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
> >>> Secondly I'm wondering about good ways of identifying such drivers and
> >>> how much this overlaps with the ISA config flag.
> 
> I was interesting in the IOPORT_NATIVE flag (or whatever we call it) as
> it solves the problem of drivers which "unconditionally do inb()/outb()
> without checking the validity of the address using firmware or other
> methods first" being built for (and loaded on and crashing) unsuitable
> systems. Such a problem is in [0]
> 
> So if we want to support that later, then it seems that someone would
> need to go back and re-edit many same driver Kconfigs – like hwon, for
> example. I think it's better to avoid that and do it now.

Could you do something where valid arguments to inb() have to come
from some kernel mapping/validation function and are never in the
range [0x0, 0x10000).
Then drivers that are cheating the system will fail.

Or, maybe, only allow [0x0, 0x10000) on systems that have a suitable bus.
With the mapping functions returning a different value (eg the KVA into
the PCI master window) that can be separately verified.
That would let drivers do (say) inb(0x120) on systems that have (something
like) and ISA bus, but not on PCI-only systems which support PCI IO
accesses through a physical address window.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Niklas Schnelle]

On Fri, 2021-12-17 at 16:30 +0000, John Garry wrote:
> On 17/12/2021 15:55, Arnd Bergmann wrote:
> > > > If you have a better way of finding the affected drivers,
> > >   > that would be great.
> > > 
> > > Assuming arm64 should select HAS_IOPORT, I am talking about f71805f as
> > > an example. According to that patch, this driver additionally depends on
> > > HAS_IOPORT; however I would rather arm64, like powerpc, should not allow
> > > that driver to be built at all.
> > Agreed, I missed these when I looked through the HAS_IOPORT users,
> > that's why I suggested to split up that part of the patch per subsystem
> > so they can be inspected more carefully.
> 
> ok
> 
>  >
>  > My feeling is that in this case we want some other dependency, e.g. a
>  > new CONFIG_LPC. It should actually be possible to use this driver on
>  > any machine with an LPC bus, which would by definition be the primary
>  > I/O space, so it should be possible to load it on Arm64.
> 
> You did suggest HARDCODED_IOPORT earlier in this thread, and the 
> definition/premise there seemed sensible to me.
> 
> Anyway it seems practical to make all these changes in a single series, 
> so need a way forward as Niklas has no such changes for this additional 
> kconfig option.
> 
> As a start, may I suggest we at least have Niklas' patch committed to a 
> dev branch based on -next or latest mainline release for further analysis?
> 
> Thanks,
> John
> 
> 

My plan would be to split the patch up into more manageable pieces as
suggested by Arnd plus of course fixes like the missing ARM select. As
Arnd suggested I'll split the HAS_IOPORT additions into the initial
introduction plus arch selects and then the HAS_IOPORT dependencies per
subsytem. I think these per subsystem dependency patches then would be
a great place to find drivers which should have a different dependency
be it on LPC or a newly introduced HARDCODED_IOPORT. The thing is we
can find and check HAS_IOPORT dependencies easily but it's hard to find
HARDCODED_IOPORT so I think the lattter should be a refinement of the
former. It can of course still go in as a single series. I'll
definitely make the next iteration available as a git branch.

Thanks,
Niklas

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

[John Garry]

On 19/12/2021 14:23, David Laight wrote:
>>>>> I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
>>>>> as running it with defconfig. I've also been using it on my Ryzen 3990X
>>>>> workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
>>>>> fewer modules compared with a similar config of v5.15.8. Hard to say
>>>>> which other systems might miss things of course.
>>>>>
>>>>> I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
>>>>> wondering two things. For one it feels like that could be a separate
>>>>> change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
>>>>> Secondly I'm wondering about good ways of identifying such drivers and
>>>>> how much this overlaps with the ISA config flag.
>> I was interesting in the IOPORT_NATIVE flag (or whatever we call it) as
>> it solves the problem of drivers which "unconditionally do inb()/outb()
>> without checking the validity of the address using firmware or other
>> methods first" being built for (and loaded on and crashing) unsuitable
>> systems. Such a problem is in [0]
>>
>> So if we want to support that later, then it seems that someone would
>> need to go back and re-edit many same driver Kconfigs – like hwmon, for
>> example. I think it's better to avoid that and do it now.
> Could you do something where valid arguments to inb() have to come
> from some kernel mapping/validation function and are never in the
> range [0x0, 0x10000).
> Then drivers that are cheating the system will fail.

That sounds like the solution which I had here:

https://lore.kernel.org/lkml/1610729929-188490-2-git-send-email-john.garry@huawei.com/

It worked for the scenario I was interested in, but Arnd had some 
concerns, which you can check there.

> 
> Or, maybe, only allow [0x0, 0x10000) on systems that have a suitable bus.
> With the mapping functions returning a different value (eg the KVA into
> the PCI master window) that can be separately verified.
> That would let drivers do (say) inb(0x120) on systems that have (something
> like) and ISA bus, but not on PCI-only systems which support PCI IO
> accesses through a physical address window.

I'm not sure how this would look in practice. What would the check for 
the suitable bus be?

Thanks,
John

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

[John Garry]

On 20/12/2021 09:27, Niklas Schnelle wrote:
>>   > My feeling is that in this case we want some other dependency, e.g. a
>>   > new CONFIG_LPC. It should actually be possible to use this driver on
>>   > any machine with an LPC bus, which would by definition be the primary
>>   > I/O space, so it should be possible to load it on Arm64.
>>
>> You did suggest HARDCODED_IOPORT earlier in this thread, and the
>> definition/premise there seemed sensible to me.
>>
>> Anyway it seems practical to make all these changes in a single series,
>> so need a way forward as Niklas has no such changes for this additional
>> kconfig option.
>>
>> As a start, may I suggest we at least have Niklas' patch committed to a
>> dev branch based on -next or latest mainline release for further analysis?
>>
>> Thanks,
>> John
>>
>>
> My plan would be to split the patch up into more manageable pieces as
> suggested by Arnd plus of course fixes like the missing ARM select. As
> Arnd suggested I'll split the HAS_IOPORT additions into the initial
> introduction plus arch selects and then the HAS_IOPORT dependencies per
> subsytem. I think these per subsystem dependency patches then would be
> a great place to find drivers which should have a different dependency
> be it on LPC or a newly introduced HARDCODED_IOPORT. The thing is we
> can find and check HAS_IOPORT dependencies easily but it's hard to find
> HARDCODED_IOPORT so I think the lattter should be a refinement of the
> former. It can of course still go in as a single series. I'll
> definitely make the next iteration available as a git branch.

I'll do an audit for what would require HARDCODED_IOPORT to understand 
the scope while you can continue the work on your current path.

Thanks,
john

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

[Niklas Schnelle]

On Tue, 2021-12-21 at 16:48 +0000, John Garry wrote:
> On 20/12/2021 09:27, Niklas Schnelle wrote:
> > >   > My feeling is that in this case we want some other dependency, e.g. a
> > >   > new CONFIG_LPC. It should actually be possible to use this driver on
> > >   > any machine with an LPC bus, which would by definition be the primary
> > >   > I/O space, so it should be possible to load it on Arm64.
> > > 
> > > You did suggest HARDCODED_IOPORT earlier in this thread, and the
> > > definition/premise there seemed sensible to me.
> > > 
> > > Anyway it seems practical to make all these changes in a single series,
> > > so need a way forward as Niklas has no such changes for this additional
> > > kconfig option.
> > > 
> > > As a start, may I suggest we at least have Niklas' patch committed to a
> > > dev branch based on -next or latest mainline release for further analysis?
> > > 
> > > Thanks,
> > > John
> > > 
> > > 
> > My plan would be to split the patch up into more manageable pieces as
> > suggested by Arnd plus of course fixes like the missing ARM select. As
> > Arnd suggested I'll split the HAS_IOPORT additions into the initial
> > introduction plus arch selects and then the HAS_IOPORT dependencies per
> > subsytem. I think these per subsystem dependency patches then would be
> > a great place to find drivers which should have a different dependency
> > be it on LPC or a newly introduced HARDCODED_IOPORT. The thing is we
> > can find and check HAS_IOPORT dependencies easily but it's hard to find
> > HARDCODED_IOPORT so I think the lattter should be a refinement of the
> > former. It can of course still go in as a single series. I'll
> > definitely make the next iteration available as a git branch.
> 
> I'll do an audit for what would require HARDCODED_IOPORT to understand 
> the scope while you can continue the work on your current path.
> 
> Thanks,
> john
> 

Sounds good, I'm open to adding such a config option given a clear
enough picture of what drivers it would affect. Meanwhile I've made
some progress splitting things up. I still need to do a bit more
testing and refining of comments before sending an RFC but if you're
curious you can check out the 'has_ioport' branch on my GitHub here:

https://github.com/niklas88/linux.git  (still figuring out if/how I can
get a proper git.kernel.org branch/repository).

Thanks,
Niklas