Message-ID: <20150603103235.GC27917@red-moon>
Date:	Wed, 3 Jun 2015 11:32:35 +0100
From:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
	Will Deacon <Will.Deacon@....com>
Subject: Re: [PATCH] PCI: Only enable IO window if supported

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@...ck-us.net>
> >>> ---
> >>>   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

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

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists