Re: [Regression] PCI resources allocation problem on HP nx6325

[Andrew Patterson <andrew.patterson@hp.com>]

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.

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