Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221780 > unrolled thread

Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-10 01:00 +0200
    Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE  notifier event Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-10 02:40 +0200
      Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE  notifier event "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-10 03:00 +0200

#1221780 — Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event

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);
> 

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221817 — Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event

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.
-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221829 — Re: [PATCH 1/7] cpufreq: remove redundant CPUFREQ_INCOMPATIBLE notifier event

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web