On Fri, Jun 28, 2019 at 11:12 AM Rafael J. Wysocki [off-list ref] wrote:

On Thu, Jun 27, 2019 at 11:02 PM Daniel Lezcano
[off-list ref] wrote:

quoted

Currently the function cpufreq_cooling_register() returns a cooling
device pointer which is used back as a pointer to call the function
cpufreq_cooling_unregister(). Even if it is correct, it would make
sense to not leak the structure inside a cpufreq driver and keep the
code thermal code self-encapsulate. Moreover, that forces to add an
extra variable in each driver using this function.

Instead of passing the cooling device to unregister, pass the policy.

Because the cpufreq_cooling_unregister() function uses the policy to
unregister itself. The only purpose of the cooling device pointer is
to unregister the cpu cooling device.

As there is no more need of this pointer, remove it.

Signed-off-by: Daniel Lezcano <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

[cut]

quoted

-void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
+void cpufreq_cooling_unregister(struct cpufreq_policy *policy)
 {
        struct cpufreq_cooling_device *cpufreq_cdev;

I would do

        struct cpufreq_cooling_device *ccd, *cpufreq_cdev = NULL;

and then ->

Not even that. ->

quoted

        bool last;

-       if (!cdev)
-               return;
-
-       cpufreq_cdev = cdev->devdata;
-
        mutex_lock(&cooling_list_lock);
-       list_del(&cpufreq_cdev->node);
-       /* Unregister the notifier for the last cpufreq cooling device */
-       last = list_empty(&cpufreq_cdev_list);
+       list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) {

-> list_for_each_entry(ccd, &cpufreq_cdev_list, node) {
                if (ccd->policy == policy) {

quoted

+               if (cpufreq_cdev->policy == policy) {

                           cpufreq_cdev = ccd;

quoted

+                       list_del(&cpufreq_cdev->node);
+                       last = list_empty(&cpufreq_cdev_list);
+                       break;
+               }
+       }
        mutex_unlock(&cooling_list_lock);

And here

if (!cpufreq_cdev)
        return;

-> It would be sufficient to simply do:

if (cpufreq_cdev->policy != policy)
        return;

here AFAICS.

And that's it.  No new functions needed.

quoted

-       if (last)
-               cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block,
-                                           CPUFREQ_POLICY_NOTIFIER);
-

And I don't that the above needs to be changed at all in any case.

quoted

-       thermal_cooling_device_unregister(cdev);
-       ida_simple_remove(&cpufreq_ida, cpufreq_cdev->id);
-       kfree(cpufreq_cdev->idle_time);
-       kfree(cpufreq_cdev);
+       if (cpufreq_cdev->policy == policy)
+               __cpufreq_cooling_unregister(cpufreq_cdev, last);
 }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel