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

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

[Viresh Kumar]

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

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

[PATCH 2/7] cpufreq: use memcpy() to copy policy

[Viresh Kumar]

cpufreq_get_policy() is useful if the pointer to policy isn't available
in advance. But if it is available, then there is no need to call
cpufreq_get_policy(). Directly use memcpy() to copy the policy.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 293f47b814bf..86d69416821b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -606,9 +606,7 @@ static ssize_t store_##file_name					\
 	int ret, temp;							\
 	struct cpufreq_policy new_policy;				\
 									\
-	ret = cpufreq_get_policy(&new_policy, policy->cpu);		\
-	if (ret)							\
-		return -EINVAL;						\
+	memcpy(&new_policy, policy, sizeof(*policy));			\
 									\
 	ret = sscanf(buf, "%u", &new_policy.object);			\
 	if (ret != 1)							\
@@ -662,9 +660,7 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 	char	str_governor[16];
 	struct cpufreq_policy new_policy;
 
-	ret = cpufreq_get_policy(&new_policy, policy->cpu);
-	if (ret)
-		return ret;
+	memcpy(&new_policy, policy, sizeof(*policy));
 
 	ret = sscanf(buf, "%15s", str_governor);
 	if (ret != 1)
-- 
2.4.0

[PATCH 3/7] cpufreq: update user_policy.* on success

[Viresh Kumar]

'user_policy' caches properties of a policy that are set by userspace.
And these must be updated only if cpufreq core was successful in
updating them based on request from user space.

In store_scaling_governor(), we are updating user_policy.policy and
user_policy.governor even if cpufreq_set_policy() failed. That's
incorrect.

Fix this by updating user_policy.* only if we were successful in
updating the properties.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 86d69416821b..8e71d8e08439 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -671,14 +671,12 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 		return -EINVAL;
 
 	ret = cpufreq_set_policy(policy, &new_policy);
+	if (ret)
+		return ret;
 
 	policy->user_policy.policy = policy->policy;
 	policy->user_policy.governor = policy->governor;
-
-	if (ret)
-		return ret;
-	else
-		return count;
+	return count;
 }
 
 /**
-- 
2.4.0

[PATCH 4/7] cpufreq: remove redundant 'governor' field from user_policy

[Viresh Kumar]

Its always same as policy->governor, and there is no need to keep
another copy of it. Remove it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 7 ++-----
 include/linux/cpufreq.h   | 1 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8e71d8e08439..3033952391fe 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -675,7 +675,6 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 		return ret;
 
 	policy->user_policy.policy = policy->policy;
-	policy->user_policy.governor = policy->governor;
 	return count;
 }
 
@@ -1323,10 +1322,9 @@ static int cpufreq_online(unsigned int cpu)
 		goto out_exit_policy;
 	}
 
-	if (new_policy) {
+	if (new_policy)
 		policy->user_policy.policy = policy->policy;
-		policy->user_policy.governor = policy->governor;
-	}
+
 	up_write(&policy->rwsem);
 
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
@@ -2305,7 +2303,6 @@ int cpufreq_update_policy(unsigned int cpu)
 	new_policy.min = policy->user_policy.min;
 	new_policy.max = policy->user_policy.max;
 	new_policy.policy = policy->user_policy.policy;
-	new_policy.governor = policy->user_policy.governor;
 
 	/*
 	 * BIOS might change freq behind our back
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index bedcc90c0757..752bf8a5c314 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -55,7 +55,6 @@ struct cpufreq_real_policy {
 	unsigned int		min;    /* in kHz */
 	unsigned int		max;    /* in kHz */
 	unsigned int		policy; /* see above */
-	struct cpufreq_governor	*governor; /* see below */
 };
 
 struct cpufreq_policy {
-- 
2.4.0

[PATCH 5/7] cpufreq: remove redundant 'policy' field from user_policy

[Viresh Kumar]

Its always same as policy->policy, and there is no need to keep another
copy of it. Remove it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 10 +---------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3033952391fe..a80bd68bbd74 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -671,11 +671,7 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
 		return -EINVAL;
 
 	ret = cpufreq_set_policy(policy, &new_policy);
-	if (ret)
-		return ret;
-
-	policy->user_policy.policy = policy->policy;
-	return count;
+	return ret ? ret : count;
 }
 
 /**
@@ -1322,9 +1318,6 @@ static int cpufreq_online(unsigned int cpu)
 		goto out_exit_policy;
 	}
 
-	if (new_policy)
-		policy->user_policy.policy = policy->policy;
-
 	up_write(&policy->rwsem);
 
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
@@ -2302,7 +2295,6 @@ int cpufreq_update_policy(unsigned int cpu)
 	memcpy(&new_policy, policy, sizeof(*policy));
 	new_policy.min = policy->user_policy.min;
 	new_policy.max = policy->user_policy.max;
-	new_policy.policy = policy->user_policy.policy;
 
 	/*
 	 * BIOS might change freq behind our back
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 752bf8a5c314..54dbbff0a55e 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -54,7 +54,6 @@ struct cpufreq_cpuinfo {
 struct cpufreq_real_policy {
 	unsigned int		min;    /* in kHz */
 	unsigned int		max;    /* in kHz */
-	unsigned int		policy; /* see above */
 };
 
 struct cpufreq_policy {
-- 
2.4.0

[PATCH 6/7] cpufreq: rename cpufreq_real_policy as cpufreq_user_policy

[Viresh Kumar]

Its all about caching min/max freq requested by userspace, and
the name 'cpufreq_real_policy' doesn't fit that well. Rename it to
cpufreq_user_policy.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/cpufreq.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 54dbbff0a55e..6ff6a4d95eea 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -51,7 +51,7 @@ struct cpufreq_cpuinfo {
 	unsigned int		transition_latency;
 };
 
-struct cpufreq_real_policy {
+struct cpufreq_user_policy {
 	unsigned int		min;    /* in kHz */
 	unsigned int		max;    /* in kHz */
 };
@@ -86,7 +86,7 @@ struct cpufreq_policy {
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */
 
-	struct cpufreq_real_policy	user_policy;
+	struct cpufreq_user_policy user_policy;
 	struct cpufreq_frequency_table	*freq_table;
 
 	struct list_head        policy_list;
-- 
2.4.0

[PATCH 7/7] cpufreq: drop !cpufreq_driver check from cpufreq_parse_governor()

[Viresh Kumar]

Driver is guaranteed to be present on a call to cpufreq_parse_governor()
and there is no need to check for !cpufreq_driver. Drop it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a80bd68bbd74..a05cc75cc45d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -520,9 +520,6 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy,
 {
 	int err = -EINVAL;
 
-	if (!cpufreq_driver)
-		goto out;
-
 	if (cpufreq_driver->setpolicy) {
 		if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
 			*policy = CPUFREQ_POLICY_PERFORMANCE;
@@ -557,7 +554,6 @@ static int cpufreq_parse_governor(char *str_governor, unsigned int *policy,
 
 		mutex_unlock(&cpufreq_governor_mutex);
 	}
-out:
 	return err;
 }
 
-- 
2.4.0

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

[Rafael J. Wysocki]

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.

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

[Viresh Kumar]

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

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

[Rafael J. Wysocki]

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

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

[Saravana Kannan]

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


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Rafael J. Wysocki]

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.

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

[Saravana Kannan]

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`

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project