All of lore.kernel.org
 help / color / mirror / Atom feed
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: linux-pci <linux-pci@vger.kernel.org>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	Richard Henderson <rth@twiddle.net>,
	Matt Turner <mattst88@gmail.com>
Subject: IORESOURCE_PCI_FIXES & __pci_bus_assign_resources()
Date: Mon, 17 Jun 2019 18:35:16 +1000	[thread overview]
Message-ID: <bd5144fc12eeb611a85d194bbccdcac577fc6084.camel@kernel.crashing.org> (raw)

Hi !

While working on consolidating resource assignment, I stumbled upon
something alpha does (and maybe others I haven't spotted yet):

On most platforms (not all), it uses the usual pair:

	pci_bus_size_bridges(bus);
	pci_bus_assign_resources(bus);

to reassign everything.

However, before doing so, it first calls pcibios_claim_one_bus() which,
on those platforms (ie those who don't set PCI_PROBE_ONLY), will
effectively claim only resources that have IORESOURCE_PCI_FIXED.

Now, let's leave alone for now the fact that this is will really only
work if those resources aren't behind a bridge as, to the best of my
undertanding of the code at this point, we aren't going to take them
into account when sizing & locating bridges. But that's not my point
right now :-)

From what I can tell, these days, pci_bus_assign_resources() will
already claim those fixed resources via pdev_assign_fixed_resources().

However, it does so *after* it has assigned and claimed resoures for
all the sibling devices on that bus.

That looks wrong to me. Shouldn't we claim the fixed resources first ?
IE something like:

--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1345,11 +1345,12 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
 	struct pci_bus *b;
 	struct pci_dev *dev;
 
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		pdev_assign_fixed_resources(dev);
+
 	pbus_assign_resources_sorted(bus, realloc_head, fail_head);
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
-		pdev_assign_fixed_resources(dev);
-
 		b = dev->subordinate;
 		if (!b)
 			continue;
?

Now, I suspect most of the time it happens to work due to the fact that
the fixed resources are generally IO resources in the legacy low ranges
( < 0x1000) and PCIBIOS_MIN_IO is *generally* set to 0x1000 but it still
sounds fishy to me.

I don't think I have HW at hand with that type of fixed stuff to test
with at the moment, so this is very much academic right now but I worry
that when I convert archs such as alpha who does that claiming before
the rest of the assignment, switching doing things the other way around
will break.

Any thoughts ?

Cheers,
Ben.
 


                 reply	other threads:[~2019-06-17  8:35 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

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=bd5144fc12eeb611a85d194bbccdcac577fc6084.camel@kernel.crashing.org \
    --to=benh@kernel.crashing.org \
    --cc=helgaas@kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mattst88@gmail.com \
    --cc=rth@twiddle.net \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.