LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [GIT PULL 1/2] asm-generic: rework PCI I/O space access
@ 2021-07-02 13:47 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 19:42 ` [GIT PULL 1/2] asm-generic: rework PCI I/O space access Linus Torvalds
  0 siblings, 2 replies; 16+ messages in thread
From: Arnd Bergmann @ 2021-07-02 13:47 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-arch, linux-pci, Linux Kernel Mailing List, Niklas Schnelle

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(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [GIT PULL 2/2] asm-generic: Unify asm/unaligned.h around struct helper
  2021-07-02 13:47 [GIT PULL 1/2] asm-generic: rework PCI I/O space access Arnd Bergmann
@ 2021-07-02 13:49 ` 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
  1 sibling, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2021-07-02 13:49 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-arch, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  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 19:42 ` Linus Torvalds
  2021-07-03 12:12   ` Arnd Bergmann
  1 sibling, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2021-07-02 19:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arch, linux-pci, Linux Kernel Mailing List, Niklas Schnelle

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 2/2] asm-generic: Unify asm/unaligned.h around struct helper
  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
  0 siblings, 0 replies; 16+ messages in thread
From: pr-tracker-bot @ 2021-07-02 20:13 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Linus Torvalds, linux-arch, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  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
  2021-07-05 10:06     ` Arnd Bergmann
  2021-07-05 12:40     ` Niklas Schnelle
  0 siblings, 2 replies; 16+ messages in thread
From: Arnd Bergmann @ 2021-07-03 12:12 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-arch, linux-pci, Linux Kernel Mailing List, Niklas Schnelle

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-07-03 12:12   ` Arnd Bergmann
@ 2021-07-05 10:06     ` Arnd Bergmann
  2021-08-03  9:46       ` John Garry
  2021-07-05 12:40     ` Niklas Schnelle
  1 sibling, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2021-07-05 10:06 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle, John Garry

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(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-07-03 12:12   ` Arnd Bergmann
  2021-07-05 10:06     ` Arnd Bergmann
@ 2021-07-05 12:40     ` Niklas Schnelle
  1 sibling, 0 replies; 16+ messages in thread
From: Niklas Schnelle @ 2021-07-05 12:40 UTC (permalink / raw)
  To: Arnd Bergmann, Linus Torvalds
  Cc: linux-arch, linux-pci, Linux Kernel Mailing List

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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-07-05 10:06     ` Arnd Bergmann
@ 2021-08-03  9:46       ` John Garry
  2021-08-03 10:06         ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: John Garry @ 2021-08-03  9:46 UTC (permalink / raw)
  To: Arnd Bergmann, Linus Torvalds
  Cc: linux-arch, linux-pci, Linux Kernel Mailing List, Niklas Schnelle

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(-)
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-03  9:46       ` John Garry
@ 2021-08-03 10:06         ` Arnd Bergmann
  2021-08-03 11:23           ` John Garry
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2021-08-03 10:06 UTC (permalink / raw)
  To: John Garry
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-03 10:06         ` Arnd Bergmann
@ 2021-08-03 11:23           ` John Garry
  2021-08-03 12:15             ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: John Garry @ 2021-08-03 11:23 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-03 11:23           ` John Garry
@ 2021-08-03 12:15             ` Arnd Bergmann
  2021-08-04  7:55               ` John Garry
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2021-08-03 12:15 UTC (permalink / raw)
  To: John Garry
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-03 12:15             ` Arnd Bergmann
@ 2021-08-04  7:55               ` John Garry
  2021-08-04  8:52                 ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: John Garry @ 2021-08-04  7:55 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-04  7:55               ` John Garry
@ 2021-08-04  8:52                 ` Arnd Bergmann
  2021-08-10  9:19                   ` John Garry
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2021-08-04  8:52 UTC (permalink / raw)
  To: John Garry
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-04  8:52                 ` Arnd Bergmann
@ 2021-08-10  9:19                   ` John Garry
  2021-08-10 11:33                     ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: John Garry @ 2021-08-10  9:19 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-10  9:19                   ` John Garry
@ 2021-08-10 11:33                     ` Arnd Bergmann
  2021-09-03  8:31                       ` Niklas Schnelle
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2021-08-10 11:33 UTC (permalink / raw)
  To: John Garry
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List,
	Niklas Schnelle

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
  2021-08-10 11:33                     ` Arnd Bergmann
@ 2021-09-03  8:31                       ` Niklas Schnelle
  0 siblings, 0 replies; 16+ messages in thread
From: Niklas Schnelle @ 2021-09-03  8:31 UTC (permalink / raw)
  To: Arnd Bergmann, John Garry
  Cc: Linus Torvalds, linux-arch, linux-pci, Linux Kernel Mailing List

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-09-03  8:31 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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-07-05 12:40     ` Niklas Schnelle

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