lkml.org 
[lkml]   [2009]   [Aug]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Regression] PCI resources allocation problem on HP nx6325
From
Date
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



\
 
 \ /
  Last update: 2009-08-03 23:15    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site