On Mon, Feb 8, 2016 at 2:30 PM, Viresh Kumar <viresh.ku...@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.ku...@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.ku...@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
