On Tue, Apr 16, 2019 at 03:38:39PM -0400, Thara Gopinath wrote:
> +static void thermal_pressure_update(struct thermal_pressure *cpu_thermal,
> + unsigned long cap_max_freq,
> + unsigned long max_freq, bool change_scale)
> +{
> + unsigned long flags = 0;
> +
> + calculate_thermal_pressure(cpu_thermal);
> + if (change_scale)
> + cpu_thermal->raw_scale =
> + (cap_max_freq << SCHED_CAPACITY_SHIFT) / max_freq;
That needs {} per coding style.
> +
> + mod_timer(&cpu_thermal->timer, jiffies +
> + usecs_to_jiffies(TICK_USEC));
> +
> + spin_unlock_irqrestore(&cpu_thermal->lock, flags);
This is busted has heck, @flags should be the result of irqsave(.flags).
> +}
> +
> +/**
> + * Function for the tick update of the thermal pressure.
> + * The thermal pressure update is aborted if already an update is
> + * happening.
> + */
> +static void thermal_pressure_timeout(struct timer_list *timer)
> +{
> + struct thermal_pressure *cpu_thermal = from_timer(cpu_thermal, timer,
> + timer);
If you split after the = the result is so very much easier to read.
> + unsigned long flags = 0;
> +
> + if (!cpu_thermal)
> + return;
> +
> + if (!spin_trylock_irqsave(&cpu_thermal->lock, flags))
> + return;
> +
> + thermal_pressure_update(cpu_thermal, 0, 0, 0);
> +}
> +
> +/**
> + * Function to update thermal pressure from cooling device
> + * or any framework responsible for capping cpu maximum
> + * capacity.
Would be useful to know how wide @cpus is, typically. Is that the power
culster?
> + */
> +void sched_update_thermal_pressure(struct cpumask *cpus,
> + unsigned long cap_max_freq,
> + unsigned long max_freq)
> +{
> + int cpu;
> + unsigned long flags = 0;
> + struct thermal_pressure *cpu_thermal;
You got them in exactly the wrong order here.
> +
> + for_each_cpu(cpu, cpus) {
> + cpu_thermal = per_cpu(thermal_pressure_cpu, cpu);
> + if (!cpu_thermal)
> + return;
> + spin_lock_irqsave(&cpu_thermal->lock, flags);
> + thermal_pressure_update(cpu_thermal, cap_max_freq, max_freq, 1);
> + }
> +}
That's just horrible style. Move the unlock out of
thermal_pressure_update() such that lock and unlock are in the same
function.
