On Tuesday, October 06, 2015 05:08:14 PM Pandruvada, Srinivas wrote:

On Mon, 2015-10-05 at 17:43 -0700, Srinivas Pandruvada wrote:

quoted

On Tue, 2015-10-06 at 00:56 +0200, Rafael J. Wysocki wrote:

[..]

quoted

quoted

 struct vid_data {

@@ -132,6 +133,8 @@ struct pstate_funcs {
    int (*get_scaling)(void);
    void (*set)(struct cpudata*, int pstate);
    void (*get_vid)(struct cpudata *);
+   u64 (*get_turbo_ratio_limit)(struct cpudata *);
+   int (*set_turbo_ratio_limit)(struct cpudata *, u64, u64);

I've started to wonder about the value of having the second new callback at all.

The guys who need it will always have pstate.turbo_ratio_limit different
from zero, so we can use this check to decide whether or not to write to
the register.

quoted

An int is always passed as the last arg to this, so why is the arg

u64?

quoted

It can be int as we care only about low byte which is max turbo ratio.

Hmm.

quoted

quoted

 };

[..]

quoted

quoted

 Instead of shifting def_ratio and new_ratio in each
step, I'd shift max_ratio and the mask:

	u64 mask = 0xff;
	u64 max_ratio = new_ratio & mask;

	while (mask) {
		if (def_ratio & mask) {
			u64 ratio;

			if (new_ratio & mask) {

So why do we need this?

If we only care about the lowest byte only, we should discard all of the upper
bytes in new_ration here, shouldn't we?

quoted

quoted

				ratio = new_ratio & mask;
			} else {
				ratio = def_ratio & mask;
				if (ratio > max_ratio)
					ratio = max_ratio;
			}
			out_ratio |= ratio;
		}
		max_ratio <<= 8;
		mask <<= 8;
	}

I will experiment with your algorithm and check.

Your algorithm works, so will change to this.

[..]

quoted

quoted

quoted

+	if (pstate_funcs.get_turbo_ratio_limit &&
+	    !cpu->pstate.turbo_ratio_limit)
+		cpu->pstate.turbo_ratio_limit =
+			pstate_funcs.get_turbo_ratio_limit(cpu);

So we read MSR_NHM_TURBO_RATIO_LIMIT and store it into pstate.turbo_ratio_limit
unless alread populated.  This means we do it only once during initialization.

What if the value we have in there is stale after, say, system suspend-resume?

Did I mess up here? You mean data stored in a variable
cpu->pstate.turbo_ratio_limit is corrupted after suspend/resume. This is
a vzalloc memory.. 
all_cpu_data = vzalloc(sizeof(void *) * num_possible_cpus()) one time
during __init intel_pstate_init

Also 	all_cpu_data[cpunum] is allocated only once during first cpufreq
callback for init and not freed
	if (!all_cpu_data[cpunum])
		all_cpu_data[cpunum] = kzalloc(sizeof(struct cpudata),
					       GFP_KERNEL);
Did STR tests, didn't see issue. Not sure how else corruption can happen
unless slab memory is corrupted. Can you tell how it will be stale?

I was concerned about the BIOS changing the value in the register in which
case we probably should restore what we wrote to it last time or users
may be confused.

Thanks,
Rafael