On 06/03/2015 03:32 AM, Lorenzo Pieralisi wrote:
On Tue, Jun 02, 2015 at 06:02:49PM +0100, Guenter Roeck wrote:
On 06/02/2015 07:55 AM, Lorenzo Pieralisi wrote:
Bjorn, Guenter,
On Wed, May 27, 2015 at 10:04:47PM +0100, Bjorn Helgaas wrote:
[+cc Lorenzo, Suravee, Will]
I cc'd Lorenzo, Suravee, and Will because Lorenzo is working on calling
pci_read_bases() from the PCI core instead of from arch code, and there are
likely some dependencies between these two things.
On Fri, May 22, 2015 at 05:52:16PM -0700, Guenter Roeck wrote:
The PCI subsystem always assumes that I/O is supported on PCIe bridges
and tries to assign an I/O window to each port even if that is not
the case.
This may result in messages such as
pcieport 0000:02:00.0: res[7]=[io 0x1000-0x0fff]
get_res_add_size add_size 1000
pcieport 0000:02:00.0: BAR 7: no space for [io size 0x1000]
pcieport 0000:02:00.0: BAR 7: failed to assign [io size 0x1000]
for each bridge port, even if a port or its parent does not support
I/O in the first place.
To avoid this message, check if a port supports I/O before trying to
enable it. Also check if port's parent supports I/O, and only modify
a port's I/O resource size if both the port and its parent support I/O.
If IO is disabled after the initial port scan, the IO base and size
registers are set to 0x00f0 to indicate that IO is disabled. A later
rescan interprets this as "IO supported" and enables the IO range,
even if the parent does not support IO. Handle this situation as well.
Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
drivers/pci/probe.c | 14 ++++++++++++++
drivers/pci/setup-bus.c | 4 ++--
include/linux/pci.h | 9 +++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6675a7a1b9fc..f4944ef45148 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -354,6 +354,20 @@ static void pci_read_bridge_io(struct pci_bus *child)
base = (io_base_lo & io_mask) << 8;
limit = (io_limit_lo & io_mask) << 8;
+ /* If necessary, check if the bridge supports an I/O aperture */
+ if (!io_base_lo && !io_limit_lo) {
+ u16 io;
+
+ if (!pci_parent_supports_io(child))
+ return;
+
+ pci_write_config_word(dev, PCI_IO_BASE, 0xe0f0);
+ pci_read_config_word(dev, PCI_IO_BASE, &io);
+ pci_write_config_word(dev, PCI_IO_BASE, 0x0);
+ if (!io)
+ return;
+ }
I really like the idea of pushing this into pci_read_bridge_io().
I wonder if we can do the same with pci_read_bridge_mmio_pref(), and
somehow get rid of pci_bridge_check_ranges() altogether?
I think I looked at doing that a while back, and it seems like there was
some wrinkle, but I don't remember what it was.
After looking into this some more, I think the wrinkle may be that
pci_read_bridge_bases() and thus pci_read_bridge_io() isn't called
on probe-only systems (if PCI_PROBE_ONLY is set). A secondary
That's what we would like to change :)
https://lkml.org/lkml/2015/5/21/359
Yes, that should help. I had a brief look last night and concluded
that this would require changes all over the place, which your patch
pretty much confirms. Glad that you are tackling it - changes all over
the place spell trouble and would probably require more time than I have
available to spend on the problem.
problem is that pci_read_bridge_io() does not enable a resource
if it is explicitly disabled (base > limit), but the subsequent call
to pci_bridge_check_ranges() unconditionally enables it.
Not really sure how to address this; my current code checks IO support
in both pci_read_bridge_io() and pci_bridge_check_ranges(). And since
pci_read_bridge_io() is not always called, I don't see how it might
be possible to get rid of pci_bridge_check_ranges(), or even the check
for IO support in pci_bridge_check_ranges().
While at it, do you think it is reasonable to also claim the bridge
windows (resources) in the respective pci_read_bridge_* calls ?
Is there a reason why we don't/can't do it ? I noticed that on
PROBE_ONLY systems on ARM/ARM64 at the moment we do not claim
the bridge apertures and this is not correct, see below:
[5.980127] pcieport 0000:00:02.1: can't enable device: BAR 8
[mem 0xbff00000 - 0xbfffffff] not claimed
[5.988056] pcieport: probe of 0000:00:02.1 failed with error -22
Is this when trying my patches or with the current upstream code ?
It is upstream code with a couple of ARM64 related patches not yet
merged. Still, it shows an issue that must be tackled.
It is not caused by your patches but it can be solved by them.
On PROBE_ONLY systems, all resources must be claimed (since they
are not reassigned, hence not claimed by the code that reassigns them),
otherwise we can't enable a device resources (ie pcibios_enable_device
calls pci_enable_resources that fails, since resources are not claimed).
That's why we are suggesting claiming the bridge apertures as soon
as they are read from the base registers, even on PROBE_ONLY systems.
I think that's the only approach Bjorn would accept, otherwise
we will have to fiddle with PROBE_ONLY on ARM64, and either avoid calling
pci_enable_resources or avoid checking if a resource is claimed in
pci_enable_resources, neither solution seems sane to me.
Looks like I'll need one of those arm64 systems at some point ;-).
Where is your patch in respect to acceptance ? Would it make sense to
merge it into my code and base my patch(es) on it, or do you expect
major changes which would make that difficult ?
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
