Re: [PATCH] pci: only release that resource index is less than 3

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Mon, 26 Oct 2009 10:32:57 -0600
Bjorn Helgaas <bjorn.helgaas@hp.com> 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@kernel.org>
> > 
> > ---
> >  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