Mailing List Archive

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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)

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