On Friday, July 06, 2012, Daniel Lezcano wrote: > On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote: > > On Thursday, July 05, 2012, Daniel Lezcano wrote: > >> We have the state index passed as parameter to the 'enter' function. > >> Most of the drivers assign their 'enter' functions several times in > >> the cpuidle_state structure, as we have the index, we can delegate > >> to the driver to handle their own callback array. > >> > >> That will have the benefit of removing multiple lines of code in the > >> different drivers. > > > > Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason. > > Among other things, this was to avoid recurrence in callbacks - please see > > acpi_idle_enter_bm() for example. > > > > Now, if .enter() is moved to the driver structure, it will have to be an > > all-purpose complicated routine calling itself recursively at least in > > some cases. I'm not quite convinced that would be an improvement. > > > > On the other hand, I don't see anything wrong with setting several callback > > pointers to the same routine. > > Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state > to a single structure stored in the driver. The drivers were modified > and cleanup to take into account this modification. > > We saw some regressions like for example the 'disable' which were not > per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a > workaround more than a real fix). And now we have new architectures > (tegra3, big.LITTLE) with different latencies per cpu. Logically we > should revert Deepthi's patches but from my POV, going back and forth is > not a good solution (also we have to undo all modifications done in the > different drivers). > > The main purpose of all these cleanup patches are to move out all > non-data information from the cpuidle_state structure in order to add a > new api which could be 'cpuidle_register_cpu_latency(int cpu, struct > cpuidle_latencies latencies)'. Well, my question is: what specifically is wrong with having callback pointers in struct cpuidle_state in case when that structure is known to be common for all the CPUs in the system? Sure, there are systems in which that is not the case, but then why should we complicate the simplest case in order to address them? > For this specific patch, the 'enter' function for all the drivers is not > used [1] and one of the driver is about to use a single function [2]. So > we have only one driver is a couple of functions for this which can be > replaced by an array of callbacks in the driver itself as we have the index. > > [1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html I agree with Deepthi. > [2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html And that is fine and dandy as long as the OMAP people want that. Evidently, that is not wanted by everybody and if it had been, the cpuidle subsystem would have been designed differently. I really would like that thing to be reconsidered before we proceed with these changes. Thanks, Rafael
