On Mon, 26 Oct 2009 10:32:57 -0600
Bjorn Helgaas <bjorn.helgaas@xxxxxx> wrote:
> On Saturday 24 October 2009 03:25:59 am Yinghai Lu wrote:
> > after
> >
> > | commit 308cf8e13f42f476dfd6552aeff58fdc0788e566
> > |
> > | PCI: get larger bridge ranges when space is available
> >
> > found one of resource of peer root bus (0x00) get released from root
> > resource. later one hotplug device can not get big range anymore.
> > other peer root buses is ok.
> >
> > it turns out it is from transparent path.
> >
> > those resources will be used for pci bridge BAR updated.
> > so need to limit it to 3.
> >
> > Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> >
> > ---
> > drivers/pci/setup-bus.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > Index: linux-2.6/drivers/pci/setup-bus.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/setup-bus.c
> > +++ linux-2.6/drivers/pci/setup-bus.c
> > @@ -344,9 +344,14 @@ static struct resource *find_free_bus_re
> > * if there is no child under that, we
> > should release
> > * and use it. don't need to reset it,
> > pbus_size_* will
> > * set it again
> > + * need to be less 3, otherwise can not
> > write it to
> > + * bridge, also need to avoid releasing it
> > from
> > + * transparent bus path
> > */
> > - if (!r->child && !release_resource(r))
> > - return r;
> > + if (i < 3 && !r->child) {
> > + if (!release_resource(r))
> > + return r;
> > + }
>
> I am bewildered.
>
> 308cf8e13f42f added the release_resource() call here in
> find_free_bus_resource(). I don't understand why the release
> should be there in the first place -- it doesn't seem to
> logically fit there. A "release" should be connected to an
> event, maybe a hot-remove or a move of a device from one place
> to another. It shouldn't be something we do as a side-effect
> of searching for a free resource.
>
> Now you're adding the magic number "3", which seems even less
> related to the job of "finding an available bus resource." I'm
> guessing the "3" is related to PCI_BRIDGE_RESOURCES or something,
> but that should be made explicit, and I really don't think it
> belongs in this function.
I agree, the 3 is a bit of magic. The free could probably be shuffled
around a bit too, though it really is related to finding bus
resources. I was a bit ambivalent about putting it in
find_free_bus_resources but when I started a reply to Yinghai and
looked for a better place it made some sense to leave it as-is.
But if we're going to be adding more logic here, we should probably
figure out a better way of doing it, maybe by adding a new pass to do
the freeing? I know we've avoided modifying bus resources too much in
the past, but I think that'll be harder and harder to avoid as Windows
moves to that model and platforms begin to expect it.
--
Jesse Barnes, Intel Open Source Technology Center
--
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
