Re: [PATCH 2/2] cpufreq: powernv: Ramp-down global pstate slower than local-pstate

[Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>]

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

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev