[PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

Sometimes boot loaders set CPU frequency to a value outside of frequency table
present with cpufreq core. In such cases CPU might be unstable if it has to run
on that frequency for long duration of time and so its better to set it to a
frequency which is specified in freq-table. This also makes cpufreq stats
inconsistent as cpufreq-stats would fail to register because current frequency
of CPU isn't found in freq-table.

Because we don't want this change to effect boot process badly, we go for the
next freq which is >= policy->cur ('cur' must be set by now, otherwise we will
end up setting freq to lowest of the table as 'cur' is initialized to zero).

In case where CPU is already running on one of the frequencies present in
freq-table, this would turn into a dummy call as __cpufreq_driver_target() would
return early.

Reported-by: Carlos Hernandez <ceh@ti.com>
Reported-and-tested-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2
- Set to (policy->cur - 1) instead of policy->cur.
- return early in case __cpufreq_driver_target() fails.

 drivers/cpufreq/cpufreq.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 02d534d..7be996c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1038,6 +1038,38 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 		}
 	}
 
+	/*
+	 * Sometimes boot loaders set CPU frequency to a value outside of
+	 * frequency table present with cpufreq core. In such cases CPU might be
+	 * unstable if it has to run on that frequency for long duration of time
+	 * and so its better to set it to a frequency which is specified in
+	 * freq-table. This also makes cpufreq stats inconsistent as
+	 * cpufreq-stats would fail to register because current frequency of CPU
+	 * isn't found in freq-table.
+	 *
+	 * Because we don't want this change to effect boot process badly, we go
+	 * for the next freq which is >= policy->cur ('cur' must be set by now,
+	 * otherwise we will end up setting freq to lowest of the table as 'cur'
+	 * is initialized to zero).
+	 *
+	 * In case where CPU is already running on one of the frequencies
+	 * present in freq-table, this would turn into a dummy call as
+	 * __cpufreq_driver_target() would return early.
+	 *
+	 * We are passing target-freq as "policy->cur - 1" otherwise
+	 * __cpufreq_driver_target() would simply fail, as policy->cur will be
+	 * equal to target-freq.
+	 */
+	if (has_target()) {
+		ret = __cpufreq_driver_target(policy, policy->cur - 1,
+				CPUFREQ_RELATION_L);
+		if (ret) {
+			pr_err("%s: Unable to set frequency from table: %d\n",
+					__func__, ret);
+			goto err_out_unregister;
+		}
+	}
+
 	/* related cpus should atleast have policy->cpus */
 	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Dirk Brandewie]

On 11/24/2013 08:23 PM, Viresh Kumar wrote:
> Sometimes boot loaders set CPU frequency to a value outside of frequency table
> present with cpufreq core. In such cases CPU might be unstable if it has to run
> on that frequency for long duration of time and so its better to set it to a
> frequency which is specified in freq-table. This also makes cpufreq stats
> inconsistent as cpufreq-stats would fail to register because current frequency
> of CPU isn't found in freq-table.
>

IMHO this issue should be fixed in the scaling driver for the platform.

The scaling driver sets policy->cur and fills in the frequency table and has the
abilty to adjust the frequency of the CPU.  Letting this leak up into the core
is wrong IMHO and just widens the window where the CPU will be running at the
wrong frequency set by the bootloader.

> Because we don't want this change to effect boot process badly, we go for the
> next freq which is >= policy->cur ('cur' must be set by now, otherwise we will
> end up setting freq to lowest of the table as 'cur' is initialized to zero).
>
> In case where CPU is already running on one of the frequencies present in
> freq-table, this would turn into a dummy call as __cpufreq_driver_target() would
> return early.
>
> Reported-by: Carlos Hernandez <ceh@ti.com>
> Reported-and-tested-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V1->V2
> - Set to (policy->cur - 1) instead of policy->cur.
> - return early in case __cpufreq_driver_target() fails.
>
>   drivers/cpufreq/cpufreq.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 02d534d..7be996c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1038,6 +1038,38 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>   		}
>   	}
>
> +	/*
> +	 * Sometimes boot loaders set CPU frequency to a value outside of
> +	 * frequency table present with cpufreq core. In such cases CPU might be
> +	 * unstable if it has to run on that frequency for long duration of time
> +	 * and so its better to set it to a frequency which is specified in
> +	 * freq-table. This also makes cpufreq stats inconsistent as
> +	 * cpufreq-stats would fail to register because current frequency of CPU
> +	 * isn't found in freq-table.
> +	 *
> +	 * Because we don't want this change to effect boot process badly, we go
> +	 * for the next freq which is >= policy->cur ('cur' must be set by now,
> +	 * otherwise we will end up setting freq to lowest of the table as 'cur'
> +	 * is initialized to zero).
> +	 *
> +	 * In case where CPU is already running on one of the frequencies
> +	 * present in freq-table, this would turn into a dummy call as
> +	 * __cpufreq_driver_target() would return early.
> +	 *
> +	 * We are passing target-freq as "policy->cur - 1" otherwise
> +	 * __cpufreq_driver_target() would simply fail, as policy->cur will be
> +	 * equal to target-freq.
> +	 */

