Hi Thomas, On Mon, Sep 24, 2018 at 02:12:03PM +0200, Thomas Petazzoni wrote: > Hello, > > On Mon, 24 Sep 2018 12:13:41 +0100, Russell King - ARM Linux wrote: > > > > But being able to unmap it would also be needed to be able to remove > > > PCI host controller drivers, and therefore compile them as module, and > > > make them more like any other drivers. > > > > > > I'm not sure why we need to guarantee that the I/O space is always > > > mapped: > > > > > > - It isn't mapped before the PCI controller driver does the mapping. > > > > > > - There is no reason for it to be accessed when the PCI controller > > > driver is not initialized: PCI devices can only be probed and > > > initialized when the PCI controller driver is probed/initialized. > > > > There are historic reasons. PCI provides ISA IO space, and when you > > have a machine with ISA peripherals present, the PCI IO space must > > never be unmapped - if it is, ISA drivers will oops the kernel. There > > is no way for a vanishing PCI controller to cause ISA drivers to be > > unbound. > > > > If you have a host controller that does unmap PCI IO space and you have > > ISA peripherals with drivers present, unbinding the PCI host controller > > will remove the IO space mapping, and next time an ISA peripheral > > touches IO space, the kernel will oops. > > Thanks for sharing some additional technical context on this, very > useful. > > I have another question though: shouldn't those ISA devices be child > devices of the PCI controller, if they use some resources of the PCI > controller ? Could you give an example of such an ISA device driver ? Legacy VGA also falls into this category - for example drivers/video/console/vgacon.c will happily use outb/inb macros to hard coded addresses which are hoped to be present on some PCI/ISA bus. With regards to ISA drivers - take a look for anything that registers with isa_register_driver - for example: drivers/input/touchscreen/htcpen.c drivers/net/ethernet/3com/3c509.c drivers/watchdog/ebc-c384_wdt.c None of these drivers do any kind of mapping before attempting to access these addresses. Thanks, Andrew Murray > This is just to understand better the issue, because there seems to be > a kind of hidden dependency between those ISA drivers and the setup of > the PCI controller. > > > > All other drivers, including on ARM, use pci_remap_iospace(), which > > > does provide the pci_unmap_iospace() counter part. > > > > ... which has been created in PCI land just to deal with PCI without > > regard for the above issue. > > > > However, there's another issue I missed - if you _do_ have ISA > > peripherals, you likely want the IO space setup from very early on, > > and you won't be using the new fangled PCI host driver support anyway. > > That uses pci_map_io_early() rather than pci_ioremap_io() or > > pci_remap_io(). > > OK. There's today a single platform (Footbridge) that uses > pci_map_io_early(), and it is indeed called through the ->map_io() > hook, which is very early in the boot process. > > BTW, look at drivers/pcmcia/at91_cf.c. It has ->probe() and ->remove(), > and does a pci_ioremap_io() in its ->probe(), and nothing in its > ->remove(). I don't think this driver, compiled as a module, will work > well after a insmod/rmmod/insmod cycle. > > > > But to me, the general direction is that the ARM-specific > > > pci_remap_io() API is fading away, and its replacement already provides > > > an unmapping capability. So why not add the same unmapping capability > > > to pci_remap_io() ? > > > > Yes, that would be a good longer term plan - we don't need three > > different ways to map PCI IO space, but it is development. > > Absolutely. Glad to hear that you agree on the longer term plan. > > > > But we have a regression and we need to fix it. Do you suggest to not > > > use the new pci_host_probe() API ? > > > > Well, arguably, the patch that caused the regression is the buggy patch, > > _not_ the lack of unmapping API for pci_ioremap_io(). > > Totally true. > > > Trying to address a regression with further development means that > > _that_ development needs thought and review, which is a slower > > process. > > > > I do understand the desire to keep moving forward and never take a > > step backwards, but sometimes backwards steps are the best way to > > resolve a regression. But I also do appreciate that a simple revert > > in this case is not possible. > > Well, I can revert: > > 42342073e38b50113354944cd51dcfed28d857a1 PCI: mvebu: Convert to use > pci_host_bridge directly ee1604381a371b3ea6aec7d5e43b6e3f5e153854 PCI: > mvebu: Only remap I/O space if configured > > so it's not a big deal either. I can revert those, and then resubmit a > more complete series later on that moves pci-mvebu to use > pci_remap_iospace(). > > > I'll accept your patch on the condition that the ARM private > > pci_ioremap_io() will go away in the very near future (please _try_ > > to get agreement on that before this patch is merged.) > > Bjorn, Lorenzo, what do you prefer ? > > If we want to get rid of pci_ioremap_io(), then we need a way to tell > pci_remap_iospace() the memory attributes that should be used for the > mapping, because on Armada 38x, we need to map the I/O space mapped > MT_UNCACHED instead of MT_DEVICE. I'm not sure how to achieve this yet. > Should pgprot_device() be changed to return MT_UNCACHED on a > platform-specific basis ? Any other idea ? > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
