[PATCH 2/2] cpufreq: powernv: Ramp-down global pstate slower than local-pstate
Akshay Adiga
akshay.adiga at linux.vnet.ibm.com
Thu Apr 14 03:57:51 AEST 2016
Hi Viresh ,
Thanks for reviewing in detail.
I will correct all comments related to coding standards in my next patch.
On 04/13/2016 10:33 AM, Viresh Kumar wrote:
> Comments mostly on the coding standards which you have *not* followed.
>
> Also, please run checkpatch --strict next time you send patches
> upstream.
Thanks for pointing out the --strict option, was not aware of that. I will
run checkpatch --strict on the next versions.
> On 12-04-16, 23:36, Akshay Adiga wrote:
> +
> +/*
> + * While resetting we don't want "timer" fields to be set to zero as we
> + * may lose track of timer and will not be able to cleanly remove it
> + */
> +#define reset_gpstates(policy) memset(policy->driver_data, 0,\
> + sizeof(struct global_pstate_info)-\
> + sizeof(struct timer_list)-\
> + sizeof(spinlock_t))
> That's super *ugly*. Why don't you create a simple routine which will
> set the 5 integer variables to 0 in a straight forward way ?
Yeh, will create a routine.
>> @@ -348,14 +395,17 @@ static void set_pstate(void *freq_data)
>> unsigned long val;
>> unsigned long pstate_ul =
>> ((struct powernv_smp_call_data *) freq_data)->pstate_id;
>> + unsigned long gpstate_ul =
>> + ((struct powernv_smp_call_data *) freq_data)->gpstate_id;
> Remove these unnecessary casts and do:
>
> struct powernv_smp_call_data *freq_data = data; //Name func arg as data
>
> And then use freq_data->*.
Ok. Will do that.
>> +/*
>> + * gpstate_timer_handler
>> + *
>> + * @data: pointer to cpufreq_policy on which timer was queued
>> + *
>> + * This handler brings down the global pstate closer to the local pstate
>> + * according quadratic equation. Queues a new timer if it is still not equal
>> + * to local pstate
>> + */
>> +void gpstate_timer_handler(unsigned long data)
>> +{
>> + struct cpufreq_policy *policy = (struct cpufreq_policy *) data;
> no need to cast.
May be i need a cast here, because data is unsigned long ( unlike other places where its void *).
On building without cast, it throws me a warning.
>> + struct global_pstate_info *gpstates = (struct global_pstate_info *)
>> + struct powernv_smp_call_data freq_data;
>> + int ret;
>> +
>> + ret = spin_trylock(&gpstates->gpstate_lock);
> no need of 'ret' for just this, simply do: if (!spin_trylock())...
Sure will do that.
> a
>> + if (!ret)
>> + return;
>> +
>> + gpstates->last_sampled_time += time_diff;
>> + gpstates->elapsed_time += time_diff;
>> + freq_data.pstate_id = gpstates->last_lpstate;
>> + if ((gpstates->last_gpstate == freq_data.pstate_id) ||
>> + (gpstates->elapsed_time > MAX_RAMP_DOWN_TIME)) {
>> + freq_data.gpstate_id = freq_data.pstate_id;
>> + reset_gpstates(policy);
>> + gpstates->highest_lpstate = freq_data.pstate_id;
>> + } else {
>> + freq_data.gpstate_id = calculate_global_pstate(
> You can't break a line after ( of a function call :)
>
> Let it go beyond 80 columns if it has to.
May be i will try to get it inside 80 columns with a temporary variable instead of
freq_data.gpstate_id.
>> + gpstates->elapsed_time, gpstates->highest_lpstate,
>> + freq_data.pstate_id);
>> + }
>> +
>> + /* If local pstate is equal to global pstate, rampdown is over
> Bad style again.
>
>> + * So timer is not required to be queued.
>> + */
>> + if (freq_data.gpstate_id != freq_data.pstate_id)
>> + ret = queue_gpstate_timer(gpstates);
> ret not used.
Should i make it void instead of returning int?, as i cannot do much even if it fails, except for notifying.
>> +gpstates_done:
>> + gpstates->last_sampled_time = cur_msec;
>> + gpstates->last_gpstate = freq_data.gpstate_id;
>> + gpstates->last_lpstate = freq_data.pstate_id;
>> +
>> /*
>> * Use smp_call_function to send IPI and execute the
>> * mtspr on target CPU. We could do that without IPI
>> * if current CPU is within policy->cpus (core)
>> */
>> smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
>> + spin_unlock_irqrestore(&gpstates->gpstate_lock, flags);
>> + return 0;
>> +}
>>
>> +static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> Add this after the init() routine.
Ok will do it.
>> + policy->driver_data = gpstates;
>> +
>> + /* initialize timer */
>> + init_timer_deferrable(&gpstates->timer);
>> + gpstates->timer.data = (unsigned long) policy;
>> + gpstates->timer.function = gpstate_timer_handler;
>> + gpstates->timer.expires = jiffies +
>> + msecs_to_jiffies(GPSTATE_TIMER_INTERVAL);
>> +
>> + pr_info("Added global_pstate_info & timer for %d cpu\n", base);
>> return cpufreq_table_validate_and_show(policy, powernv_freqs);
> Who will free gpstates if this fails ?
Thanks for pointing out. Will fix in v2.
Regards
Akshay Adiga
More information about the Linuxppc-dev
mailing list