linux-kernel - Re: [PATCH v2 1/5] arch_topology: Introduce thermal pressure update function

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:   Wed, 27 Oct 2021 09:56:28 +0100
From:   Lukasz Luba <lukasz.luba@....com>
To:     Dietmar Eggemann <dietmar.eggemann@....com>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, sudeep.holla@....com,
        will@...nel.org, catalin.marinas@....com, linux@...linux.org.uk,
        gregkh@...uxfoundation.org, rafael@...nel.org,
        viresh.kumar@...aro.org, amitk@...nel.org,
        daniel.lezcano@...aro.org, amit.kachhap@...il.com,
        thara.gopinath@...aro.org, bjorn.andersson@...aro.org,
        agross@...nel.org
Subject: Re: [PATCH v2 1/5] arch_topology: Introduce thermal pressure update
 function

Hi Dietmar,

Thank you for having a look at this.

On 10/26/21 5:51 PM, Dietmar Eggemann wrote:
> On 15/10/2021 16:45, Lukasz Luba wrote:

[snip]

>> +#define arch_thermal_pressure_update	topology_thermal_pressure_update
> 
> s/thermal_pressure_update/update_thermal_pressure ?

I can reorder that naming.

> 
> The scheme seems to be {arch|topology}_*foo*_thermal_pressure
> 
> But ...
> 
>>   

[snip]

>> +void topology_thermal_pressure_update(const struct cpumask *cpus,
>> +				      unsigned long capped_freq)
>> +{
> 
> ... why not just s/unsigned long th_pressure/unsigned long capped_freq
> in existing topology_set_thermal_pressure() and move code the
> frequency/capacity conversion in there? The patch set will become
> considerably smaller.

I've been trying to avoid confusion when changing actually behavior
of the API function. Thus, introducing new would IMO opinion
make sure the old 'set' function was expecting proper pressure
value, while the new 'update' expects frequency.

I agree that the patch set would be smaller in that case, but I'm
not sure if that would not hide some issues. This one would
definitely break compilation of some vendor modules (or drivers
queuing or under review), not silently passing them through (with wrong
argument).

> 
>   void topology_set_thermal_pressure(const struct cpumask *cpus,
> -                              unsigned long th_pressure)
> +                              unsigned long capped_freq)

[snip]

>   EXPORT_SYMBOL_GPL(topology_set_thermal_pressure);
> 
> And a user like [drivers/thermal/cpufreq_cooling.c] can call
> arch_set_thermal_pressure(cpus, frequency).
> 
> [...]
> 

I'm not sure if that is a safe way.

Regards,
Lukasz