On Mon, Feb 8, 2016 at 2:30 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 08-02-16, 14:21, Rafael J. Wysocki wrote:
On Mon, Feb 8, 2016 at 12:39 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
An instance of 'struct dbs_data' can support multiple 'struct policy_dbs_info' instances. To traverse all policy_dbs supported by a dbs_data, create a list of policy_dbs within dbs_data.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Good idea overall, I like this.
Thanks.
drivers/cpufreq/cpufreq_governor.c | 12 ++++++++++++ drivers/cpufreq/cpufreq_governor.h | 7 ++++++- 2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index ee3c2d92da53..e267acc67067 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -489,6 +489,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy) dbs_data->usage_count++; policy_dbs->dbs_data = dbs_data; policy->governor_data = policy_dbs;
mutex_lock(&dbs_data->mutex);list_add(&policy_dbs->list, &dbs_data->policy_dbs_list);mutex_unlock(&dbs_data->mutex);The previous statements should be under the mutex too IMO, at least the usage count incrementation in case two instances of this happen at the same time.
That can't happen now, but if we want to get rid of dbs_data_mutex going forward, having it under the mutex will be actually useful.
I think we should keep it precise for now. Right now, we are only concerned about the list modification, so just lock around that.
Once we are going to remove dbs_data_mutex, then we can cover more things under it.
Is there anything that is broken right now ?
Yes, the logic.
The counter technically is the number of items in policy_dbs_list. Updating the list alone under the lock is simply illogical.
return 0; }@@ -500,8 +505,11 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy)
dbs_data->usage_count = 1; dbs_data->gov = gov;
INIT_LIST_HEAD(&dbs_data->policy_dbs_list); mutex_init(&dbs_data->mutex);list_add(&policy_dbs->list, &dbs_data->policy_dbs_list);That line should go to where policy_dbs->dbs_data is set so it is clear that they go together.
Okay.
And I'd set the usage count to 1 in there too for consistency.
I am not sure about including any updates within the lock, which don't need protection in current state of code.
Well, if you're not sure, then please simply follow my request. :-)
Thanks, Rafael