lkml.org 
[lkml]   [2009]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/12] pci: add pci_bridge_release_unused_res and pci_bus_release_unused_bridge_res -v2


On Fri, 18 Dec 2009, Yinghai Lu wrote:
>
> so later we could use it to release small resource before pci assign unassign res

However, I think this one is wrong.

> +static void release_child_resources(struct resource *r)
> +{
> + struct resource *p;
> + resource_size_t size;
> +
> + p = r->child;
> + while (p) {
> + release_child_resources(p);
> + release_resource(p);

So not only is this releasing resources that aren't necessarily PCI
devices, it's releasing the whole tree - regardless of how they were
allocated and initialized. That makes me nervous to begin with. It's in
the wrong file.

But the locking is crap too!

You need to hold the resource lock for the whole operation - you can't
just walk the resource tree and release them.

And once you do that, then using "release_resrouce()" is the wrong thing,
since it turns into just "__release_resource()" and you notice that that
walks the chain looking for them - which makes it pointless to have
_another_ outer loop that walks the chain to release them!

So you'd need to

- move this to kernel/resource.c

- do it all under 'write_lock(&resource_lock);'

- stop the silly double list loop, and just do it as a single loop that
does

p = old->parent->child;
old->parent = NULL;
while (p) {
struct resource *tmp = p;
p = p->sibling;
.. do whatever you do to free tmp ..
}
and it's much simpler, more efficient, has the rigth locking, and is in
the right place.

That said, it's still unclear if you can ever do this! Why would the PCI
layer be allowed to release ACPI resources int he tree, for example?

So I can see fixing the _implementation_ issues I have like above, but I'd
still be nervous about the whole concept of the patch..

Linus


\
 
 \ /
  Last update: 2009-12-18 22:27    [from the cache]
©2003-2011 Jasper Spaans