Shouldn't there be a check to see if the problem exists at all?  Otherwise
the core is setting a policy for ALL platform even those where the issue does
not exist.

> +	if (has_target()) {
> +		ret = __cpufreq_driver_target(policy, policy->cur - 1,
> +				CPUFREQ_RELATION_L);
> +		if (ret) {
> +			pr_err("%s: Unable to set frequency from table: %d\n",
> +					__func__, ret);
> +			goto err_out_unregister;
> +		}
> +	}
> +
>   	/* related cpus should atleast have policy->cpus */
>   	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
>
>

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

On 25 November 2013 22:08, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> IMHO this issue should be fixed in the scaling driver for the platform.
>
> The scaling driver sets policy->cur and fills in the frequency table and has

Not anymore, policy->cur is set in the core for most of the drivers now.
Drivers just provide ->get() callbacks.

> the ability to adjust the frequency of the CPU.

I believe this kind of decisions should stay with the core, drivers should
just provide the backend instead of intelligence..

> Letting this leak up into the core
> is wrong IMHO and just widens the window where the CPU will be running at
> the wrong frequency set by the bootloader.

It doesn't stay there for long enough.. we get to this point in
cpufreq core just
after calling ->init(), and if the CPU is working without issues until
now, it will
stay stable for few more milliseconds.

> Shouldn't there be a check to see if the problem exists at all?  Otherwise
> the core is setting a policy for ALL platform even those where the issue
> does
> not exist.

That is taken care of by __cpufreq_driver_target(). It checks if we are
already running at requested frequency or not (after getting the next
higher frequency)... If current freq is present in table,
cpufreq_frequency_table_target() will return current frequency only for
policy->cur -1. And so we will not end up configuring hardware
unnecessarily.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Dirk Brandewie]

On 11/25/2013 09:01 AM, Viresh Kumar wrote:
> On 25 November 2013 22:08, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
>> IMHO this issue should be fixed in the scaling driver for the platform.
>>
>> The scaling driver sets policy->cur and fills in the frequency table and has
>
> Not anymore, policy->cur is set in the core for most of the drivers now.
> Drivers just provide ->get() callbacks.
>
>> the ability to adjust the frequency of the CPU.
>
> I believe this kind of decisions should stay with the core, drivers should
> just provide the backend instead of intelligence..
>


This is a platform specific bug fix AFAICT and belongs in a platform
specific piece of code


>> Letting this leak up into the core
>> is wrong IMHO and just widens the window where the CPU will be running at
>> the wrong frequency set by the bootloader.
>
> It doesn't stay there for long enough.. we get to this point in
> cpufreq core just
> after calling ->init(), and if the CPU is working without issues until
> now, it will
> stay stable for few more milliseconds.
>

And this is where the scaling driver should detect and fix the issue in ->init()
on platforms we know about today, What happens if platforms in the future are
more sensitive to the issue?

>> Shouldn't there be a check to see if the problem exists at all?  Otherwise
>> the core is setting a policy for ALL platform even those where the issue
>> does
>> not exist.
>
> That is taken care of by __cpufreq_driver_target(). It checks if we are
> already running at requested frequency or not (after getting the next
> higher frequency)... If current freq is present in table,
> cpufreq_frequency_table_target() will return current frequency only for
> policy->cur -1. And so we will not end up configuring hardware
> unnecessarily.
>

The core should not be working around bootloader bugs IMHO.  Silently
fixing it is evil IMHO at a minimum the core should complain LOUDLY
about this happening otherwise the bootloaders will have no incentive to
get their act together.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

