RE: maxcpus=1 broken, ACPI bug?

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Linus Torvalds [mailto:torvalds@xxxxxxxx] 
>Sent: Thursday, November 17, 2005 11:55 AM
>To: Maneesh Soni
>Cc: LKML; Pallipadi, Venkatesh; Brown, Len; Andrew Morton
>Subject: Re: maxcpus=1 broken, ACPI bug?
>
>
>
>On Thu, 17 Nov 2005, Maneesh Soni wrote:
>> 
>> Using maxcpus=1 boot option, hangs the system while booting. It was
>> working till 2.6.13-rc2. After git bisect I found that after backing
>> out this ACPI patch it works again, though I had to manually sort the
>> reject while backing out.
>> 
>> 
>http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.
6.git;a=commitdiff;h=acf05f4b7f558051ea0028e8e617144123650272
>
>Hmm. That patch had a totally idiotic thinko in it (look at 
>the for-loop 
>in acpi_processor_get_power_info_default() and notice how it doesn't 
>actually change anything in the loop).
>
>That thinko was later fixed (albeit in a really stupid way, 
>and the same 
>cut-and-paste bug still exists in 
>acpi_processor_get_power_info_fadt()).
>

Oops. I had missed the memset in ..info_fadt() in my stupid bug fixing
patch.

There is another patch here http://bugme.osdl.org/show_bug.cgi?id=4485
That touches the same code and doing memset in a loop turned out to be
useful
as we want to initialize only some of those states.

>Anyway, can you test this diff? It
>
> (a) removes the insane (and in one case incorrect) memset loop
> (b) makes the code that sets "pr->flags.power = 1" match the 
>comment and 
>     the previous behaviour.

Regarding the last hunk in the patch below..
Actually, we want to have acpi_processor_idle() used even when only C1
is supported and that C1 has nothing to do with ACPI. The reason being
that there is no generic interface that can show the cstate usage and
time spent in cstate etc. We want to reuse the cstate statistics in
/proc interface in acpi_processor_idle(). However, in long term, this
needs a cleanup and a generic cstate handler is required (similar to
cpufreq for P-state) and acpi should just plugin the handler and latency
and similar details for each cstate into this generic cstate handler.

And I tried on couple of Xeons here and haven't had luck in reproducing
this hang yet. 

Thanks,
Venki

>
>Does that make a difference?
>
>		Linus
>
>---
>diff --git a/drivers/acpi/processor_idle.c 
>b/drivers/acpi/processor_idle.c
>index 573b6a9..2445828 100644
>--- a/drivers/acpi/processor_idle.c
>+++ b/drivers/acpi/processor_idle.c
>@@ -524,8 +524,7 @@ static int acpi_processor_get_power_info
> 	if (!pr->pblk)
> 		return_VALUE(-ENODEV);
> 
>-	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
>-		memset(pr->power.states, 0, sizeof(struct 
>acpi_processor_cx));
>+	memset(pr->power.states, 0, sizeof(pr->power.states));
> 
> 	/* if info is obtained from pblk/fadt, type equals state */
> 	pr->power.states[ACPI_STATE_C1].type = ACPI_STATE_C1;
>@@ -559,9 +558,7 @@ static int acpi_processor_get_power_info
> 
> 	ACPI_FUNCTION_TRACE("acpi_processor_get_power_info_default_c1");
> 
>-	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
>-		memset(&(pr->power.states[i]), 0,
>-		       sizeof(struct acpi_processor_cx));
>+	memset(pr->power.states, 0, sizeof(pr->power.states));
> 
> 	/* if info is obtained from pblk/fadt, type equals state */
> 	pr->power.states[ACPI_STATE_C1].type = ACPI_STATE_C1;
>@@ -873,7 +870,8 @@ static int acpi_processor_get_power_info
> 	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> 		if (pr->power.states[i].valid) {
> 			pr->power.count = i;
>-			pr->flags.power = 1;
>+			if (pr->power.states[i].type >= ACPI_STATE_C2)
>+				pr->flags.power = 1;
> 		}
> 	}
> 
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/