by Jesse Barnes

[permalink] [raw]

Subject: Re: PCI BAR mem resource allocation "regression"

On Monday, December 15, 2008 4:25 pm Linus Torvalds wrote:> On Mon, 15 Dec 2008, Jesse Barnes wrote:> > I can put your patch in my -next branch if you think it would help (not> > that - next gets a ton of testing, but hey)...>> Better than nothing, I guess.>> We may end up back-porting it (since Chen confirms that it's not just a> warning, it actually affects functionality), but let's get it some testing> in -next first and then eventually in the merge window. So you might mark> it for attention for '[email protected]' as you commit it.
I pushed it to my -next branch, though I forgot to add the stable@ cc and fix up the comments while I was there... I think I'll have to rebase anyway, so I'll take care of both of those when I do.
Thanks,-- Jesse Barnes, Intel Open Source Technology Center
commit d4e9908a08199fe4d728c68cc7c5ca2d827ae99fAuthor: Linus Torvalds <[email protected]>Date: Sat Dec 13 17:37:57 2008 -0800
resource: revert parent nesting of identical ranges Revert the change that causes inverted resource nesting in the case of identical resource ranges. Initially introduced by d33b6fba2c4350651f3f61ff2ab858a2f116e9a4, this change was provided without a justification and seems to cause problems on some machines in the face of PCI hot plugging. The patch makes resources registered against an existing but identical range nest as children of the existing resource, rather than parents. Reported-by: Alex Chiang <[email protected]> Tested-by: Alex Chiang <[email protected]> Tested-by: Justin Chen <[email protected]> Signed-off-by: Linus Torvalds <[email protected]> Signed-off-by: Jesse Barnes <[email protected]>
diff --git a/kernel/resource.c b/kernel/resource.cindex c251b0f..741357f 100644--- a/kernel/resource.c+++ b/kernel/resource.c@@ -381,8 +381,6 @@ static struct resource * __insert_resource(struct resource *parent, struct resou if ((first->start > new->start) || (first->end < new->end)) break;- if ((first->start == new->start) && (first->end == new->end))- break; } for (next = first; ; next = next->sibling) {????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2008-12-17 04:06:04

by Matthew Wilcox

[permalink] [raw]

Subject: Re: PCI BAR mem resource allocation "regression"

On Sat, Dec 13, 2008 at 09:41:05AM -0800, Linus Torvalds wrote:
> Matthew: I do suspect that the "insert below" patch is wrong. Look at
> "pci_claim_resource()", for example: it uses insert_resource() to insert
> the PCI device resource into the resource tree. But if the PCI device
> resource has the same size as the bus window, that commit changes it to
> insert it as the _parent_ of the bus window, no?

You're quite correct. This was actually the intended behaviour of
insert_resource() ... the problem is that pci_claim_resource() is using
it wrong.

> Of course, on a PC, the only user of pci_claim_resource() would be some
> PCI quirks that should never trigger this, but it's an example of the kind
> of behavioural change that that commit introduced, and which looks
> like it could result in a wrong resource tree.
>
> I'm not finding the original discussion that resulted in that patch,
> though, so I have a somewhat hard time to judge the reasoning. It's from
> almost three years ago, I don't remember details even if I had been
> involved with it (and judging by the sign-off path, I hadn't).
>
> Matthew, do you remember the context of that commmit d33b6fba2?

Basically it was that we came across a machine with the opposite problem
-- that we found a parent after we found a child (and claimed the
child's resources), and had no way to insert the parent's region above
the child's region. Alex's machine finds the child after the parent and
needs to insert the child's resource inside the parent's resource.

pci_claim_resource() should be looking through the resources that belong
to the parent bus, trying to insert inside each of them, not just
taking the root resource and wandering all the way down. I'll work on a
patch tomorrow to do that unless someone beats me to it.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."