| Message ID | CAK8P3a2oZ-+qd3Nhpy9VVXCJB3DU5N-y-ta2JpP0t6NHh=GVXw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [GIT,PULL,1/2] asm-generic: rework PCI I/O space access | expand |
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
On 19/12/2021 14:23, David Laight wrote: >>>>> I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well >>>>> as running it with defconfig. I've also been using it on my Ryzen 3990X >>>>> workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB >>>>> fewer modules compared with a similar config of v5.15.8. Hard to say >>>>> which other systems might miss things of course. >>>>> >>>>> I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm >>>>> wondering two things. For one it feels like that could be a separate >>>>> change on top since HAS_IOPORT + LEGACY_PCI is already quite big. >>>>> Secondly I'm wondering about good ways of identifying such drivers and >>>>> how much this overlaps with the ISA config flag. >> I was interesting in the IOPORT_NATIVE flag (or whatever we call it) as >> it solves the problem of drivers which "unconditionally do inb()/outb() >> without checking the validity of the address using firmware or other >> methods first" being built for (and loaded on and crashing) unsuitable >> systems. Such a problem is in [0] >> >> So if we want to support that later, then it seems that someone would >> need to go back and re-edit many same driver Kconfigs – like hwmon, for >> example. I think it's better to avoid that and do it now. > Could you do something where valid arguments to inb() have to come > from some kernel mapping/validation function and are never in the > range [0x0, 0x10000). > Then drivers that are cheating the system will fail. That sounds like the solution which I had here: https://lore.kernel.org/lkml/1610729929-188490-2-git-send-email-john.garry@huawei.com/ It worked for the scenario I was interested in, but Arnd had some concerns, which you can check there. > > Or, maybe, only allow [0x0, 0x10000) on systems that have a suitable bus. > With the mapping functions returning a different value (eg the KVA into > the PCI master window) that can be separately verified. > That would let drivers do (say) inb(0x120) on systems that have (something > like) and ISA bus, but not on PCI-only systems which support PCI IO > accesses through a physical address window. I'm not sure how this would look in practice. What would the check for the suitable bus be? Thanks, John
On 20/12/2021 09:27, Niklas Schnelle wrote: >> > My feeling is that in this case we want some other dependency, e.g. a >> > new CONFIG_LPC. It should actually be possible to use this driver on >> > any machine with an LPC bus, which would by definition be the primary >> > I/O space, so it should be possible to load it on Arm64. >> >> You did suggest HARDCODED_IOPORT earlier in this thread, and the >> definition/premise there seemed sensible to me. >> >> Anyway it seems practical to make all these changes in a single series, >> so need a way forward as Niklas has no such changes for this additional >> kconfig option. >> >> As a start, may I suggest we at least have Niklas' patch committed to a >> dev branch based on -next or latest mainline release for further analysis? >> >> Thanks, >> John >> >> > My plan would be to split the patch up into more manageable pieces as > suggested by Arnd plus of course fixes like the missing ARM select. As > Arnd suggested I'll split the HAS_IOPORT additions into the initial > introduction plus arch selects and then the HAS_IOPORT dependencies per > subsytem. I think these per subsystem dependency patches then would be > a great place to find drivers which should have a different dependency > be it on LPC or a newly introduced HARDCODED_IOPORT. The thing is we > can find and check HAS_IOPORT dependencies easily but it's hard to find > HARDCODED_IOPORT so I think the lattter should be a refinement of the > former. It can of course still go in as a single series. I'll > definitely make the next iteration available as a git branch. I'll do an audit for what would require HARDCODED_IOPORT to understand the scope while you can continue the work on your current path. Thanks, john
On Tue, 2021-12-21 at 16:48 +0000, John Garry wrote: > On 20/12/2021 09:27, Niklas Schnelle wrote: > > > > My feeling is that in this case we want some other dependency, e.g. a > > > > new CONFIG_LPC. It should actually be possible to use this driver on > > > > any machine with an LPC bus, which would by definition be the primary > > > > I/O space, so it should be possible to load it on Arm64. > > > > > > You did suggest HARDCODED_IOPORT earlier in this thread, and the > > > definition/premise there seemed sensible to me. > > > > > > Anyway it seems practical to make all these changes in a single series, > > > so need a way forward as Niklas has no such changes for this additional > > > kconfig option. > > > > > > As a start, may I suggest we at least have Niklas' patch committed to a > > > dev branch based on -next or latest mainline release for further analysis? > > > > > > Thanks, > > > John > > > > > > > > My plan would be to split the patch up into more manageable pieces as > > suggested by Arnd plus of course fixes like the missing ARM select. As > > Arnd suggested I'll split the HAS_IOPORT additions into the initial > > introduction plus arch selects and then the HAS_IOPORT dependencies per > > subsytem. I think these per subsystem dependency patches then would be > > a great place to find drivers which should have a different dependency > > be it on LPC or a newly introduced HARDCODED_IOPORT. The thing is we > > can find and check HAS_IOPORT dependencies easily but it's hard to find > > HARDCODED_IOPORT so I think the lattter should be a refinement of the > > former. It can of course still go in as a single series. I'll > > definitely make the next iteration available as a git branch. > > I'll do an audit for what would require HARDCODED_IOPORT to understand > the scope while you can continue the work on your current path. > > Thanks, > john > Sounds good, I'm open to adding such a config option given a clear enough picture of what drivers it would affect. Meanwhile I've made some progress splitting things up. I still need to do a bit more testing and refining of comments before sending an RFC but if you're curious you can check out the 'has_ioport' branch on my GitHub here: https://github.com/niklas88/linux.git (still figuring out if/how I can get a proper git.kernel.org branch/repository). Thanks, Niklas
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(-)