| Message ID | 406f55ac8030043f0349b084878c9b8d04f7ad86.1438571116.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday, August 03, 2015 08:36:14 AM Viresh Kumar wrote: > What's being done from CPUFREQ_INCOMPATIBLE, can also be done with > CPUFREQ_ADJUST. There is nothing special with CPUFREQ_INCOMPATIBLE > notifier. The above part of the changelog is a disaster to me. :-( It not only doesn't explain what really goes on, but it's actively confusing. What really happens is that the core sends CPUFREQ_INCOMPATIBLE notifications unconditionally right after sending the CPUFREQ_ADJUST ones, so the former is just redundant and it's more efficient to merge the two into one. > Kill CPUFREQ_INCOMPATIBLE and fix its usage sites. > > This also updates the numbering of notifier events to remove holes. Why don't you redefine CPUFREQ_ADJUST as 1 instead? > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > Documentation/cpu-freq/core.txt | 7 ++----- > drivers/acpi/processor_perflib.c | 2 +- > drivers/cpufreq/cpufreq.c | 4 ---- > drivers/cpufreq/ppc_cbe_cpufreq_pmi.c | 4 ++-- > drivers/video/fbdev/pxafb.c | 1 - > drivers/video/fbdev/sa1100fb.c | 1 - > include/linux/cpufreq.h | 9 ++++----- > 7 files changed, 9 insertions(+), 19 deletions(-) > > diff --git a/Documentation/cpu-freq/core.txt b/Documentation/cpu-freq/core.txt > index 70933eadc308..ba78e7c2a069 100644 > --- a/Documentation/cpu-freq/core.txt > +++ b/Documentation/cpu-freq/core.txt > @@ -55,16 +55,13 @@ transition notifiers. > ---------------------------- > > These are notified when a new policy is intended to be set. Each > -CPUFreq policy notifier is called three times for a policy transition: > +CPUFreq policy notifier is called twice for a policy transition: > > 1.) During CPUFREQ_ADJUST all CPUFreq notifiers may change the limit if > they see a need for this - may it be thermal considerations or > hardware limitations. > > -2.) During CPUFREQ_INCOMPATIBLE only changes may be done in order to avoid > - hardware failure. > - > -3.) And during CPUFREQ_NOTIFY all notifiers are informed of the new policy > +2.) And during CPUFREQ_NOTIFY all notifiers are informed of the new policy > - if two hardware drivers failed to agree on a new policy before this > stage, the incompatible hardware shall be shut down, and the user > informed of this. > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > index 47af702bb6a2..bb01dea39fdc 100644 > --- a/drivers/acpi/processor_perflib.c > +++ b/drivers/acpi/processor_perflib.c > @@ -83,7 +83,7 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb, > if (ignore_ppc) > return 0; > > - if (event != CPUFREQ_INCOMPATIBLE) > + if (event != CPUFREQ_ADJUST) > return 0; > > mutex_lock(&performance_mutex); > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 76a26609d96b..293f47b814bf 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2206,10 +2206,6 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, > blocking_notifier_call_chain(&cpufreq_policy_notifier_list, > CPUFREQ_ADJUST, new_policy); > > - /* adjust if necessary - hardware incompatibility*/ > - blocking_notifier_call_chain(&cpufreq_policy_notifier_list, > - CPUFREQ_INCOMPATIBLE, new_policy); > - > /* > * verify the cpu speed can be set within this limit, which might be > * different to the first one > diff --git a/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c b/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c > index d29e8da396a0..7969f7690498 100644 > --- a/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c > +++ b/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c > @@ -97,8 +97,8 @@ static int pmi_notifier(struct notifier_block *nb, > struct cpufreq_frequency_table *cbe_freqs; > u8 node; > > - /* Should this really be called for CPUFREQ_ADJUST, CPUFREQ_INCOMPATIBLE > - * and CPUFREQ_NOTIFY policy events?) > + /* Should this really be called for CPUFREQ_ADJUST and CPUFREQ_NOTIFY > + * policy events?) > */ > if (event == CPUFREQ_START) > return 0; > diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c > index 7245611ec963..94813af97f09 100644 > --- a/drivers/video/fbdev/pxafb.c > +++ b/drivers/video/fbdev/pxafb.c > @@ -1668,7 +1668,6 @@ pxafb_freq_policy(struct notifier_block *nb, unsigned long val, void *data) > > switch (val) { > case CPUFREQ_ADJUST: > - case CPUFREQ_INCOMPATIBLE: > pr_debug("min dma period: %d ps, " > "new clock %d kHz\n", pxafb_display_dma_period(var), > policy->max); > diff --git a/drivers/video/fbdev/sa1100fb.c b/drivers/video/fbdev/sa1100fb.c > index 89dd7e02197f..dcf774c15889 100644 > --- a/drivers/video/fbdev/sa1100fb.c > +++ b/drivers/video/fbdev/sa1100fb.c > @@ -1042,7 +1042,6 @@ sa1100fb_freq_policy(struct notifier_block *nb, unsigned long val, > > switch (val) { > case CPUFREQ_ADJUST: > - case CPUFREQ_INCOMPATIBLE: > dev_dbg(fbi->dev, "min dma period: %d ps, " > "new clock %d kHz\n", sa1100fb_min_dma_period(fbi), > policy->max); > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index bde1e567b3a9..bedcc90c0757 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -369,11 +369,10 @@ static inline void cpufreq_resume(void) {} > > /* Policy Notifiers */ > #define CPUFREQ_ADJUST (0) > -#define CPUFREQ_INCOMPATIBLE (1) > -#define CPUFREQ_NOTIFY (2) > -#define CPUFREQ_START (3) > -#define CPUFREQ_CREATE_POLICY (4) > -#define CPUFREQ_REMOVE_POLICY (5) > +#define CPUFREQ_NOTIFY (1) > +#define CPUFREQ_START (2) > +#define CPUFREQ_CREATE_POLICY (3) > +#define CPUFREQ_REMOVE_POLICY (4) > > #ifdef CONFIG_CPU_FREQ > int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list); >
On 10-09-15, 01:26, Rafael J. Wysocki wrote: > On Monday, August 03, 2015 08:36:14 AM Viresh Kumar wrote: > > What's being done from CPUFREQ_INCOMPATIBLE, can also be done with > > CPUFREQ_ADJUST. There is nothing special with CPUFREQ_INCOMPATIBLE > > notifier. > > The above part of the changelog is a disaster to me. :-( > > It not only doesn't explain what really goes on, but it's actively confusing. > > What really happens is that the core sends CPUFREQ_INCOMPATIBLE notifications > unconditionally right after sending the CPUFREQ_ADJUST ones, so the former is > just redundant and it's more efficient to merge the two into one. Undoubtedly this looks far better :) But, isn't this series already applied some time back ? > > Kill CPUFREQ_INCOMPATIBLE and fix its usage sites. > > > > This also updates the numbering of notifier events to remove holes. > > Why don't you redefine CPUFREQ_ADJUST as 1 instead? So that there is no request with 0? Yeah that could have been done.
Hi, On Thu, Sep 10, 2015 at 2:39 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 10-09-15, 01:26, Rafael J. Wysocki wrote: >> On Monday, August 03, 2015 08:36:14 AM Viresh Kumar wrote: >> > What's being done from CPUFREQ_INCOMPATIBLE, can also be done with >> > CPUFREQ_ADJUST. There is nothing special with CPUFREQ_INCOMPATIBLE >> > notifier. >> >> The above part of the changelog is a disaster to me. :-( >> >> It not only doesn't explain what really goes on, but it's actively confusing. >> >> What really happens is that the core sends CPUFREQ_INCOMPATIBLE notifications >> unconditionally right after sending the CPUFREQ_ADJUST ones, so the former is >> just redundant and it's more efficient to merge the two into one. > > Undoubtedly this looks far better :) > > But, isn't this series already applied some time back ? Right, never mind. For some reason that patch was left in the "New" state. The code is OK. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/06/2016 02:21 PM, Rafael J. Wysocki wrote: > On Wed, Apr 6, 2016 at 10:30 PM, Saravana Kannan <skannan@codeaurora.org> wrote: >> On 09/09/2015 05:53 PM, Rafael J. Wysocki wrote: >>> >>> Hi, >>> >>> On Thu, Sep 10, 2015 at 2:39 AM, Viresh Kumar <viresh.kumar@linaro.org> >>> wrote: >>>> >>>> On 10-09-15, 01:26, Rafael J. Wysocki wrote: >>>>> >>>>> On Monday, August 03, 2015 08:36:14 AM Viresh Kumar wrote: >>>>>> >>>>>> What's being done from CPUFREQ_INCOMPATIBLE, can also be done with >>>>>> CPUFREQ_ADJUST. There is nothing special with CPUFREQ_INCOMPATIBLE >>>>>> notifier. >>>>> >>>>> >>>>> The above part of the changelog is a disaster to me. :-( >>>>> >>>>> It not only doesn't explain what really goes on, but it's actively >>>>> confusing. >>>>> >>>>> What really happens is that the core sends CPUFREQ_INCOMPATIBLE >>>>> notifications >>>>> unconditionally right after sending the CPUFREQ_ADJUST ones, so the >>>>> former is >>>>> just redundant and it's more efficient to merge the two into one. >>>> >>>> >>>> Undoubtedly this looks far better :) >>>> >>>> But, isn't this series already applied some time back ? >>> >>> >>> Right, never mind. For some reason that patch was left in the "New" >>> state. >>> >>> The code is OK. >> >> >> >> I guess I didn't notice this change when it was sent out. >> >> The comment that was deleted in this patch clearly states why the >> INCOMPATIBLE notifier is needed. Some client might want to boost the CPU min >> freq for performance or other reasons, but thermal might want to limit it. >> So, by having thermal register for INCOMPATIBLE notifiers to enforce the >> limits, we provide a way to guarantee it gets the final say. >> >> The real fix should have been to change drivers/thermal/cpu_cooling.c to use >> CPUFREQ_INCOMPATIBLE instead of CPUFREQ_ADJUST. >> >> Is there something I'm missing? If not, can we please revert this patch? > > Well, nobody was using that event. > True, but that's more of a bug in drivers/thermal/cpu-cooling.c and drivers/acpi/processor_thermal.c. We should revert this patch and fix those drivers. Does that seem acceptable to you? -Saravana
On Wed, Apr 6, 2016 at 11:29 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 04/06/2016 02:21 PM, Rafael J. Wysocki wrote: >> >> On Wed, Apr 6, 2016 at 10:30 PM, Saravana Kannan <skannan@codeaurora.org> >> wrote: >>> >>> On 09/09/2015 05:53 PM, Rafael J. Wysocki wrote: >>>> [cut] >> >> Well, nobody was using that event. >> > > True, but that's more of a bug in drivers/thermal/cpu-cooling.c and > drivers/acpi/processor_thermal.c. We should revert this patch and fix those > drivers. Does that seem acceptable to you? I'd rather see a patch series adding the event back along with some users. One user at least. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/06/2016 02:45 PM, Rafael J. Wysocki wrote: > On Wed, Apr 6, 2016 at 11:29 PM, Saravana Kannan <skannan@codeaurora.org> wrote: >> On 04/06/2016 02:21 PM, Rafael J. Wysocki wrote: >>> >>> On Wed, Apr 6, 2016 at 10:30 PM, Saravana Kannan <skannan@codeaurora.org> >>> wrote: >>>> >>>> On 09/09/2015 05:53 PM, Rafael J. Wysocki wrote: >>>>> > > [cut] > >>> >>> Well, nobody was using that event. >>> >> >> True, but that's more of a bug in drivers/thermal/cpu-cooling.c and >> drivers/acpi/processor_thermal.c. We should revert this patch and fix those >> drivers. Does that seem acceptable to you? > > I'd rather see a patch series adding the event back along with some > users. One user at least. > Ok, I'll make those two drivers use them and send it out. It's very clearly a bug in those drivers. -Saravana`
diff --git a/Documentation/cpu-freq/core.txt b/Documentation/cpu-freq/core.txt index 70933eadc308..ba78e7c2a069 100644 --- a/Documentation/cpu-freq/core.txt +++ b/Documentation/cpu-freq/core.txt @@ -55,16 +55,13 @@ transition notifiers. ---------------------------- These are notified when a new policy is intended to be set. Each -CPUFreq policy notifier is called three times for a policy transition: +CPUFreq policy notifier is called twice for a policy transition: 1.) During CPUFREQ_ADJUST all CPUFreq notifiers may change the limit if they see a need for this - may it be thermal considerations or hardware limitations. -2.) During CPUFREQ_INCOMPATIBLE only changes may be done in order to avoid - hardware failure. - -3.) And during CPUFREQ_NOTIFY all notifiers are informed of the new policy +2.) And during CPUFREQ_NOTIFY all notifiers are informed of the new policy - if two hardware drivers failed to agree on a new policy before this stage, the incompatible hardware shall be shut down, and the user informed of this. diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c index 47af702bb6a2..bb01dea39fdc 100644 --- a/drivers/acpi/processor_perflib.c +++ b/drivers/acpi/processor_perflib.c @@ -83,7 +83,7 @@ static int acpi_processor_ppc_notifier(struct notifier_block *nb, if (ignore_ppc) return 0; - if (event != CPUFREQ_INCOMPATIBLE) + if (event != CPUFREQ_ADJUST) return 0; mutex_lock(&performance_mutex); diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 76a26609d96b..293f47b814bf 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2206,10 +2206,6 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_ADJUST, new_policy); - /* adjust if necessary - hardware incompatibility*/ - blocking_notifier_call_chain(&cpufreq_policy_notifier_list, - CPUFREQ_INCOMPATIBLE, new_policy); - /* * verify the cpu speed can be set within this limit, which might be * different to the first one diff --git a/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c b/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c index d29e8da396a0..7969f7690498 100644 --- a/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c +++ b/drivers/cpufreq/ppc_cbe_cpufreq_pmi.c @@ -97,8 +97,8 @@ static int pmi_notifier(struct notifier_block *nb, struct cpufreq_frequency_table *cbe_freqs; u8 node; - /* Should this really be called for CPUFREQ_ADJUST, CPUFREQ_INCOMPATIBLE - * and CPUFREQ_NOTIFY policy events?) + /* Should this really be called for CPUFREQ_ADJUST and CPUFREQ_NOTIFY + * policy events?) */ if (event == CPUFREQ_START) return 0; diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c index 7245611ec963..94813af97f09 100644 --- a/drivers/video/fbdev/pxafb.c +++ b/drivers/video/fbdev/pxafb.c @@ -1668,7 +1668,6 @@ pxafb_freq_policy(struct notifier_block *nb, unsigned long val, void *data) switch (val) { case CPUFREQ_ADJUST: - case CPUFREQ_INCOMPATIBLE: pr_debug("min dma period: %d ps, " "new clock %d kHz\n", pxafb_display_dma_period(var), policy->max); diff --git a/drivers/video/fbdev/sa1100fb.c b/drivers/video/fbdev/sa1100fb.c index 89dd7e02197f..dcf774c15889 100644 --- a/drivers/video/fbdev/sa1100fb.c +++ b/drivers/video/fbdev/sa1100fb.c @@ -1042,7 +1042,6 @@ sa1100fb_freq_policy(struct notifier_block *nb, unsigned long val, switch (val) { case CPUFREQ_ADJUST: - case CPUFREQ_INCOMPATIBLE: dev_dbg(fbi->dev, "min dma period: %d ps, " "new clock %d kHz\n", sa1100fb_min_dma_period(fbi), policy->max); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index bde1e567b3a9..bedcc90c0757 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -369,11 +369,10 @@ static inline void cpufreq_resume(void) {} /* Policy Notifiers */ #define CPUFREQ_ADJUST (0) -#define CPUFREQ_INCOMPATIBLE (1) -#define CPUFREQ_NOTIFY (2) -#define CPUFREQ_START (3) -#define CPUFREQ_CREATE_POLICY (4) -#define CPUFREQ_REMOVE_POLICY (5) +#define CPUFREQ_NOTIFY (1) +#define CPUFREQ_START (2) +#define CPUFREQ_CREATE_POLICY (3) +#define CPUFREQ_REMOVE_POLICY (4) #ifdef CONFIG_CPU_FREQ int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);
What's being done from CPUFREQ_INCOMPATIBLE, can also be done with CPUFREQ_ADJUST. There is nothing special with CPUFREQ_INCOMPATIBLE notifier. Kill CPUFREQ_INCOMPATIBLE and fix its usage sites. This also updates the numbering of notifier events to remove holes. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- Documentation/cpu-freq/core.txt | 7 ++----- drivers/acpi/processor_perflib.c | 2 +- drivers/cpufreq/cpufreq.c | 4 ---- drivers/cpufreq/ppc_cbe_cpufreq_pmi.c | 4 ++-- drivers/video/fbdev/pxafb.c | 1 - drivers/video/fbdev/sa1100fb.c | 1 - include/linux/cpufreq.h | 9 ++++----- 7 files changed, 9 insertions(+), 19 deletions(-)