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

[Lukasz Luba <lukasz.luba@arm.com>]

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