From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukasz Majewski Subject: Re: [PATCH v6 2/8] cpufreq: Add boost frequency support in core Date: Fri, 26 Jul 2013 15:08:22 +0200 Message-ID: <20130726150822.62891d43@amdc308.digital.local> References: <1370502472-7249-1-git-send-email-l.majewski@samsung.com> <20130726103321.21238bbb@amdc308.digital.local> <13618968.iglDI8cQ6h@vostro.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <13618968.iglDI8cQ6h@vostro.rjw.lan> Sender: cpufreq-owner@vger.kernel.org To: "Rafael J. Wysocki" Cc: Viresh Kumar , Zhang Rui , Eduardo Valentin , "cpufreq@vger.kernel.org" , Linux PM list , Jonghwa Lee , Lukasz Majewski , linux-kernel , Bartlomiej Zolnierkiewicz , Daniel Lezcano , Kukjin Kim , durgadoss.r@intel.com, Lists linaro-kernel List-Id: linux-pm@vger.kernel.org On Fri, 26 Jul 2013 14:36:08 +0200 Rafael J. Wysocki rjw@sisk.pl wrote, > On Friday, July 26, 2013 10:33:21 AM Lukasz Majewski wrote: > > On Fri, 26 Jul 2013 12:47:15 +0530 Viresh Kumar wrote, > > > On 25 July 2013 22:03, Lukasz Majewski > > > wrote: > > > > diff --git a/drivers/cpufreq/cpufreq.c > > > > b/drivers/cpufreq/cpufreq.c > > > > > > > /********************************************************************* > > > > + * > > > > BOOST * > > > > + > > > > *********************************************************************/ > > > > +static int cpufreq_boost_set_sw(int state) +{ > > > > + struct cpufreq_frequency_table *freq_table; > > > > + struct cpufreq_policy *policy; > > > > + int ret = -EINVAL; > > > > + > > > > + list_for_each_entry(policy, &cpufreq_policy_list, > > > > policy_list) { > > > > + freq_table = > > > > cpufreq_frequency_get_table(policy->cpu); > > > > + if (freq_table) { > > > > + ret = > > > > cpufreq_frequency_table_cpuinfo(policy, > > > > + > > > > freq_table); > > > > + if (!ret) { > > > > + policy->user_policy.max = > > > > policy->max; > > > > + __cpufreq_governor(policy, > > > > CPUFREQ_GOV_LIMITS); > > > > + } > > > > + } > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +int cpufreq_boost_trigger_state(int state) > > > > +{ > > > > + unsigned long flags; > > > > + int ret = 0; > > > > + > > > > + if (cpufreq_driver->boost_enabled == state) > > > > + return 0; > > > > + > > > > + write_lock_irqsave(&cpufreq_driver_lock, flags); > > > > + cpufreq_driver->boost_enabled = state; > > > > + write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > ^^^^^^^^^^^^^^^^^^^^ [*] > > > > > > Not sure if we should leave the lock at this point of time, as we > > > haven't enabled boost until now. > > > > The problem here is with the cpufreq_driver->set_boost() call. > > > > I tried to avoid acquiring lock at one function and release it at > > another (in this case cpufreq_boost_set_sw), especially since the > > __cpufreq_governor() acquires its own lock - good place for > > deadlock. > > > > Is it OK for you to grab lock at one function > > (cpufreq_boost_trigger_state()) and then at other function > > (cpufreq_boost_set_sw) release it before calling > > __cpufreq_governor() and grab it again after its completion? > > It generally is better to avoid doing that, although it is not > unheard of. In this particular case, one also needs to pass the "flags" parameter to the set_boost() function. This looks a bit unnatural to mix lock layer with the boost. > > Thanks, > Rafael > > -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group