On Monday, November 25, 2013 09:43:59 AM Dirk Brandewie wrote:
> On 11/25/2013 09:01 AM, Viresh Kumar wrote:
> > On 25 November 2013 22:08, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> >> IMHO this issue should be fixed in the scaling driver for the platform.
> >>
> >> The scaling driver sets policy->cur and fills in the frequency table and has
> >
> > Not anymore, policy->cur is set in the core for most of the drivers now.
> > Drivers just provide ->get() callbacks.
> >
> >> the ability to adjust the frequency of the CPU.
> >
> > I believe this kind of decisions should stay with the core, drivers should
> > just provide the backend instead of intelligence..
> >
> 
> 
> This is a platform specific bug fix AFAICT and belongs in a platform
> specific piece of code
> 
> 
> >> Letting this leak up into the core
> >> is wrong IMHO and just widens the window where the CPU will be running at
> >> the wrong frequency set by the bootloader.
> >
> > It doesn't stay there for long enough.. we get to this point in
> > cpufreq core just
> > after calling ->init(), and if the CPU is working without issues until
> > now, it will
> > stay stable for few more milliseconds.
> >
> 
> And this is where the scaling driver should detect and fix the issue in ->init()
> on platforms we know about today, What happens if platforms in the future are
> more sensitive to the issue?

So what should the core do if the driver is careless and lets the bug slip
through?  Should it blindly trust the driver and let go?

Honestly, I don't think so.

> >> Shouldn't there be a check to see if the problem exists at all?  Otherwise
> >> the core is setting a policy for ALL platform even those where the issue
> >> does
> >> not exist.
> >
> > That is taken care of by __cpufreq_driver_target(). It checks if we are
> > already running at requested frequency or not (after getting the next
> > higher frequency)... If current freq is present in table,
> > cpufreq_frequency_table_target() will return current frequency only for
> > policy->cur -1. And so we will not end up configuring hardware
> > unnecessarily.
> >
> 
> The core should not be working around bootloader bugs IMHO.  Silently
> fixing it is evil IMHO at a minimum the core should complain LOUDLY
> about this happening otherwise the bootloaders will have no incentive to
> get their act together.

Yes, we can add a WARN_ON() there.  Still, though, the core's responsibility
is to ensure that (a) either we can continue safely or (b) we can't, in which
case it should just fail the initialization.  Whether or not it should panic()
I'm not sure.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

On 26 November 2013 02:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, November 25, 2013 09:43:59 AM Dirk Brandewie wrote:
>> This is a platform specific bug fix AFAICT and belongs in a platform
>> specific piece of code

In case we end up doing that, we will do lots of code redundancy in
cpufreq drivers. And as Rafael said, some platforms might never
know they have booted with an out of table frequency and so taking
care of this at a single place is better, where we are sure that it
will get fixed.

>> The core should not be working around bootloader bugs IMHO.  Silently
>> fixing it is evil IMHO at a minimum the core should complain LOUDLY
>> about this happening otherwise the bootloaders will have no incentive to
>> get their act together.

That looks correct..

> Yes, we can add a WARN_ON() there.  Still, though, the core's responsibility
> is to ensure that (a) either we can continue safely or (b) we can't, in which
> case it should just fail the initialization.  Whether or not it should panic()
> I'm not sure.

But is this that big crime, that we do a WARN on it? CPU was running on
a workable frequency, it wasn't mentioned in the table, that's it.

Probably just throw an print message that CPU found to be running on
out of table frequency, and that got fixed..

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[viresh kumar]

On Tuesday 26 November 2013 07:31 AM, Viresh Kumar wrote:
> Probably just throw an print message that CPU found to be running on
> out of table frequency, and that got fixed..

And here is the patch to test:

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Mon, 18 Nov 2013 10:15:50 +0530
Subject: [PATCH] cpufreq: Make sure CPU is running on a freq from freq-table

Sometimes boot loaders set CPU frequency to a value outside of frequency table
present with cpufreq core. In such cases CPU might be unstable if it has to run
on that frequency for long duration of time and so its better to set it to a
frequency which is specified in freq-table. This also makes cpufreq stats
inconsistent as cpufreq-stats would fail to register because current frequency
of CPU isn't found in freq-table.

