linux-kernel - Re: cpuidle/powernv: Read target_residency value of idle states from DT if available

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54C70DDD.9040408@linux.vnet.ibm.com>
Date:	Tue, 27 Jan 2015 09:32:37 +0530
From:	Preeti U Murthy <preeti@...ux.vnet.ibm.com>
To:	Michael Ellerman <mpe@...erman.id.au>, rafael.j.wysocki@...el.com
CC:	linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org
Subject: Re: cpuidle/powernv: Read target_residency value of idle states from
 DT if available

On 01/23/2015 10:29 AM, Michael Ellerman wrote:
> On Tue, 2015-20-01 at 11:26:49 UTC, Preeti U Murthy wrote:
>> @@ -177,34 +178,39 @@ static int powernv_add_idle_states(void)
>>  		return nr_idle_states;
>>  	}
>>  
>> -	idle_state_latency = of_get_property(power_mgt,
>> -			"ibm,cpu-idle-state-latencies-ns", NULL);
>> -	if (!idle_state_latency) {
>> +	dt_idle_states = len_flags / sizeof(u32);
>> +
>> +	latency_ns = kzalloc(sizeof(*latency_ns) * dt_idle_states, GFP_KERNEL);
>> +	rc = of_property_read_u32(power_mgt,
>> +			"ibm,cpu-idle-state-latencies-ns", latency_ns);
> 
> That's only reading the first value.
> 
> If you want to read the full property you need the _array version.

Right, thanks for pointing this out.
> 
>> +	if (rc) {
>>  		pr_warn("DT-PowerMgmt: missing ibm,cpu-idle-state-latencies-ns\n");
> 
> You missed my hint that "DT-PowerMgmt" is a weird and ugly prefix. Can you use
> "cpuidle-powernv:" instead?

Yes will change this.
> 
>>  
>> -	dt_idle_states = len_flags / sizeof(u32);
>> +	residency_ns = kzalloc(sizeof(*residency_ns) * dt_idle_states, GFP_KERNEL);
>> +	rc = of_property_read_u32(power_mgt,
>> +			"ibm,cpu-idle-state-residency-ns", residency_ns);
>> +	if (rc) {
>> +		pr_warn("DT-PowerMgmt: missing ibm,cpu-idle-state-residency-ns\n");
>> +		pr_warn("Falling back to default values\n");
> 
> I don't think this is worth a warning seeing as we know there are firmwares out
> there which do not have the property.
> 
>>  	for (i = 0; i < dt_idle_states; i++) {
>>  
>>  		flags = be32_to_cpu(idle_state_flags[i]);
>> -
>> -		/* Cpuidle accepts exit_latency in us and we estimate
>> -		 * target residency to be 10x exit_latency
>> +		/*
>> +		 * Cpuidle accepts exit_latency and target_residency in us.
>> +		 * Use default target_residency values if f/w does not expose it.
>>  		 */
>> -		latency_ns = be32_to_cpu(idle_state_latency[i]);
>>  		if (flags & OPAL_PM_NAP_ENABLED) {
>>  			/* Add NAP state */
>>  			strcpy(powernv_states[nr_idle_states].name, "Nap");
>>  			strcpy(powernv_states[nr_idle_states].desc, "Nap");
>>  			powernv_states[nr_idle_states].flags = 0;
>> -			powernv_states[nr_idle_states].exit_latency =
>> -					((unsigned int)latency_ns) / 1000;
>> -			powernv_states[nr_idle_states].target_residency =
>> -					((unsigned int)latency_ns / 100);
>> +			powernv_states[nr_idle_states].target_residency = 100;
>>  			powernv_states[nr_idle_states].enter = &nap_loop;
>> -			nr_idle_states++;
> 
> That looks wrong? Or do you mean to do that?

Are you pointing at removing the lines that populate the exit_latency ?
I read the latency value from the DT outside of these conditions because
it is guaranteed to be present at this point. We return back from this
function early on if we do not find it in the DT.

If you are pointing at the lines that remove increment of
nr_idle_states, it has to be done so towards the end of the iteration
because we are yet to populate the exit_latency and target_residency
outside of these conditions using this index.

Regards
Preeti U Murthy
> 
> cheers
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/