cpufreq: tegra194: remove opp table in exit hook

PATCH Superseded Sumit Gupta 2 participants 5 messages Aug 9, 2023 – Aug 25, 2023

Subsystems (2)

Tegra architecture support CPU frequency scaling framework

lore

Share & tools

lore

Sumit Gupta

author

2023-08-09 15:40 UTC #0 lore raw

PATCH 1/1 cpufreq: tegra194: remove opp table in exit hook

Add exit hook and remove OPP table when all the CPU's in a policy are offlined. It will fix the below error messages when onlining first CPU from a policy whose all CPU's were previously offlined.

debugfs: File 'cpu5' in directory 'opp' already present!
debugfs: File 'cpu6' in directory 'opp' already present!
debugfs: File 'cpu7' in directory 'opp' already present!

Fixes f41e1442ac5b ("cpufreq: tegra194: add OPP support and set bandwidth")

Signed-off-by Sumit Gupta <[email protected]>

---
 drivers/cpufreq/tegra194-cpufreq.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/cpufreq/tegra194-cpufreq.c +13 -0

index c90b30469165..66a9c23544db 100644
@@ -454,6 +454,8 @@ static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
 		if (ret < 0)
 			return ret;
 
+		dev_pm_opp_put(opp);
+
 		freq_table[j].driver_data = pos->driver_data;
 		freq_table[j].frequency = pos->frequency;
 		j++;

@@ -508,6 +510,16 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
 	return 0;
 }
 
+static int tegra194_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	struct device *cpu_dev = get_cpu_device(policy->cpu);
+
+	dev_pm_opp_remove_all_dynamic(cpu_dev);
+	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
+
+	return 0;
+}
+
 static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
 				       unsigned int index)
 {

@@ -535,6 +547,7 @@ static struct cpufreq_driver tegra194_cpufreq_driver = {
 	.target_index = tegra194_cpufreq_set_target,
 	.get = tegra194_get_speed,
 	.init = tegra194_cpufreq_init,
+	.exit = tegra194_cpufreq_exit,
 	.attr = cpufreq_generic_attr,
 };

Signature · 1 line

2.17.1

Viresh Kumar

maintainer

2023-08-10 05:31 UTC #1 lore raw

Quoted 45 lines · Sumit Gupta

Add exit hook and remove OPP table when all the CPU's in a policy are offlined. It will fix the below error messages when onlining first CPU from a policy whose all CPU's were previously offlined.
debugfs: File 'cpu5' in directory 'opp' already present!
debugfs: File 'cpu6' in directory 'opp' already present!
debugfs: File 'cpu7' in directory 'opp' already present!
Fixes f41e1442ac5b ("cpufreq: tegra194: add OPP support and set bandwidth")
Signed-off-by Sumit Gupta <[email protected]>
---
 drivers/cpufreq/tegra194-cpufreq.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/cpufreq/tegra194-cpufreq.c +12 -0
index c90b30469165..66a9c23544db 100644
@@ -454,6 +454,8 @@ static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
 		if (ret < 0)
 			return ret;
 
+		dev_pm_opp_put(opp);
+
 		freq_table[j].driver_data = pos->driver_data;
 		freq_table[j].frequency = pos->frequency;
 		j++;
@@ -508,6 +510,16 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
 	return 0;
 }
 
