diff mbox series

OPP: Fix _set_required_opps when opp is NULL

Message ID 20231223023421.3818297-1-bryan.odonoghue@linaro.org (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series OPP: Fix _set_required_opps when opp is NULL | expand

Commit Message

Bryan O'Donoghue Dec. 23, 2023, 2:34 a.m. UTC
_set_required_opps can be called with opp NULL in _disable_opp_table().

commit e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs")
requires the opp pointer to be non-NULL to function.

[   81.253439] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000048
[   81.438407] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[   81.445296] Workqueue: pm pm_runtime_work
[   81.449446] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   81.456609] pc : _set_required_opps+0x178/0x28c
[   81.461288] lr : _set_required_opps+0x178/0x28c
[   81.465962] sp : ffff80008078bb00
[   81.469375] x29: ffff80008078bb00 x28: ffffd1cd71bfe308 x27: 0000000000000000
[   81.476730] x26: ffffd1cd70ebc578 x25: ffffd1cd70a08710 x24: 00000000ffffffff
[   81.484083] x23: 00000000ffffffff x22: 0000000000000000 x21: ffff56ff892b3c48
[   81.491435] x20: ffff56f1071c10 x19: 0000000000000000 x18: ffffffffffffffff
[   81.498788] x17: 2030207865646e69 x16: 2030303131207370 x15: 706f5f6465726975
[   81.506141] x14: 7165725f7465735f x13: ffff5700f5c00000 x12: 00000000000008ac
[   81.513495] x11: 00000000000002e4 x10: ffff5700f6700000 x9 : ffff5700f5c00000
[   81.520848] x8 : 00000000fffdffff x7 : ffff5700f6700000 x6 : 80000000fffe0000
[   81.528200] x5 : ffff5700fef40d08 x4 : 0000000000000000 x3 : 0000000000000000
[   81.535551] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff56ff81298f80
[   81.542904] Call trace:
[   81.545437]  _set_required_opps+0x178/0x28c
[   81.549754]  _set_opp+0x3fc/0x5c0
[   81.553181]  dev_pm_opp_set_rate+0x90/0x26c
[   81.557498]  core_power_v4+0x44/0x15c [venus_core]
[   81.562509]  venus_runtime_suspend+0x40/0xd0 [venus_core]
[   81.568135]  pm_generic_runtime_suspend+0x2c/0x44
[   81.572983]  __rpm_callback+0x48/0x1d8
[   81.576852]  rpm_callback+0x6c/0x78
[   81.580453]  rpm_suspend+0x10c/0x570
[   81.584143]  pm_runtime_work+0xc4/0xc8
[   81.588011]  process_one_work+0x138/0x244
[   81.592153]  worker_thread+0x320/0x438
[   81.596021]  kthread+0x110/0x114
[   81.599355]  ret_from_fork+0x10/0x20
[   81.603052] Code: f10000ff fa5410e0 54fffbe1 97f05ae8 (f94026c5)
[   81.609317] ---[ end trace 0000000000000000 ]---

When the opp pointer is NULL in _set_required_opps() use dev_pm_opp_set_opp()
to set performance state 0.

