[PATCH 00/31] OPP: Add new configuration interface: dev_pm_opp_set_config()

[PATCH 00/31] OPP: Add new configuration interface: dev_pm_opp_set_config()

[Viresh Kumar]

Hello,

We have too many configuration specific APIs currently, six of them already,
like dev_pm_opp_set_regulators(). This makes it complex/messy for both the OPP
core and its users to manage. There is also code redundancy in these API, in the
way they add/manage the OPP table specific stuff.

This patch series is an attempt to simplify these interfaces by adding a single
interface, dev_pm_opp_set_config(), which replaces all the existing ones. This
also migrates the users to the new API.

The first two patches help get the API in place, followed by patches to migrate
the end users. Once all the users are migrated, the last few patches remove the
now unused interfaces.

I have lightly tested this on Hikey960 for now and also getting help from
various build/boot bots, gitlab and lkp, to get these tested. It would be
helpful if someone with access to the affected platforms can give it a try.

This is pushed here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/config

The entire patchset shall get merged via the OPP tree in 5.20-rc1, please do not
merge individual patches.

Thanks.

--
Viresh

Viresh Kumar (31):
  OPP: Track if clock name is configured by platform
  OPP: Add dev_pm_opp_set_config() and friends
  cpufreq: dt: Migrate to dev_pm_opp_set_config()
  cpufreq: imx: Migrate to dev_pm_opp_set_config()
  cpufreq: qcom-nvmem: Migrate to dev_pm_opp_set_config()
  cpufreq: sti: Migrate to dev_pm_opp_set_config()
  cpufreq: sun50i: Migrate to dev_pm_opp_set_config()
  cpufreq: tegra20: Migrate to dev_pm_opp_set_config()
  cpufreq: ti: Migrate to dev_pm_opp_set_config()
  devfreq: exynos: Migrate to dev_pm_opp_set_config()
  devfreq: sun8i: Migrate to dev_pm_opp_set_config()
  devfreq: tegra30: Migrate to dev_pm_opp_set_config()
  drm/lima: Migrate to dev_pm_opp_set_config()
  drm/msm: Migrate to dev_pm_opp_set_config()
  drm/panfrost: Migrate to dev_pm_opp_set_config()
  drm/tegra: Migrate to dev_pm_opp_set_config()
  media: venus: Migrate to dev_pm_opp_set_config()
  media: tegra: Migrate to dev_pm_opp_set_config()
  mmc: sdhci-msm: Migrate to dev_pm_opp_set_config()
  OPP: ti: Migrate to dev_pm_opp_set_config()
  soc/tegra: Remove the call to devm_pm_opp_set_clkname()
  soc/tegra: Migrate to dev_pm_opp_set_config()
  spi: qcom: Migrate to dev_pm_opp_set_config()
  serial: qcom: Migrate to dev_pm_opp_set_config()
  OPP: Remove dev_pm_opp_set_regulators() and friends
  OPP: Remove dev_pm_opp_set_supported_hw() and friends
  OPP: Remove dev_pm_opp_set_clkname() and friends
  OPP: Remove dev_pm_opp_register_set_opp_helper() and friends
  OPP: Remove dev_pm_opp_attach_genpd() and friends
  OPP: Remove dev_pm_opp_set_prop_name() and friends
  OPP: Rearrange dev_pm_opp_set_config() and friends

 drivers/cpufreq/cpufreq-dt.c                  |  14 +-
 drivers/cpufreq/imx-cpufreq-dt.c              |  12 +-
 drivers/cpufreq/qcom-cpufreq-nvmem.c          | 107 +---
 drivers/cpufreq/sti-cpufreq.c                 |  22 +-
 drivers/cpufreq/sun50i-cpufreq-nvmem.c        |  11 +-
 drivers/cpufreq/tegra20-cpufreq.c             |  12 +-
 drivers/cpufreq/ti-cpufreq.c                  |  38 +-
 drivers/devfreq/exynos-bus.c                  |  14 +-
 drivers/devfreq/sun8i-a33-mbus.c              |   7 +-
 drivers/devfreq/tegra30-devfreq.c             |   8 +-
 drivers/gpu/drm/lima/lima_devfreq.c           |  11 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c         |   8 +-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c         |  10 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   5 +-
 drivers/gpu/drm/msm/dp/dp_ctrl.c              |   5 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c            |   5 +-
 drivers/gpu/drm/panfrost/panfrost_devfreq.c   |   9 +-
 drivers/gpu/drm/tegra/gr3d.c                  |   6 +-
 .../media/platform/qcom/venus/pm_helpers.c    |  16 +-
 drivers/memory/tegra/tegra124-emc.c           |  14 +-
 drivers/mmc/host/sdhci-msm.c                  |   5 +-
 drivers/opp/core.c                            | 540 +++++++-----------
 drivers/opp/opp.h                             |   2 +
 drivers/opp/ti-opp-supply.c                   |   6 +-
 drivers/soc/tegra/common.c                    |  14 +-
 drivers/soc/tegra/pmc.c                       |   8 +-
 drivers/spi/spi-geni-qcom.c                   |   5 +-
 drivers/spi/spi-qcom-qspi.c                   |   5 +-
 drivers/tty/serial/qcom_geni_serial.c         |   5 +-
 include/linux/pm_opp.h                        | 118 ++--
 30 files changed, 444 insertions(+), 598 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

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;
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Chanwoo Choi]

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)



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Chanwoo Choi]

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);


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

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;
 }

-- 
viresh

Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Chanwoo Choi]

On 5/31/22 1:38 PM, Viresh Kumar wrote:
> 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;
>  }
> 

This change is enough to remove the null pointer error. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH 10/31] devfreq: exynos: Migrate to dev_pm_opp_set_config()

[Chanwoo Choi]

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)


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics