Thinkpad R60 hangs when booting recent 8.4-STABLE

[Don Lewis]

On  2 May, John Baldwin wrote:
> On Friday, May 02, 2014 5:39:19 am Don Lewis wrote:

>> This expression will overflow:
>> 		if (s->r_start + count - 1 > end) {
>> I don't understand the point of this test.  Is it an optimization to
>> exit the loop early based on an assumption about the ordering of
>> elements in the list?
> 
> Yes.  The elements in an rman list are supposed to be sorted.

I missed this comment earlier.

>> This expression will also overflow:
>> 			rstart = (rstart + amask) & ~amask;
>> which is where I think the start address is getting set to 0.
>> 
>> 
>> After reverting subr_rman.c and applying the following patch, I see:
>> considering [0xfec00000, 0xffffffff]
>> no unshared regions found
>> Then nexus_alloc_resource() gets called and I see
>> considering [0x3ff60000, 0xfebfffff]
>> and then all the right stuff happens.
>> 
>> 
>> Index: sys/kern/subr_rman.c
>> ===================================================================
>> --- sys/kern/subr_rman.c	(revision 262226)
>> +++ sys/kern/subr_rman.c	(working copy)
>> @@ -468,11 +468,9 @@
>>  	 */
>>  	for (s = r; s; s = TAILQ_NEXT(s, r_link)) {
>>  		DPRINTF(("considering [%#lx, %#lx]\n", s->r_start, s->r_end));
>> -		if (s->r_start + count - 1 > end) {
>> -			DPRINTF(("s->r_start (%#lx) + count - 1> end (%#lx)\n",
>> -			    s->r_start, end));
>> -			break;
>> -		}
>> +		if (s->r_end < start + count - 1 ||
>> +		    s->r_start > end - count + 1)
>> +			continue;
>>  		if (s->r_flags & RF_ALLOCATED) {
>>  			DPRINTF(("region is allocated\n"));
>>  			continue;
>> 
>> 
> 
> I think this looks good.

I just committed a variation of this fix which preserves the early exit
optimization to HEAD in r265363.  It also tweaks the intial start of
search test so that the s->r_start > end - count + 1 test above is not
needed.

I'm also considering some changes to the second for loop, but I've got
some questions about the (s->r_flags & flags) != flags test:

	Are there any cases where we get a false match because a bit
	is set in s->r_flags that is not set in flags?

	Should there be a KASSERT to check that the bits passed in flags
	are valid?

	Should the RF_ALIGNMENT_MASK bits participate in this
        comparision?  If so, I think that the comparison should
        either be strict equality or should be a magnitude comparison
        and not a bitwise comparison.

	If the RF_ALIGNMENT_MASK bits should be compared, then the
	fact that they are not preserved could be a problem.  Just a
	few lines below:
                rv->r_flags = s->r_flags &
                        (RF_ALLOCATED | RF_SHAREABLE | RF_TIMESHARE);

	If the RF_ALIGNMENT_MASK bits are equal, then the
	(s->r_start & amask) == 0 test would be redundant.