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
--
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
