| Message ID | 052c4937ce408a01de5cd7d7e359d333f9b11e57.1653564321.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | OPP: Add new configuration interface: dev_pm_opp_set_config() | expand |
Hi, On 5/30/22 6:45 PM, Chanwoo Choi wrote: > Hi, > > On 5/26/22 8:42 PM, Viresh Kumar wrote: >> The OPP core now provides a unified API for setting all configuration >> types, i.e. dev_pm_opp_set_config(). >> >> Lets start using it. >> >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> drivers/devfreq/exynos-bus.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c >> index e689101abc93..780e525eb92a 100644 >> --- a/drivers/devfreq/exynos-bus.c >> +++ b/drivers/devfreq/exynos-bus.c >> @@ -161,7 +161,7 @@ static void exynos_bus_exit(struct device *dev) >> >> dev_pm_opp_of_remove_table(dev); >> clk_disable_unprepare(bus->clk); >> - dev_pm_opp_put_regulators(bus->opp_table); >> + dev_pm_opp_clear_config(bus->opp_table); >> bus->opp_table = NULL; >> } >> >> @@ -182,11 +182,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np, >> struct opp_table *opp_table; >> const char *vdd = "vdd"; >> int i, ret, count, size; >> + struct dev_pm_opp_config config = { >> + .regulator_names = &vdd, >> + .regulator_count = 1, >> + }; >> >> - opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1); >> + opp_table = dev_pm_opp_set_config(dev, &config); >> if (IS_ERR(opp_table)) { >> ret = PTR_ERR(opp_table); >> - dev_err(dev, "failed to set regulators %d\n", ret); >> + dev_err(dev, "failed to set OPP config %d\n", ret); >> return ret; >> } >> >> @@ -236,7 +240,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np, >> return 0; >> >> err_regulator: >> - dev_pm_opp_put_regulators(bus->opp_table); >> + dev_pm_opp_clear_config(bus->opp_table); >> bus->opp_table = NULL; >> >> return ret; >> @@ -459,7 +463,7 @@ static int exynos_bus_probe(struct platform_device *pdev) >> dev_pm_opp_of_remove_table(dev); >> clk_disable_unprepare(bus->clk); >> err_reg: >> - dev_pm_opp_put_regulators(bus->opp_table); >> + dev_pm_opp_clear_config(bus->opp_table); >> bus->opp_table = NULL; >> >> return ret; >> > > When I tested them with patch1/2 and patch10/11/12, > I got the following message. > > [ 1.083669] Unable to handle kernel NULL pointer dereference at virtual address 000000b4 > [ 1.083683] [000000b4] *pgd=00000000 > [ 1.083719] Internal error: Oops: 5 [#1] PREEMPT SMP ARM > [ 1.083735] Modules linked in: > [ 1.083764] CPU: 6 PID: 1 Comm: swapper/0 Not tainted 5.18.0-11272-g7093c1f2a99c #2 > [ 1.083780] Hardware name: Samsung Exynos (Flattened Device Tree) > [ 1.083797] PC is at dev_pm_opp_clear_config+0x10/0x90 > [ 1.083823] LR is at exynos_bus_probe+0x384/0x634 > [ 1.083843] pc : [<c0933740>] lr : [<c09a33e0>] psr: 60000013 > [ 1.083859] sp : f0835d38 ip : c1988000 fp : 00000001 > [ 1.083874] r10: c207bc10 r9 : c1b1a580 r8 : c160b708 > [ 1.083890] r7 : c207bc00 r6 : c1b1a580 r5 : fffffdfb r4 : 00000000 > [ 1.083907] r3 : c1988000 r2 : 00000000 r1 : 00000000 r0 : 00000000 > [ 1.083924] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > [ 1.083940] Control: 10c5387d Table: 4000406a DAC: 00000051 > [ 1.083955] Register r0 information: NULL pointer > [ 1.083989] Register r1 information: NULL pointer > [ 1.084021] Register r2 information: NULL pointer > [ 1.084053] Register r3 information: slab task_struct start c1988000 pointer offset 0 > [ 1.084126] Register r4 information: NULL pointer > [ 1.084157] Register r5 information: non-paged memory > [ 1.084189] Register r6 information: slab kmalloc-64 start c1b1a580 pointer offset 0 size 64 > [ 1.084270] Register r7 information: slab kmalloc-1k start c207bc00 pointer offset 0 size 1024 > [ 1.084351] Register r8 information: non-slab/vmalloc memory > [ 1.084383] Register r9 information: slab kmalloc-64 start c1b1a580 pointer offset 0 size 64 > [ 1.084463] Register r10 information: slab kmalloc-1k start c207bc00 pointer offset 16 size 1024 > [ 1.084542] Register r11 information: non-paged memory > [ 1.084573] Register r12 information: slab task_struct start c1988000 pointer offset 0 > [ 1.084635] Process swapper/0 (pid: 1, stack limit = 0x(ptrval)) > [ 1.084651] Stack: (0xf0835d38 to 0xf0836000) > [ 1.084669] 5d20: c26b3940 c09a33e0 > [ 1.084687] 5d40: 00000000 f0835d9c a0000013 cfd97040 df8a6280 c0e031ec c17188f8 c0994758 > [ 1.084703] 5d60: 069db9c0 c0e0338c 60000013 c0e03380 c1784000 c0e0338c c17188f8 c0994758 > [ 1.084717] 5d80: f0835dc8 c160b708 ffffffff df8a6280 c12de76c c0994bb8 c181fdd4 df8a601c > [ 1.084737] 5da0: 00000000 c160b708 c1718f84 c1765ea0 00000000 000001bb c1550860 c0995684 > [ 1.084755] 5dc0: ffffffff 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > [ 1.084772] 5de0: 00000000 0f7634d1 00000000 c207bc10 00000000 c1718f84 c1765ea0 00000000 > [ 1.084787] 5e00: 000001bb c1550860 c1784000 c061fb8c c207bc10 00000000 c1718f84 c1765ea0 > [ 1.084803] 5e20: 00000000 c061d078 c207bc10 c062b764 c207bc10 c207bc10 c1718f84 c1718f84 > [ 1.084818] 5e40: c1765ea0 c061d3e0 c1988000 000001bb c1550860 c17cb200 c17cb204 c1718f84 > [ 1.084834] 5e60: c207bc10 00000000 000001bb c1550860 c1784000 c061d588 00000000 c207bc10 > [ 1.084849] 5e80: c1718f84 c160b708 c1988000 c061dabc 00000000 c1718f84 c061d9d0 c061ac7c > [ 1.084865] 5ea0: c1784000 c2040074 c2054bc0 0f7634d1 c1718f84 c1718f84 cfd92f00 c16edb98 > [ 1.084882] 5ec0: 00000000 c061c050 c1367300 c1758040 c1533c54 c1718f84 c1758040 c1533c54 > [ 1.084895] 5ee0: 00000000 c061e568 c160b708 c1758040 c1533c54 c0102078 c0191390 c1500504 > [ 1.084911] 5f00: c1762f78 00000000 00000006 c132f7c4 c136690c 00000000 00000000 c160b708 > [ 1.084927] 5f20: c12a2e5c c1297aec 39360000 c202722b c2027233 0f7634d1 c168112c c1406728 > [ 1.084943] 5f40: 00000007 0f7634d1 c1406728 c15a7da0 00000007 c1550840 c2027200 c15014d0 > [ 1.084959] 5f60: 00000006 00000006 00000000 c1500504 00000000 c1500504 00000000 c160b700 > [ 1.084976] 5f80: c0df8c68 00000000 00000000 00000000 00000000 00000000 00000000 c0df8c88 > [ 1.084991] 5fa0: 00000000 c0df8c68 00000000 c0100148 00000000 00000000 00000000 00000000 > [ 1.085006] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > [ 1.085022] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 > [ 1.085042] dev_pm_opp_clear_config from exynos_bus_probe+0x384/0x634 > [ 1.085063] exynos_bus_probe from platform_probe+0x64/0xc0 > [ 1.085085] platform_probe from really_probe+0x104/0x3c4 > [ 1.085109] really_probe from __driver_probe_device+0xa8/0x214 > [ 1.085130] __driver_probe_device from driver_probe_device+0x3c/0xdc > [ 1.085149] driver_probe_device from __driver_attach+0xec/0x190 > [ 1.085170] __driver_attach from bus_for_each_dev+0x7c/0xbc > [ 1.085191] bus_for_each_dev from bus_add_driver+0x17c/0x218 > [ 1.085211] bus_add_driver from driver_register+0x7c/0x110 > [ 1.085234] driver_register from do_one_initcall+0x50/0x268 > [ 1.085257] do_one_initcall from kernel_init_freeable+0x234/0x280 > [ 1.085279] kernel_init_freeable from kernel_init+0x20/0x140 > [ 1.085305] kernel_init from ret_from_fork+0x14/0x2c > [ 1.085322] Exception stack(0xf0835fb0 to 0xf0835ff8) I try to find the cause of this warning. I think that dev_pm_opp_clear_config needs to check whether 'opp_table' is NULL or not as following: diff --git a/drivers/opp/core.c b/drivers/opp/core.c index fba6e2b20b8f..cbf8f10b9ff0 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config); */ void dev_pm_opp_clear_config(struct opp_table *opp_table) { + if (unlikely(!opp_table)) + return; + if (opp_table->genpd_virt_devs) dev_pm_opp_detach_genpd(opp_table);
On 31-05-22, 09:45, Viresh Kumar wrote: > On 31-05-22, 13:12, Chanwoo Choi wrote: > > I try to find the cause of this warning. > > I think that dev_pm_opp_clear_config needs to check > > whether 'opp_table' is NULL or not as following: > > > > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > index fba6e2b20b8f..cbf8f10b9ff0 100644 > > --- a/drivers/opp/core.c > > +++ b/drivers/opp/core.c > > @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config); > > */ > > void dev_pm_opp_clear_config(struct opp_table *opp_table) > > { > > + if (unlikely(!opp_table)) > > + return; > > + > > if (opp_table->genpd_virt_devs) > > dev_pm_opp_detach_genpd(opp_table); > > Does this fixes it for you ? > > It isn't allowed to call this routine with opp_table as NULL, I should > rather have a WARN() for the same instead. > > Can you check why exynos is passing NULL here as I don't see an > obvious reason currently. Looking at the exynos devfreq driver again, it feels like the design of the driver itself is causing all these issues. Ideally, whatever resources are acquired by probe() must be freed only and only by remove()/shutdown(). But you are trying to do it from exynos_bus_exit() as well. Calling dev_pm_opp_of_remove_table() as well from this function is wrong as you may very well end up corrupting the OPP refcount and OPP may never get freed or something else may come up. For now I am adding following to the patch, please see if it fixes it or not (I have pushed the changes to my branch as well). diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 780e525eb92a..8fca24565e7d 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -161,8 +161,11 @@ static void exynos_bus_exit(struct device *dev) dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); - dev_pm_opp_clear_config(bus->opp_table); - bus->opp_table = NULL; + + if (bus->opp_table) { + dev_pm_opp_clear_config(bus->opp_table); + bus->opp_table = NULL; + } } static void exynos_bus_passive_exit(struct device *dev) @@ -463,8 +466,10 @@ static int exynos_bus_probe(struct platform_device *pdev) dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); err_reg: - dev_pm_opp_clear_config(bus->opp_table); - bus->opp_table = NULL; + if (bus->opp_table) { + dev_pm_opp_clear_config(bus->opp_table); + bus->opp_table = NULL; + } return ret; }
On 5/31/22 1:15 PM, Viresh Kumar wrote: > On 31-05-22, 13:12, Chanwoo Choi wrote: >> I try to find the cause of this warning. >> I think that dev_pm_opp_clear_config needs to check >> whether 'opp_table' is NULL or not as following: >> >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index fba6e2b20b8f..cbf8f10b9ff0 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config); >> */ >> void dev_pm_opp_clear_config(struct opp_table *opp_table) >> { >> + if (unlikely(!opp_table)) >> + return; >> + >> if (opp_table->genpd_virt_devs) >> dev_pm_opp_detach_genpd(opp_table); > > Does this fixes it for you ? > > It isn't allowed to call this routine with opp_table as NULL, I should > rather have a WARN() for the same instead. > > Can you check why exynos is passing NULL here as I don't see an > obvious reason currently. > exynos-bus.c contains the two type of devfreq device as following: 1. devfreq device controls the regulator 2. devfreq device doesn't control the regulator Above two devfreq devices share the same error path because two devices are similar. As you said, if you use WARN(), I can change it on exynos-bus.c as following: if (bus->opp_table) dev_pm_opp_clear_config(bus->opp_table)
On 31-05-22, 14:05, Chanwoo Choi wrote: > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c > > index 780e525eb92a..8fca24565e7d 100644 > > --- a/drivers/devfreq/exynos-bus.c > > +++ b/drivers/devfreq/exynos-bus.c > > @@ -161,8 +161,11 @@ static void exynos_bus_exit(struct device *dev) > > > > dev_pm_opp_of_remove_table(dev); > > clk_disable_unprepare(bus->clk); > > - dev_pm_opp_clear_config(bus->opp_table); > > - bus->opp_table = NULL; > > + > > + if (bus->opp_table) { > > + dev_pm_opp_clear_config(bus->opp_table); > > + bus->opp_table = NULL; > > + } > > } > > > > static void exynos_bus_passive_exit(struct device *dev) > > @@ -463,8 +466,10 @@ static int exynos_bus_probe(struct platform_device *pdev) > > dev_pm_opp_of_remove_table(dev); > > clk_disable_unprepare(bus->clk); > > err_reg: > > - dev_pm_opp_clear_config(bus->opp_table); > > - bus->opp_table = NULL; > > + if (bus->opp_table) { > > + dev_pm_opp_clear_config(bus->opp_table); > > + bus->opp_table = NULL; > > + } > > > > return ret; > > } > > > > This change is enough to remove the null pointer error. Thanks. Pushed this and WARN_ON() in OPP core. Thanks.
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index e689101abc93..780e525eb92a 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -161,7 +161,7 @@ static void exynos_bus_exit(struct device *dev) dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); - dev_pm_opp_put_regulators(bus->opp_table); + dev_pm_opp_clear_config(bus->opp_table); bus->opp_table = NULL; } @@ -182,11 +182,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np, struct opp_table *opp_table; const char *vdd = "vdd"; int i, ret, count, size; + struct dev_pm_opp_config config = { + .regulator_names = &vdd, + .regulator_count = 1, + }; - opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1); + opp_table = dev_pm_opp_set_config(dev, &config); if (IS_ERR(opp_table)) { ret = PTR_ERR(opp_table); - dev_err(dev, "failed to set regulators %d\n", ret); + dev_err(dev, "failed to set OPP config %d\n", ret); return ret; } @@ -236,7 +240,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np, return 0; err_regulator: - dev_pm_opp_put_regulators(bus->opp_table); + dev_pm_opp_clear_config(bus->opp_table); bus->opp_table = NULL; return ret; @@ -459,7 +463,7 @@ static int exynos_bus_probe(struct platform_device *pdev) dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); err_reg: - dev_pm_opp_put_regulators(bus->opp_table); + dev_pm_opp_clear_config(bus->opp_table); bus->opp_table = NULL; return ret;
The OPP core now provides a unified API for setting all configuration types, i.e. dev_pm_opp_set_config(). Lets start using it. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/devfreq/exynos-bus.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)