Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 02/01/2013 07:40 PM, Len Brown wrote:
> On 02/01/2013 03:44 AM, Daniel Lezcano wrote:
>> On 02/01/2013 05:11 AM, Len Brown wrote:
>>> From: Len Brown <len.brown@intel.com>
>>>
>>> The commit, 4202735e8ab6ecfb0381631a0d0b58fefe0bd4e2
>>> (cpuidle: Split cpuidle_state structure and move per-cpu statistics fields)
>>> observed that the MWAIT flags for Cn on every processor to date were the
>>> same, and created get_driver_data() to supply them.
>>>
>>> Unfortunately, that assumption is false, going forward.
>>> So here we restore the MWAIT flags to the cpuidle_state table.
>>> However, instead restoring the old "driver_data" field,
>>> we put the flags into the existing "flags" field,
>>> where they probalby should have lived all along.
>>
>> Removing the driver_data is a good thing but I am not sure it is the
>> case by moving the MWAIT flags to the cpuidle_state's flags field. We
>> are mixing arch specific flags with a generic code.
>>
>> This is prone to errors because new flags could appear for the cpuidle
>> core code and could collide with the arch specific flags.
>>
>> Wouldn't make sense to add a private field in the struct cpuidle_state
>> structure to let the driver/arch specific to store whatever is needed ?
>>
>> struct cpuidle_state {
>>
>> 	...
>> 	unsigned long private;
>> 	...
>>
>> }
> 
> The top half of the flags are reserved for the driver,
> as noted by the definition of CPUIDLE_DRIVER_FLAGS_MASK
> with the generic flag definitions:
> 
> #define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */
> #define CPUIDLE_FLAG_COUPLED    (0x02) /* state applies to multiple cpus */
> 
> #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
> 
> intel_idle already uses a driver-specific flag:
> 
> #define CPUIDLE_FLAG_TLB_FLUSHED        0x10000
> 
> This patch just uses 4 more bits along with that one.

Ok. In this case it would make sense to move this flag out of the
generic core code to the intel_idle.c file ? and move the [dec/en]coding
macro flags_2_MWAIT_EAX and MWAIT_EAX_2_flags (with a more appropriate
name for a generic code) to the cpuidle.h file ?

  -- Daniel



> Sure, if we run out of space, we can add an additional field.
> But I don't see an immediate need for it.
> 
> thanks,
> Len Brown
> Intel Open Source Technology Center
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog