linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: thomas.petazzoni@bootlin.com (Thomas Petazzoni)
To: linux-arm-kernel@lists.infradead.org
Subject: [BISECTED] Regression: Solidrun Clearfog Base won't boot since "PCI: mvebu: Only remap I/O space if configured"
Date: Mon, 24 Sep 2018 14:12:03 +0200	[thread overview]
Message-ID: <20180924141203.3df9707a@windsurf> (raw)
In-Reply-To: <20180924111341.GP30658@n2100.armlinux.org.uk>

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

  reply	other threads:[~2018-09-24 12:12 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-12 16:11 [BISECTED] Regression: Solidrun Clearfog Base won't boot since "PCI: mvebu: Only remap I/O space if configured" Jan Kundrát
2018-09-12 18:49 ` Baruch Siach
2018-09-12 18:50   ` Thomas Petazzoni
2018-09-12 19:00     ` Jan Kundrát
2018-09-12 23:10   ` Russell King - ARM Linux
2018-09-13  3:19     ` Baruch Siach
2018-09-13  7:45     ` Thomas Petazzoni
2018-09-13  8:20       ` Jan Kundrát
2018-09-13  8:42         ` Thomas Petazzoni
2018-09-24 10:02           ` Jan Kundrát
2018-09-24 10:10             ` Thomas Petazzoni
2018-09-24 10:12           ` Russell King - ARM Linux
2018-09-24 10:26             ` Thomas Petazzoni
2018-09-24 11:13               ` Russell King - ARM Linux
2018-09-24 12:12                 ` Thomas Petazzoni [this message]
2018-09-24 12:46                   ` Lorenzo Pieralisi
2018-09-24 13:10                     ` Thomas Petazzoni
2018-09-24 14:15                       ` Lorenzo Pieralisi
2018-09-24 14:52                         ` Thomas Petazzoni
2018-09-24 16:42                           ` Lorenzo Pieralisi
2018-10-01 10:56                           ` Jan Kundrát
2018-10-01 12:51                             ` Thomas Petazzoni
2018-10-01 21:01                               ` Bjorn Helgaas
2018-09-25  8:18                   ` Andrew Murray

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180924141203.3df9707a@windsurf \
    --to=thomas.petazzoni@bootlin.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).