From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753967AbcBHNaO (ORCPT ); Mon, 8 Feb 2016 08:30:14 -0500 Received: from mail-pa0-f53.google.com ([209.85.220.53]:36653 "EHLO mail-pa0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753469AbcBHNaK (ORCPT ); Mon, 8 Feb 2016 08:30:10 -0500 Date: Mon, 8 Feb 2016 19:00:06 +0530 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: Rafael Wysocki , Juri Lelli , Lists linaro-kernel , "linux-pm@vger.kernel.org" , Saravana Kannan , Peter Zijlstra , Michael Turquette , Steve Muckle , Vincent Guittot , Morten Rasmussen , dietmar.eggemann@arm.com, Shilpasri G Bhat , Linux Kernel Mailing List Subject: Re: [PATCH V3 11/13] cpufreq: governor: Keep list of policy_dbs within dbs_data Message-ID: <20160208133006.GJ8294@vireshk> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08-02-16, 14:21, Rafael J. Wysocki wrote: > On Mon, Feb 8, 2016 at 12:39 PM, Viresh Kumar 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 > > 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