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 12:10:16 +0200 Message-ID: <20130726121016.6af979ca@amdc308.digital.local> References: <1370502472-7249-1-git-send-email-l.majewski@samsung.com> <1374770011-22171-1-git-send-email-l.majewski@samsung.com> <1374770011-22171-3-git-send-email-l.majewski@samsung.com> <20130726103321.21238bbb@amdc308.digital.local> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-reply-to: Sender: cpufreq-owner@vger.kernel.org To: Viresh Kumar Cc: "Rafael J. Wysocki" , Zhang Rui , Eduardo Valentin , "cpufreq@vger.kernel.org" , Linux PM list , Jonghwa Lee , Lukasz Majewski , linux-kernel , Bartlomiej Zolnierkiewicz , Daniel Lezcano , Kukjin Kim , Myungjoo Ham , durgadoss.r@intel.com, Lists linaro-kernel List-Id: linux-pm@vger.kernel.org On Fri, 26 Jul 2013 15:03:34 +0530 Viresh Kumar wrote, > On 26 July 2013 14:03, Lukasz Majewski wrote: > > On Fri, 26 Jul 2013 12:47:15 +0530 Viresh Kumar wrote, > >> On 25 July 2013 22:03, Lukasz Majewski > >> wrote: > > >> > +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? > > >> > + ret = cpufreq_driver->set_boost(state); > >> > + if (ret) { > >> > + write_lock_irqsave(&cpufreq_driver_lock, flags); > >> > + cpufreq_driver->boost_enabled = 0; > >> > >> should be: > >> cpufreq_driver->boost_enabled = !state; > > > > For me = 0 (or = false) is more readable. > > If you wish, I will change it to = !state. > > Its not about readability but logic... What if boost was enabled > earlier and we are disabling it now.. and we reach here.. We > need to enable boost again, whereas you are disabling it. You are right here. I will change this to = !state > > >> > +int cpufreq_boost_supported(void) > >> > +{ > >> > + if (cpufreq_driver) > >> > >> This routine is always called from places where cpufreq_driver > >> can't be NULL.. > > > > It is also called from thermal. And it happens that thermal is > > initialized earlier. > > Then "NULL pointer dereference" happens. > > Ok.. Put a likely() around this check for cpufreq_driver.. Ok. > > > In my opinion at [1] we don't need the if (cpufreq_driver) check. > > But it is up to you to decide. > > leave it as is. Ok. > > > If we agree about above comments, I will post v7 ASAP. > > Don't post it ASAP, wait for few more days for others to give > comments.. And also I haven't finished reviewing it until > now. Ok. -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group