kern/108581: [sysctl] sysctl: hw.acpi.cpu.cx_lowest: Invalid argument

[Andriy Gapon]

on 26/03/2009 17:29 John Baldwin said the following:
> On Thursday 26 March 2009 11:15:39 am Andriy Gapon wrote:
>> on 26/03/2009 17:10 Bruce Cran said the following:
>>> On Thu, 26 Mar 2009 17:04:19 +0200
>>> Andriy Gapon <avg at icyb.net.ua> wrote:
>>>
>>>> on 26/03/2009 16:41 Bruce Cran said the following:
>>>>> I added lots of printfs to acpi_cpu.c and found that it's occuring
>>>>> in acpi_cpu_startup; initializing it to 3 in that function (which I
>>>>> wrongly assumed was the lowest Cx state supported in ACPI) fixed
>>>>> the problem on my Athlon XP PC because the generic cx handling code
>>>>> then lowered cpu_cx_count to 1 based on the fact that
>>>>> sc->cpu_cx_count was also 1.
>>>>>
>>>> Ok, yes, the real issue is in acpi_cpu_generic_cx_probe, namely in
>>>> early exits from it. So,  sc->cpu_cx_count is always set to at least
>>>> 1, but if we exit via one of the returns before the end of function,
>>>> then global cpu_cx_count is never updated.
>>>>
>>> Exactly:
>>>
>>> acpi: acpi_cpu_startup: initializing cpu_cx_count to 0
>>> acpi_cpu_generic_cx_probe
>>> if sc->cpu_p_blk_len < 5  [sc->cpu_p_blk_len = 0]
>>> acpi: acpi_cpu_startup: cpu 0,cpu_cx_count = 0,sc->cpu_cx_count = 1
>>>
>>> So we're hitting an early exit in acpi_cpu_generic_cx_probe.
>>>
>> John, what would be a better fix - initialize the global variable to 1 or 
> use goto
>> in acpi_cpu_generic_cx_probe?
>> I think the latter is more consistent and obvious, the former is simpler and
>> safer, though.
> 
> I would rather move the cpu_cx_count code out into acpi_cpu_startup() 
> completely.  It would more closely match the _CST code in that case.  It is 
> also easier to follow the logic this way as well as it is only modified in 
> one place and not via a secret side-effect.

Perfect!

> --- //depot/vendor/freebsd/src/sys/dev/acpica/acpi_cpu.c	2009/02/19 14:40:18
> +++ //depot/user/jhb/acpipci/dev/acpica/acpi_cpu.c	2009/03/26 15:28:32
> @@ -609,10 +609,6 @@
>  	    sc->cpu_cx_count++;
>  	}
>      }
> -
> -    /* Update the largest cx_count seen so far */
> -    if (sc->cpu_cx_count > cpu_cx_count)
> -	cpu_cx_count = sc->cpu_cx_count;
>  }
>  
>  /*
> @@ -752,6 +748,8 @@
>  	for (i = 0; i < cpu_ndevices; i++) {
>  	    sc = device_get_softc(cpu_devices[i]);
>  	    acpi_cpu_generic_cx_probe(sc);
> +	    if (sc->cpu_cx_count > cpu_cx_count)
> +		    cpu_cx_count = sc->cpu_cx_count;
>  	}
>  
>  	/*
> 
> 


-- 
Andriy Gapon