+static int tegra194_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	struct device *cpu_dev = get_cpu_device(policy->cpu);
+
+	dev_pm_opp_remove_all_dynamic(cpu_dev);
+	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
+
+	return 0;
+}
+
 static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
 				       unsigned int index)
 {

drivers/cpufreq/tegra194-cpufreq.c static struct cpufreq_driver tegra194_cpufreq_driver = { · line 552

Quoted 6 lines

drivers/cpufreq/tegra194-cpufreq.c +1 -0

@@ -535,6 +547,7 @@ static struct cpufreq_driver tegra194_cpufreq_driver = {
 	.target_index = tegra194_cpufreq_set_target,
 	.get = tegra194_get_speed,
 	.init = tegra194_cpufreq_init,
+	.exit = tegra194_cpufreq_exit,
 	.attr = cpufreq_generic_attr,
 };

If it is only about hotplugging of the CPUs, then you can also do this I guess.

commit 263abfe74b5f ("cpufreq: dt: Implement online/offline() callbacks")

But since your driver is capable of being built as a module, I suggest you try to build it as one and insert remove it multiple times. It must cause you some trouble as you don't implement an .exit() before this patch.

Eventually, I think you need to do both, what this patch and 263abfe74b5f do. Just that the reasons need to be correct for both the changes.

Signature · 1 line

viresh

Sumit Gupta

author

2023-08-14 18:14 UTC #2 lore raw

Quoted 64 lines · Viresh Kumar

External email: Use caution opening links or attachments
Quoted 45 lines · Sumit Gupta
Add exit hook and remove OPP table when all the CPU's in a policy are offlined. It will fix the below error messages when onlining first CPU from a policy whose all CPU's were previously offlined.
debugfs: File 'cpu5' in directory 'opp' already present!
debugfs: File 'cpu6' in directory 'opp' already present!
debugfs: File 'cpu7' in directory 'opp' already present!
Fixes f41e1442ac5b ("cpufreq: tegra194: add OPP support and set bandwidth")
Signed-off-by Sumit Gupta <[email protected]>
---
  drivers/cpufreq/tegra194-cpufreq.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
drivers/cpufreq/tegra194-cpufreq.c +12 -0
index c90b30469165..66a9c23544db 100644
@@ -454,6 +454,8 @@ static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
               if (ret < 0)
                       return ret;
 
+             dev_pm_opp_put(opp);
+
               freq_table[j].driver_data = pos->driver_data;
               freq_table[j].frequency = pos->frequency;
               j++;
@@ -508,6 +510,16 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
       return 0;
  }
 
+static int tegra194_cpufreq_exit(struct cpufreq_policy *policy)
+{
+     struct device *cpu_dev = get_cpu_device(policy->cpu);
+
+     dev_pm_opp_remove_all_dynamic(cpu_dev);
+     dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
+
+     return 0;
+}
+
  static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
                                      unsigned int index)
  {
drivers/cpufreq/tegra194-cpufreq.c static struct cpufreq_driver tegra194_cpufreq_driver = { · line 552
Quoted 6 lines
drivers/cpufreq/tegra194-cpufreq.c +1 -0
@@ -535,6 +547,7 @@ static struct cpufreq_driver tegra194_cpufreq_driver = {
       .target_index = tegra194_cpufreq_set_target,
       .get = tegra194_get_speed,
       .init = tegra194_cpufreq_init,
+     .exit = tegra194_cpufreq_exit,
       .attr = cpufreq_generic_attr,
  };
If it is only about hotplugging of the CPUs, then you can also do this I guess.
commit 263abfe74b5f ("cpufreq: dt: Implement online/offline() callbacks")
But since your driver is capable of being built as a module, I suggest you try to build it as one and insert remove it multiple times. It must cause you some trouble as you don't implement an .exit() before this patch.

Quoted 5 lines

Eventually, I think you need to do both, what this patch and 263abfe74b5f do. Just that the reasons need to be correct for both the changes.
--
viresh

Hi Viresh,
I got the same message on inserting and removing the module multiple times as you suggested. After applying this change, the message is not coming. So, the current change is resolving both scenarios as __cpufreq_offline() calls either exit() or offline(). I can update the commit message to mention both scenarios and keep change as it is?

cpufreq_remove_dev
|-__cpufreq_offline
|--tegra194_cpufreq_exit

cpuhp_cpufreq_offline
|-__cpufreq_offline
|--tegra194_cpufreq_exit

Thank you,
Sumit Gupta

Viresh Kumar

maintainer

2023-08-16 03:37 UTC #3 lore raw

Quoted 59 lines · Sumit Gupta

Quoted 58 lines · Viresh Kumar
External email: Use caution opening links or attachments
Quoted 45 lines · Sumit Gupta
Add exit hook and remove OPP table when all the CPU's in a policy are offlined. It will fix the below error messages when onlining first CPU from a policy whose all CPU's were previously offlined.
debugfs: File 'cpu5' in directory 'opp' already present!
debugfs: File 'cpu6' in directory 'opp' already present!
debugfs: File 'cpu7' in directory 'opp' already present!
Fixes f41e1442ac5b ("cpufreq: tegra194: add OPP support and set bandwidth")
Signed-off-by Sumit Gupta <[email protected]>
---
  drivers/cpufreq/tegra194-cpufreq.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
drivers/cpufreq/tegra194-cpufreq.c +12 -0
index c90b30469165..66a9c23544db 100644
@@ -454,6 +454,8 @@ static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
               if (ret < 0)
                       return ret;
 
+             dev_pm_opp_put(opp);
+
               freq_table[j].driver_data = pos->driver_data;
               freq_table[j].frequency = pos->frequency;
               j++;
@@ -508,6 +510,16 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
       return 0;
  }
 
+static int tegra194_cpufreq_exit(struct cpufreq_policy *policy)
+{
+     struct device *cpu_dev = get_cpu_device(policy->cpu);
+
+     dev_pm_opp_remove_all_dynamic(cpu_dev);
+     dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
+
+     return 0;
+}
+
  static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
                                      unsigned int index)
  {
drivers/cpufreq/tegra194-cpufreq.c static struct cpufreq_driver tegra194_cpufreq_driver = { · line 552
Quoted 6 lines
drivers/cpufreq/tegra194-cpufreq.c +1 -0
@@ -535,6 +547,7 @@ static struct cpufreq_driver tegra194_cpufreq_driver = {
       .target_index = tegra194_cpufreq_set_target,
       .get = tegra194_get_speed,
       .init = tegra194_cpufreq_init,
+     .exit = tegra194_cpufreq_exit,
       .attr = cpufreq_generic_attr,
  };
If it is only about hotplugging of the CPUs, then you can also do this I guess.

Quoted 1 line

Quoted 1 line
commit 263abfe74b5f ("cpufreq: dt: Implement online/offline() callbacks")

You should do this as well, this makes hotplugging paths much faster. i.e. on top of this patch.

Quoted 25 lines

Quoted 9 lines
But since your driver is capable of being built as a module, I suggest you try to build it as one and insert remove it multiple times. It must cause you some trouble as you don't implement an .exit() before this patch.
Eventually, I think you need to do both, what this patch and 263abfe74b5f do. Just that the reasons need to be correct for both the changes.
--
viresh
Hi Viresh,
I got the same message on inserting and removing the module multiple times as you suggested. After applying this change, the message is not coming. So, the current change is resolving both scenarios as __cpufreq_offline() calls either exit() or offline().
I can update the commit message to mention both scenarios and keep change as it is?
cpufreq_remove_dev
|-__cpufreq_offline
|--tegra194_cpufreq_exit
cpuhp_cpufreq_offline
|-__cpufreq_offline
|--tegra194_cpufreq_exit

Signature · 1 line

viresh

Sumit Gupta

author

2023-08-25 11:34 UTC #4 lore raw

Quoted 55 lines

Quoted 46 lines

Add exit hook and remove OPP table when all the CPU's in a policy are offlined. It will fix the below error messages when onlining first CPU from a policy whose all CPU's were previously offlined.

debugfs: File 'cpu5' in directory 'opp' already present!
debugfs: File 'cpu6' in directory 'opp' already present!
debugfs: File 'cpu7' in directory 'opp' already present!

Fixes f41e1442ac5b ("cpufreq: tegra194: add OPP support and set bandwidth")

Signed-off-by Sumit Gupta <[email protected]>

---
   drivers/cpufreq/tegra194-cpufreq.c | 13 +++++++++++++
   1 file changed, 13 insertions(+)

drivers/cpufreq/tegra194-cpufreq.c +12 -0

index c90b30469165..66a9c23544db 100644
@@ -454,6 +454,8 @@ static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
                if (ret < 0)
                        return ret;
 
+             dev_pm_opp_put(opp);
+
                freq_table[j].driver_data = pos->driver_data;
                freq_table[j].frequency = pos->frequency;
                j++;

@@ -508,6 +510,16 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
        return 0;
   }
 
+static int tegra194_cpufreq_exit(struct cpufreq_policy *policy)
+{
+     struct device *cpu_dev = get_cpu_device(policy->cpu);
+
+     dev_pm_opp_remove_all_dynamic(cpu_dev);
+     dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
+
+     return 0;
+}
+
   static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
                                       unsigned int index)
   {

drivers/cpufreq/tegra194-cpufreq.c static struct cpufreq_driver tegra194_cpufreq_driver = { · line 552

Quoted 6 lines

drivers/cpufreq/tegra194-cpufreq.c +1 -0

@@ -535,6 +547,7 @@ static struct cpufreq_driver tegra194_cpufreq_driver = {
        .target_index = tegra194_cpufreq_set_target,
        .get = tegra194_get_speed,
        .init = tegra194_cpufreq_init,
+     .exit = tegra194_cpufreq_exit,
        .attr = cpufreq_generic_attr,
   };

If it is only about hotplugging of the CPUs, then you can also do this I guess.

Quoted 5 lines

Quoted 1 line
Quoted 1 line
commit 263abfe74b5f ("cpufreq: dt: Implement online/offline() callbacks")
You should do this as well, this makes hotplugging paths much faster. i.e. on top of this patch.

Sent a separate patch to add online/offline callbacks.
https://lore.kernel.org/lkml/[email protected]/

Also, sent v2 of this patch with updated commit description.
https://lore.kernel.org/lkml/[email protected]/

Thank you,
Sumit Gupta

Quoted 28 lines

Quoted 18 lines
Quoted 9 lines
But since your driver is capable of being built as a module, I suggest you try to build it as one and insert remove it multiple times. It must cause you some trouble as you don't implement an .exit() before this patch.
Eventually, I think you need to do both, what this patch and 263abfe74b5f do. Just that the reasons need to be correct for both the changes.
--
viresh
Hi Viresh,
I got the same message on inserting and removing the module multiple times as you suggested. After applying this change, the message is not coming. So, the current change is resolving both scenarios as __cpufreq_offline() calls either exit() or offline().
I can update the commit message to mention both scenarios and keep change as it is?
Quoted 7 lines
   cpufreq_remove_dev
   |-__cpufreq_offline
   |--tegra194_cpufreq_exit

   cpuhp_cpufreq_offline
   |-__cpufreq_offline
   |--tegra194_cpufreq_exit
--
viresh