On 27 November 2013 12:38, Lan Tianyu <tianyu.lan@xxxxxxxxx> wrote:
> Hi Viresh:
Hey Lan,
> First, I agree the new solution you are working on. :)
Thanks :)
> But actually I don't totally agree my origin patch have design issue.
> Because I think governor should have the ability to check whether it has
> been EXIT when doing INIT and it should return error code at that point.
> The design is to make governor code stronger to deal with the case that
> governor is reinitialized before EXIT. Just from my view.
> Sorry for noise.
Ahh, these are useful discussions. Everyone have their own thoughts and
its upto all of us to get meaningful stuff out of it..
I agree to whatever you wrote above but this isn't exactly what's being
done in your patch. I was more concerned about this stuff:
On 22 November 2013 13:08, Lan Tianyu <tianyu.lan@xxxxxxxxx> wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> + if (has_target() && !frozen) {
> ret = __cpufreq_governor(policy,
> CPUFREQ_GOV_POLICY_EXIT);
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> @@ -204,9 +204,20 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>
> switch (event) {
> case CPUFREQ_GOV_POLICY_INIT:
> + /*
> + * In order to keep governor data across suspend/resume,
> + * Governor doesn't exit when suspend and will be
> + * reinitialized when resume. Here check policy governor
> + * data to determine whether the governor has been exited.
> + * If not, return EALREADY.
> + */
> if (have_governor_per_policy()) {
> - WARN_ON(dbs_data);
> + if (dbs_data)
> + return -EALREADY;
> } else if (dbs_data) {
> + if (policy->governor_data == dbs_data)
> + return -EALREADY;
> +
> dbs_data->usage_count++;
> policy->governor_data = dbs_data;
> return 0;
Here the cpufreq core has skipped the call to governor's EXIT,
and so it shouldn't pass on the following INIT call to them..
That's a bit wrong. These two calls work in pairs and are exactly
opposite to each other. And so if some decision has to be taken
then either that should be done completely at governor level
or core level. Doing stuff partly in governor and partly in core
is like giving invitation to new bugs/problems :)
Nothing personal otherwise. Recently there were patches sent
by people, you, Nishanth, etc, which I have just overridden with
my versions.. It wasn't about getting my count higher :) but
getting the solution at right places instead of solving them at
wrong locations..
I am already having tough time upstreaming patches for cpufreq
consolidation, as the number of patches is huge. It takes time
for people to absorb/test them. Though Rafael has taken almost all
of them in v3.13 finally, but I understand its difficult for him as
well and he did his job wonderfully :)
And so I don't really want to get any new stuff in which will surely
get consolidated later. Lets do it now, we had enough of it :)
Even, related to your patch, I was already thinking of getting
rid of "frozen" variable and parameter to functions, as we already
know status from a global variable now, cpufreq_suspended. And
so we don't actually need to pass any additional parameters
to many routines, which have something like 'frozen' currently.
--
viresh
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
