Messages in this thread | | | Date | Fri, 01 Feb 2013 23:16:37 +0100 | From | Daniel Lezcano <> | Subject | Re: [PATCH 1/4] intel_idle: stop using driver_data for static flags |
| |
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
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |