On Sun, 2009-08-02 at 21:10 -0600, Andrew Patterson wrote: > On Sun, 2009-08-02 at 09:39 -0700, Linus Torvalds wrote: > > > > On Sun, 2 Aug 2009, Rafael J. Wysocki wrote: > > > > > > Hi Matthew, > > > > > > As reported at > > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=13891 > > > > > > there is a problem with allocating PCI resources on HP nx6325 introduced by > > > your commit a76117dfd687ec4be0a9a05214f3009cc5f73a42 > > > (x86: Use pci_claim_resource). > > > > Ooh, interesting. I thought that patch was a functionally equivalent > > cleanup of > > > > pr = pci_find_parent_resource(dev, r); > > if (!pr || request_resource(pr, r) < 0) { > > > > to > > > > if (pci_claim_resource(dev, idx) < 0) { > > > > but yeah, it's not exactly the same. pci_claim_resource() uses > > 'insert_resource()' rather than 'request_resource()'. > > > > We could certainly revert the commit, but I also wonder whether we should > > just change 'pci_claim_resource()' to use request_resource() instead. > > > > I _think_ the use of "insert_resource()" is purely historical, and is > > because that broken function _used_ to not look up the parent, but instead > > do that crazy "pcibios_select_root()" thing, and then it really does need > > to recurse down and "insert" the resource in the right place. > > > > We should no longer _need_ to do the "insert_resource()" thing, since we > > are inserting it into the exact parent that we want (as of commit > > cebd78a8c: "Fix pci_claim_resource"). > > > > And if that "insert_resource()" in pci_claim_resource() ever does anything > > fancier than the raw "request_resource()", then that's a problem anyway. > > > > Willy, comments? x86 historically has never used pci_claim_resource() at > > all (it always open-coded the above) except for some quirk handling. So > > I'm pretty sure that a patch like the below should be safe and correct. > > But it's parisc machines that always seem to break. > > > > Added Andrew Patterson to the Cc, because his report was what caused us to > > originally look at pci_claim_resource() and make it use > > "pci_find_parent_resource()". We just never went whole hog, and we left > > that broken "insert_resource()" around. > > > > So Rafael and AndrewP, does this work for you? (I also moved the "dtype" > > thing around, it bothered me). > > It works fine for me on the original hardware where the problem was > reported. I don't see any change between iomem/ioport layout between the > kernel without this patch and the kernel with this patch. I don't have > the problem card I had in the same slot, so I would like to move it and > run another test. > This change also works with the original "problem" card. I don't see any issue with the serial port, but I have only tested on an rx6600. > Andrew > > > > > Linus > > > > --- > > drivers/pci/setup-res.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c > > index b711fb7..1898c7b 100644 > > --- a/drivers/pci/setup-res.c > > +++ b/drivers/pci/setup-res.c > > @@ -100,16 +100,16 @@ int pci_claim_resource(struct pci_dev *dev, int resource) > > { > > struct resource *res = &dev->resource[resource]; > > struct resource *root; > > - char *dtype = resource < PCI_BRIDGE_RESOURCES ? "device" : "bridge"; > > int err; > > > > root = pci_find_parent_resource(dev, res); > > > > err = -EINVAL; > > if (root != NULL) > > - err = insert_resource(root, res); > > + err = request_resource(root, res); > > > > if (err) { > > + const char *dtype = resource < PCI_BRIDGE_RESOURCES ? "device" : "bridge"; > > dev_err(&dev->dev, "BAR %d: %s of %s %pR\n", > > resource, > > root ? "address space collision on" : > > -- Andrew Patterson Hewlett-Packard -- 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