Because we don't want this change to effect boot process badly, we go for the
next freq which is >= policy->cur ('cur' must be set by now, otherwise we will
end up setting freq to lowest of the table as 'cur' is initialized to zero).

In case current frequency doesn't match any frequency from freq-table, we throw
warnings to user, so that user can get this fixed in their bootloaders or
freq-tables.

Reported-by: Carlos Hernandez <ceh@ti.com>
Reported-and-tested-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c    | 37 +++++++++++++++++++++++++++++++++++++
 drivers/cpufreq/freq_table.c | 24 ++++++++++++++++++++++++
 include/linux/cpufreq.h      |  2 ++
 3 files changed, 63 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 02d534d..5beb16d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1038,6 +1038,43 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 		}
 	}

+	/*
+	 * Sometimes boot loaders set CPU frequency to a value outside of
+	 * frequency table present with cpufreq core. In such cases CPU might be
+	 * unstable if it has to run on that frequency for long duration of time
+	 * and so its better to set it to a frequency which is specified in
+	 * freq-table. This also makes cpufreq stats inconsistent as
+	 * cpufreq-stats would fail to register because current frequency of CPU
+	 * isn't found in freq-table.
+	 *
+	 * Because we don't want this change to effect boot process badly, we go
+	 * for the next freq which is >= policy->cur ('cur' must be set by now,
+	 * otherwise we will end up setting freq to lowest of the table as 'cur'
+	 * is initialized to zero).
+	 *
+	 * We are passing target-freq as "policy->cur - 1" otherwise
+	 * __cpufreq_driver_target() would simply fail, as policy->cur will be
+	 * equal to target-freq.
+	 */
+	if (has_target()) {
+		/* Are we running at unknown frequency ? */
+		ret = cpufreq_frequency_table_get_index(policy, policy->cur);
+		if (ret == -EINVAL) {
+			/* Warn user and fix it */
+			pr_warn("%s: CPU%d: running at invalid freq: %u KHz\n",
+					__func__, policy->cpu, policy->cur);
+			ret = __cpufreq_driver_target(policy, policy->cur - 1,
+					CPUFREQ_RELATION_L);
+			if (ret) {
+				pr_err("%s: Unable to set frequency from table: %d\n",
+						__func__, ret);
+				goto err_out_unregister;
+			}
+			pr_warn("%s: CPU%d: frequency changed to: %u KHz\n",
+					__func__, policy->cpu, policy->cur);
+		}
+	}
+
 	/* related cpus should atleast have policy->cpus */
 	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 3458d27..0d6cc0e 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -178,7 +178,31 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
 }
 EXPORT_SYMBOL_GPL(cpufreq_frequency_table_target);

+int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
+		unsigned int freq)
+{
+	struct cpufreq_frequency_table *table;
+	int index = -EINVAL, i = 0;
+
+	table = cpufreq_frequency_get_table(policy->cpu);
+	if (unlikely(!table)) {
+		pr_debug("%s: Unable to find freq_table\n", __func__);
+		return -ENOENT;
+	}
+
+	for (; table[i].frequency != CPUFREQ_TABLE_END; i++) {
+		if (table[i].frequency == freq) {
+			index = i;
+			break;
+		}
+	}
+
+	return index;
+}
+EXPORT_SYMBOL_GPL(cpufreq_frequency_table_get_index);
+
 static DEFINE_PER_CPU(struct cpufreq_frequency_table *, cpufreq_show_table);
+
 /**
  * show_available_freqs - show available frequencies for the specified CPU
  */
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dc196bb..71646ce 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -439,6 +439,8 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
 				   unsigned int target_freq,
 				   unsigned int relation,
 				   unsigned int *index);
+int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
+		unsigned int freq);

 void cpufreq_frequency_table_update_policy_cpu(struct cpufreq_policy *policy);
 ssize_t cpufreq_show_cpus(const struct cpumask *mask, char *buf);
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

