* Re: [PATCH] cpufreq: Make cpufreq_quick_get() safe to call. [not found] <1457622636-10028-1-git-send-email-rcochran@linutronix.de> @ 2016-03-10 22:20 ` Rafael J. Wysocki 2016-03-11 2:23 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2016-03-10 22:20 UTC (permalink / raw) To: Richard Cochran Cc: linux-kernel, rt, Rafael J. Wysocki, Viresh Kumar, Linux PM list On Thursday, March 10, 2016 04:10:36 PM Richard Cochran wrote: > The function, cpufreq_quick_get, accesses the global 'cpufreq_driver' and > its fields without taking the associated lock, cpufreq_driver_lock. > > Without the locking, nothing guarantees that 'cpufreq_driver' remains > consistent during the call. This patch fixes the issue by taking the lock > before accessing the data structure. > > Cc: Dirk Brandewie <dirk.brandewie@gmail.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Richard Cochran <rcochran@linutronix.de> Can you please CC PM-related patches to linux-pm@vger.kernel.org? They are much easier to handle for me then. > --- > drivers/cpufreq/cpufreq.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index e979ec7..ce02b2b 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1457,9 +1457,17 @@ unsigned int cpufreq_quick_get(unsigned int cpu) > { > struct cpufreq_policy *policy; > unsigned int ret_freq = 0; > + unsigned long flags; > + > + read_lock_irqsave(&cpufreq_driver_lock, flags); > > if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) > - return cpufreq_driver->get(cpu); > + ret_freq = cpufreq_driver->get(cpu); > + > + read_unlock_irqrestore(&cpufreq_driver_lock, flags); > + > + if (ret_freq) > + return ret_freq; > > policy = cpufreq_cpu_get(cpu); > if (policy) { > I would prefer something like this: read_lock_irqsave(&cpufreq_driver_lock, flags); if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) { unsigned int ret_freq = cpufreq_driver->get(cpu); read_unlock_irqrestore(&cpufreq_driver_lock, flags); return ret_freq; } read_unlock_irqrestore(&cpufreq_driver_lock, flags); Thanks, Rafael ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: Make cpufreq_quick_get() safe to call. 2016-03-10 22:20 ` [PATCH] cpufreq: Make cpufreq_quick_get() safe to call Rafael J. Wysocki @ 2016-03-11 2:23 ` Rafael J. Wysocki 2016-03-11 8:43 ` [PATCH v2] " Richard Cochran 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2016-03-11 2:23 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Richard Cochran, Linux Kernel Mailing List, rt, Rafael J. Wysocki, Viresh Kumar, Linux PM list On Thu, Mar 10, 2016 at 11:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Thursday, March 10, 2016 04:10:36 PM Richard Cochran wrote: >> The function, cpufreq_quick_get, accesses the global 'cpufreq_driver' and >> its fields without taking the associated lock, cpufreq_driver_lock. >> >> Without the locking, nothing guarantees that 'cpufreq_driver' remains >> consistent during the call. This patch fixes the issue by taking the lock >> before accessing the data structure. >> >> Cc: Dirk Brandewie <dirk.brandewie@gmail.com> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Signed-off-by: Richard Cochran <rcochran@linutronix.de> > > Can you please CC PM-related patches to linux-pm@vger.kernel.org? They > are much easier to handle for me then. > >> --- >> drivers/cpufreq/cpufreq.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index e979ec7..ce02b2b 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1457,9 +1457,17 @@ unsigned int cpufreq_quick_get(unsigned int cpu) >> { >> struct cpufreq_policy *policy; >> unsigned int ret_freq = 0; >> + unsigned long flags; >> + >> + read_lock_irqsave(&cpufreq_driver_lock, flags); >> >> if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) >> - return cpufreq_driver->get(cpu); >> + ret_freq = cpufreq_driver->get(cpu); >> + >> + read_unlock_irqrestore(&cpufreq_driver_lock, flags); >> + >> + if (ret_freq) >> + return ret_freq; >> >> policy = cpufreq_cpu_get(cpu); >> if (policy) { >> > > I would prefer something like this: > > read_lock_irqsave(&cpufreq_driver_lock, flags); > > if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) { > unsigned int ret_freq = cpufreq_driver->get(cpu); Sorry, ret_freq is needed outside of this block anyway, so that would be ret_freq = cpufreq_driver->get(cpu); > > read_unlock_irqrestore(&cpufreq_driver_lock, flags); > return ret_freq; > } > > read_unlock_irqrestore(&cpufreq_driver_lock, flags); ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] cpufreq: Make cpufreq_quick_get() safe to call. 2016-03-11 2:23 ` Rafael J. Wysocki @ 2016-03-11 8:43 ` Richard Cochran 2016-03-11 23:52 ` Viresh Kumar 0 siblings, 1 reply; 5+ messages in thread From: Richard Cochran @ 2016-03-11 8:43 UTC (permalink / raw) To: linux-kernel Cc: rt, Dirk Brandewie, Rafael J. Wysocki, Viresh Kumar, linux-pm The function, cpufreq_quick_get, accesses the global 'cpufreq_driver' and its fields without taking the associated lock, cpufreq_driver_lock. Without the locking, nothing guarantees that 'cpufreq_driver' remains consistent during the call. This patch fixes the issue by taking the lock before accessing the data structure. Cc: Dirk Brandewie <dirk.brandewie@gmail.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: linux-pm@vger.kernel.org Signed-off-by: Richard Cochran <rcochran@linutronix.de> --- drivers/cpufreq/cpufreq.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index e979ec7..053aa1f 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1457,9 +1457,17 @@ unsigned int cpufreq_quick_get(unsigned int cpu) { struct cpufreq_policy *policy; unsigned int ret_freq = 0; + unsigned long flags; - if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) - return cpufreq_driver->get(cpu); + read_lock_irqsave(&cpufreq_driver_lock, flags); + + if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) { + ret_freq = cpufreq_driver->get(cpu); + read_unlock_irqrestore(&cpufreq_driver_lock, flags); + return ret_freq; + } + + read_unlock_irqrestore(&cpufreq_driver_lock, flags); policy = cpufreq_cpu_get(cpu); if (policy) { -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] cpufreq: Make cpufreq_quick_get() safe to call. 2016-03-11 8:43 ` [PATCH v2] " Richard Cochran @ 2016-03-11 23:52 ` Viresh Kumar 2016-03-18 23:34 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Viresh Kumar @ 2016-03-11 23:52 UTC (permalink / raw) To: Richard Cochran Cc: linux-kernel, rt, Dirk Brandewie, Rafael J. Wysocki, linux-pm On 11-03-16, 09:43, Richard Cochran wrote: > The function, cpufreq_quick_get, accesses the global 'cpufreq_driver' and > its fields without taking the associated lock, cpufreq_driver_lock. > > Without the locking, nothing guarantees that 'cpufreq_driver' remains > consistent during the call. This patch fixes the issue by taking the lock > before accessing the data structure. > > Cc: Dirk Brandewie <dirk.brandewie@gmail.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Richard Cochran <rcochran@linutronix.de> > --- > drivers/cpufreq/cpufreq.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- viresh ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] cpufreq: Make cpufreq_quick_get() safe to call. 2016-03-11 23:52 ` Viresh Kumar @ 2016-03-18 23:34 ` Rafael J. Wysocki 0 siblings, 0 replies; 5+ messages in thread From: Rafael J. Wysocki @ 2016-03-18 23:34 UTC (permalink / raw) To: Viresh Kumar, Richard Cochran, Rafael J. Wysocki Cc: linux-kernel, rt, linux-pm On Saturday, March 12, 2016 06:52:37 AM Viresh Kumar wrote: > On 11-03-16, 09:43, Richard Cochran wrote: > > The function, cpufreq_quick_get, accesses the global 'cpufreq_driver' and > > its fields without taking the associated lock, cpufreq_driver_lock. > > > > Without the locking, nothing guarantees that 'cpufreq_driver' remains > > consistent during the call. This patch fixes the issue by taking the lock > > before accessing the data structure. > > > > Cc: Dirk Brandewie <dirk.brandewie@gmail.com> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Cc: Viresh Kumar <viresh.kumar@linaro.org> > > Cc: linux-pm@vger.kernel.org > > Signed-off-by: Richard Cochran <rcochran@linutronix.de> > > --- > > drivers/cpufreq/cpufreq.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> Applied, thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-18 23:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1457622636-10028-1-git-send-email-rcochran@linutronix.de>
2016-03-10 22:20 ` [PATCH] cpufreq: Make cpufreq_quick_get() safe to call Rafael J. Wysocki
2016-03-11 2:23 ` Rafael J. Wysocki
2016-03-11 8:43 ` [PATCH v2] " Richard Cochran
2016-03-11 23:52 ` Viresh Kumar
2016-03-18 23:34 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).