| 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 |
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; }
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>
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 --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; }
_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(-)