On Tuesday, November 26, 2013 07:31:50 AM Viresh Kumar wrote:
> On 26 November 2013 02:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, November 25, 2013 09:43:59 AM Dirk Brandewie wrote:
> >> This is a platform specific bug fix AFAICT and belongs in a platform
> >> specific piece of code
> 
> In case we end up doing that, we will do lots of code redundancy in
> cpufreq drivers. And as Rafael said, some platforms might never
> know they have booted with an out of table frequency and so taking
> care of this at a single place is better, where we are sure that it
> will get fixed.
> 
> >> The core should not be working around bootloader bugs IMHO.  Silently
> >> fixing it is evil IMHO at a minimum the core should complain LOUDLY
> >> about this happening otherwise the bootloaders will have no incentive to
> >> get their act together.
> 
> That looks correct..
> 
> > Yes, we can add a WARN_ON() there.  Still, though, the core's responsibility
> > is to ensure that (a) either we can continue safely or (b) we can't, in which
> > case it should just fail the initialization.  Whether or not it should panic()
> > I'm not sure.
> 
> But is this that big crime, that we do a WARN on it? CPU was running on
> a workable frequency, it wasn't mentioned in the table, that's it.
> 
> Probably just throw an print message that CPU found to be running on
> out of table frequency, and that got fixed..

I was talking about the case when your

__cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);

fails.  The other case is not really interesting.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

On 27 November 2013 01:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> I was talking about the case when your
>
> __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);
>
> fails.  The other case is not really interesting.

Okay.. I actually thought the context of this chat is about "not fixing the
frequency silently". That's what Dirk pointed out, if I am not wrong.

And hence I also support the new pr_warn I have added in those cases.
But yes a WARN_ON() can be printed out in case
__cpufreq_driver_target() fails.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

On Wednesday, November 27, 2013 08:31:02 AM Viresh Kumar wrote:
> On 27 November 2013 01:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > I was talking about the case when your
> >
> > __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);
> >
> > fails.  The other case is not really interesting.
> 
> Okay.. I actually thought the context of this chat is about "not fixing the
> frequency silently". That's what Dirk pointed out, if I am not wrong.

In that case you can simply print a message bashing the boot loader. :-)

> And hence I also support the new pr_warn I have added in those cases.

Sure.

> But yes a WARN_ON() can be printed out in case
> __cpufreq_driver_target() fails.

And here my question was: Is it safe to continue at all in that case?

Rafael

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

On 27 November 2013 19:52, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> And here my question was: Is it safe to continue at all in that case?

Hmm.. Honestly speaking I haven't thought about it earlier. And from
the kind of inputs we got from Nishanth its not safe at all and so we
really need a BUG_ON in this case, instead of WARN_ON.

What do you say?

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

On Wednesday, November 27, 2013 09:22:01 PM Viresh Kumar wrote:
> On 27 November 2013 19:52, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > And here my question was: Is it safe to continue at all in that case?
> 
> Hmm.. Honestly speaking I haven't thought about it earlier. And from
> the kind of inputs we got from Nishanth its not safe at all and so we
> really need a BUG_ON in this case, instead of WARN_ON.
> 
> What do you say?

Yeah, BUG_ON() sounds like the right thing to do here.

I have a concern that on some systems you can't really say what frequency
you're running at the moment, however.  So there should be a flag for
drivers indicating whether or not frequencies (or operation points in
general) are directly testable and the check should only be done for
the drivers with the flag set.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

On 28 November 2013 01:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> I have a concern that on some systems you can't really say what frequency
> you're running at the moment, however.

Which ones? I know ACPI tries to play smart by handling the frequency stuff
itself by marking CPUs not-related to each other for the kernel where they
might actually be sharing clock line... But probably in these cases as well,
atleast the cpufreq core should believe that it is running on a valid frequency
even if actual hardware is running at something different..

Any other platforms you are aware of that implement ->target/target_index
and where we can't say what freq are they running at?

> So there should be a flag for
> drivers indicating whether or not frequencies (or operation points in
> general) are directly testable and the check should only be done for
> the drivers with the flag set.

Probably a flag with properties exactly opposite to what you mentioned,
so that we don't need to modify most of the drivers..

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

On Thursday, November 28, 2013 08:50:20 AM Viresh Kumar wrote:
> On 28 November 2013 01:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > I have a concern that on some systems you can't really say what frequency
> > you're running at the moment, however.
> 
> Which ones? I know ACPI tries to play smart by handling the frequency stuff
> itself by marking CPUs not-related to each other for the kernel where they
> might actually be sharing clock line... But probably in these cases as well,
> atleast the cpufreq core should believe that it is running on a valid frequency
> even if actual hardware is running at something different..
> 
> Any other platforms you are aware of that implement ->target/target_index
> and where we can't say what freq are they running at?

