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

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

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