| Message ID | 046513da96dfec919a1a41d270c167147d4a9c8d.1385353358.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
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); > >
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.
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.
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!
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..
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!
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.
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
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?
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!
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..
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!
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 ??
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 ?
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?
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
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);
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(+)