Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Thursday, November 28, 2013 07:44:02 PM Viresh Kumar wrote: > On 28 November 2013 19:42, Rafael J. Wysocki 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 28 November 2013 19:42, Rafael J. Wysocki 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Thursday, November 28, 2013 07:11:17 PM Viresh Kumar wrote: > On 28 November 2013 18:39, Rafael J. Wysocki 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 28 November 2013 18:39, Rafael J. Wysocki 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 ?? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Thursday, November 28, 2013 08:50:20 AM Viresh Kumar wrote: > On 28 November 2013 01:51, Rafael J. Wysocki 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Thursday, November 28, 2013 08:50:20 AM Viresh Kumar wrote: On 28 November 2013 01:51, Rafael J. Wysocki r...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 28 November 2013 18:39, Rafael J. Wysocki r...@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 ?? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Thursday, November 28, 2013 07:11:17 PM Viresh Kumar wrote: On 28 November 2013 18:39, Rafael J. Wysocki r...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 28 November 2013 19:42, Rafael J. Wysocki r...@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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Thursday, November 28, 2013 07:44:02 PM Viresh Kumar wrote: On 28 November 2013 19:42, Rafael J. Wysocki r...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 28 November 2013 01:51, Rafael J. Wysocki 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.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Wednesday, November 27, 2013 09:22:01 PM Viresh Kumar wrote: > On 27 November 2013 19:52, Rafael J. Wysocki 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 27 November 2013 19:52, Rafael J. Wysocki 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Wednesday, November 27, 2013 08:31:02 AM Viresh Kumar wrote: > On 27 November 2013 01:51, Rafael J. Wysocki 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Wednesday, November 27, 2013 08:31:02 AM Viresh Kumar wrote: On 27 November 2013 01:51, Rafael J. Wysocki r...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 27 November 2013 19:52, Rafael J. Wysocki r...@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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Wednesday, November 27, 2013 09:22:01 PM Viresh Kumar wrote: On 27 November 2013 19:52, Rafael J. Wysocki r...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 28 November 2013 01:51, Rafael J. Wysocki r...@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.. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 27 November 2013 01:51, Rafael J. Wysocki 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Tuesday, November 26, 2013 07:31:50 AM Viresh Kumar wrote: > On 26 November 2013 02:43, Rafael J. Wysocki 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On Tuesday, November 26, 2013 07:31:50 AM Viresh Kumar wrote: On 26 November 2013 02:43, Rafael J. Wysocki r...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 27 November 2013 01:51, Rafael J. Wysocki r...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
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
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
Reported-and-tested-by: Nishanth Menon
Signed-off-by: Viresh Kumar
---
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 *,
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 26 November 2013 02:43, Rafael J. Wysocki 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.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 11/25/2013 09:01 AM, Viresh Kumar wrote: On 25 November 2013 22:08, Dirk Brandewie 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 25 November 2013 22:08, Dirk Brandewie 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
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
Reported-and-tested-by: Nishanth Menon
Signed-off-by: Viresh Kumar
---
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
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 c...@ti.com
Reported-and-tested-by: Nishanth Menon n...@ti.com
Signed-off-by: Viresh Kumar viresh.ku...@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);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 25 November 2013 22:08, Dirk Brandewie dirk.brande...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 11/25/2013 09:01 AM, Viresh Kumar wrote: On 25 November 2013 22:08, Dirk Brandewie dirk.brande...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
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.brande...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
On 26 November 2013 02:43, Rafael J. Wysocki r...@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.. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table
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.ku...@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 c...@ti.com
Reported-and-tested-by: Nishanth Menon n...@ti.com
Signed-off-by: Viresh Kumar viresh.ku...@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
[PATCH V2] 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 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
Reported-and-tested-by: Nishanth Menon
Signed-off-by: Viresh Kumar
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2] 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 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 c...@ti.com
Reported-and-tested-by: Nishanth Menon n...@ti.com
Signed-off-by: Viresh Kumar viresh.ku...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
