Re: [PATCH 3/3] x86,pci,acpi: Handle invalid _CRS

["H. Peter Anvin" <hpa@zytor.com>]

On 04/21/2010 04:04 PM, Bjorn Helgaas wrote:
> On Wednesday 21 April 2010 04:33:28 pm H. Peter Anvin wrote:
>> Do you have opinions on patches 1-2 of the series?
>>
>> I'm getting concerned about how the size of the patchset has grown, and
>> we're past -rc5 already... but it is a regression so we can't just defer
>> it to .35.
> 
> Part 1: the essential part of this seems to be the trim_bios_range()
> change, and that part is not too big.  In v4, Yinghai also removed
> probe_roms_32.c.  That sounds like the right thing to do, but I'd
> rather have that in separate patch so it doesn't obfuscate the other
> change, and I don't know whether it *has* to be done for .34; maybe
> it could be deferred.

I would agree with that.

> Part 2: IMHO, we're putting way too much crap in kernel/resource.c.
> A name like "reserve_region_with_split_check_child()" is a pretty
> good clue that we've lost our way somewhere.  But that's mostly a
> cosmetic thing, and the end result does seem to be something that
> fixes the current regression.

It's not just a good clue we have lost our way, it's also completely
impossible for anyone but Yinghai to divine what the intended semantics
are supposed to be.  This *greatly* concerns me, especially given
previous track record.

Even the checkin comment is almost unparsable, which makes it very
likely that someone is going to trip up on some of this in the future.
I really would like to get a better description.

The use of a string match in:

+       if (check_child && !conflict->child && strstr(conflict->name,
"PCI Bus"))
^^^^^^^^^

... screams "wrong! ugly! bad!" in my opinion.  I utterly fail to see
how that could be acceptable under any circumstances.  I thought that
had been flagged earlier in the conversation, but it is apparently still
there.

> So I guess that in spite of my issues with the implementation, I'm
> still OK with the concept.

OK, but what is "the implementation" and what is "the concept" here?

	-hap