Re: [PATCH V4 2/3] thermal/drivers/cpu_cooling: Unregister with the policy

[Rafael J. Wysocki]

On Fri, Jun 28, 2019 at 11:12 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, Jun 27, 2019 at 11:02 PM Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:
> >
> > 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 <daniel.lezcano@xxxxxxxxxx>
> > Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

[cut]

> > -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. ->

>
> >         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) {
>
> > +               if (cpufreq_cdev->policy == policy) {
>
>                            cpufreq_cdev = ccd;
>
> > +                       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.
>
> > -       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.
>
>
> > -       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);
> >  }