acpi-cpufreq is one at least.

Anyway, this isn't about ACPI or anything like that, but hardware.  Generally
speaking, on modern Intel hardware the processor itself chooses the frequency
to run at and it may do that behind your back.  Moreover, it can choose a
frequency different from the one you asked for.  And it won't choose one that
it can't run at for that matter. :-)

Overall, I don't believe that the problem you're trying to address is relevant
for any non-exotic x86 hardware.

> > So there should be a flag for
> > drivers indicating whether or not frequencies (or operation points in
> > general) are directly testable and the check should only be done for
> > the drivers with the flag set.
> 
> Probably a flag with properties exactly opposite to what you mentioned,
> so that we don't need to modify most of the drivers..

That would work too if you prefer it.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

On 28 November 2013 18:39, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> acpi-cpufreq is one at least.
>
> Anyway, this isn't about ACPI or anything like that, but hardware.  Generally
> speaking, on modern Intel hardware the processor itself chooses the frequency
> to run at and it may do that behind your back.  Moreover, it can choose a
> frequency different from the one you asked for.  And it won't choose one that
> it can't run at for that matter. :-)
>
> Overall, I don't believe that the problem you're trying to address is relevant
> for any non-exotic x86 hardware.

Okay.. So wouldn't it be better that we add this special flag only when we
face a real problem? Otherwise this flag might stay unused for long time
and then we might end up removing it..

>> > So there should be a flag for
>> > drivers indicating whether or not frequencies (or operation points in
>> > general) are directly testable and the check should only be done for
>> > the drivers with the flag set.
>>
>> Probably a flag with properties exactly opposite to what you mentioned,
>> so that we don't need to modify most of the drivers..
>
> That would work too if you prefer it.

In case we need this flag, what should we name it?
ALLOW_UNKNOWN_FREQ ??

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

On Thursday, November 28, 2013 07:11:17 PM Viresh Kumar wrote:
> On 28 November 2013 18:39, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > acpi-cpufreq is one at least.
> >
> > Anyway, this isn't about ACPI or anything like that, but hardware.  Generally
> > speaking, on modern Intel hardware the processor itself chooses the frequency
> > to run at and it may do that behind your back.  Moreover, it can choose a
> > frequency different from the one you asked for.  And it won't choose one that
> > it can't run at for that matter. :-)
> >
> > Overall, I don't believe that the problem you're trying to address is relevant
> > for any non-exotic x86 hardware.
> 
> Okay.. So wouldn't it be better that we add this special flag only when we
> face a real problem? Otherwise this flag might stay unused for long time
> and then we might end up removing it..
> 
> >> > So there should be a flag for
> >> > drivers indicating whether or not frequencies (or operation points in
> >> > general) are directly testable and the check should only be done for
> >> > the drivers with the flag set.
> >>
> >> Probably a flag with properties exactly opposite to what you mentioned,
> >> so that we don't need to modify most of the drivers..
> >
> > That would work too if you prefer it.
> 
> In case we need this flag, what should we name it?
> ALLOW_UNKNOWN_FREQ ??

SKIP_INITIAL_FREQUENCY_CHECK ?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Viresh Kumar]

On 28 November 2013 19:42, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, November 28, 2013 07:11:17 PM Viresh Kumar wrote:
>> Okay.. So wouldn't it be better that we add this special flag only when we
>> face a real problem? Otherwise this flag might stay unused for long time
>> and then we might end up removing it..

What about this one?

> SKIP_INITIAL_FREQUENCY_CHECK ?

Looks fine.. In case this is required, you want this to be set initially for any
driver?

Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

[Rafael J. Wysocki]

On Thursday, November 28, 2013 07:44:02 PM Viresh Kumar wrote:
> On 28 November 2013 19:42, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, November 28, 2013 07:11:17 PM Viresh Kumar wrote:
> >> Okay.. So wouldn't it be better that we add this special flag only when we
> >> face a real problem? Otherwise this flag might stay unused for long time
> >> and then we might end up removing it..
> 
> What about this one?

Well, so are you saying that the problem is only theoretical now?

> > SKIP_INITIAL_FREQUENCY_CHECK ?
> 
> Looks fine.. In case this is required, you want this to be set initially for any
> driver?

That depends on what is less work and code churn.  How many platforms/drivers
have this problem today?  Which ones are they?

Rafael