Fixes: e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/opp/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Viresh Kumar Dec. 26, 2023, 5:59 a.m. UTC | #1
On 23-12-23, 02:34, Bryan O'Donoghue wrote:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index c022d548067d7..182e07ab6baf3 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1083,7 +1083,11 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
>  
>  	while (index != target) {
>  		if (devs[index]) {
> -			ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
> +			if (opp)
> +				ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
> +			else
> +				ret = dev_pm_domain_set_performance_state(devs[index], 0);
> +
>  			if (ret)
>  				return ret;
>  		}

Sorry about that, my mistake. Can you test below instead please ?

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c022d548067d..c4d695e0e5fd 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1061,6 +1061,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
                              struct dev_pm_opp *opp, bool up)
 {
        struct device **devs = opp_table->required_devs;
+       struct dev_pm_opp *required_opp;
        int index, target, delta, ret;

        if (!devs)
@@ -1083,7 +1084,9 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,

        while (index != target) {
                if (devs[index]) {
-                       ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
+                       required_opp = opp ? opp->required_opps[index] : NULL;
+
+                       ret = dev_pm_opp_set_opp(devs[index], required_opp);
                        if (ret)
                                return ret;
                }
Bryan O'Donoghue Dec. 27, 2023, 12:41 p.m. UTC | #2
On 26/12/2023 05:59, Viresh Kumar wrote:
> On 23-12-23, 02:34, Bryan O'Donoghue wrote:
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index c022d548067d7..182e07ab6baf3 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1083,7 +1083,11 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
>>   
>>   	while (index != target) {
>>   		if (devs[index]) {
>> -			ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
>> +			if (opp)
>> +				ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
>> +			else
>> +				ret = dev_pm_domain_set_performance_state(devs[index], 0);
>> +
>>   			if (ret)
>>   				return ret;
>>   		}
> 
> Sorry about that, my mistake. Can you test below instead please ?
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index c022d548067d..c4d695e0e5fd 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1061,6 +1061,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
>                                struct dev_pm_opp *opp, bool up)
>   {
>          struct device **devs = opp_table->required_devs;
> +       struct dev_pm_opp *required_opp;
>          int index, target, delta, ret;
> 
>          if (!devs)
> @@ -1083,7 +1084,9 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
> 
>          while (index != target) {
>                  if (devs[index]) {
> -                       ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
> +                       required_opp = opp ? opp->required_opps[index] : NULL;
> +
> +                       ret = dev_pm_opp_set_opp(devs[index], required_opp);
>                          if (ret)
>                                  return ret;
>                  }
> 

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Viresh Kumar Dec. 28, 2023, 5:29 a.m. UTC | #3
On 27-12-23, 12:41, Bryan O'Donoghue wrote:
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Thanks Bryan. Here is the merged commit:

From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Date: Sat, 23 Dec 2023 02:34:21 +0000
Subject: [PATCH] OPP: Fix _set_required_opps when opp is NULL

_set_required_opps can be called with opp NULL in _disable_opp_table().

commit e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs")
requires the opp pointer to be non-NULL to function.

[   81.253439] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000048
[   81.438407] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[   81.445296] Workqueue: pm pm_runtime_work
[   81.449446] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   81.456609] pc : _set_required_opps+0x178/0x28c
[   81.461288] lr : _set_required_opps+0x178/0x28c
[   81.465962] sp : ffff80008078bb00
[   81.469375] x29: ffff80008078bb00 x28: ffffd1cd71bfe308 x27: 0000000000000000
[   81.476730] x26: ffffd1cd70ebc578 x25: ffffd1cd70a08710 x24: 00000000ffffffff
[   81.484083] x23: 00000000ffffffff x22: 0000000000000000 x21: ffff56ff892b3c48
[   81.491435] x20: ffff56f1071c10 x19: 0000000000000000 x18: ffffffffffffffff
[   81.498788] x17: 2030207865646e69 x16: 2030303131207370 x15: 706f5f6465726975
[   81.506141] x14: 7165725f7465735f x13: ffff5700f5c00000 x12: 00000000000008ac
[   81.513495] x11: 00000000000002e4 x10: ffff5700f6700000 x9 : ffff5700f5c00000
[   81.520848] x8 : 00000000fffdffff x7 : ffff5700f6700000 x6 : 80000000fffe0000
[   81.528200] x5 : ffff5700fef40d08 x4 : 0000000000000000 x3 : 0000000000000000
[   81.535551] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff56ff81298f80
[   81.542904] Call trace:
[   81.545437]  _set_required_opps+0x178/0x28c
[   81.549754]  _set_opp+0x3fc/0x5c0
[   81.553181]  dev_pm_opp_set_rate+0x90/0x26c
[   81.557498]  core_power_v4+0x44/0x15c [venus_core]
[   81.562509]  venus_runtime_suspend+0x40/0xd0 [venus_core]
[   81.568135]  pm_generic_runtime_suspend+0x2c/0x44
[   81.572983]  __rpm_callback+0x48/0x1d8
[   81.576852]  rpm_callback+0x6c/0x78
[   81.580453]  rpm_suspend+0x10c/0x570
[   81.584143]  pm_runtime_work+0xc4/0xc8
[   81.588011]  process_one_work+0x138/0x244
[   81.592153]  worker_thread+0x320/0x438
[   81.596021]  kthread+0x110/0x114
[   81.599355]  ret_from_fork+0x10/0x20
[   81.603052] Code: f10000ff fa5410e0 54fffbe1 97f05ae8 (f94026c5)
[   81.609317] ---[ end trace 0000000000000000 ]---

Fix it.

Fixes: e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
[ Viresh: Implemented the fix differently ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/opp/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 49b429984bdb..a6e80f566e9b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1066,6 +1066,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
 			      struct dev_pm_opp *opp, bool up)
 {
 	struct device **devs = opp_table->required_devs;
+	struct dev_pm_opp *required_opp;
 	int index, target, delta, ret;
 
 	if (!devs)
@@ -1088,7 +1089,9 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
 
 	while (index != target) {
 		if (devs[index]) {
-			ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
+			required_opp = opp ? opp->required_opps[index] : NULL;
+
+			ret = dev_pm_opp_set_opp(devs[index], required_opp);
 			if (ret)
 				return ret;
 		}
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c022d548067d7..182e07ab6baf3 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1083,7 +1083,11 @@  static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
 
 	while (index != target) {
 		if (devs[index]) {
-			ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
+			if (opp)
+				ret = dev_pm_opp_set_opp(devs[index], opp->required_opps[index]);
+			else
+				ret = dev_pm_domain_set_performance_state(devs[index], 0);
+
 			if (ret)
 				return ret;
 		}