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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Jun 2019 12:07:23 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Viresh Kumar <viresh.kumar@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Eduardo Valentin <edubezval@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Amit Daniel Kachhap <amit.kachhap@...il.com>,
        Javi Merino <javi.merino@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Keerthy <j-keerthy@...com>,
        "open list:CPU FREQUENCY DRIVERS - ARM BIG LITTLE" 
        <linux-pm@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:TI BANDGAP AND THERMAL DRIVER" 
        <linux-omap@...r.kernel.org>
Subject: Re: [PATCH V4 2/3] thermal/drivers/cpu_cooling: Unregister with the policy

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

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