On 08-02-16, 14:21, Rafael J. Wysocki wrote: > On Mon, Feb 8, 2016 at 12:39 PM, Viresh Kumar <[email protected]> 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 <[email protected]> > > 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 ? > > + > > 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. -- viresh
