linux-kernel - Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal pressure definition

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]

Message-ID: <20200620174912.GA18358@arm.com>
Date:   Sat, 20 Jun 2020 18:49:12 +0100
From:   Ionela Voinescu <ionela.voinescu@....com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     Valentin Schneider <valentin.schneider@....com>,
        Juri Lelli <juri.lelli@...hat.com>,
        "open list:THERMAL" <linux-pm@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Amit Daniel Kachhap <amit.kachhap@...il.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Russell King <linux@...linux.org.uk>,
        Thara Gopinath <thara.gopinath@...aro.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...nel.org>,
        LAK <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/3] thermal/cpu-cooling, sched/core: Cleanup thermal
 pressure definition

Hi Vincent,

On Thursday 18 Jun 2020 at 17:03:24 (+0200), Vincent Guittot wrote:
> On Sun, 14 Jun 2020 at 03:10, Valentin Schneider
> <valentin.schneider@....com> wrote:
[..]
> > diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> > index e297e135c031..a1efd379b683 100644
> > --- a/drivers/thermal/cpufreq_cooling.c
> > +++ b/drivers/thermal/cpufreq_cooling.c
> > @@ -417,6 +417,11 @@ static int cpufreq_get_cur_state(struct thermal_cooling_device *cdev,
> >         return 0;
> >  }
> >
> > +__weak void
> > +arch_set_thermal_pressure(const struct cpumask *cpus, unsigned long th_pressure)
> > +{
> > +}
> 
> Having this weak function declared in cpufreq_cooling is weird. This
> means that we will have to do so for each one that wants to use it.
> 
> Can't you declare an empty function in a common header file ?

Do we expect anyone other than cpufreq_cooling to call
arch_set_thermal_pressure()?

I'm not against any of the options, either having it here as a week
default definition (same as done for arch_set_freq_scale() in cpufreq.c)
or in a common header (as done for arch_scale_freq_capacity() in sched.h).

But for me, Valentin's implementation seems more natural as setters are
usually only called from within the framework that does the control
(throttling for thermal or frequency setting for cpufreq) and we
probably want to think twice if we want to call them from other places.

Thanks,
Ionela.