2014-12-16 00:50:54

by Dmitry Torokhov

Subject: [PATCH] cpufreq: imx6: prorect calls to dev_pm_opp_get_opp_count with RCU lock

dev_pm_opp_get_opp_count() must be called with RCU lock held.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

Not tested at all...

drivers/cpufreq/imx6q-cpufreq.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 380a90d..851d4fd 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -200,7 +200,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
* Just, incase the platform did not supply the OPP
* table, it will try to get it.
*/
+ rcu_read_lock();
num = dev_pm_opp_get_opp_count(cpu_dev);
+ rcu_read_unlock();
if (num < 0) {
ret = of_init_opp_table(cpu_dev);
if (ret < 0) {
@@ -211,7 +213,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
/* Because we have added the OPPs here, we must free them */
free_opp = true;

+ rcu_read_lock();
num = dev_pm_opp_get_opp_count(cpu_dev);
+ rcu_read_unlock();
if (num < 0) {
ret = num;
dev_err(cpu_dev, "no OPP table is found: %d\n", ret);
--
2.2.0.rc0.207.ga3a616c


--
Dmitry

2014-12-16 05:33:35

by Viresh Kumar

Subject: Re: [PATCH] cpufreq: imx6: prorect calls to dev_pm_opp_get_opp_count with RCU lock

On 16 December 2014 at 06:20, Dmitry Torokhov <[email protected]> wrote:
> dev_pm_opp_get_opp_count() must be called with RCU lock held.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> Not tested at all...
>
> drivers/cpufreq/imx6q-cpufreq.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 380a90d..851d4fd 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -200,7 +200,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> * Just, incase the platform did not supply the OPP
> * table, it will try to get it.
> */
> + rcu_read_lock();
> num = dev_pm_opp_get_opp_count(cpu_dev);
> + rcu_read_unlock();
> if (num < 0) {
> ret = of_init_opp_table(cpu_dev);
> if (ret < 0) {
> @@ -211,7 +213,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
> /* Because we have added the OPPs here, we must free them */
> free_opp = true;
>
> + rcu_read_lock();
> num = dev_pm_opp_get_opp_count(cpu_dev);
> + rcu_read_unlock();
> if (num < 0) {
> ret = num;
> dev_err(cpu_dev, "no OPP table is found: %d\n", ret);

This one looks fine for sure but I was wondering for the users which just
need to call opp-count from under the locks, what about something like:

dev_pm_opp_get_opp_count_locked() ?

--
viresh

2014-12-16 05:41:08

by Dmitry Torokhov

Subject: Re: [PATCH] cpufreq: imx6: prorect calls to dev_pm_opp_get_opp_count with RCU lock

On Mon, Dec 15, 2014 at 9:33 PM, Viresh Kumar <[email protected]> wrote:
> On 16 December 2014 at 06:20, Dmitry Torokhov <[email protected]> wrote:
>> dev_pm_opp_get_opp_count() must be called with RCU lock held.
>>
>> Signed-off-by: Dmitry Torokhov <[email protected]>
>> ---
>>
>> Not tested at all...
>>
>> drivers/cpufreq/imx6q-cpufreq.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
>> index 380a90d..851d4fd 100644
>> --- a/drivers/cpufreq/imx6q-cpufreq.c
>> +++ b/drivers/cpufreq/imx6q-cpufreq.c
>> @@ -200,7 +200,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>> * Just, incase the platform did not supply the OPP
>> * table, it will try to get it.
>> */
>> + rcu_read_lock();
>> num = dev_pm_opp_get_opp_count(cpu_dev);
>> + rcu_read_unlock();
>> if (num < 0) {
>> ret = of_init_opp_table(cpu_dev);
>> if (ret < 0) {
>> @@ -211,7 +213,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>> /* Because we have added the OPPs here, we must free them */
>> free_opp = true;
>>
>> + rcu_read_lock();
>> num = dev_pm_opp_get_opp_count(cpu_dev);
>> + rcu_read_unlock();
>> if (num < 0) {
>> ret = num;
>> dev_err(cpu_dev, "no OPP table is found: %d\n", ret);
>
> This one looks fine for sure but I was wondering for the users which just
> need to call opp-count from under the locks, what about something like:
>
> dev_pm_opp_get_opp_count_locked() ?

Yeah, for the majority of callers taking the lock won't hurt as they
call it in their init code.

Thanks,
Dmitry