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    [W:0.027 / U:0.348 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site