Re: [PATCH v2 06/13] cpufreq: powernv-cpufreq: Functions only used in call-backs should be static

2020-07-15 Thread Viresh Kumar
On 15-07-20, 09:26, Lee Jones wrote:
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/cpufreq/powernv-cpufreq.c:669:6: warning: no previous prototype for 
> ‘gpstate_timer_handler’ [-Wmissing-prototypes]
>  drivers/cpufreq/powernv-cpufreq.c:902:6: warning: no previous prototype for 
> ‘powernv_cpufreq_work_fn’ [-Wmissing-prototypes]
> 
> Cc: Michael Ellerman 
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Lee Jones 
> Acked-by: Viresh Kumar 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 8646eb197cd96..068cc53abe320 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -666,7 +666,7 @@ static inline void  queue_gpstate_timer(struct 
> global_pstate_info *gpstates)
>   * according quadratic equation. Queues a new timer if it is still not equal
>   * to local pstate
>   */
> -void gpstate_timer_handler(struct timer_list *t)
> +static void gpstate_timer_handler(struct timer_list *t)
>  {
>   struct global_pstate_info *gpstates = from_timer(gpstates, t, timer);
>   struct cpufreq_policy *policy = gpstates->policy;
> @@ -899,7 +899,7 @@ static struct notifier_block powernv_cpufreq_reboot_nb = {
>   .notifier_call = powernv_cpufreq_reboot_notifier,
>  };
>  
> -void powernv_cpufreq_work_fn(struct work_struct *work)
> +static void powernv_cpufreq_work_fn(struct work_struct *work)
>  {
>   struct chip *chip = container_of(work, struct chip, throttle);
>   struct cpufreq_policy *policy;

Don't you want to drop this patch now ? As you already reviewed the
other one on the list ?

-- 
viresh


Re: [PATCH 05/13] cpufreq/arch: powerpc: pasemi: Move prototypes to shared header

2020-07-15 Thread Viresh Kumar
On 15-07-20, 07:36, Lee Jones wrote:
> I searched for "include.*platforms/" in drivers/, and was scared off
> this method since no one else does this.

Yeah its not right for generic drivers, but this is very much platform
specific so it is fine here.

-- 
viresh


Re: [PATCH 05/13] cpufreq/arch: powerpc: pasemi: Move prototypes to shared header

2020-07-14 Thread Viresh Kumar
On 14-07-20, 20:49, Olof Johansson wrote:
> On Tue, Jul 14, 2020 at 8:07 PM Viresh Kumar  wrote:
> >
> > On 14-07-20, 15:50, Lee Jones wrote:
> > > If function callers and providers do not share the same prototypes the
> > > compiler complains of missing prototypes.  Fix this by moving the
> > > already existing prototypes out to a mutually convenient location.
> > >
> > > Fixes the following W=1 kernel build warning(s):
> > >
> > >  drivers/cpufreq/pasemi-cpufreq.c:109:5: warning: no previous prototype 
> > > for ‘check_astate’ [-Wmissing-prototypes]
> > >  109 | int check_astate(void)
> > >  | ^~~~
> > >  drivers/cpufreq/pasemi-cpufreq.c:114:6: warning: no previous prototype 
> > > for ‘restore_astate’ [-Wmissing-prototypes]
> > >  114 | void restore_astate(int cpu)
> > >  | ^~
> > >
> > > Cc: Olof Johansson 
> > > Cc: Michael Ellerman 
> > > Cc: Benjamin Herrenschmidt 
> > > Cc: Paul Mackerras 
> > > Cc: linuxppc-dev@lists.ozlabs.org
> > > Signed-off-by: Lee Jones 
> > > ---
> > >  arch/powerpc/platforms/pasemi/pasemi.h| 15 
> >
> > Is there no sane way we can include this file directly to the cpufreq
> > file ?
> 
> Yep. arch/powerpc seems to be in the search path for modules on powerpc, so:
> 
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c 
> b/drivers/cpufreq/pasemi-cpufreq.c
> index c66f566a854cb..815645170c4de 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -22,6 +22,8 @@
>  #include 
>  #include 
> 
> +#include 
> +
>  #define SDCASR_REG 0x0100
>  #define SDCASR_REG_STRIDE  0x1000
>  #define SDCPWR_CFGA0_REG   0x0100

Fantastic. Thanks.

-- 
viresh


Re: [PATCH -next] cpufreq: powernv: Make some symbols static

2020-07-14 Thread Viresh Kumar
On 14-07-20, 22:23, Wei Yongjun wrote:
> The sparse tool complains as follows:
> 
> drivers/cpufreq/powernv-cpufreq.c:88:1: warning:
>  symbol 'pstate_revmap' was not declared. Should it be static?
> drivers/cpufreq/powernv-cpufreq.c:383:18: warning:
>  symbol 'cpufreq_freq_attr_cpuinfo_nominal_freq' was not declared. Should it 
> be static?
> drivers/cpufreq/powernv-cpufreq.c:669:6: warning:
>  symbol 'gpstate_timer_handler' was not declared. Should it be static?
> drivers/cpufreq/powernv-cpufreq.c:902:6: warning:
>  symbol 'powernv_cpufreq_work_fn' was not declared. Should it be static?
> 
> Those symbols are not used outside of this file, so mark
> them static.
> 
> Reported-by: Hulk Robot 
> Signed-off-by: Wei Yongjun 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Lee also sent a fix for this, but yours look complete :)

https://lore.kernel.org/lkml/20200714145049.2496163-7-lee.jo...@linaro.org/

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH 07/13] cpufreq: powernv-cpufreq: Fix a bunch of kerneldoc related issues

2020-07-14 Thread Viresh Kumar
On 14-07-20, 15:50, Lee Jones wrote:
> Repair problems with formatting and missing attributes/parameters, and
> demote header comments which do not meet the required standards
> applicable to kerneldoc.
> 
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/cpufreq/powernv-cpufreq.c:84: warning: Function parameter or member 
> 'last_lpstate_idx' not described in 'global_pstate_info'
>  drivers/cpufreq/powernv-cpufreq.c:84: warning: Function parameter or member 
> 'last_gpstate_idx' not described in 'global_pstate_info'
>  drivers/cpufreq/powernv-cpufreq.c:84: warning: Function parameter or member 
> 'policy' not described in 'global_pstate_info'
>  drivers/cpufreq/powernv-cpufreq.c:182: warning: Function parameter or member 
> 'i' not described in 'idx_to_pstate'
>  drivers/cpufreq/powernv-cpufreq.c:201: warning: Function parameter or member 
> 'pstate' not described in 'pstate_to_idx'
>  drivers/cpufreq/powernv-cpufreq.c:670: warning: Function parameter or member 
> 't' not described in 'gpstate_timer_handler'
>  drivers/cpufreq/powernv-cpufreq.c:670: warning: Excess function parameter 
> 'data' description in 'gpstate_timer_handler'
> 
> Cc: Michael Ellerman 
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Lee Jones 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 068cc53abe320..2e5a8b8a4abaa 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -64,13 +64,14 @@
>   *   highest_lpstate_idx
>   * @last_sampled_time:   Time from boot in ms when global 
> pstates were
>   *   last set
> - * @last_lpstate_idx,Last set value of local pstate and 
> global
> - * last_gpstate_idx  pstate in terms of cpufreq table index
> + * @last_lpstate_idx:Last set value of local pstate and 
> global
> + * @last_gpstate_idx:pstate in terms of cpufreq table index
>   * @timer:   Is used for ramping down if cpu goes idle for
>   *   a long time with global pstate held high
>   * @gpstate_lock:A spinlock to maintain synchronization between
>   *   routines called by the timer handler and
>   *   governer's target_index calls
> + * @policy:  Associated CPUFreq policy
>   */
>  struct global_pstate_info {
>   int highest_lpstate_idx;
> @@ -170,7 +171,7 @@ static inline u8 extract_pstate(u64 pmsr_val, unsigned 
> int shift)
>  
>  /* Use following functions for conversions between pstate_id and index */
>  
> -/**
> +/*
>   * idx_to_pstate : Returns the pstate id corresponding to the
>   *  frequency in the cpufreq frequency table
>   *  powernv_freqs indexed by @i.
> @@ -188,7 +189,7 @@ static inline u8 idx_to_pstate(unsigned int i)
>   return powernv_freqs[i].driver_data;
>  }
>  
> -/**
> +/*
>   * pstate_to_idx : Returns the index in the cpufreq frequencytable
>   *  powernv_freqs for the frequency whose corresponding
>   *  pstate id is @pstate.
> @@ -660,7 +661,7 @@ static inline void  queue_gpstate_timer(struct 
> global_pstate_info *gpstates)
>  /**
>   * gpstate_timer_handler
>   *
> - * @data: pointer to cpufreq_policy on which timer was queued
> + * @t: Timer context used to fetch global pstate info struct
>   *
>   * This handler brings down the global pstate closer to the local pstate
>   * according quadratic equation. Queues a new timer if it is still not equal

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH 06/13] cpufreq: powernv-cpufreq: Functions only used in call-backs should be static

2020-07-14 Thread Viresh Kumar
On 14-07-20, 15:50, Lee Jones wrote:
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/cpufreq/powernv-cpufreq.c:669:6: warning: no previous prototype for 
> ‘gpstate_timer_handler’ [-Wmissing-prototypes]
>  drivers/cpufreq/powernv-cpufreq.c:902:6: warning: no previous prototype for 
> ‘powernv_cpufreq_work_fn’ [-Wmissing-prototypes]
> 
> Cc: Michael Ellerman 
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Lee Jones 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 8646eb197cd96..068cc53abe320 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -666,7 +666,7 @@ static inline void  queue_gpstate_timer(struct 
> global_pstate_info *gpstates)
>   * according quadratic equation. Queues a new timer if it is still not equal
>   * to local pstate
>   */
> -void gpstate_timer_handler(struct timer_list *t)
> +static void gpstate_timer_handler(struct timer_list *t)
>  {
>   struct global_pstate_info *gpstates = from_timer(gpstates, t, timer);
>   struct cpufreq_policy *policy = gpstates->policy;
> @@ -899,7 +899,7 @@ static struct notifier_block powernv_cpufreq_reboot_nb = {
>   .notifier_call = powernv_cpufreq_reboot_notifier,
>  };
>  
> -void powernv_cpufreq_work_fn(struct work_struct *work)
> +static void powernv_cpufreq_work_fn(struct work_struct *work)
>  {
>   struct chip *chip = container_of(work, struct chip, throttle);
>   struct cpufreq_policy *policy;

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH 05/13] cpufreq/arch: powerpc: pasemi: Move prototypes to shared header

2020-07-14 Thread Viresh Kumar
On 14-07-20, 15:50, Lee Jones wrote:
> If function callers and providers do not share the same prototypes the
> compiler complains of missing prototypes.  Fix this by moving the
> already existing prototypes out to a mutually convenient location.
> 
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/cpufreq/pasemi-cpufreq.c:109:5: warning: no previous prototype for 
> ‘check_astate’ [-Wmissing-prototypes]
>  109 | int check_astate(void)
>  | ^~~~
>  drivers/cpufreq/pasemi-cpufreq.c:114:6: warning: no previous prototype for 
> ‘restore_astate’ [-Wmissing-prototypes]
>  114 | void restore_astate(int cpu)
>  | ^~
> 
> Cc: Olof Johansson 
> Cc: Michael Ellerman 
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Lee Jones 
> ---
>  arch/powerpc/platforms/pasemi/pasemi.h| 15 

Is there no sane way we can include this file directly to the cpufreq
file ?

>  arch/powerpc/platforms/pasemi/powersave.S |  2 ++
>  drivers/cpufreq/pasemi-cpufreq.c  |  1 +
>  include/linux/platform_data/pasemi.h  | 28 +++
>  4 files changed, 31 insertions(+), 15 deletions(-)
>  create mode 100644 include/linux/platform_data/pasemi.h

-- 
viresh


[PATCH V4 2/3] cpufreq: Register governors at core_initcall

2020-06-29 Thread Viresh Kumar
From: Quentin Perret 

Currently, most CPUFreq governors are registered at core_initcall time
when used as default, and module_init otherwise. In preparation for
letting users specify the default governor on the kernel command line,
change all of them to use core_initcall unconditionally, as is already
the case for schedutil and performance. This will enable us to assume
builtin governors have been registered before the builtin CPUFreq
drivers probe.

And since all governors now have similar init/exit patterns, introduce
two new macros cpufreq_governor_{init,exit}() to factorize the code.

Acked-by: Viresh Kumar 
Signed-off-by: Quentin Perret 
Signed-off-by: Viresh Kumar 
---
 .../platforms/cell/cpufreq_spudemand.c| 26 ++-
 drivers/cpufreq/cpufreq_conservative.c| 22 
 drivers/cpufreq/cpufreq_ondemand.c| 24 +
 drivers/cpufreq/cpufreq_performance.c | 14 ++
 drivers/cpufreq/cpufreq_powersave.c   | 18 +++--
 drivers/cpufreq/cpufreq_userspace.c   | 18 +++--
 include/linux/cpufreq.h   | 14 ++
 kernel/sched/cpufreq_schedutil.c  |  6 +
 8 files changed, 36 insertions(+), 106 deletions(-)

diff --git a/arch/powerpc/platforms/cell/cpufreq_spudemand.c 
b/arch/powerpc/platforms/cell/cpufreq_spudemand.c
index 55b31eadb3c8..ca7849e113d7 100644
--- a/arch/powerpc/platforms/cell/cpufreq_spudemand.c
+++ b/arch/powerpc/platforms/cell/cpufreq_spudemand.c
@@ -126,30 +126,8 @@ static struct cpufreq_governor spu_governor = {
.stop = spu_gov_stop,
.owner = THIS_MODULE,
 };
-
-/*
- * module init and destoy
- */
-
-static int __init spu_gov_init(void)
-{
-   int ret;
-
-   ret = cpufreq_register_governor(_governor);
-   if (ret)
-   printk(KERN_ERR "registration of governor failed\n");
-   return ret;
-}
-
-static void __exit spu_gov_exit(void)
-{
-   cpufreq_unregister_governor(_governor);
-}
-
-
-module_init(spu_gov_init);
-module_exit(spu_gov_exit);
+cpufreq_governor_init(spu_governor);
+cpufreq_governor_exit(spu_governor);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Christian Krafft ");
-
diff --git a/drivers/cpufreq/cpufreq_conservative.c 
b/drivers/cpufreq/cpufreq_conservative.c
index 737ff3b9c2c0..aa39ff31ec9f 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -322,17 +322,7 @@ static struct dbs_governor cs_governor = {
.start = cs_start,
 };
 
-#define CPU_FREQ_GOV_CONSERVATIVE  (_governor.gov)
-
-static int __init cpufreq_gov_dbs_init(void)
-{
-   return cpufreq_register_governor(CPU_FREQ_GOV_CONSERVATIVE);
-}
-
-static void __exit cpufreq_gov_dbs_exit(void)
-{
-   cpufreq_unregister_governor(CPU_FREQ_GOV_CONSERVATIVE);
-}
+#define CPU_FREQ_GOV_CONSERVATIVE  (cs_governor.gov)
 
 MODULE_AUTHOR("Alexander Clouter ");
 MODULE_DESCRIPTION("'cpufreq_conservative' - A dynamic cpufreq governor for "
@@ -343,11 +333,9 @@ MODULE_LICENSE("GPL");
 #ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE
 struct cpufreq_governor *cpufreq_default_governor(void)
 {
-   return CPU_FREQ_GOV_CONSERVATIVE;
+   return _FREQ_GOV_CONSERVATIVE;
 }
-
-core_initcall(cpufreq_gov_dbs_init);
-#else
-module_init(cpufreq_gov_dbs_init);
 #endif
-module_exit(cpufreq_gov_dbs_exit);
+
+cpufreq_governor_init(CPU_FREQ_GOV_CONSERVATIVE);
+cpufreq_governor_exit(CPU_FREQ_GOV_CONSERVATIVE);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 82a4d37ddecb..ac361a8b1d3b 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -408,7 +408,7 @@ static struct dbs_governor od_dbs_gov = {
.start = od_start,
 };
 
-#define CPU_FREQ_GOV_ONDEMAND  (_dbs_gov.gov)
+#define CPU_FREQ_GOV_ONDEMAND  (od_dbs_gov.gov)
 
 static void od_set_powersave_bias(unsigned int powersave_bias)
 {
@@ -429,7 +429,7 @@ static void od_set_powersave_bias(unsigned int 
powersave_bias)
continue;
 
policy = cpufreq_cpu_get_raw(cpu);
-   if (!policy || policy->governor != CPU_FREQ_GOV_ONDEMAND)
+   if (!policy || policy->governor != _FREQ_GOV_ONDEMAND)
continue;
 
policy_dbs = policy->governor_data;
@@ -461,16 +461,6 @@ void od_unregister_powersave_bias_handler(void)
 }
 EXPORT_SYMBOL_GPL(od_unregister_powersave_bias_handler);
 
-static int __init cpufreq_gov_dbs_init(void)
-{
-   return cpufreq_register_governor(CPU_FREQ_GOV_ONDEMAND);
-}
-
-static void __exit cpufreq_gov_dbs_exit(void)
-{
-   cpufreq_unregister_governor(CPU_FREQ_GOV_ONDEMAND);
-}
-
 MODULE_AUTHOR("Venkatesh Pallipadi ");
 MODULE_AUTHOR("Alexey Starikovskiy ");
 MODULE_DESCRIPTION("'cpufreq_ondemand' - A dynamic cpufreq governor for "
@@ -480,11 +47

[PATCH V4 0/3] cpufreq: Allow default governor on cmdline and fix locking issues

2020-06-29 Thread Viresh Kumar
Hi,

I have picked Quentin's series over my patch, modified both and tested.

V3->V4:
- Do __module_get() for cpufreq_default_governor() case as well and get
  rid of an extra variable.
- Use a single character array, default_governor, instead of two of them.

V2->V3:
- default_governor is a string now and we don't set it on governor
  registration or unregistration anymore.
- Fixed locking issues in cpufreq_init_policy().

--
Viresh

Original cover letter fro Quentin:

This series enables users of prebuilt kernels (e.g. distro kernels) to
specify their CPUfreq governor of choice using the kernel command line,
instead of having to wait for the system to fully boot to userspace to
switch using the sysfs interface. This is helpful for 2 reasons:
  1. users get to choose the governor that runs during the actual boot;
  2. it simplifies the userspace boot procedure a bit (one less thing to
 worry about).

To enable this, the first patch moves all governor init calls to
core_initcall, to make sure they are registered by the time the drivers
probe. This should be relatively low impact as registering a governor
is a simple procedure (it gets added to a llist), and all governors
already load at core_initcall anyway when they're set as the default
in Kconfig. This also allows to clean-up the governors' init/exit code,
and reduces boilerplate.

The second patch introduces the new command line parameter, inspired by
its cpuidle counterpart. More details can be found in the respective
patch headers.

Changes in v2:
 - added Viresh's ack to patch 01
 - moved the assignment of 'default_governor' in patch 02 to the governor
   registration path instead of the driver registration (Viresh)

Quentin Perret (2):
  cpufreq: Register governors at core_initcall
  cpufreq: Specify default governor on command line

Viresh Kumar (1):
  cpufreq: Fix locking issues with governors

 .../admin-guide/kernel-parameters.txt |  5 ++
 Documentation/admin-guide/pm/cpufreq.rst  |  6 +-
 .../platforms/cell/cpufreq_spudemand.c| 26 +-
 drivers/cpufreq/cpufreq.c | 87 ---
 drivers/cpufreq/cpufreq_conservative.c| 22 ++---
 drivers/cpufreq/cpufreq_ondemand.c| 24 ++---
 drivers/cpufreq/cpufreq_performance.c | 14 +--
 drivers/cpufreq/cpufreq_powersave.c   | 18 +---
 drivers/cpufreq/cpufreq_userspace.c   | 18 +---
 include/linux/cpufreq.h   | 14 +++
 kernel/sched/cpufreq_schedutil.c  |  6 +-
 11 files changed, 100 insertions(+), 140 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af



[PATCH V3 2/3] cpufreq: Register governors at core_initcall

2020-06-25 Thread Viresh Kumar
From: Quentin Perret 

Currently, most CPUFreq governors are registered at core_initcall time
when used as default, and module_init otherwise. In preparation for
letting users specify the default governor on the kernel command line,
change all of them to use core_initcall unconditionally, as is already
the case for schedutil and performance. This will enable us to assume
builtin governors have been registered before the builtin CPUFreq
drivers probe.

And since all governors now have similar init/exit patterns, introduce
two new macros cpufreq_governor_{init,exit}() to factorize the code.

Acked-by: Viresh Kumar 
Signed-off-by: Quentin Perret 
Signed-off-by: Viresh Kumar 
---
 .../platforms/cell/cpufreq_spudemand.c| 26 ++-
 drivers/cpufreq/cpufreq_conservative.c| 22 
 drivers/cpufreq/cpufreq_ondemand.c| 24 +
 drivers/cpufreq/cpufreq_performance.c | 14 ++
 drivers/cpufreq/cpufreq_powersave.c   | 18 +++--
 drivers/cpufreq/cpufreq_userspace.c   | 18 +++--
 include/linux/cpufreq.h   | 14 ++
 kernel/sched/cpufreq_schedutil.c  |  6 +
 8 files changed, 36 insertions(+), 106 deletions(-)

diff --git a/arch/powerpc/platforms/cell/cpufreq_spudemand.c 
b/arch/powerpc/platforms/cell/cpufreq_spudemand.c
index 55b31eadb3c8..ca7849e113d7 100644
--- a/arch/powerpc/platforms/cell/cpufreq_spudemand.c
+++ b/arch/powerpc/platforms/cell/cpufreq_spudemand.c
@@ -126,30 +126,8 @@ static struct cpufreq_governor spu_governor = {
.stop = spu_gov_stop,
.owner = THIS_MODULE,
 };
-
-/*
- * module init and destoy
- */
-
-static int __init spu_gov_init(void)
-{
-   int ret;
-
-   ret = cpufreq_register_governor(_governor);
-   if (ret)
-   printk(KERN_ERR "registration of governor failed\n");
-   return ret;
-}
-
-static void __exit spu_gov_exit(void)
-{
-   cpufreq_unregister_governor(_governor);
-}
-
-
-module_init(spu_gov_init);
-module_exit(spu_gov_exit);
+cpufreq_governor_init(spu_governor);
+cpufreq_governor_exit(spu_governor);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Christian Krafft ");
-
diff --git a/drivers/cpufreq/cpufreq_conservative.c 
b/drivers/cpufreq/cpufreq_conservative.c
index 737ff3b9c2c0..aa39ff31ec9f 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -322,17 +322,7 @@ static struct dbs_governor cs_governor = {
.start = cs_start,
 };
 
-#define CPU_FREQ_GOV_CONSERVATIVE  (_governor.gov)
-
-static int __init cpufreq_gov_dbs_init(void)
-{
-   return cpufreq_register_governor(CPU_FREQ_GOV_CONSERVATIVE);
-}
-
-static void __exit cpufreq_gov_dbs_exit(void)
-{
-   cpufreq_unregister_governor(CPU_FREQ_GOV_CONSERVATIVE);
-}
+#define CPU_FREQ_GOV_CONSERVATIVE  (cs_governor.gov)
 
 MODULE_AUTHOR("Alexander Clouter ");
 MODULE_DESCRIPTION("'cpufreq_conservative' - A dynamic cpufreq governor for "
@@ -343,11 +333,9 @@ MODULE_LICENSE("GPL");
 #ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE
 struct cpufreq_governor *cpufreq_default_governor(void)
 {
-   return CPU_FREQ_GOV_CONSERVATIVE;
+   return _FREQ_GOV_CONSERVATIVE;
 }
-
-core_initcall(cpufreq_gov_dbs_init);
-#else
-module_init(cpufreq_gov_dbs_init);
 #endif
-module_exit(cpufreq_gov_dbs_exit);
+
+cpufreq_governor_init(CPU_FREQ_GOV_CONSERVATIVE);
+cpufreq_governor_exit(CPU_FREQ_GOV_CONSERVATIVE);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 82a4d37ddecb..ac361a8b1d3b 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -408,7 +408,7 @@ static struct dbs_governor od_dbs_gov = {
.start = od_start,
 };
 
-#define CPU_FREQ_GOV_ONDEMAND  (_dbs_gov.gov)
+#define CPU_FREQ_GOV_ONDEMAND  (od_dbs_gov.gov)
 
 static void od_set_powersave_bias(unsigned int powersave_bias)
 {
@@ -429,7 +429,7 @@ static void od_set_powersave_bias(unsigned int 
powersave_bias)
continue;
 
policy = cpufreq_cpu_get_raw(cpu);
-   if (!policy || policy->governor != CPU_FREQ_GOV_ONDEMAND)
+   if (!policy || policy->governor != _FREQ_GOV_ONDEMAND)
continue;
 
policy_dbs = policy->governor_data;
@@ -461,16 +461,6 @@ void od_unregister_powersave_bias_handler(void)
 }
 EXPORT_SYMBOL_GPL(od_unregister_powersave_bias_handler);
 
-static int __init cpufreq_gov_dbs_init(void)
-{
-   return cpufreq_register_governor(CPU_FREQ_GOV_ONDEMAND);
-}
-
-static void __exit cpufreq_gov_dbs_exit(void)
-{
-   cpufreq_unregister_governor(CPU_FREQ_GOV_ONDEMAND);
-}
-
 MODULE_AUTHOR("Venkatesh Pallipadi ");
 MODULE_AUTHOR("Alexey Starikovskiy ");
 MODULE_DESCRIPTION("'cpufreq_ondemand' - A dynamic cpufreq governor for "
@@ -480,11 +47

[PATCH v3 0/3] cpufreq: Allow default governor on cmdline and fix locking issues

2020-06-25 Thread Viresh Kumar
Hi,

I have picked Quentin's series over my patch, modified both and tested.

V2->V3:
- default_governor is a string now and we don't set it on governor
  registration or unregistration anymore.
- Fixed locking issues in cpufreq_init_policy().

--
Viresh

Original cover letter fro Quentin:

This series enables users of prebuilt kernels (e.g. distro kernels) to
specify their CPUfreq governor of choice using the kernel command line,
instead of having to wait for the system to fully boot to userspace to
switch using the sysfs interface. This is helpful for 2 reasons:
  1. users get to choose the governor that runs during the actual boot;
  2. it simplifies the userspace boot procedure a bit (one less thing to
 worry about).

To enable this, the first patch moves all governor init calls to
core_initcall, to make sure they are registered by the time the drivers
probe. This should be relatively low impact as registering a governor
is a simple procedure (it gets added to a llist), and all governors
already load at core_initcall anyway when they're set as the default
in Kconfig. This also allows to clean-up the governors' init/exit code,
and reduces boilerplate.

The second patch introduces the new command line parameter, inspired by
its cpuidle counterpart. More details can be found in the respective
patch headers.

Changes in v2:
 - added Viresh's ack to patch 01
 - moved the assignment of 'default_governor' in patch 02 to the governor
   registration path instead of the driver registration (Viresh)

Quentin Perret (2):
  cpufreq: Register governors at core_initcall
  cpufreq: Specify default governor on command line

Viresh Kumar (1):
  cpufreq: Fix locking issues with governors

 .../admin-guide/kernel-parameters.txt |  5 +
 Documentation/admin-guide/pm/cpufreq.rst  |  6 +-
 .../platforms/cell/cpufreq_spudemand.c| 26 +-
 drivers/cpufreq/cpufreq.c | 93 ---
 drivers/cpufreq/cpufreq_conservative.c| 22 +
 drivers/cpufreq/cpufreq_ondemand.c| 24 ++---
 drivers/cpufreq/cpufreq_performance.c | 14 +--
 drivers/cpufreq/cpufreq_powersave.c   | 18 +---
 drivers/cpufreq/cpufreq_userspace.c   | 18 +---
 include/linux/cpufreq.h   | 14 +++
 kernel/sched/cpufreq_schedutil.c  |  6 +-
 11 files changed, 106 insertions(+), 140 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af



Re: [PATCH v2 2/2] cpufreq: Specify default governor on command line

2020-06-25 Thread Viresh Kumar
On 23-06-20, 15:21, Quentin Perret wrote:
> @@ -2789,7 +2796,13 @@ static int __init cpufreq_core_init(void)
>   cpufreq_global_kobject = kobject_create_and_add("cpufreq", 
> _subsys.dev_root->kobj);
>   BUG_ON(!cpufreq_global_kobject);
>  
> + mutex_lock(_governor_mutex);
> + if (!default_governor)

Also is this check really required ? The pointer will always be NULL
at this point, isn't it ?

> + default_governor = cpufreq_default_governor();
> + mutex_unlock(_governor_mutex);
> +
>   return 0;
>  }
>  module_param(off, int, 0444);
> +module_param_string(default_governor, cpufreq_param_governor, 
> CPUFREQ_NAME_LEN, 0444);
>  core_initcall(cpufreq_core_init);
> -- 
> 2.27.0.111.gc72c7da667-goog

-- 
viresh


Re: [PATCH v2 2/2] cpufreq: Specify default governor on command line

2020-06-23 Thread Viresh Kumar
  } else if (default_governor) {
> + gov = default_governor;
>   } else {
>   return -ENODATA;
>   }
> @@ -1074,8 +1076,8 @@ static int cpufreq_init_policy(struct cpufreq_policy 
> *policy)
>   /* Use the default policy if there is no last_policy. */
>   if (policy->last_policy) {
>   pol = policy->last_policy;
> - } else if (def_gov) {
> - pol = cpufreq_parse_policy(def_gov->name);
> + } else if (default_governor) {
> + pol = cpufreq_parse_policy(default_governor->name);
>   /*
>* In case the default governor is neiter "performance"
>* nor "powersave", fall back to the initial policy
> @@ -2320,6 +2322,9 @@ int cpufreq_register_governor(struct cpufreq_governor 
> *governor)
>   list_add(>governor_list, _governor_list);
>   }
>  
> + if (!strncasecmp(cpufreq_param_governor, governor->name, 
> CPUFREQ_NAME_LEN))
> + default_governor = governor;
> +
>   mutex_unlock(_governor_mutex);
>   return err;
>  }
> @@ -2348,6 +2353,8 @@ void cpufreq_unregister_governor(struct 
> cpufreq_governor *governor)
>  
>   mutex_lock(_governor_mutex);
>   list_del(>governor_list);
> + if (governor == default_governor)
> + default_governor = cpufreq_default_governor();
>   mutex_unlock(_governor_mutex);
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_unregister_governor);
> @@ -2789,7 +2796,13 @@ static int __init cpufreq_core_init(void)
>   cpufreq_global_kobject = kobject_create_and_add("cpufreq", 
> _subsys.dev_root->kobj);
>   BUG_ON(!cpufreq_global_kobject);
>  
> + mutex_lock(_governor_mutex);
> + if (!default_governor)
> + default_governor = cpufreq_default_governor();
> + mutex_unlock(_governor_mutex);

I don't think locking is required here at core-initcall level. Apart
from that:

Acked-by: Viresh Kumar 

> +
>   return 0;
>  }
>  module_param(off, int, 0444);
> +module_param_string(default_governor, cpufreq_param_governor, 
> CPUFREQ_NAME_LEN, 0444);
>  core_initcall(cpufreq_core_init);
> -- 
> 2.27.0.111.gc72c7da667-goog

-- 
viresh


Re: [PATCH 2/2] cpufreq: Specify default governor on command line

2020-06-16 Thread Viresh Kumar
On 16-06-20, 10:48, Quentin Perret wrote:
> ---8<---
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 0f05caedc320..a9219404e07f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2340,6 +2340,11 @@ int cpufreq_register_governor(struct cpufreq_governor 
> *governor)
>   list_add(>governor_list, _governor_list);
>   }
>  
> + if (!strncasecmp(cpufreq_param_governor, governor->name, 
> CPUFREQ_NAME_LEN))
> + default_governor = governor;
> + else if (!default_governor && cpufreq_default_governor() == governor)
> + default_governor = cpufreq_default_governor();

Instead of the else part here, maybe just do this from
cpufreq_core_init() only once, and so we will always have
default_governor set.

> +
>   mutex_unlock(_governor_mutex);
>   return err;
>  }
> @@ -2368,6 +2373,8 @@ void cpufreq_unregister_governor(struct 
> cpufreq_governor *governor)
>  
>   mutex_lock(_governor_mutex);
>   list_del(>governor_list);
> + if (governor == default_governor)
> + default_governor = cpufreq_default_governor();
>   mutex_unlock(_governor_mutex);
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_unregister_governor);
> --->8---
> 
> should do the trick. That removes the unnecessary reference count, and
> feels like a good place to hook things -- that is how cpuidle does it
> too IIRC.
> 
> I'll double check the locking/synchronization, but that shouldn't be too
> bad (famous last words).
> 
> Cheers,
> Quentin

-- 
viresh


Re: [PATCH 2/2] cpufreq: Specify default governor on command line

2020-06-16 Thread Viresh Kumar
On 16-06-20, 09:31, Quentin Perret wrote:
> Right, so the reason I avoided cpufreq_core_init() was because it is
> called at core_initcall() time, which means I can't really assume the
> governors have been loaded by that time. By waiting for the driver to
> probe before detecting the default gov, we get that nice ordering. But
> yes, it feels odd to have it here :/
> 
> Thinking about it more, the natural fit for this would rather be the
> register/unregister path for governors directly. If that sounds good to
> you (?) I'll try to move it there in v2.

There is another problem here which we need to look at. Any governor
which is built as a module and isn't currently used, should be allowed
to unload. And this needs to be tested by you as well, should be easy
enough.

With the current implementation, you take a reference to the default
governor when the driver is registered and drop it only when the
driver goes away. Which means we won't be able to unload the module of
the governor even if it isn't used. Which is wrong. The solution I
proposed had the same issue as well.

You need to figure out a way where we don't need to keep holding the
module hostage even when it isn't used. I see two ways at least for
the same:

- Do that from the existing place: cpufreq_init_policy().

- And I think this can be done from governor-register/unregister as
  well.

Second one sounds good, if it is feasible to do that.

-- 
viresh


Re: [PATCH 2/2] cpufreq: Specify default governor on command line

2020-06-15 Thread Viresh Kumar
On 15-06-20, 17:55, Quentin Perret wrote:
> +static void cpufreq_get_default_governor(void)
> +{
> + default_governor = cpufreq_parse_governor(cpufreq_param_governor);
> + if (!default_governor) {
> + if (*cpufreq_param_governor)
> + pr_warn("Failed to find %s\n", cpufreq_param_governor);
> + default_governor = cpufreq_default_governor();

A module_get() never happened for this case and so maybe a
module_put() should never get called.

> + }
> +}
> +
> +static void cpufreq_put_default_governor(void)
> +{
> + if (!default_governor)
> + return;
> + module_put(default_governor->owner);
> + default_governor = NULL;
> +}
> +
>  static int cpufreq_init_governor(struct cpufreq_policy *policy)
>  {
>   int ret;
> @@ -2701,6 +2721,8 @@ int cpufreq_register_driver(struct cpufreq_driver 
> *driver_data)
>  
>   if (driver_data->setpolicy)
>   driver_data->flags |= CPUFREQ_CONST_LOOPS;
> + else
> + cpufreq_get_default_governor();
>  
>   if (cpufreq_boost_supported()) {
>   ret = create_boost_sysfs_file();
> @@ -2769,6 +2791,7 @@ int cpufreq_unregister_driver(struct cpufreq_driver 
> *driver)
>   subsys_interface_unregister(_interface);
>   remove_boost_sysfs_file();
>   cpuhp_remove_state_nocalls_cpuslocked(hp_online);
> + cpufreq_put_default_governor();
>  
>   write_lock_irqsave(_driver_lock, flags);
>  
> @@ -2792,4 +2815,5 @@ static int __init cpufreq_core_init(void)
>   return 0;
>  }

And since this is a per boot thing, there is perhaps no need of doing
these at driver register/unregister, I would rather do it at:
cpufreq_core_init() time itself and so we will never need to run
cpufreq_put_default_governor() and so can be removed.

And another thing I am not able to understand (despite you commenting
about that in the commit log) is what happens if the default governor
chosen is built as a module ?

-- 
viresh


Re: [PATCH 1/2] cpufreq: Register governors at core_initcall

2020-06-15 Thread Viresh Kumar
On 15-06-20, 17:55, Quentin Perret wrote:
> Currently, most CPUFreq governors are registered at core_initcall time
> when used as default, and module_init otherwise. In preparation for
> letting users specify the default governor on the kernel command line,
> change all of them to use core_initcall unconditionally, as is already
> the case for schedutil and performance. This will enable us to assume
> builtin governors have been registered before the builtin CPUFreq
> drivers probe.
> 
> And since all governors now have similar init/exit patterns, introduce
> two new macros cpufreq_governor_{init,exit}() to factorize the code.
> 
> Signed-off-by: Quentin Perret 
> ---
> Note: I couldn't boot-test the change to spudemand, by lack of hardware.
> But I can confirm cell_defconfig compiles just fine.
> ---
>  .../platforms/cell/cpufreq_spudemand.c| 26 ++-
>  drivers/cpufreq/cpufreq_conservative.c| 22 
>  drivers/cpufreq/cpufreq_ondemand.c| 24 +
>  drivers/cpufreq/cpufreq_performance.c | 14 ++
>  drivers/cpufreq/cpufreq_powersave.c   | 18 +++--
>  drivers/cpufreq/cpufreq_userspace.c   | 18 +++--
>  include/linux/cpufreq.h   | 14 ++
>  kernel/sched/cpufreq_schedutil.c  |  6 +
>  8 files changed, 36 insertions(+), 106 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH v2 1/2] cpufreq: qoriq: convert to a platform driver

2020-05-06 Thread Viresh Kumar
On 28-04-20, 16:31, Viresh Kumar wrote:
> On 21-04-20, 10:29, Mian Yousaf Kaukab wrote:
> > The driver has to be manually loaded if it is built as a module. It
> > is neither exporting MODULE_DEVICE_TABLE nor MODULE_ALIAS. Moreover,
> > no platform-device is created (and thus no uevent is sent) for the
> > clockgen nodes it depends on.
> > 
> > Convert the module to a platform driver with its own alias. Moreover,
> > drop whitelisted SOCs. Platform device will be created only for the
> > compatible platforms.
> > 
> > Reviewed-by: Yuantian Tang 
> > Acked-by: Viresh Kumar 
> > Signed-off-by: Mian Yousaf Kaukab 
> > ---
> > v2:
> >  +Rafael, Stephen, linux-clk
> >  Add Reviewed-by and Acked-by tags
> > 
> >  drivers/cpufreq/qoriq-cpufreq.c | 76 
> > -
> >  1 file changed, 29 insertions(+), 47 deletions(-)
> 
> @Rafael,
> 
> Though this looks to be PPC stuff, but it is used on both ARM and PPC. Do you
> want to pick them up or should I do that ?

Applied now. Thanks.

-- 
viresh


Re: [PATCH v2 1/2] cpufreq: qoriq: convert to a platform driver

2020-04-28 Thread Viresh Kumar
On 21-04-20, 10:29, Mian Yousaf Kaukab wrote:
> The driver has to be manually loaded if it is built as a module. It
> is neither exporting MODULE_DEVICE_TABLE nor MODULE_ALIAS. Moreover,
> no platform-device is created (and thus no uevent is sent) for the
> clockgen nodes it depends on.
> 
> Convert the module to a platform driver with its own alias. Moreover,
> drop whitelisted SOCs. Platform device will be created only for the
> compatible platforms.
> 
> Reviewed-by: Yuantian Tang 
> Acked-by: Viresh Kumar 
> Signed-off-by: Mian Yousaf Kaukab 
> ---
> v2:
>  +Rafael, Stephen, linux-clk
>  Add Reviewed-by and Acked-by tags
> 
>  drivers/cpufreq/qoriq-cpufreq.c | 76 
> -
>  1 file changed, 29 insertions(+), 47 deletions(-)

@Rafael,

Though this looks to be PPC stuff, but it is used on both ARM and PPC. Do you
want to pick them up or should I do that ?

-- 
viresh


Re: [PATCH 1/2] cpufreq: qoriq: convert to a platform driver

2020-04-05 Thread Viresh Kumar
On 03-04-20, 23:21, Mian Yousaf Kaukab wrote:
> The driver has to be manually loaded if it is built as a module. It
> is neither exporting MODULE_DEVICE_TABLE nor MODULE_ALIAS. Moreover,
> no platform-device is created (and thus no uevent is sent) for the
> clockgen nodes it depends on.
> 
> Convert the module to a platform driver with its own alias. Moreover,
> drop whitelisted SOCs. Platform device will be created only for the
> compatible platforms.
> 
> Signed-off-by: Mian Yousaf Kaukab 
> ---
>  drivers/cpufreq/qoriq-cpufreq.c | 76 
> -
>  1 file changed, 29 insertions(+), 47 deletions(-)

For both patches,

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH v3] cpufreq: powernv: fix stack bloat and hard limit on num cpus

2019-10-31 Thread Viresh Kumar
On Thu, 31 Oct 2019 at 10:52, John Hubbard  wrote:
>
> The following build warning occurred on powerpc 64-bit builds:
>
> drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
> drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of
> 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> This is with a cross-compiler based on gcc 8.1.0, which I got from:
>   https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/
>
> The warning is due to putting 1024 bytes on the stack:
>
> unsigned int chip[256];
>
> ...and it's also undesirable to have a hard limit on the number of
> CPUs here.
>
> Fix both problems by dynamically allocating based on num_possible_cpus,
> as recommended by Michael Ellerman.
>
> Fixes: 053819e0bf840 ("cpufreq: powernv: Handle throttling due to Pmax 
> capping at chip level")
> Cc: Michael Ellerman 
> Cc: Shilpasri G Bhat 
> Cc: Preeti U Murthy 
> Cc: Viresh Kumar 
> Cc: Rafael J. Wysocki 
> Cc: linux...@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: John Hubbard 
> ---

Acked-by: Viresh Kumar 


Re: [PATCH v2] cpufreq: powernv: fix stack bloat and NR_CPUS limitation

2019-10-17 Thread Viresh Kumar
On 17-10-19, 21:55, John Hubbard wrote:
> The following build warning occurred on powerpc 64-bit builds:
> 
> drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
> drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of 1040 
> bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> This is due to putting 1024 bytes on the stack:
> 
> unsigned int chip[256];
> 
> ...and while looking at this, it also has a bug: it fails with a stack
> overrun, if CONFIG_NR_CPUS > 256.
> 
> Fix both problems by dynamically allocating based on CONFIG_NR_CPUS.
> 
> Fixes: 053819e0bf840 ("cpufreq: powernv: Handle throttling due to Pmax 
> capping at chip level")
> Cc: Shilpasri G Bhat 
> Cc: Preeti U Murthy 
> Cc: Viresh Kumar 
> Cc: Rafael J. Wysocki 
> Cc: linux...@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: John Hubbard 
> ---
> 
> Changes since v1: includes Viresh's review commit fixes.
> 
>  drivers/cpufreq/powernv-cpufreq.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH] cpufreq: powernv: fix stack bloat and NR_CPUS limitation

2019-10-17 Thread Viresh Kumar
On 17-10-19, 21:41, John Hubbard wrote:
> On 10/17/19 9:38 PM, Viresh Kumar wrote:
> > On 17-10-19, 21:34, John Hubbard wrote:
> >> On 10/17/19 9:27 PM, Viresh Kumar wrote:
> >>> On 17-10-19, 17:04, John Hubbard wrote:
> >>>> The following build warning occurred on powerpc 64-bit builds:
> >>>>
> >>>> drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
> >>>> drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of 
> >>>> 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> >>>
> >>> How come we are catching this warning after 4 years ?
> >>>
> >>
> >> Newer compilers. And btw, I don't spend a lot of time in powerpc
> >> code, so I just recently ran this, and I guess everyone has been on less
> >> new compilers so far, it seems.
> >>
> >> I used a gcc 8.1 cross compiler in this case:
> > 
> > Hmm, okay.
> > 
> > I hope you haven't missed my actual review comments on your patch,
> > just wanted to make sure we don't end up waiting for each other
> > indefinitely here :)
> > 
> 
> Ha, I did overlook those. It's late around here, I guess. :)

Good that I reminded you then :)

-- 
viresh


Re: [PATCH] cpufreq: powernv: fix stack bloat and NR_CPUS limitation

2019-10-17 Thread Viresh Kumar
On 17-10-19, 21:34, John Hubbard wrote:
> On 10/17/19 9:27 PM, Viresh Kumar wrote:
> > On 17-10-19, 17:04, John Hubbard wrote:
> >> The following build warning occurred on powerpc 64-bit builds:
> >>
> >> drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
> >> drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of 1040 
> >> bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > 
> > How come we are catching this warning after 4 years ?
> > 
> 
> Newer compilers. And btw, I don't spend a lot of time in powerpc
> code, so I just recently ran this, and I guess everyone has been on less
> new compilers so far, it seems.
> 
> I used a gcc 8.1 cross compiler in this case:

Hmm, okay.

I hope you haven't missed my actual review comments on your patch,
just wanted to make sure we don't end up waiting for each other
indefinitely here :)

-- 
viresh


Re: [PATCH] cpufreq: powernv: fix stack bloat and NR_CPUS limitation

2019-10-17 Thread Viresh Kumar
On 17-10-19, 17:04, John Hubbard wrote:
> The following build warning occurred on powerpc 64-bit builds:
> 
> drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
> drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of 1040 
> bytes is larger than 1024 bytes [-Wframe-larger-than=]

How come we are catching this warning after 4 years ?

> 
> This is due to putting 1024 bytes on the stack:
> 
> unsigned int chip[256];
> 
> ...and while looking at this, it also has a bug: it fails with a stack
> overrun, if CONFIG_NR_CPUS > 256.
> 
> Fix both problems by dynamically allocating based on CONFIG_NR_CPUS.
> 
> Fixes: 053819e0bf840 ("cpufreq: powernv: Handle throttling due to Pmax 
> capping at chip level")
> Cc: Shilpasri G Bhat 
> Cc: Preeti U Murthy 
> Cc: Viresh Kumar 
> Cc: Rafael J. Wysocki 
> Cc: linux...@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: John Hubbard 
> ---
> 
> Hi,
> 
> I have only compile-tested this, so I would appreciate if anyone
> could do a basic runtime test on it. But (famous last words) it
> seems simple enough that I'm confident it's correct. oh boy. :)
> 
> thanks,
> John Hubbard
> NVIDIA
> 
>  drivers/cpufreq/powernv-cpufreq.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 6061850e59c9..78e04402125f 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -1041,9 +1041,14 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
>  
>  static int init_chip_info(void)
>  {
> - unsigned int chip[256];
> + unsigned int *chip;
>   unsigned int cpu, i;
>   unsigned int prev_chip_id = UINT_MAX;
> + int ret = 0;
> +
> + chip = kcalloc(CONFIG_NR_CPUS, sizeof(int), GFP_KERNEL);

   sizeof(*chip)

> + if (!chips)

   (!chip)

> + return -ENOMEM;
>  
>   for_each_possible_cpu(cpu) {
>   unsigned int id = cpu_to_chip_id(cpu);
> @@ -1055,8 +1060,10 @@ static int init_chip_info(void)
>   }
>  
>   chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL);
> - if (!chips)
> - return -ENOMEM;
> + if (!chips) {
> + ret = -ENOMEM;
> + goto free_and_return;
> + }
>  
>   for (i = 0; i < nr_chips; i++) {
>   chips[i].id = chip[i];
> @@ -1066,7 +1073,9 @@ static int init_chip_info(void)
>   per_cpu(chip_info, cpu) =  [i];
>   }
>  
> - return 0;
> +free_and_return:
> + kfree(chip);
> + return ret;
>  }
>  
>  static inline void clean_chip_info(void)
> -- 
> 2.23.0

-- 
viresh


[PATCH V2 03/10] powerpc: macintosh: Switch to QoS requests instead of cpufreq notifier

2019-07-23 Thread Viresh Kumar
The cpufreq core now takes the min/max frequency constraints via QoS
requests and the CPUFREQ_ADJUST notifier shall get removed later on.

Switch over to using the QoS request for maximum frequency constraint
for windfarm_cpufreq_clamp driver.

Signed-off-by: Viresh Kumar 
---
 drivers/macintosh/windfarm_cpufreq_clamp.c | 77 ++
 1 file changed, 50 insertions(+), 27 deletions(-)

diff --git a/drivers/macintosh/windfarm_cpufreq_clamp.c 
b/drivers/macintosh/windfarm_cpufreq_clamp.c
index 52fd5fca89a0..705c6200814b 100644
--- a/drivers/macintosh/windfarm_cpufreq_clamp.c
+++ b/drivers/macintosh/windfarm_cpufreq_clamp.c
@@ -3,9 +3,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -16,36 +18,24 @@
 
 static int clamped;
 static struct wf_control *clamp_control;
-
-static int clamp_notifier_call(struct notifier_block *self,
-  unsigned long event, void *data)
-{
-   struct cpufreq_policy *p = data;
-   unsigned long max_freq;
-
-   if (event != CPUFREQ_ADJUST)
-   return 0;
-
-   max_freq = clamped ? (p->cpuinfo.min_freq) : (p->cpuinfo.max_freq);
-   cpufreq_verify_within_limits(p, 0, max_freq);
-
-   return 0;
-}
-
-static struct notifier_block clamp_notifier = {
-   .notifier_call = clamp_notifier_call,
-};
+static struct dev_pm_qos_request qos_req;
+static unsigned int min_freq, max_freq;
 
 static int clamp_set(struct wf_control *ct, s32 value)
 {
-   if (value)
+   unsigned int freq;
+
+   if (value) {
+   freq = min_freq;
printk(KERN_INFO "windfarm: Clamping CPU frequency to "
   "minimum !\n");
-   else
+   } else {
+   freq = max_freq;
printk(KERN_INFO "windfarm: CPU frequency unclamped !\n");
+   }
clamped = value;
-   cpufreq_update_policy(0);
-   return 0;
+
+   return dev_pm_qos_update_request(_req, freq);
 }
 
 static int clamp_get(struct wf_control *ct, s32 *value)
@@ -74,27 +64,60 @@ static const struct wf_control_ops clamp_ops = {
 
 static int __init wf_cpufreq_clamp_init(void)
 {
+   struct cpufreq_policy *policy;
struct wf_control *clamp;
+   struct device *dev;
+   int ret;
+
+   policy = cpufreq_cpu_get(0);
+   if (!policy) {
+   pr_warn("%s: cpufreq policy not found cpu0\n", __func__);
+   return -EPROBE_DEFER;
+   }
+
+   min_freq = policy->cpuinfo.min_freq;
+   max_freq = policy->cpuinfo.max_freq;
+   cpufreq_cpu_put(policy);
+
+   dev = get_cpu_device(0);
+   if (unlikely(!dev)) {
+   pr_warn("%s: No cpu device for cpu0\n", __func__);
+   return -ENODEV;
+   }
 
clamp = kmalloc(sizeof(struct wf_control), GFP_KERNEL);
if (clamp == NULL)
return -ENOMEM;
-   cpufreq_register_notifier(_notifier, CPUFREQ_POLICY_NOTIFIER);
+
+   ret = dev_pm_qos_add_request(dev, _req, DEV_PM_QOS_MAX_FREQUENCY,
+max_freq);
+   if (ret < 0) {
+   pr_err("%s: Failed to add freq constraint (%d)\n", __func__,
+  ret);
+   goto free;
+   }
+
clamp->ops = _ops;
clamp->name = "cpufreq-clamp";
-   if (wf_register_control(clamp))
+   ret = wf_register_control(clamp);
+   if (ret)
goto fail;
clamp_control = clamp;
return 0;
  fail:
+   dev_pm_qos_remove_request(_req);
+
+ free:
kfree(clamp);
-   return -ENODEV;
+   return ret;
 }
 
 static void __exit wf_cpufreq_clamp_exit(void)
 {
-   if (clamp_control)
+   if (clamp_control) {
wf_unregister_control(clamp_control);
+   dev_pm_qos_remove_request(_req);
+   }
 }
 
 
-- 
2.21.0.rc0.269.g1a574e7a288b



[PATCH V2 00/10] cpufreq: Migrate users of policy notifiers to QoS requests

2019-07-23 Thread Viresh Kumar
Hello,

Now that cpufreq core supports taking QoS requests for min/max cpu
frequencies, lets migrate rest of the users to using them instead of the
policy notifiers.

The CPUFREQ_NOTIFY and CPUFREQ_ADJUST events of the policy notifiers are
removed as a result, but we have to add CPUFREQ_CREATE_POLICY and
CPUFREQ_REMOVE_POLICY events to it for the acpi stuff specifically,
though they are also used by arch_topology stuff now. So the policy
notifiers aren't completely removed.

Boot tested on my x86 PC and ARM hikey board.

This has already gone through build bot for a few days now.

V1->V2:
- Added Acked-by tags
- Reordered to keep cleanups at the bottom
- Rebased over 5.3-rc1

--
viresh

Viresh Kumar (10):
  cpufreq: Add policy create/remove notifiers
  thermal: cpu_cooling: Switch to QoS requests instead of cpufreq
notifier
  powerpc: macintosh: Switch to QoS requests instead of cpufreq notifier
  cpufreq: powerpc_cbe: Switch to QoS requests instead of cpufreq
notifier
  ACPI: cpufreq: Switch to QoS requests instead of cpufreq notifier
  arch_topology: Use CPUFREQ_CREATE_POLICY instead of CPUFREQ_NOTIFY
  video: sa1100fb: Remove cpufreq policy notifier
  video: pxafb: Remove cpufreq policy notifier
  cpufreq: Remove CPUFREQ_ADJUST and CPUFREQ_NOTIFY policy notifier
events
  Documentation: cpufreq: Update policy notifier documentation

 Documentation/cpu-freq/core.txt|  16 +--
 drivers/acpi/processor_driver.c|  44 -
 drivers/acpi/processor_perflib.c   | 106 +---
 drivers/acpi/processor_thermal.c   |  81 ---
 drivers/base/arch_topology.c   |   2 +-
 drivers/cpufreq/cpufreq.c  |  51 --
 drivers/cpufreq/ppc_cbe_cpufreq.c  |  19 +++-
 drivers/cpufreq/ppc_cbe_cpufreq.h  |   8 ++
 drivers/cpufreq/ppc_cbe_cpufreq_pmi.c  |  96 +++---
 drivers/macintosh/windfarm_cpufreq_clamp.c |  77 ++-
 drivers/thermal/cpu_cooling.c  | 110 +
 drivers/video/fbdev/pxafb.c|  21 
 drivers/video/fbdev/pxafb.h|   1 -
 drivers/video/fbdev/sa1100fb.c |  27 -
 drivers/video/fbdev/sa1100fb.h |   1 -
 include/acpi/processor.h   |  22 +++--
 include/linux/cpufreq.h|   4 +-
 17 files changed, 327 insertions(+), 359 deletions(-)

-- 
2.21.0.rc0.269.g1a574e7a288b



Re: [PATCH v7] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()

2019-07-16 Thread Viresh Kumar
On 17-07-19, 11:55, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
> 
> Fixes: a9acc26b75f6 ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang 
> Cc: "Rafael J. Wysocki" 
> Cc: Viresh Kumar 
> Cc: Michael Ellerman 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> v7: adapt to commit ("cpufreq: Make cpufreq_generic_init() return void")
> v6: keep the blank line and fix warning: label 'out_unmap_sdcpwr' defined but 
> not used.
> v5: put together the code to get, use, and release cpu device_node.
> v4: restore the blank line.
> v3: fix a leaked reference.
> v2: clean up the code according to the advice of viresh.
> 
>  drivers/cpufreq/pasemi-cpufreq.c | 23 +++++--
>  1 file changed, 9 insertions(+), 14 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH 00/10] cpufreq: Migrate users of policy notifiers to QoS requests

2019-07-16 Thread Viresh Kumar
On 16-07-19, 12:06, Rafael J. Wysocki wrote:
> On Tue, Jul 16, 2019 at 11:49 AM Viresh Kumar  wrote:
> >
> > Hello,
> >
> > Now that cpufreq core supports taking QoS requests for min/max cpu
> > frequencies, lets migrate rest of the users to using them instead of the
> > policy notifiers.
> 
> Technically, this still is linux-next only. :-)

True :)

> > The CPUFREQ_NOTIFY and CPUFREQ_ADJUST events of the policy notifiers are
> > removed as a result, but we have to add CPUFREQ_CREATE_POLICY and
> > CPUFREQ_REMOVE_POLICY events to it for the acpi stuff specifically. So
> > the policy notifiers aren't completely removed.
> 
> That's not entirely accurate, because arch_topology is going to use
> CPUFREQ_CREATE_POLICY now too.

Yeah, I thought about that while writing this patchset and
coverletter. But had it not been required for ACPI, I would have done
it differently for the arch-topology code. Maybe direct calling of
arch-topology routine from cpufreq core. I wanted to get rid of the
policy notifiers completely but I couldn't find a better way of doing
it for ACPI stuff.

> > Boot tested on my x86 PC and ARM hikey board. Nothing looked broken :)
> >
> > This has already gone through build bot for a few days now.
> 
> So I'd prefer patches [5-8] to go right after the first one and then
> do the cleanups on top of that, as somebody may want to backport the
> essential changes without the cleanups.

In the exceptional case where nobody finds anything wrong with the
patches (highly unlikely), do you want me to resend with reordering or
you can reorder them while applying? There are no dependencies between
those patches anyway.

--
viresh


[PATCH 06/10] powerpc: macintosh: Switch to QoS requests instead of cpufreq notifier

2019-07-16 Thread Viresh Kumar
The cpufreq core now takes the min/max frequency constraints via QoS
requests and the CPUFREQ_ADJUST notifier shall get removed later on.

Switch over to using the QoS request for maximum frequency constraint
for windfarm_cpufreq_clamp driver.

Signed-off-by: Viresh Kumar 
---
 drivers/macintosh/windfarm_cpufreq_clamp.c | 77 ++
 1 file changed, 50 insertions(+), 27 deletions(-)

diff --git a/drivers/macintosh/windfarm_cpufreq_clamp.c 
b/drivers/macintosh/windfarm_cpufreq_clamp.c
index 52fd5fca89a0..705c6200814b 100644
--- a/drivers/macintosh/windfarm_cpufreq_clamp.c
+++ b/drivers/macintosh/windfarm_cpufreq_clamp.c
@@ -3,9 +3,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -16,36 +18,24 @@
 
 static int clamped;
 static struct wf_control *clamp_control;
-
-static int clamp_notifier_call(struct notifier_block *self,
-  unsigned long event, void *data)
-{
-   struct cpufreq_policy *p = data;
-   unsigned long max_freq;
-
-   if (event != CPUFREQ_ADJUST)
-   return 0;
-
-   max_freq = clamped ? (p->cpuinfo.min_freq) : (p->cpuinfo.max_freq);
-   cpufreq_verify_within_limits(p, 0, max_freq);
-
-   return 0;
-}
-
-static struct notifier_block clamp_notifier = {
-   .notifier_call = clamp_notifier_call,
-};
+static struct dev_pm_qos_request qos_req;
+static unsigned int min_freq, max_freq;
 
 static int clamp_set(struct wf_control *ct, s32 value)
 {
-   if (value)
+   unsigned int freq;
+
+   if (value) {
+   freq = min_freq;
printk(KERN_INFO "windfarm: Clamping CPU frequency to "
   "minimum !\n");
-   else
+   } else {
+   freq = max_freq;
printk(KERN_INFO "windfarm: CPU frequency unclamped !\n");
+   }
clamped = value;
-   cpufreq_update_policy(0);
-   return 0;
+
+   return dev_pm_qos_update_request(_req, freq);
 }
 
 static int clamp_get(struct wf_control *ct, s32 *value)
@@ -74,27 +64,60 @@ static const struct wf_control_ops clamp_ops = {
 
 static int __init wf_cpufreq_clamp_init(void)
 {
+   struct cpufreq_policy *policy;
struct wf_control *clamp;
+   struct device *dev;
+   int ret;
+
+   policy = cpufreq_cpu_get(0);
+   if (!policy) {
+   pr_warn("%s: cpufreq policy not found cpu0\n", __func__);
+   return -EPROBE_DEFER;
+   }
+
+   min_freq = policy->cpuinfo.min_freq;
+   max_freq = policy->cpuinfo.max_freq;
+   cpufreq_cpu_put(policy);
+
+   dev = get_cpu_device(0);
+   if (unlikely(!dev)) {
+   pr_warn("%s: No cpu device for cpu0\n", __func__);
+   return -ENODEV;
+   }
 
clamp = kmalloc(sizeof(struct wf_control), GFP_KERNEL);
if (clamp == NULL)
return -ENOMEM;
-   cpufreq_register_notifier(_notifier, CPUFREQ_POLICY_NOTIFIER);
+
+   ret = dev_pm_qos_add_request(dev, _req, DEV_PM_QOS_MAX_FREQUENCY,
+max_freq);
+   if (ret < 0) {
+   pr_err("%s: Failed to add freq constraint (%d)\n", __func__,
+  ret);
+   goto free;
+   }
+
clamp->ops = _ops;
clamp->name = "cpufreq-clamp";
-   if (wf_register_control(clamp))
+   ret = wf_register_control(clamp);
+   if (ret)
goto fail;
clamp_control = clamp;
return 0;
  fail:
+   dev_pm_qos_remove_request(_req);
+
+ free:
kfree(clamp);
-   return -ENODEV;
+   return ret;
 }
 
 static void __exit wf_cpufreq_clamp_exit(void)
 {
-   if (clamp_control)
+   if (clamp_control) {
wf_unregister_control(clamp_control);
+   dev_pm_qos_remove_request(_req);
+   }
 }
 
 
-- 
2.21.0.rc0.269.g1a574e7a288b



[PATCH 00/10] cpufreq: Migrate users of policy notifiers to QoS requests

2019-07-16 Thread Viresh Kumar
Hello,

Now that cpufreq core supports taking QoS requests for min/max cpu
frequencies, lets migrate rest of the users to using them instead of the
policy notifiers.

The CPUFREQ_NOTIFY and CPUFREQ_ADJUST events of the policy notifiers are
removed as a result, but we have to add CPUFREQ_CREATE_POLICY and
CPUFREQ_REMOVE_POLICY events to it for the acpi stuff specifically. So
the policy notifiers aren't completely removed.

Boot tested on my x86 PC and ARM hikey board. Nothing looked broken :)

This has already gone through build bot for a few days now.

--
viresh

Viresh Kumar (10):
  cpufreq: Add policy create/remove notifiers
  video: sa1100fb: Remove cpufreq policy notifier
  video: pxafb: Remove cpufreq policy notifier
  arch_topology: Use CPUFREQ_CREATE_POLICY instead of CPUFREQ_NOTIFY
  thermal: cpu_cooling: Switch to QoS requests instead of cpufreq
notifier
  powerpc: macintosh: Switch to QoS requests instead of cpufreq notifier
  cpufreq: powerpc_cbe: Switch to QoS requests instead of cpufreq
notifier
  ACPI: cpufreq: Switch to QoS requests instead of cpufreq notifier
  cpufreq: Remove CPUFREQ_ADJUST and CPUFREQ_NOTIFY policy notifier
events
  Documentation: cpufreq: Update policy notifier documentation

 Documentation/cpu-freq/core.txt|  16 +--
 drivers/acpi/processor_driver.c|  44 -
 drivers/acpi/processor_perflib.c   | 106 +---
 drivers/acpi/processor_thermal.c   |  81 ---
 drivers/base/arch_topology.c   |   2 +-
 drivers/cpufreq/cpufreq.c  |  51 --
 drivers/cpufreq/ppc_cbe_cpufreq.c  |  19 +++-
 drivers/cpufreq/ppc_cbe_cpufreq.h  |   8 ++
 drivers/cpufreq/ppc_cbe_cpufreq_pmi.c  |  96 +++---
 drivers/macintosh/windfarm_cpufreq_clamp.c |  77 ++-
 drivers/thermal/cpu_cooling.c  | 110 +
 drivers/video/fbdev/pxafb.c|  21 
 drivers/video/fbdev/pxafb.h|   1 -
 drivers/video/fbdev/sa1100fb.c |  27 -
 drivers/video/fbdev/sa1100fb.h |   1 -
 include/acpi/processor.h   |  22 +++--
 include/linux/cpufreq.h|   4 +-
 17 files changed, 327 insertions(+), 359 deletions(-)

-- 
2.21.0.rc0.269.g1a574e7a288b



Re: [PATCH v6] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()

2019-07-16 Thread Viresh Kumar
On 16-07-19, 16:26, wen.yan...@zte.com.cn wrote:
> Okay thank you.
> Now this patch
> (https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.ku...@linaro.org/)
>  
> seems to have not been merged into the linux-next.
> 
> In order to avoid code conflicts, we will wait until this patch is merged in 
> and then send v7.

Please rebase on PM tree's linux-next branch instead and resend your
patch.

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git

-- 
viresh


[PATCH V3] cpufreq: Make cpufreq_generic_init() return void

2019-07-15 Thread Viresh Kumar
It always returns 0 (success) and its return type should really be void.
Over that, many drivers have added error handling code based on its
return value, which is not required at all.

change its return type to void and update all the callers.

Signed-off-by: Viresh Kumar 
---
V2->V3:
- Update bmips cpufreq driver to avoid "warning: 'ret' may be used
  uninitialized".
- Build bot reported this issue almost after 4 days of posting this
  patch, I was expecting this a lot earlier :)

 drivers/cpufreq/bmips-cpufreq.c | 17 ++---
 drivers/cpufreq/cpufreq.c   |  4 +---
 drivers/cpufreq/davinci-cpufreq.c   |  3 ++-
 drivers/cpufreq/imx6q-cpufreq.c |  6 ++
 drivers/cpufreq/kirkwood-cpufreq.c  |  3 ++-
 drivers/cpufreq/loongson1-cpufreq.c |  8 +++-
 drivers/cpufreq/loongson2_cpufreq.c |  3 ++-
 drivers/cpufreq/maple-cpufreq.c |  3 ++-
 drivers/cpufreq/omap-cpufreq.c  | 15 +--
 drivers/cpufreq/pasemi-cpufreq.c|  3 ++-
 drivers/cpufreq/pmac32-cpufreq.c|  3 ++-
 drivers/cpufreq/pmac64-cpufreq.c|  3 ++-
 drivers/cpufreq/s3c2416-cpufreq.c   |  9 ++---
 drivers/cpufreq/s3c64xx-cpufreq.c   | 15 +++
 drivers/cpufreq/s5pv210-cpufreq.c   |  3 ++-
 drivers/cpufreq/sa1100-cpufreq.c|  3 ++-
 drivers/cpufreq/sa1110-cpufreq.c|  3 ++-
 drivers/cpufreq/spear-cpufreq.c |  3 ++-
 drivers/cpufreq/tegra20-cpufreq.c   |  8 +---
 include/linux/cpufreq.h |  2 +-
 20 files changed, 46 insertions(+), 71 deletions(-)

diff --git a/drivers/cpufreq/bmips-cpufreq.c b/drivers/cpufreq/bmips-cpufreq.c
index 56a4ebbf00e0..f7c23fa468f0 100644
--- a/drivers/cpufreq/bmips-cpufreq.c
+++ b/drivers/cpufreq/bmips-cpufreq.c
@@ -131,23 +131,18 @@ static int bmips_cpufreq_exit(struct cpufreq_policy 
*policy)
 static int bmips_cpufreq_init(struct cpufreq_policy *policy)
 {
struct cpufreq_frequency_table *freq_table;
-   int ret;
 
freq_table = bmips_cpufreq_get_freq_table(policy);
if (IS_ERR(freq_table)) {
-   ret = PTR_ERR(freq_table);
-   pr_err("%s: couldn't determine frequency table (%d).\n",
-   BMIPS_CPUFREQ_NAME, ret);
-   return ret;
+   pr_err("%s: couldn't determine frequency table (%ld).\n",
+   BMIPS_CPUFREQ_NAME, PTR_ERR(freq_table));
+   return PTR_ERR(freq_table);
}
 
-   ret = cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
-   if (ret)
-   bmips_cpufreq_exit(policy);
-   else
-   pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
+   cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
+   pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
 
-   return ret;
+   return 0;
 }
 
 static struct cpufreq_driver bmips_cpufreq_driver = {
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d6043ee7834..8dda62367816 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -159,7 +159,7 @@ EXPORT_SYMBOL_GPL(arch_set_freq_scale);
  * - set policies transition latency
  * - policy->cpus with all possible CPUs
  */
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency)
 {
@@ -171,8 +171,6 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
 * share the clock and voltage and clock.
 */
cpumask_setall(policy->cpus);
-
-   return 0;
 }
 EXPORT_SYMBOL_GPL(cpufreq_generic_init);
 
diff --git a/drivers/cpufreq/davinci-cpufreq.c 
b/drivers/cpufreq/davinci-cpufreq.c
index 3de48ae60c29..297d23cad8b5 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -90,7 +90,8 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
 * Setting the latency to 2000 us to accommodate addition of drivers
 * to pre/post change notification list.
 */
-   return cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+   cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+   return 0;
 }
 
 static struct cpufreq_driver davinci_driver = {
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 47ccfa6b17b7..648a09a1778a 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -190,14 +190,12 @@ static int imx6q_set_target(struct cpufreq_policy 
*policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
-   int ret;
-
policy->clk = clks[ARM].clk;
-   ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+   cpufreq_generic_init(policy, freq_table, transition_latency);
policy->suspend_freq = max_freq;
dev_pm_opp_of_register_em(policy->cpus);
 
-   

[PATCH V2] cpufreq: Make cpufreq_generic_init() return void

2019-07-13 Thread Viresh Kumar
It always returns 0 (success) and its return type should really be void.
Over that, many drivers have added error handling code based on its
return value, which is not required at all.

change its return type to void and update all the callers.

Signed-off-by: Viresh Kumar 
---
V1->V2:
- Fixed compilation warning with s3c64xx driver by replace %d with %ld
  (Reported by buildbot).

 drivers/cpufreq/bmips-cpufreq.c |  7 ++-
 drivers/cpufreq/cpufreq.c   |  4 +---
 drivers/cpufreq/davinci-cpufreq.c   |  3 ++-
 drivers/cpufreq/imx6q-cpufreq.c |  6 ++
 drivers/cpufreq/kirkwood-cpufreq.c  |  3 ++-
 drivers/cpufreq/loongson1-cpufreq.c |  8 +++-
 drivers/cpufreq/loongson2_cpufreq.c |  3 ++-
 drivers/cpufreq/maple-cpufreq.c |  3 ++-
 drivers/cpufreq/omap-cpufreq.c  | 15 +--
 drivers/cpufreq/pasemi-cpufreq.c|  3 ++-
 drivers/cpufreq/pmac32-cpufreq.c|  3 ++-
 drivers/cpufreq/pmac64-cpufreq.c|  3 ++-
 drivers/cpufreq/s3c2416-cpufreq.c   |  9 ++---
 drivers/cpufreq/s3c64xx-cpufreq.c   | 15 +++
 drivers/cpufreq/s5pv210-cpufreq.c   |  3 ++-
 drivers/cpufreq/sa1100-cpufreq.c|  3 ++-
 drivers/cpufreq/sa1110-cpufreq.c|  3 ++-
 drivers/cpufreq/spear-cpufreq.c |  3 ++-
 drivers/cpufreq/tegra20-cpufreq.c   |  8 +---
 include/linux/cpufreq.h |  2 +-
 20 files changed, 42 insertions(+), 65 deletions(-)

diff --git a/drivers/cpufreq/bmips-cpufreq.c b/drivers/cpufreq/bmips-cpufreq.c
index 56a4ebbf00e0..2b187d802fe3 100644
--- a/drivers/cpufreq/bmips-cpufreq.c
+++ b/drivers/cpufreq/bmips-cpufreq.c
@@ -141,11 +141,8 @@ static int bmips_cpufreq_init(struct cpufreq_policy 
*policy)
return ret;
}
 
-   ret = cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
-   if (ret)
-   bmips_cpufreq_exit(policy);
-   else
-   pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
+   cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
+   pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
 
return ret;
 }
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d6043ee7834..8dda62367816 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -159,7 +159,7 @@ EXPORT_SYMBOL_GPL(arch_set_freq_scale);
  * - set policies transition latency
  * - policy->cpus with all possible CPUs
  */
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency)
 {
@@ -171,8 +171,6 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
 * share the clock and voltage and clock.
 */
cpumask_setall(policy->cpus);
-
-   return 0;
 }
 EXPORT_SYMBOL_GPL(cpufreq_generic_init);
 
diff --git a/drivers/cpufreq/davinci-cpufreq.c 
b/drivers/cpufreq/davinci-cpufreq.c
index 3de48ae60c29..297d23cad8b5 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -90,7 +90,8 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
 * Setting the latency to 2000 us to accommodate addition of drivers
 * to pre/post change notification list.
 */
-   return cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+   cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+   return 0;
 }
 
 static struct cpufreq_driver davinci_driver = {
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 47ccfa6b17b7..648a09a1778a 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -190,14 +190,12 @@ static int imx6q_set_target(struct cpufreq_policy 
*policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
-   int ret;
-
policy->clk = clks[ARM].clk;
-   ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+   cpufreq_generic_init(policy, freq_table, transition_latency);
policy->suspend_freq = max_freq;
dev_pm_opp_of_register_em(policy->cpus);
 
-   return ret;
+   return 0;
 }
 
 static struct cpufreq_driver imx6q_cpufreq_driver = {
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c 
b/drivers/cpufreq/kirkwood-cpufreq.c
index 7ab564c1f7ae..cb74bdc5baaa 100644
--- a/drivers/cpufreq/kirkwood-cpufreq.c
+++ b/drivers/cpufreq/kirkwood-cpufreq.c
@@ -85,7 +85,8 @@ static int kirkwood_cpufreq_target(struct cpufreq_policy 
*policy,
 /* Module init and exit code */
 static int kirkwood_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-   return cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+   cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+   return 0;
 }
 
 static struct cpufreq_driver kirkwood_cpufreq_driver = {
diff --git a/drivers/cpufreq/loongson1-cpufreq.c 
b/drivers/cpufreq/loongson1-cpufreq.c
i

Re: [PATCH v6] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()

2019-07-11 Thread Viresh Kumar
On 12-07-19, 10:44, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
> 
> Fixes: a9acc26b75f6 ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang 
> Cc: "Rafael J. Wysocki" 
> Cc: Viresh Kumar 
> Cc: Michael Ellerman 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> v6: keep the blank line and fix warning: label 'out_unmap_sdcpwr' defined but 
> not used.
> v5: put together the code to get, use, and release cpu device_node.
> v4: restore the blank line.
> v3: fix a leaked reference.
> v2: clean up the code according to the advice of viresh.
> 
>  drivers/cpufreq/pasemi-cpufreq.c | 26 ++
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c 
> b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..7d557f9 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -131,10 +131,18 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   int err = -ENODEV;
>  
>   cpu = of_get_cpu_node(policy->cpu, NULL);
> + if (!cpu)
> + goto out;
>  
> + max_freqp = of_get_property(cpu, "clock-frequency", NULL);
>   of_node_put(cpu);
> - if (!cpu)
> + if (!max_freqp) {
> + err = -EINVAL;
>   goto out;
> + }
> +
> + /* we need the freq in kHz */
> + max_freq = *max_freqp / 1000;
>  
>   dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
>   if (!dn)
> @@ -171,16 +179,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   }
>  
>   pr_debug("init cpufreq on CPU %d\n", policy->cpu);
> -
> - max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> - if (!max_freqp) {
> - err = -EINVAL;
> - goto out_unmap_sdcpwr;
> - }
> -
> - /* we need the freq in kHz */
> - max_freq = *max_freqp / 1000;
> -
>   pr_debug("max clock-frequency is at %u kHz\n", max_freq);
>   pr_debug("initializing frequency table\n");
>  
> @@ -196,7 +194,11 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   policy->cur = pas_freqs[cur_astate].frequency;
>   ppc_proc_freq = policy->cur * 1000ul;
>  
> - return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
> + err = cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());

So you are trying to fix an earlier issue here with this. Should have
been a separate patch. Over that I have just sent a patch now to make
this routine return void.

https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.ku...@linaro.org/

So all you need to do is to remove the label out_unmap_sdcpwr instead.

> + if (err)
> + goto out_unmap_sdcpwr;
> +
> + return 0;
>  
>  out_unmap_sdcpwr:
>   iounmap(sdcpwr_mapbase);
> -- 
> 2.9.5

-- 
viresh


[PATCH] cpufreq: Make cpufreq_generic_init() return void

2019-07-11 Thread Viresh Kumar
It always returns 0 (success) and its return type should really be void.
Over that, many drivers have added error handling code based on its
return value, which is not required at all.

change its return type to void and update all the callers.

Signed-off-by: Viresh Kumar 
---
 drivers/cpufreq/bmips-cpufreq.c |  7 ++-
 drivers/cpufreq/cpufreq.c   |  4 +---
 drivers/cpufreq/davinci-cpufreq.c   |  3 ++-
 drivers/cpufreq/imx6q-cpufreq.c |  6 ++
 drivers/cpufreq/kirkwood-cpufreq.c  |  3 ++-
 drivers/cpufreq/loongson1-cpufreq.c |  8 +++-
 drivers/cpufreq/loongson2_cpufreq.c |  3 ++-
 drivers/cpufreq/maple-cpufreq.c |  3 ++-
 drivers/cpufreq/omap-cpufreq.c  | 15 +--
 drivers/cpufreq/pasemi-cpufreq.c|  3 ++-
 drivers/cpufreq/pmac32-cpufreq.c|  3 ++-
 drivers/cpufreq/pmac64-cpufreq.c|  3 ++-
 drivers/cpufreq/s3c2416-cpufreq.c   |  9 ++---
 drivers/cpufreq/s3c64xx-cpufreq.c   | 15 +++
 drivers/cpufreq/s5pv210-cpufreq.c   |  3 ++-
 drivers/cpufreq/sa1100-cpufreq.c|  3 ++-
 drivers/cpufreq/sa1110-cpufreq.c|  3 ++-
 drivers/cpufreq/spear-cpufreq.c |  3 ++-
 drivers/cpufreq/tegra20-cpufreq.c   |  8 +---
 include/linux/cpufreq.h |  2 +-
 20 files changed, 42 insertions(+), 65 deletions(-)

diff --git a/drivers/cpufreq/bmips-cpufreq.c b/drivers/cpufreq/bmips-cpufreq.c
index 56a4ebbf00e0..2b187d802fe3 100644
--- a/drivers/cpufreq/bmips-cpufreq.c
+++ b/drivers/cpufreq/bmips-cpufreq.c
@@ -141,11 +141,8 @@ static int bmips_cpufreq_init(struct cpufreq_policy 
*policy)
return ret;
}
 
-   ret = cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
-   if (ret)
-   bmips_cpufreq_exit(policy);
-   else
-   pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
+   cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
+   pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
 
return ret;
 }
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d6043ee7834..8dda62367816 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -159,7 +159,7 @@ EXPORT_SYMBOL_GPL(arch_set_freq_scale);
  * - set policies transition latency
  * - policy->cpus with all possible CPUs
  */
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency)
 {
@@ -171,8 +171,6 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
 * share the clock and voltage and clock.
 */
cpumask_setall(policy->cpus);
-
-   return 0;
 }
 EXPORT_SYMBOL_GPL(cpufreq_generic_init);
 
diff --git a/drivers/cpufreq/davinci-cpufreq.c 
b/drivers/cpufreq/davinci-cpufreq.c
index 3de48ae60c29..297d23cad8b5 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -90,7 +90,8 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
 * Setting the latency to 2000 us to accommodate addition of drivers
 * to pre/post change notification list.
 */
-   return cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+   cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+   return 0;
 }
 
 static struct cpufreq_driver davinci_driver = {
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 47ccfa6b17b7..648a09a1778a 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -190,14 +190,12 @@ static int imx6q_set_target(struct cpufreq_policy 
*policy, unsigned int index)
 
 static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 {
-   int ret;
-
policy->clk = clks[ARM].clk;
-   ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+   cpufreq_generic_init(policy, freq_table, transition_latency);
policy->suspend_freq = max_freq;
dev_pm_opp_of_register_em(policy->cpus);
 
-   return ret;
+   return 0;
 }
 
 static struct cpufreq_driver imx6q_cpufreq_driver = {
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c 
b/drivers/cpufreq/kirkwood-cpufreq.c
index 7ab564c1f7ae..cb74bdc5baaa 100644
--- a/drivers/cpufreq/kirkwood-cpufreq.c
+++ b/drivers/cpufreq/kirkwood-cpufreq.c
@@ -85,7 +85,8 @@ static int kirkwood_cpufreq_target(struct cpufreq_policy 
*policy,
 /* Module init and exit code */
 static int kirkwood_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-   return cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+   cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+   return 0;
 }
 
 static struct cpufreq_driver kirkwood_cpufreq_driver = {
diff --git a/drivers/cpufreq/loongson1-cpufreq.c 
b/drivers/cpufreq/loongson1-cpufreq.c
index 21c9ce8526c0..0ea88778882a 100644
--- a/drivers/cpufreq/loongson1-cpufreq.c
+++ b/drivers/cpufreq/loongson1

Re: [PATCH v5] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()

2019-07-09 Thread Viresh Kumar
On 09-07-19, 16:04, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
> 
> Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang 
> Cc: "Rafael J. Wysocki" 
> Cc: Viresh Kumar 
> Cc: Michael Ellerman 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> v5: put together the code to get, use, and release cpu device_node.
> v4: restore the blank line.
> v3: fix a leaked reference.
> v2: clean up the code according to the advice of viresh.
> 
>  drivers/cpufreq/pasemi-cpufreq.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c 
> b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..1f0beb7 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -131,10 +131,17 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   int err = -ENODEV;
>  
>   cpu = of_get_cpu_node(policy->cpu, NULL);
> -
> - of_node_put(cpu);
>   if (!cpu)
>   goto out;

I would have loved a blank line here :)

> + max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> + of_node_put(cpu);
> + if (!max_freqp) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + /* we need the freq in kHz */
> + max_freq = *max_freqp / 1000;
>  
>   dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
>   if (!dn)
> @@ -171,16 +178,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   }
>  
>   pr_debug("init cpufreq on CPU %d\n", policy->cpu);
> -
> - max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> - if (!max_freqp) {
> - err = -EINVAL;
> - goto out_unmap_sdcpwr;
> - }
> -
> - /* we need the freq in kHz */
> - max_freq = *max_freqp / 1000;
> -
>   pr_debug("max clock-frequency is at %u kHz\n", max_freq);
>   pr_debug("initializing frequency table\n");

Though, enough versions have happened now.

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH v4] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()

2019-07-08 Thread Viresh Kumar
On 09-07-19, 07:39, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
> 
> Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang 
> Cc: "Rafael J. Wysocki" 
> Cc: Viresh Kumar 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> v4: restore the blank line.

I don't find it restored in the below code.

> v3: fix a leaked reference.
> v2: clean up the code according to the advice of viresh.
> 
>  drivers/cpufreq/pasemi-cpufreq.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c 
> b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..f0c98fc 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -128,20 +128,21 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   int cur_astate, idx;
>   struct resource res;
>   struct device_node *cpu, *dn;
> - int err = -ENODEV;
> + int err;
>  
>   cpu = of_get_cpu_node(policy->cpu, NULL);
>  
> - of_node_put(cpu);
>   if (!cpu)
> - goto out;
> + return -ENODEV;
>  
>   dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
>   if (!dn)
>   dn = of_find_compatible_node(NULL, NULL,
>"pasemi,pwrficient-sdc");
> - if (!dn)
> + if (!dn) {
> + err = -ENODEV;
>   goto out;
> + }
>   err = of_address_to_resource(dn, 0, );
>   of_node_put(dn);
>   if (err)
> @@ -196,6 +197,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   policy->cur = pas_freqs[cur_astate].frequency;
>   ppc_proc_freq = policy->cur * 1000ul;
>  
> + of_node_put(cpu);
>   return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
>  
>  out_unmap_sdcpwr:
> @@ -204,6 +206,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>  out_unmap_sdcasr:
>   iounmap(sdcasr_mapbase);
>  out:
> + of_node_put(cpu);
>   return err;
>  }
>  
> -- 
> 2.9.5

-- 
viresh


Re: [PATCH v3] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()

2019-07-08 Thread Viresh Kumar
On 08-07-19, 16:48, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
> 
> Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang 
> Cc: "Rafael J. Wysocki" 
> Cc: Viresh Kumar 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> v3: fix a leaked reference.
> v2: clean up the code according to the advice of viresh.
> 
>  drivers/cpufreq/pasemi-cpufreq.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c 
> b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..9dc5163 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -128,20 +128,20 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   int cur_astate, idx;
>   struct resource res;
>   struct device_node *cpu, *dn;
> - int err = -ENODEV;
> + int err;
>  
>   cpu = of_get_cpu_node(policy->cpu, NULL);
> -
> - of_node_put(cpu);
>   if (!cpu)
> - goto out;
> + return -ENODEV;
>  
>   dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
>   if (!dn)
>   dn = of_find_compatible_node(NULL, NULL,
>"pasemi,pwrficient-sdc");
> - if (!dn)
> + if (!dn) {
> + err = -ENODEV;
>   goto out;
> + }

Please restore the blank line here.

>   err = of_address_to_resource(dn, 0, );
>   of_node_put(dn);
>   if (err)
> @@ -196,6 +196,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   policy->cur = pas_freqs[cur_astate].frequency;
>   ppc_proc_freq = policy->cur * 1000ul;
>  
> + of_node_put(cpu);
>   return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
>  
>  out_unmap_sdcpwr:
> @@ -204,6 +205,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>  out_unmap_sdcasr:
>   iounmap(sdcasr_mapbase);
>  out:
> + of_node_put(cpu);
>   return err;
>  }
>  
> -- 
> 2.9.5

-- 
viresh


Re: [PATCH v2] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()

2019-07-08 Thread Viresh Kumar
On 08-07-19, 15:19, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
> 
> Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang 
> Cc: "Rafael J. Wysocki" 
> Cc: Viresh Kumar 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> v2: clean up the code according to the advice of viresh.
> 
>  drivers/cpufreq/pasemi-cpufreq.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c 
> b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..c6d464b 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -128,20 +128,18 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   int cur_astate, idx;
>   struct resource res;
>   struct device_node *cpu, *dn;
> - int err = -ENODEV;
> + int err;
>  
>   cpu = of_get_cpu_node(policy->cpu, NULL);
> -
> - of_node_put(cpu);
>   if (!cpu)
> - goto out;
> + return -ENODEV;
>  


>   dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
>   if (!dn)
>   dn = of_find_compatible_node(NULL, NULL,
>"pasemi,pwrficient-sdc");
>   if (!dn)
> - goto out;
> + return -ENODEV;

This change looks incorrect. You still need to drop reference to cpu ?

>   err = of_address_to_resource(dn, 0, );
>   of_node_put(dn);
>   if (err)
> @@ -196,6 +194,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   policy->cur = pas_freqs[cur_astate].frequency;
>   ppc_proc_freq = policy->cur * 1000ul;
>  
> + of_node_put(cpu);
>   return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
>  
>  out_unmap_sdcpwr:
> @@ -204,6 +203,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>  out_unmap_sdcasr:
>   iounmap(sdcasr_mapbase);
>  out:
> + of_node_put(cpu);
>   return err;
>  }
>  
> -- 
> 2.9.5

-- 
viresh


Re: [PATCH] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()

2019-07-08 Thread Viresh Kumar
On 08-07-19, 14:19, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
> 
> Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang 
> Cc: "Rafael J. Wysocki" 
> Cc: Viresh Kumar 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
>  drivers/cpufreq/pasemi-cpufreq.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)

I will suggest some changes here.

> diff --git a/drivers/cpufreq/pasemi-cpufreq.c 
> b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..d2dd47b 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -132,7 +132,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)

Don't initialize "err" anymore.

>   cpu = of_get_cpu_node(policy->cpu, NULL);
>  
> - of_node_put(cpu);
>   if (!cpu)
>   goto out;

Do return -ENODEV; here.

>  
> @@ -141,15 +140,15 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   dn = of_find_compatible_node(NULL, NULL,
>"pasemi,pwrficient-sdc");
>   if (!dn)
> - goto out;
> + goto out_put_cpu_node;
>   err = of_address_to_resource(dn, 0, );
>   of_node_put(dn);
>   if (err)
> - goto out;
> + goto out_put_cpu_node;
>   sdcasr_mapbase = ioremap(res.start + SDCASR_OFFSET, 0x2000);
>   if (!sdcasr_mapbase) {
>   err = -EINVAL;
> - goto out;
> + goto out_put_cpu_node;
>   }

Don't do above changes.

>  
>   dn = of_find_compatible_node(NULL, NULL, "1682m-gizmo");
> @@ -177,6 +176,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>   err = -EINVAL;
>   goto out_unmap_sdcpwr;
>   }
> + of_node_put(cpu);
>  
>   /* we need the freq in kHz */
>   max_freq = *max_freqp / 1000;
> @@ -203,6 +203,8 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
>  
>  out_unmap_sdcasr:
>   iounmap(sdcasr_mapbase);
> +out_put_cpu_node:

Don't add this label, instead use "out" for also having the below
code.

> + of_node_put(cpu);
>  out:
>   return err;
>  }
> -- 
> 2.9.5

-- 
viresh


Re: [PATCH] cpufreq: powernv: fix missing check of return value in init_powernv_pstates()

2019-02-17 Thread Viresh Kumar
On 16-02-19, 12:06, Yangtao Li wrote:
> kmalloc() could fail, so insert a check of its return value. And
> if it fails, returns -ENOMEM.
> 
> And remove (struct pstate_idx_revmap_data *) to fix coccinelle WARNING
> by the way.
> 
> WARNING: casting value returned by memory allocation function to (struct
> pstate_idx_revmap_data *) is useless.
> 
> Signed-off-by: Yangtao Li 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 7e7ad3879c4e..d2230812fa4b 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -244,6 +244,7 @@ static int init_powernv_pstates(void)
>   u32 len_ids, len_freqs;
>   u32 pstate_min, pstate_max, pstate_nominal;
>   u32 pstate_turbo, pstate_ultra_turbo;
> + int rc = -ENODEV;
>  
>   power_mgt = of_find_node_by_path("/ibm,opal/power-mgt");
>   if (!power_mgt) {
> @@ -327,8 +328,11 @@ static int init_powernv_pstates(void)
>   powernv_freqs[i].frequency = freq * 1000; /* kHz */
>   powernv_freqs[i].driver_data = id & 0xFF;
>  
> - revmap_data = (struct pstate_idx_revmap_data *)
> -   kmalloc(sizeof(*revmap_data), GFP_KERNEL);
> + revmap_data = kmalloc(sizeof(*revmap_data), GFP_KERNEL);
> + if (!revmap_data) {
> + rc = -ENOMEM;
> + goto out;
> + }
>  
>   revmap_data->pstate_id = id & 0xFF;
>   revmap_data->cpufreq_table_idx = i;
> @@ -357,7 +361,7 @@ static int init_powernv_pstates(void)
>   return 0;
>  out:
>   of_node_put(power_mgt);
> - return -ENODEV;
> + return rc;
>  }
>  
>  /* Returns the CPU frequency corresponding to the pstate_id. */

Applied. Thanks.

-- 
viresh


Re: [PATCH v2 28/34] dt-bindings: arm: Convert SPEAr board/soc bindings to json-schema

2018-12-03 Thread Viresh Kumar
On 03-12-18, 15:32, Rob Herring wrote:
> Convert SPEAr SoC bindings to DT schema format using json-schema.
> 
> Cc: Viresh Kumar 
> Cc: Shiraz Hashim 
> Cc: Mark Rutland 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Rob Herring 
> ---
>  .../devicetree/bindings/arm/spear.txt | 26 ---
>  .../devicetree/bindings/arm/spear.yaml| 25 ++
>  2 files changed, 25 insertions(+), 26 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/spear.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/spear.yaml

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH v2] cpufreq: pmac64: add of_node_put()

2018-11-25 Thread Viresh Kumar
On 23-11-18, 08:33, Yangtao Li wrote:
> of_find_node_by_path() acquires a reference to the node
> returned by it and that reference needs to be dropped by its caller.
> g5_neo2_cpufreq_init() doesn't do that, so fix it.
> 
> Signed-off-by: Yangtao Li 
> Acked-by: Viresh Kumar 
> ---
> Changes in v2:
> -update changelog
> ---
>  drivers/cpufreq/pmac64-cpufreq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cpufreq/pmac64-cpufreq.c 
> b/drivers/cpufreq/pmac64-cpufreq.c
> index be623dd7b9f2..1d32a863332d 100644
> --- a/drivers/cpufreq/pmac64-cpufreq.c
> +++ b/drivers/cpufreq/pmac64-cpufreq.c
> @@ -411,6 +411,7 @@ static int __init g5_neo2_cpufreq_init(struct device_node 
> *cpunode)
>   pfunc_set_vdnap0 = pmf_find_function(root, "set-vdnap0");
>   pfunc_vdnap0_complete =
>   pmf_find_function(root, "slewing-done");
> + of_node_put(root);
>   if (pfunc_set_vdnap0 == NULL ||
>   pfunc_vdnap0_complete == NULL) {
>   pr_err("Can't find required platform function\n");

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH v2] cpufreq: powernv: add of_node_put()

2018-11-20 Thread Viresh Kumar
On 20-11-18, 11:05, Yangtao Li wrote:
> The of_find_node_by_path() returns a node pointer with refcount
> incremented,but there is the lack of use of the of_node_put() when
> done.Add the missing of_node_put() to release the refcount.
> 
> Signed-off-by: Yangtao Li 
> ---
> Changes in v2
> -update changelog
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH] cpufreq: pmac64: add of_node_put()

2018-11-20 Thread Viresh Kumar
On 20-11-18, 07:54, Yangtao Li wrote:
> use of_node_put to release the refcount of node.
> 
> Signed-off-by: Yangtao Li 
> ---
>  drivers/cpufreq/pmac64-cpufreq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cpufreq/pmac64-cpufreq.c 
> b/drivers/cpufreq/pmac64-cpufreq.c
> index be623dd7b9f2..1d32a863332d 100644
> --- a/drivers/cpufreq/pmac64-cpufreq.c
> +++ b/drivers/cpufreq/pmac64-cpufreq.c
> @@ -411,6 +411,7 @@ static int __init g5_neo2_cpufreq_init(struct device_node 
> *cpunode)
>   pfunc_set_vdnap0 = pmf_find_function(root, "set-vdnap0");
>   pfunc_vdnap0_complete =
>   pmf_find_function(root, "slewing-done");
> + of_node_put(root);
>   if (pfunc_set_vdnap0 == NULL ||
>   pfunc_vdnap0_complete == NULL) {
>       pr_err("Can't find required platform function\n");

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH 1/7] drivers/cpufreq: change CONFIG_6xx to CONFIG_PPC_BOOK3S_32

2018-11-18 Thread Viresh Kumar
On 17-11-18, 10:24, Christophe Leroy wrote:
> Today, powerpc has three CONFIG labels which means exactly the same:
> - CONFIG_6xx
> - CONFIG_PPC_BOOK3S_32
> - CONFIG_PPC_STD_MMU_32
> 
> By consistency with PPC64, CONFIG_PPC_BOOK3S_32 is the preferred one.
> Using a label with includes _PPC_ also makes it clearer that it is
> linked to powerpc.
> 
> In preparation of the removal of CONFIG_6xx, this patch replaces it
> by CONFIG_PPC_BOOK3S_32
> 
> Signed-off-by: Christophe Leroy 
> ---
>  drivers/cpufreq/pmac32-cpufreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/pmac32-cpufreq.c 
> b/drivers/cpufreq/pmac32-cpufreq.c
> index 61ae06ca008e..52f0d91d30c1 100644
> --- a/drivers/cpufreq/pmac32-cpufreq.c
> +++ b/drivers/cpufreq/pmac32-cpufreq.c
> @@ -128,7 +128,7 @@ static int cpu_750fx_cpu_speed(int low_speed)
>   mtspr(SPRN_HID2, hid2);
>   }
>   }
> -#ifdef CONFIG_6xx
> +#ifdef CONFIG_PPC_BOOK3S_32
>   low_choose_750fx_pll(low_speed);
>  #endif
>   if (low_speed == 1) {
> @@ -166,7 +166,7 @@ static int dfs_set_cpu_speed(int low_speed)
>   }
>  
>   /* set frequency */
> -#ifdef CONFIG_6xx
> +#ifdef CONFIG_PPC_BOOK3S_32
>   low_choose_7447a_dfs(low_speed);
>  #endif
>   udelay(100);

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH v4 4/6] clk: qoriq: Add clockgen support for lx2160a

2018-10-04 Thread Viresh Kumar
On 04-10-18, 06:33, Vabhav Sharma wrote:
> diff --git a/drivers/cpufreq/qoriq-cpufreq.c b/drivers/cpufreq/qoriq-cpufreq.c
> index 3d773f6..83921b7 100644
> --- a/drivers/cpufreq/qoriq-cpufreq.c
> +++ b/drivers/cpufreq/qoriq-cpufreq.c
> @@ -295,6 +295,7 @@ static const struct of_device_id node_matches[] 
> __initconst = {
>   { .compatible = "fsl,ls1046a-clockgen", },
>   { .compatible = "fsl,ls1088a-clockgen", },
>   { .compatible = "fsl,ls2080a-clockgen", },
> + { .compatible = "fsl,lx2160a-clockgen", },
>   { .compatible = "fsl,p4080-clockgen", },
>   { .compatible = "fsl,qoriq-clockgen-1.0", },
>   { .compatible = "fsl,qoriq-clockgen-2.0", },

Acked-by: Viresh Kumar 

-- 
viresh


Re: [PATCH V3] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt

2018-04-25 Thread Viresh Kumar
On 25-04-18, 16:29, Shilpasri G Bhat wrote:
> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> on the requested core. This causes the below hard lockup:
> 
> [c03fe566b320] [c01d5340] smp_call_function_single+0x110/0x180 
> (unreliable)
> [c03fe566b390] [c01d55e0] smp_call_function_any+0x180/0x250
> [c03fe566b3f0] [c0acd3e8] gpstate_timer_handler+0x1e8/0x580
> [c03fe566b4a0] [c01b46b0] call_timer_fn+0x50/0x1c0
> [c03fe566b520] [c01b4958] expire_timers+0x138/0x1f0
> [c03fe566b590] [c01b4bf8] run_timer_softirq+0x1e8/0x270
> [c03fe566b630] [c0d0d6c8] __do_softirq+0x158/0x3e4
> [c03fe566b710] [c0114be8] irq_exit+0xe8/0x120
> [c03fe566b730] [c0024d0c] timer_interrupt+0x9c/0xe0
> [c03fe566b760] [c0009014] decrementer_common+0x114/0x120
> -- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> LR = arch_send_call_function_ipi_mask+0x120/0x130
> [c03fe566ba50] [c004876c]
> arch_send_call_function_ipi_mask+0x4c/0x130
> [c03fe566ba90] [c01d59f0] smp_call_function_many+0x340/0x450
> [c03fe566bb00] [c0075f18] pmdp_invalidate+0x98/0xe0
> [c03fe566bb30] [c03a1120] change_huge_pmd+0xe0/0x270
> [c03fe566bba0] [c0349278] change_protection_range+0xb88/0xe40
> [c03fe566bcf0] [c03496c0] mprotect_fixup+0x140/0x340
> [c03fe566bdb0] [c0349a74] SyS_mprotect+0x1b4/0x350
> [c03fe566be30] [c000b184] system_call+0x58/0x6c
> 
> One way to avoid this is removing the smp-call. We can ensure that the timer
> always runs on one of the policy-cpus. If the timer gets migrated to a
> cpu outside the policy then re-queue it back on the policy->cpus. This way
> we can get rid of the smp-call which was being used to set the pstate
> on the policy->cpus.
> 
> Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as 
> pinned)
> Cc: <sta...@vger.kernel.org>[4.8+]
> Reported-by: Nicholas Piggin <npig...@gmail.com>
> Reported-by: Pridhiviraj Paidipeddi <ppaid...@linux.vnet.ibm.com>
> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
> ---
> Changes from V2:
> - Remove the check for active policy while requeing the migrated timer
> Changes from V1:
> - Remove smp_call in the pstate handler.

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [PATCH V2] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt

2018-04-25 Thread Viresh Kumar
On 25-04-18, 15:32, Shilpasri G Bhat wrote:
> Hi,
> 
> On 04/25/2018 02:47 PM, Viresh Kumar wrote:
> > On 25-04-18, 14:32, Shilpasri G Bhat wrote:
> >> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> >> on the requested core. This causes the below hard lockup:
> >>
> >> [c03fe566b320] [c01d5340] smp_call_function_single+0x110/0x180 
> >> (unreliable)
> >> [c03fe566b390] [c01d55e0] smp_call_function_any+0x180/0x250
> >> [c03fe566b3f0] [c0acd3e8] gpstate_timer_handler+0x1e8/0x580
> >> [c03fe566b4a0] [c01b46b0] call_timer_fn+0x50/0x1c0
> >> [c03fe566b520] [c01b4958] expire_timers+0x138/0x1f0
> >> [c03fe566b590] [c01b4bf8] run_timer_softirq+0x1e8/0x270
> >> [c03fe566b630] [c0d0d6c8] __do_softirq+0x158/0x3e4
> >> [c03fe566b710] [c0114be8] irq_exit+0xe8/0x120
> >> [c03fe566b730] [c0024d0c] timer_interrupt+0x9c/0xe0
> >> [c03fe566b760] [c0009014] decrementer_common+0x114/0x120
> >> -- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> >> LR = arch_send_call_function_ipi_mask+0x120/0x130
> >> [c03fe566ba50] [c004876c]
> >> arch_send_call_function_ipi_mask+0x4c/0x130
> >> [c03fe566ba90] [c01d59f0] smp_call_function_many+0x340/0x450
> >> [c03fe566bb00] [c0075f18] pmdp_invalidate+0x98/0xe0
> >> [c03fe566bb30] [c03a1120] change_huge_pmd+0xe0/0x270
> >> [c03fe566bba0] [c0349278] change_protection_range+0xb88/0xe40
> >> [c03fe566bcf0] [c03496c0] mprotect_fixup+0x140/0x340
> >> [c03fe566bdb0] [c0349a74] SyS_mprotect+0x1b4/0x350
> >> [c03fe566be30] [c000b184] system_call+0x58/0x6c
> >>
> >> One way to avoid this is removing the smp-call. We can ensure that the 
> >> timer
> >> always runs on one of the policy-cpus. If the timer gets migrated to a
> >> cpu outside the policy then re-queue it back on the policy->cpus. This way
> >> we can get rid of the smp-call which was being used to set the pstate
> >> on the policy->cpus.
> >>
> >> Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer 
> >> as pinned)
> >> Cc: <sta...@vger.kernel.org>[4.8+]
> >> Reported-by: Nicholas Piggin <npig...@gmail.com>
> >> Reported-by: Pridhiviraj Paidipeddi <ppaid...@linux.vnet.ibm.com>
> >> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
> >> ---
> >> Changes from V1:
> >> - Remove smp_call in the pstate handler.
> >>
> >>  drivers/cpufreq/powernv-cpufreq.c | 23 ---
> >>  1 file changed, 20 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> >> b/drivers/cpufreq/powernv-cpufreq.c
> >> index 71f8682..dc8ffb5 100644
> >> --- a/drivers/cpufreq/powernv-cpufreq.c
> >> +++ b/drivers/cpufreq/powernv-cpufreq.c
> >> @@ -679,6 +679,25 @@ void gpstate_timer_handler(struct timer_list *t)
> >>  
> >>if (!spin_trylock(>gpstate_lock))
> >>return;
> >> +  /*
> >> +   * If the timer has migrated to the different cpu then bring
> >> +   * it back to one of the policy->cpus
> >> +   */
> >> +  if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
> >> +  /*
> >> +   * Timer should be deleted if policy is inactive.
> >> +   * If policy is active then re-queue on one of the
> >> +   * policy->cpus.
> >> +   */
> > 
> > This looks racy. Shouldn't you guarantee that the timer is already
> > removed in a synchronous way before de-activating the policy ?
> > 
> 
> The timer is deleted in driver->stop_cpu(). So we ensure to remove the timer
> before de-activating the policy.
> 
> 
> >> +  if (!cpumask_empty(policy->cpus)) {
> 
> So are you suggesting to remove ^^ the check for active policy here?
> (I put that as a safety check.)

Either you are sure or you are not, and you don't need a safety check
if you are sure :)

-- 
viresh


Re: [PATCH V2] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt

2018-04-25 Thread Viresh Kumar
On 25-04-18, 14:32, Shilpasri G Bhat wrote:
> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> on the requested core. This causes the below hard lockup:
> 
> [c03fe566b320] [c01d5340] smp_call_function_single+0x110/0x180 
> (unreliable)
> [c03fe566b390] [c01d55e0] smp_call_function_any+0x180/0x250
> [c03fe566b3f0] [c0acd3e8] gpstate_timer_handler+0x1e8/0x580
> [c03fe566b4a0] [c01b46b0] call_timer_fn+0x50/0x1c0
> [c03fe566b520] [c01b4958] expire_timers+0x138/0x1f0
> [c03fe566b590] [c01b4bf8] run_timer_softirq+0x1e8/0x270
> [c03fe566b630] [c0d0d6c8] __do_softirq+0x158/0x3e4
> [c03fe566b710] [c0114be8] irq_exit+0xe8/0x120
> [c03fe566b730] [c0024d0c] timer_interrupt+0x9c/0xe0
> [c03fe566b760] [c0009014] decrementer_common+0x114/0x120
> -- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> LR = arch_send_call_function_ipi_mask+0x120/0x130
> [c03fe566ba50] [c004876c]
> arch_send_call_function_ipi_mask+0x4c/0x130
> [c03fe566ba90] [c01d59f0] smp_call_function_many+0x340/0x450
> [c03fe566bb00] [c0075f18] pmdp_invalidate+0x98/0xe0
> [c03fe566bb30] [c03a1120] change_huge_pmd+0xe0/0x270
> [c03fe566bba0] [c0349278] change_protection_range+0xb88/0xe40
> [c03fe566bcf0] [c03496c0] mprotect_fixup+0x140/0x340
> [c03fe566bdb0] [c0349a74] SyS_mprotect+0x1b4/0x350
> [c03fe566be30] [c000b184] system_call+0x58/0x6c
> 
> One way to avoid this is removing the smp-call. We can ensure that the timer
> always runs on one of the policy-cpus. If the timer gets migrated to a
> cpu outside the policy then re-queue it back on the policy->cpus. This way
> we can get rid of the smp-call which was being used to set the pstate
> on the policy->cpus.
> 
> Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as 
> pinned)
> Cc: [4.8+]
> Reported-by: Nicholas Piggin 
> Reported-by: Pridhiviraj Paidipeddi 
> Signed-off-by: Shilpasri G Bhat 
> ---
> Changes from V1:
> - Remove smp_call in the pstate handler.
> 
>  drivers/cpufreq/powernv-cpufreq.c | 23 ---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 71f8682..dc8ffb5 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -679,6 +679,25 @@ void gpstate_timer_handler(struct timer_list *t)
>  
>   if (!spin_trylock(>gpstate_lock))
>   return;
> + /*
> +  * If the timer has migrated to the different cpu then bring
> +  * it back to one of the policy->cpus
> +  */
> + if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
> + /*
> +  * Timer should be deleted if policy is inactive.
> +  * If policy is active then re-queue on one of the
> +  * policy->cpus.
> +  */

This looks racy. Shouldn't you guarantee that the timer is already
removed in a synchronous way before de-activating the policy ?

> + if (!cpumask_empty(policy->cpus)) {
> + gpstates->timer.expires = jiffies +
> + msecs_to_jiffies(1);
> + add_timer_on(>timer,
> + cpumask_first(policy->cpus));
> + }
> + spin_unlock(>gpstate_lock);
> + return;
> + }
>  
>   /*
>* If PMCR was last updated was using fast_swtich then
> @@ -718,10 +737,8 @@ void gpstate_timer_handler(struct timer_list *t)
>   if (gpstate_idx != gpstates->last_lpstate_idx)
>   queue_gpstate_timer(gpstates);
>  
> + set_pstate(_data);
>   spin_unlock(>gpstate_lock);
> -
> - /* Timer may get migrated to a different cpu on cpu hot unplug */
> - smp_call_function_any(policy->cpus, set_pstate, _data, 1);
>  }
>  
>  /*
> -- 
> 1.8.3.1

-- 
viresh


Re: [PATCH] cpufreq: powernv: Remove global pstate ramp-down timer in POWER9

2018-04-25 Thread Viresh Kumar
On 25-04-18, 11:44, Shilpasri G Bhat wrote:
> POWER9 doesnot support global pstate requests for the chip. So remove
> the timer logic which slowly ramps down the global pstate in P9
> platforms.
> 
> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 23 ++-
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 0591874..71f8682 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -750,8 +750,13 @@ static int powernv_cpufreq_target_index(struct 
> cpufreq_policy *policy,
>  
>   cur_msec = jiffies_to_msecs(get_jiffies_64());
>  
> - spin_lock(>gpstate_lock);
>   freq_data.pstate_id = idx_to_pstate(new_index);
> + if (!gpstates) {
> + freq_data.gpstate_id = freq_data.pstate_id;
> + goto no_gpstate;
> + }
> +
> + spin_lock(>gpstate_lock);
>  
>   if (!gpstates->last_sampled_time) {
>   gpstate_idx = new_index;
> @@ -801,6 +806,7 @@ static int powernv_cpufreq_target_index(struct 
> cpufreq_policy *policy,
>  
>   spin_unlock(>gpstate_lock);
>  
> +no_gpstate:
>   /*
>* Use smp_call_function to send IPI and execute the
>* mtspr on target CPU.  We could do that without IPI
> @@ -835,6 +841,13 @@ static int powernv_cpufreq_cpu_init(struct 
> cpufreq_policy *policy)
>   kernfs_put(kn);
>   }
>  
> + policy->freq_table = powernv_freqs;
> + policy->fast_switch_possible = true;
> +
> + if (pvr_version_is(PVR_POWER9))
> + return 0;
> +
> + /* Initialise Gpstate ramp-down timer only on POWER8 */
>   gpstates =  kzalloc(sizeof(*gpstates), GFP_KERNEL);
>   if (!gpstates)
>   return -ENOMEM;
> @@ -849,15 +862,14 @@ static int powernv_cpufreq_cpu_init(struct 
> cpufreq_policy *policy)
>   msecs_to_jiffies(GPSTATE_TIMER_INTERVAL);
>   spin_lock_init(>gpstate_lock);
>  
> - policy->freq_table = powernv_freqs;
> - policy->fast_switch_possible = true;
>   return 0;
>  }
>  
>  static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>  {
>   /* timer is deleted in cpufreq_cpu_stop() */
> - kfree(policy->driver_data);
> + if (policy->driver_data)
> + kfree(policy->driver_data);
>  
>   return 0;
>  }
> @@ -990,7 +1002,8 @@ static void powernv_cpufreq_stop_cpu(struct 
> cpufreq_policy *policy)
>   freq_data.pstate_id = idx_to_pstate(powernv_pstate_info.min);
>   freq_data.gpstate_id = idx_to_pstate(powernv_pstate_info.min);
>   smp_call_function_single(policy->cpu, set_pstate, _data, 1);
> - del_timer_sync(>timer);
> + if (gpstates)
> + del_timer_sync(>timer);
>  }
>  
>  static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [PATCH 00/27] cpufreq: Stop validating cpufreq table in drivers

2018-03-09 Thread Viresh Kumar
On 26-02-18, 10:38, Viresh Kumar wrote:
> Hi,
> 
> A patchset [1] sent last week already updated the cpufreq core to start
> validating cpufreq table if the policy contains a valid
> "policy->freq_table" pointer.
> 
> This series updates all such drivers to stop validating the cpufreq
> table directly and let only the core handle it.
> 
> It depends on the previous series [1] and two cleanup patches [2][3] and
> is rebased over 4.16-rc3.
> 
> It is already tested by the 0-day testing infrastructure and no issues
> were reported.

We will assume no objections or concerns if you (cc'd here and on the individual
commits) don't respond and the series will then get merged into linux-next next
week.

Thanks.

--
viresh


[PATCH V2 15/27] cpufreq: powernv: Don't validate the frequency table twice

2018-03-04 Thread Viresh Kumar
The cpufreq core is already validating the CPU frequency table after
calling the ->init() callback of the cpufreq drivers and the drivers
don't need to do the same anymore. Though they need to set the
policy->freq_table field directly from the ->init() callback now.

Stop validating the frequency table from powernv driver.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
V1->V2:
- s/powerenv/powernv/

 drivers/cpufreq/powernv-cpufreq.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index 29cdec198657..0591874856d3 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -812,7 +812,7 @@ static int powernv_cpufreq_target_index(struct 
cpufreq_policy *policy,
 
 static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-   int base, i, ret;
+   int base, i;
struct kernfs_node *kn;
struct global_pstate_info *gpstates;
 
@@ -848,15 +848,10 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy 
*policy)
gpstates->timer.expires = jiffies +
msecs_to_jiffies(GPSTATE_TIMER_INTERVAL);
spin_lock_init(>gpstate_lock);
-   ret = cpufreq_table_validate_and_show(policy, powernv_freqs);
-
-   if (ret < 0) {
-   kfree(policy->driver_data);
-   return ret;
-   }
 
+   policy->freq_table = powernv_freqs;
policy->fast_switch_possible = true;
-   return ret;
+   return 0;
 }
 
 static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
-- 
2.15.0.194.g9af6a3dea062



Re: [PATCH 15/27] cpufreq: powerenv: Don't validate the frequency table twice

2018-02-26 Thread Viresh Kumar
On 26-02-18, 22:53, Michael Ellerman wrote:
> Viresh Kumar <viresh.ku...@linaro.org> writes:
> > Subject: Re: [PATCH 15/27] cpufreq: powerenv: Don't validate the frequency 
> > table twice
>^
>   powernv
> 
> > The cpufreq core is already validating the CPU frequency table after
> > calling the ->init() callback of the cpufreq drivers and the drivers
> > don't need to do the same anymore. Though they need to set the
> > policy->freq_table field directly from the ->init() callback now.
> >
> > Stop validating the frequency table from powerenv driver.

 powernv :)

Will fix both of them.

-- 
viresh


[PATCH 15/27] cpufreq: powerenv: Don't validate the frequency table twice

2018-02-25 Thread Viresh Kumar
The cpufreq core is already validating the CPU frequency table after
calling the ->init() callback of the cpufreq drivers and the drivers
don't need to do the same anymore. Though they need to set the
policy->freq_table field directly from the ->init() callback now.

Stop validating the frequency table from powerenv driver.

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

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index 29cdec198657..0591874856d3 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -812,7 +812,7 @@ static int powernv_cpufreq_target_index(struct 
cpufreq_policy *policy,
 
 static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-   int base, i, ret;
+   int base, i;
struct kernfs_node *kn;
struct global_pstate_info *gpstates;
 
@@ -848,15 +848,10 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy 
*policy)
gpstates->timer.expires = jiffies +
msecs_to_jiffies(GPSTATE_TIMER_INTERVAL);
spin_lock_init(>gpstate_lock);
-   ret = cpufreq_table_validate_and_show(policy, powernv_freqs);
-
-   if (ret < 0) {
-   kfree(policy->driver_data);
-   return ret;
-   }
 
+   policy->freq_table = powernv_freqs;
policy->fast_switch_possible = true;
-   return ret;
+   return 0;
 }
 
 static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
-- 
2.15.0.194.g9af6a3dea062



[PATCH 00/27] cpufreq: Stop validating cpufreq table in drivers

2018-02-25 Thread Viresh Kumar
Hi,

A patchset [1] sent last week already updated the cpufreq core to start
validating cpufreq table if the policy contains a valid
"policy->freq_table" pointer.

This series updates all such drivers to stop validating the cpufreq
table directly and let only the core handle it.

It depends on the previous series [1] and two cleanup patches [2][3] and
is rebased over 4.16-rc3.

It is already tested by the 0-day testing infrastructure and no issues
were reported.

--
viresh

[1] 
https://lkml.kernel.org/r/bd8c6133ad0bdd56c936802bcf26878d7cbdb679.1519279148.git.viresh.ku...@linaro.org
[2] 
https://lkml.kernel.org/r/77d470741dab32c2076a35253b9c0c2f0136583b.1519293292.git.viresh.ku...@linaro.org
[3] 
https://lkml.kernel.org/r/6b737a9c285840b4b2036fa51b692ee835664ec8.1519358505.git.viresh.ku...@linaro.org

Viresh Kumar (27):
  cpufreq: imx6q: Find max freq from frequency table itself
  cpufreq: Don't validate cpufreq table from cpufreq_generic_init()
  cpufreq: acpi: Don't validate the frequency table twice
  cpufreq: arm_big_little: Don't validate the frequency table twice
  cpufreq: blackfin: Don't validate the frequency table twice
  cpufreq: brcmstb: Don't validate the frequency table twice
  cpufreq: cpufreq-dt: Don't validate the frequency table twice
  cpufreq: e_powersaver: Don't validate the frequency table twice
  cpufreq: elanfreq: Don't validate the frequency table twice
  cpufreq: ia64-acpi: Don't validate the frequency table twice
  cpufreq: longhaul: Don't validate the frequency table twice
  cpufreq: mediatek: Don't validate the frequency table twice
  cpufreq: p4-clockmod: Don't validate the frequency table twice
  cpufreq: powernow: Don't validate the frequency table twice
  cpufreq: powerenv: Don't validate the frequency table twice
  cpufreq: ppc_cbe: Don't validate the frequency table twice
  cpufreq: pxa: Don't validate the frequency table twice
  cpufreq: qoirq: Don't validate the frequency table twice
  cpufreq: s3c24xx: Don't validate the frequency table twice
  cpufreq: sc520: Don't validate the frequency table twice
  cpufreq: scpi: Don't validate the frequency table twice
  cpufreq: sfi: Don't validate the frequency table twice
  cpufreq: sh: Don't validate the frequency table twice
  cpufreq: sparc: Don't validate the frequency table twice
  cpufreq: speedstep: Don't validate the frequency table twice
  cpufreq: tegra186: Don't validate the frequency table twice
  cpufreq: Drop cpufreq_table_validate_and_show()

 Documentation/cpu-freq/core.txt| 12 +---
 Documentation/cpu-freq/cpu-drivers.txt |  6 ++
 drivers/cpufreq/acpi-cpufreq.c | 20 +++-
 drivers/cpufreq/arm_big_little.c   |  9 +
 drivers/cpufreq/blackfin-cpufreq.c |  3 ++-
 drivers/cpufreq/brcmstb-avs-cpufreq.c  |  6 +-
 drivers/cpufreq/cpufreq-dt.c   |  8 +---
 drivers/cpufreq/cpufreq.c  |  9 +
 drivers/cpufreq/e_powersaver.c |  8 +---
 drivers/cpufreq/elanfreq.c |  3 ++-
 drivers/cpufreq/freq_table.c   | 14 --
 drivers/cpufreq/ia64-acpi-cpufreq.c|  7 +--
 drivers/cpufreq/imx6q-cpufreq.c|  7 ---
 drivers/cpufreq/longhaul.c |  3 ++-
 drivers/cpufreq/mediatek-cpufreq.c | 11 +--
 drivers/cpufreq/p4-clockmod.c  |  3 ++-
 drivers/cpufreq/powernow-k6.c  |  3 ++-
 drivers/cpufreq/powernow-k7.c  |  3 ++-
 drivers/cpufreq/powernow-k8.c  | 10 +-
 drivers/cpufreq/powernv-cpufreq.c  | 11 +++
 drivers/cpufreq/ppc_cbe_cpufreq.c  |  5 ++---
 drivers/cpufreq/pxa2xx-cpufreq.c   |  4 ++--
 drivers/cpufreq/pxa3xx-cpufreq.c   |  4 +++-
 drivers/cpufreq/qoriq-cpufreq.c| 13 ++---
 drivers/cpufreq/s3c24xx-cpufreq.c  |  5 +
 drivers/cpufreq/sc520_freq.c   |  3 ++-
 drivers/cpufreq/scpi-cpufreq.c | 10 +-
 drivers/cpufreq/sfi-cpufreq.c  |  3 ++-
 drivers/cpufreq/sh-cpufreq.c   | 22 --
 drivers/cpufreq/sparc-us2e-cpufreq.c   |  3 ++-
 drivers/cpufreq/sparc-us3-cpufreq.c|  3 ++-
 drivers/cpufreq/speedstep-centrino.c   |  4 ++--
 drivers/cpufreq/speedstep-ich.c|  4 +++-
 drivers/cpufreq/speedstep-smi.c|  4 +++-
 drivers/cpufreq/tegra186-cpufreq.c |  2 +-
 include/linux/cpufreq.h|  2 --
 36 files changed, 85 insertions(+), 162 deletions(-)

-- 
2.15.0.194.g9af6a3dea062



Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

2018-02-21 Thread Viresh Kumar
On 21-02-18, 11:17, Rafael J. Wysocki wrote:
> The driver is expected to call cpufreq_table_validate_and_show() at
> ->init() time and fail ->init() if that fails.
> 
> That's kind of fragile, because it depends on the driver to do the right 
> thing.

That's exactly what I am trying to explore here, i.e. Call
validate/show from core instead of drivers.

-- 
viresh


Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

2018-02-21 Thread Viresh Kumar
On 21-02-18, 10:27, Rafael J. Wysocki wrote:
> To be precise, ->init() should fail as that's where the table is
> created.  The registration fails as a result then.
> 
> But what if the bug is that ->init() doesn't fail when it should?
> 
> I guess the core could double check the frequency table after ->init()
> if ->target_index is not NULL.
> 
> The overall point here is that if you get a negative index in
> ->fast_switch(), that's way too late anyway and we should be able to
> catch that error much earlier.

I don't want to end up doing double checking as some of it is already
done at init, but let me check on what can be done.

-- 
viresh


Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

2018-02-20 Thread Viresh Kumar
On 21-02-18, 16:39, Michael Ellerman wrote:
> Viresh Kumar <viresh.ku...@linaro.org> writes:

> > AFAICT, you will get -1 here only if the freq table had no valid
> > frequencies (or the freq table is empty). Why would that happen ?
> 
> Bugs?

The cupfreq driver shouldn't have registered itself in that case (i.e.
if the cpufreq table is empty).

> Or if you ask for a target_freq that is higher than anything in the
> table.

You will still get a valid index in that case.

There is only once case where we return -1, when the cpufreq table
doesn't have any valid frequencies.

> Or the API changes, and we forget to update this call site.

I am not sure we can do much about that right now.

> If you're saying that cpufreq_table_find_index_dl() can NEVER fail,

Yes, if we have at least one valid frequency in the table, otherwise
the cpufreq driver shouldn't have registered itself.

> then
> write it so that it can never fail and change it to return unsigned int.

But what should we do when there is no frequency in the cpufreq table?
Just in case where a driver is buggy and tries to call this routine
for an invalid table.

> Having it potentially return -1, which is then used to index an array
> and not handling that is just asking for bugs to happen.

I understand what you are trying to say here, but I don't know what
can be done to prevent this here.

What we can do is change the return type to void and pass a int
pointer to the routine, but that wouldn't change anything at all. That
pointers variable can still have -1 in it.

-- 
viresh


Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

2018-02-12 Thread Viresh Kumar
On 12-02-18, 16:03, Shilpasri G Bhat wrote:
> I agree too. There is no way we can get -1 with initialized cpu frequency 
> table.
> We don't initialize powernv-cpufreq if we don't have valid CPU frequency
> entries. Is there any other way to suppress the Coverity tool warning apart 
> from
> ignoring it?

So IIUC, this warning is generated by an external tool after static
analysis of the code ?

If yes, then just ignore the warning. We shouldn't try fixing the
kernel because a tool isn't smart enough to catch intentional
ignorance of the return value here.

-- 
viresh


Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

2018-02-12 Thread Viresh Kumar
On 12-02-18, 15:51, Shilpasri G Bhat wrote:
> This patch fixes the below Coverity warning:
> 
> *** CID 182816:  Memory - illegal accesses  (NEGATIVE_RETURNS)
> /drivers/cpufreq/powernv-cpufreq.c: 1008 in powernv_fast_switch()
> 1002  unsigned int target_freq)
> 1003 {
> 1004  int index;
> 1005  struct powernv_smp_call_data freq_data;
> 1006
> 1007  index = cpufreq_table_find_index_dl(policy, target_freq);
> >>> CID 182816:  Memory - illegal accesses  (NEGATIVE_RETURNS)
> >>> Using variable "index" as an index to array "powernv_freqs".
> 1008  freq_data.pstate_id = powernv_freqs[index].driver_data;
> 1009  freq_data.gpstate_id = powernv_freqs[index].driver_data;
> 1010  set_pstate(_data);
> 1011
> 1012  return powernv_freqs[index].frequency;
> 1013 }
> 
> Signed-off-by: Shilpasri G Bhat 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index 29cdec1..69edfe9 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct 
> cpufreq_policy *policy,
>   struct powernv_smp_call_data freq_data;
>  
>   index = cpufreq_table_find_index_dl(policy, target_freq);
> + if (unlikely(index < 0))
> + index = get_nominal_index();
> +

AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?

>   freq_data.pstate_id = powernv_freqs[index].driver_data;
>   freq_data.gpstate_id = powernv_freqs[index].driver_data;
>   set_pstate(_data);
> -- 
> 1.8.3.1

-- 
viresh


Re: [PATCH] cpufreq: powernv: Dont assume distinct pstate values for nominal and pmin

2018-01-12 Thread Viresh Kumar
On 12-01-18, 12:43, Shilpasri G Bhat wrote:
> Some OpenPOWER boxes can have same pstate values for nominal and
> pmin pstates. In these boxes the current code will not initialize
> 'powernv_pstate_info.min' variable and result in erroneous CPU
> frequency reporting. This patch fixes this problem.
> 
> Fixes: 09ca4c9b5958 ("cpufreq: powernv: Replacing pstate_id with frequency 
> table index")
> Reported-by: Alvin Wang <wan...@tw.ibm.com>
> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index b6d7c4c..da7fdb4 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -288,9 +288,9 @@ static int init_powernv_pstates(void)
>  
>   if (id == pstate_max)
>   powernv_pstate_info.max = i;
> - else if (id == pstate_nominal)
> + if (id == pstate_nominal)
>   powernv_pstate_info.nominal = i;
> - else if (id == pstate_min)
> + if (id == pstate_min)
>   powernv_pstate_info.min = i;
>  
>   if (powernv_pstate_info.wof_enabled && id == pstate_turbo) {

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [v3 PATCH 0/3] powernv-cpufreq: Multiple pstate related fixes.

2018-01-10 Thread Viresh Kumar
On 13-12-17, 12:27, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <e...@linux.vnet.ibm.com>
> 
>  This is a third version of the patch to fix pstate related issues in
>  the powernv-cpufreq driver.

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [PATCH v2] cpufreq: powernv: Add support of frequency domain

2017-12-19 Thread Viresh Kumar
On 20-12-17, 12:12, Abhishek Goel wrote:
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index b6d7c4c..fd642bc 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -37,6 +37,7 @@
>  #include  /* Required for cpu_sibling_mask() in UP configs */
>  #include 
>  #include 
> +#include 
>  
>  #define POWERNV_MAX_PSTATES  256
>  #define PMSR_PSAFE_ENABLE(1UL << 30)
> @@ -130,6 +131,9 @@ static struct chip {
>  static int nr_chips;
>  static DEFINE_PER_CPU(struct chip *, chip_info);
>  
> +static u32 freq_domain_indicator;
> +static u32 flag;

I wouldn't name it as flag, its unreadable. Maybe its better to name
it based on the quirk you are trying to workaround with ?

> +
>  /*
>   * Note:
>   * The set of pstates consists of contiguous integers.
> @@ -194,6 +198,38 @@ static inline void reset_gpstates(struct cpufreq_policy 
> *policy)
>   gpstates->last_gpstate_idx = 0;
>  }
>  
> +#define SIZE NR_CPUS
> +#define ORDER_FREQ_MAP ilog2(SIZE)
> +
> +static DEFINE_HASHTABLE(freq_domain_map, ORDER_FREQ_MAP);
> +
> +struct hashmap {
> + cpumask_t mask;
> + int chip_id;
> + u32 pir_key;
> + struct hlist_node hash_node;
> +};
> +
> +static void insert(u32 key, int cpu)
> +{
> + struct hashmap *data;
> +
> + hash_for_each_possible(freq_domain_map, data, hash_node, key%SIZE) {
> + if (data->chip_id == cpu_to_chip_id(cpu) &&
> + data->pir_key == key) {
> + cpumask_set_cpu(cpu, >mask);
> + return;
> + }
> + }
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + hash_add(freq_domain_map, >hash_node, key%SIZE);
> + cpumask_set_cpu(cpu, >mask);
> + data->chip_id = cpu_to_chip_id(cpu);
> + data->pir_key = key;
> +
> +}
> +
>  /*
>   * Initialize the freq table based on data obtained
>   * from the firmware passed via device-tree
> @@ -206,7 +242,9 @@ static int init_powernv_pstates(void)
>   u32 len_ids, len_freqs;
>   u32 pstate_min, pstate_max, pstate_nominal;
>   u32 pstate_turbo, pstate_ultra_turbo;
> + u32 key;
>  
> + flag = 0;

Isn't flag already 0 (global-uninitialized) ?

>   power_mgt = of_find_node_by_path("/ibm,opal/power-mgt");
>   if (!power_mgt) {
>   pr_warn("power-mgt node not found\n");
> @@ -229,6 +267,17 @@ static int init_powernv_pstates(void)
>   return -ENODEV;
>   }
>  
> + if (of_device_is_compatible(power_mgt, "freq-domain-v1") &&
> + of_property_read_u32(power_mgt, "ibm,freq-domain-indicator",
> +  _domain_indicator)) {
> + pr_warn("ibm,freq-domain-indicator not found\n");
> + freq_domain_indicator = 0;

You shouldn't be required to set it to 0 here.

> + }
> +
> + if (of_device_is_compatible(power_mgt, "P9-occ-quirk")) {
> + flag = 1;
> + }

Remove {} and a better name like p9_occ_quirk would be good for flag.
Also making it a bool may be better ?

> +
>   if (of_property_read_u32(power_mgt, "ibm,pstate-ultra-turbo",
>_ultra_turbo)) {
>   powernv_pstate_info.wof_enabled = false;
> @@ -249,6 +298,7 @@ static int init_powernv_pstates(void)
>  next:
>   pr_info("cpufreq pstate min %d nominal %d max %d\n", pstate_min,
>   pstate_nominal, pstate_max);
> + pr_info("frequency domain indicator %d", freq_domain_indicator);
>   pr_info("Workload Optimized Frequency is %s in the platform\n",
>   (powernv_pstate_info.wof_enabled) ? "enabled" : "disabled");
>  
> @@ -276,6 +326,15 @@ static int init_powernv_pstates(void)
>   return -ENODEV;
>   }
>  
> + if (freq_domain_indicator) {
> + hash_init(freq_domain_map);
> + for_each_possible_cpu(i) {
> + key = ((u32) get_hard_smp_processor_id(i) &
> + freq_domain_indicator);

Maybe break it like:

key = (u32) get_hard_smp_processor_id(i);
key &= freq_domain_indicator;

to make it easily readable ?

> + insert(key, i);
> + }
> + }
> +
>   powernv_pstate_info.nr_pstates = nr_pstates;
>   pr_debug("NR PStates %d\n", nr_pstates);
>   for (i = 0; i < nr_pstates; i++) {
> @@ -693,6 +752,7 @@ static int powernv_cpufreq_target_index(struct 
> cpufreq_policy *policy,
>  {
>   struct powernv_smp_call_data freq_data;
>   unsigned int cur_msec, gpstate_idx;
> +

:(

>   struct global_pstate_info *gpstates = policy->driver_data;
>  
>   if (unlikely(rebooting) && new_index != get_nominal_index())
> @@ -760,25 +820,55 @@ static int powernv_cpufreq_target_index(struct 
> cpufreq_policy *policy,
>  
>   spin_unlock(>gpstate_lock);
>  
> - /*
> -  * Use smp_call_function to send IPI and execute the
> -  * mtspr on 

Re: [PATCH] cpufreq: powernv: Add support of frequency domain

2017-12-18 Thread Viresh Kumar
On 18-12-17, 10:41, Abhishek wrote:
> We need to do it in this way as the current implementation takes the max of
> the PMSR of the cores. Thus, when the frequency is required to be ramped up,
> it suffices to write to just the local PMSR, but when the frequency is to be
> ramped down, if we don't send the IPI it breaks the compatibility with P8.

Looks strange really that you have to program this differently for speeding up
or down. These CPUs are part of one cpufreq policy and so I would normally
expect changes to any CPU should reflect for other CPUs as well.

@Goutham: Do you know why it is so ?

-- 
viresh


Re: [PATCH] cpufreq: powernv: Add support of frequency domain

2017-12-13 Thread Viresh Kumar
+ Gautham,

@Gautham: Can you please help reviewing this one ?

On 13-12-17, 13:49, Abhishek Goel wrote:
> @@ -693,6 +746,8 @@ static int powernv_cpufreq_target_index(struct 
> cpufreq_policy *policy,
>  {
>   struct powernv_smp_call_data freq_data;
>   unsigned int cur_msec, gpstate_idx;
> + cpumask_t temp;
> + u32 cpu;
>   struct global_pstate_info *gpstates = policy->driver_data;
>  
>   if (unlikely(rebooting) && new_index != get_nominal_index())
> @@ -761,24 +816,48 @@ static int powernv_cpufreq_target_index(struct 
> cpufreq_policy *policy,
>   spin_unlock(>gpstate_lock);
>  
>   /*
> -  * Use smp_call_function to send IPI and execute the
> -  * mtspr on target CPU.  We could do that without IPI
> -  * if current CPU is within policy->cpus (core)
> +  * Use smp_call_function to send IPI and execute the mtspr on CPU.
> +  * This needs to be done on every core of the policy

Why on each CPU ?

>*/
> - smp_call_function_any(policy->cpus, set_pstate, _data, 1);
> + cpumask_copy(, policy->cpus);
> +
> + while (!cpumask_empty()) {
> + cpu = cpumask_first();
> + smp_call_function_any(cpu_sibling_mask(cpu),
> + set_pstate, _data, 1);
> + cpumask_andnot(, , cpu_sibling_mask(cpu));
> + }
> +
>   return 0;
>  }

-- 
viresh


Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

2017-12-04 Thread Viresh Kumar
On 30-11-17, 10:13, Shilpasri G Bhat wrote:
> From: "Gautham R. Shenoy" <e...@linux.vnet.ibm.com>
> 
> Pstates are 8bit values but on POWER8 they are negative and on POWER9
> they are positive. This patch adds helper routines to differentiate
> the sign to read the correct pstate value.
> 
> Signed-off-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
> Tested-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 43 
> ++-
>  1 file changed, 33 insertions(+), 10 deletions(-)

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


[PATCH V3 8/9] cpufreq: Add CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING cpufreq driver flag

2017-07-19 Thread Viresh Kumar
The policy->transition_latency field is used for multiple purposes
today and its not straight forward at all. This is how it is used:

A. Set the correct transition_latency value.

B. Set it to CPUFREQ_ETERNAL because:
   1. We don't want automatic dynamic switching (with
  ondemand/conservative) to happen at all.
   2. We don't know the transition latency.

This patch handles the B.1. case in a more readable way. A new flag for
the cpufreq drivers is added to disallow use of cpufreq governors which
have dynamic_switching flag set.

All the current cpufreq drivers which are setting transition_latency
unconditionally to CPUFREQ_ETERNAL are updated to use it. They don't
need to set transition_latency anymore.

There shouldn't be any functional change after this patch.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 drivers/cpufreq/cpufreq-nforce2.c  | 2 +-
 drivers/cpufreq/cpufreq.c  | 5 +++--
 drivers/cpufreq/elanfreq.c | 4 +---
 drivers/cpufreq/gx-suspmod.c   | 2 +-
 drivers/cpufreq/pmac32-cpufreq.c   | 7 +--
 drivers/cpufreq/sa1100-cpufreq.c   | 5 +++--
 drivers/cpufreq/sa1110-cpufreq.c   | 5 +++--
 drivers/cpufreq/sh-cpufreq.c   | 3 +--
 drivers/cpufreq/speedstep-smi.c| 2 +-
 drivers/cpufreq/unicore2-cpufreq.c | 3 +--
 include/linux/cpufreq.h| 6 ++
 11 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-nforce2.c 
b/drivers/cpufreq/cpufreq-nforce2.c
index 5503d491b016..dbf82f36d270 100644
--- a/drivers/cpufreq/cpufreq-nforce2.c
+++ b/drivers/cpufreq/cpufreq-nforce2.c
@@ -357,7 +357,6 @@ static int nforce2_cpu_init(struct cpufreq_policy *policy)
/* cpuinfo and default policy values */
policy->min = policy->cpuinfo.min_freq = min_fsb * fid * 100;
policy->max = policy->cpuinfo.max_freq = max_fsb * fid * 100;
-   policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 
return 0;
 }
@@ -369,6 +368,7 @@ static int nforce2_cpu_exit(struct cpufreq_policy *policy)
 
 static struct cpufreq_driver nforce2_driver = {
.name = "nforce2",
+   .flags = CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING,
.verify = nforce2_verify,
.target = nforce2_target,
.get = nforce2_get,
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2debfb5f9126..a4d9b47c4af4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2016,11 +2016,12 @@ static int cpufreq_init_governor(struct cpufreq_policy 
*policy)
 
/* Platform doesn't want dynamic frequency switching ? */
if (policy->governor->dynamic_switching &&
-   policy->cpuinfo.transition_latency == CPUFREQ_ETERNAL) {
+   (cpufreq_driver->flags & CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING ||
+   policy->cpuinfo.transition_latency == CPUFREQ_ETERNAL)) {
struct cpufreq_governor *gov = cpufreq_fallback_governor();
 
if (gov) {
-   pr_warn("Transition latency set to CPUFREQ_ETERNAL, 
can't use %s governor. Fallback to %s governor\n",
+   pr_warn("Can't use %s governor as dynamic switching is 
disallowed. Fallback to %s governor\n",
policy->governor->name, gov->name);
policy->governor = gov;
} else {
diff --git a/drivers/cpufreq/elanfreq.c b/drivers/cpufreq/elanfreq.c
index bfce11cba1df..45e2ca62515e 100644
--- a/drivers/cpufreq/elanfreq.c
+++ b/drivers/cpufreq/elanfreq.c
@@ -165,9 +165,6 @@ static int elanfreq_cpu_init(struct cpufreq_policy *policy)
if (pos->frequency > max_freq)
pos->frequency = CPUFREQ_ENTRY_INVALID;
 
-   /* cpuinfo and default policy values */
-   policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
-
return cpufreq_table_validate_and_show(policy, elanfreq_table);
 }
 
@@ -196,6 +193,7 @@ __setup("elanfreq=", elanfreq_setup);
 
 static struct cpufreq_driver elanfreq_driver = {
.get= elanfreq_get_cpu_frequency,
+   .flags  = CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING,
.verify = cpufreq_generic_frequency_table_verify,
.target_index   = elanfreq_target,
.init   = elanfreq_cpu_init,
diff --git a/drivers/cpufreq/gx-suspmod.c b/drivers/cpufreq/gx-suspmod.c
index 3488c9c175eb..8f52a06664e3 100644
--- a/drivers/cpufreq/gx-suspmod.c
+++ b/drivers/cpufreq/gx-suspmod.c
@@ -428,7 +428,6 @@ static int cpufreq_gx_cpu_init(struct cpufreq_policy 
*policy)
policy->max = maxfreq;
policy->cpuinfo.min_freq = maxfreq / max_duration;
policy->cpuinfo.max_freq = maxfreq;
-   policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 
return 0;
 }
@@ -438,6 +437,7 @@ static int cpufreq_gx_cpu_init(struct cpufreq_policy 
*p

[PATCH V3 0/9] cpufreq: transition-latency cleanups

2017-07-19 Thread Viresh Kumar
Hi Rafael,

This series tries to cleanup the code around transition-latency and its
users. Some of the old legacy code, which may not make much sense now,
is dropped as well. And some code consolidation is also done across
governors.

Based of: v4.13-rc1
Tested on: ARM64 Hikey board.

I have pushed it here as well (which gets tested by kbuild test bot):

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git 
cpufreq/transition-latency

V2->V3:
- Rearranged patches to keep related stuff together
- Introduce CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING flag (Rafael)
- Minor optimization in cpufreq_policy_transition_delay_us() and moved
  it to cpufreq.c (Rafael)
- Allow dynamic switching for drivers which don't know their transition
  latency.

V1->V2:
- While we still get rid of the limitation of 10ms for using
  ondemand/conservative, but we preserve the earlier behavior where the
  transition latency set to CPUFREQ_ETERNAL would not allow use of
  ondemand/conservative governors. Thanks to Dominik for his feedback on
  that.

--
viresh

Viresh Kumar (9):
  cpufreq: governor: Drop min_sampling_rate
  cpufreq: Use transition_delay_us for legacy governors as well
  cpufreq: Cap the default transition delay value to 10 ms
  cpufreq: Don't set transition_latency for setpolicy drivers
  cpufreq: arm_big_little: Make ->get_transition_latency() mandatory
  cpufreq: Replace "max_transition_latency" with "dynamic_switching"
  cpufreq: schedutil: Set dynamic_switching to true
  cpufreq: Add CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING cpufreq driver flag
  cpufreq: Allow dynamic switching with CPUFREQ_ETERNAL latency

 Documentation/admin-guide/pm/cpufreq.rst |  8 
 drivers/cpufreq/arm_big_little.c | 10 --
 drivers/cpufreq/cpufreq-nforce2.c|  2 +-
 drivers/cpufreq/cpufreq.c| 34 
 drivers/cpufreq/cpufreq_conservative.c   |  6 --
 drivers/cpufreq/cpufreq_governor.c   | 17 ++--
 drivers/cpufreq/cpufreq_governor.h   |  3 +--
 drivers/cpufreq/cpufreq_ondemand.c   | 12 ---
 drivers/cpufreq/elanfreq.c   |  4 +---
 drivers/cpufreq/gx-suspmod.c |  2 +-
 drivers/cpufreq/intel_pstate.c   |  1 -
 drivers/cpufreq/longrun.c|  1 -
 drivers/cpufreq/pmac32-cpufreq.c |  7 +--
 drivers/cpufreq/sa1100-cpufreq.c |  5 +++--
 drivers/cpufreq/sa1110-cpufreq.c |  5 +++--
 drivers/cpufreq/sh-cpufreq.c |  3 +--
 drivers/cpufreq/speedstep-smi.c  |  2 +-
 drivers/cpufreq/unicore2-cpufreq.c   |  3 +--
 include/linux/cpufreq.h  | 18 -
 kernel/sched/cpufreq_schedutil.c | 12 ++-
 20 files changed, 65 insertions(+), 90 deletions(-)

-- 
2.13.0.71.gd7076ec9c9cb



Re: [PATCH] cpufreq: Convert to using %pOF instead of full_name

2017-07-19 Thread Viresh Kumar
On 18-07-17, 16:42, Rob Herring wrote:
> Now that we have a custom printf format specifier, convert users of
> full_name to use %pOF instead. This is preparation to remove storing
> of the full path string for each node.
> 
> Signed-off-by: Rob Herring <r...@kernel.org>
> Cc: "Rafael J. Wysocki" <r...@rjwysocki.net>
> Cc: Viresh Kumar <viresh.ku...@linaro.org>
> Cc: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> Cc: Paul Mackerras <pau...@samba.org>
> Cc: Michael Ellerman <m...@ellerman.id.au>
> Cc: Patrice Chotard <patrice.chot...@st.com>
> Cc: linux...@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: ker...@stlinux.com
> ---
>  drivers/cpufreq/pmac64-cpufreq.c | 2 +-
>  drivers/cpufreq/sti-cpufreq.c| 8 
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/pmac64-cpufreq.c 
> b/drivers/cpufreq/pmac64-cpufreq.c
> index 267e0894c62d..be623dd7b9f2 100644
> --- a/drivers/cpufreq/pmac64-cpufreq.c
> +++ b/drivers/cpufreq/pmac64-cpufreq.c
> @@ -516,7 +516,7 @@ static int __init g5_pm72_cpufreq_init(struct device_node 
> *cpunode)
>   goto bail;
>   }
> 
> - DBG("cpufreq: i2c clock chip found: %s\n", hwclock->full_name);
> + DBG("cpufreq: i2c clock chip found: %pOF\n", hwclock);
> 
>   /* Now get all the platform functions */
>   pfunc_cpu_getfreq =
> diff --git a/drivers/cpufreq/sti-cpufreq.c b/drivers/cpufreq/sti-cpufreq.c
> index d2d0430d09d4..47105735df12 100644
> --- a/drivers/cpufreq/sti-cpufreq.c
> +++ b/drivers/cpufreq/sti-cpufreq.c
> @@ -65,8 +65,8 @@ static int sti_cpufreq_fetch_major(void) {
>   ret = of_property_read_u32_index(np, "st,syscfg",
>MAJOR_ID_INDEX, _offset);
>   if (ret) {
> - dev_err(dev, "No major number offset provided in %s [%d]\n",
> - np->full_name, ret);
> + dev_err(dev, "No major number offset provided in %pOF [%d]\n",
> + np, ret);
>   return ret;
>   }
> 
> @@ -92,8 +92,8 @@ static int sti_cpufreq_fetch_minor(void)
>MINOR_ID_INDEX, _offset);
>   if (ret) {
>   dev_err(dev,
> - "No minor number offset provided %s [%d]\n",
> - np->full_name, ret);
> + "No minor number offset provided %pOF [%d]\n",
> + np, ret);
>   return ret;
>   }

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [patch 09/18] cpufreq/pasemi: Adjust system_state check

2017-05-14 Thread Viresh Kumar
On 14-05-17, 20:27, Thomas Gleixner wrote:
> To enable smp_processor_id() and might_sleep() debug checks earlier, it's
> required to add system states between SYSTEM_BOOTING and SYSTEM_RUNNING.
> 
> Adjust the system_state check in pas_cpufreq_cpu_exit() to handle the extra
> states.
> 
> Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> Cc: "Rafael J. Wysocki" <r...@rjwysocki.net>
> Cc: Viresh Kumar <viresh.ku...@linaro.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
>  drivers/cpufreq/pasemi-cpufreq.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -226,7 +226,7 @@ static int pas_cpufreq_cpu_exit(struct c
>* We don't support CPU hotplug. Don't unmap after the system
>* has already made it to a running state.
>*/
> - if (system_state != SYSTEM_BOOTING)
> + if (system_state >= SYSTEM_RUNNING)
>   return 0;
>  
>   if (sdcasr_mapbase)

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


[PATCH] cpufreq: Remove CONFIG_CPU_FREQ_STAT_DETAILS config option

2017-01-05 Thread Viresh Kumar
This doesn't have any benefit apart from saving a small amount of memory
when it is disabled. The ifdef hackery in the code makes it dirty
unnecessarily.

Clean it up by removing the Kconfig option completely. Few defconfigs
are also updated and CONFIG_CPU_FREQ_STAT_DETAILS is replaced with
CONFIG_CPU_FREQ_STAT now in them, as users wanted stats to be enabled.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 arch/arm/configs/exynos_defconfig |  2 +-
 arch/arm/configs/multi_v5_defconfig   |  2 +-
 arch/arm/configs/multi_v7_defconfig   |  2 +-
 arch/arm/configs/mvebu_v5_defconfig   |  2 +-
 arch/arm/configs/pxa_defconfig|  2 +-
 arch/arm/configs/shmobile_defconfig   |  2 +-
 arch/mips/configs/lemote2f_defconfig  |  1 -
 arch/powerpc/configs/ppc6xx_defconfig |  1 -
 arch/sh/configs/sh7785lcr_32bit_defconfig |  2 +-
 drivers/cpufreq/Kconfig   |  8 
 drivers/cpufreq/cpufreq_stats.c   | 14 --
 11 files changed, 7 insertions(+), 31 deletions(-)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 79c415c33f69..809f0bf3042a 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -24,7 +24,7 @@ CONFIG_ARM_APPENDED_DTB=y
 CONFIG_ARM_ATAG_DTB_COMPAT=y
 CONFIG_CMDLINE="root=/dev/ram0 rw ramdisk=8192 initrd=0x4100,8M 
console=ttySAC1,115200 init=/linuxrc mem=256M"
 CONFIG_CPU_FREQ=y
-CONFIG_CPU_FREQ_STAT_DETAILS=y
+CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=m
 CONFIG_CPU_FREQ_GOV_USERSPACE=m
diff --git a/arch/arm/configs/multi_v5_defconfig 
b/arch/arm/configs/multi_v5_defconfig
index 361686a362f1..69a4bd13eea5 100644
--- a/arch/arm/configs/multi_v5_defconfig
+++ b/arch/arm/configs/multi_v5_defconfig
@@ -58,7 +58,7 @@ CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_ARM_APPENDED_DTB=y
 CONFIG_ARM_ATAG_DTB_COMPAT=y
 CONFIG_CPU_FREQ=y
-CONFIG_CPU_FREQ_STAT_DETAILS=y
+CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
 CONFIG_CPU_IDLE=y
 CONFIG_ARM_KIRKWOOD_CPUIDLE=y
diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index b01a43851294..2dcac90eba01 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -132,7 +132,7 @@ CONFIG_ARM_ATAG_DTB_COMPAT=y
 CONFIG_KEXEC=y
 CONFIG_EFI=y
 CONFIG_CPU_FREQ=y
-CONFIG_CPU_FREQ_STAT_DETAILS=y
+CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=m
 CONFIG_CPU_FREQ_GOV_USERSPACE=m
diff --git a/arch/arm/configs/mvebu_v5_defconfig 
b/arch/arm/configs/mvebu_v5_defconfig
index f7f6039419aa..4b598da0d086 100644
--- a/arch/arm/configs/mvebu_v5_defconfig
+++ b/arch/arm/configs/mvebu_v5_defconfig
@@ -44,7 +44,7 @@ CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_ARM_APPENDED_DTB=y
 CONFIG_ARM_ATAG_DTB_COMPAT=y
 CONFIG_CPU_FREQ=y
-CONFIG_CPU_FREQ_STAT_DETAILS=y
+CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
 CONFIG_CPU_IDLE=y
 CONFIG_ARM_KIRKWOOD_CPUIDLE=y
diff --git a/arch/arm/configs/pxa_defconfig b/arch/arm/configs/pxa_defconfig
index e4314b1227a3..271dc7e78e43 100644
--- a/arch/arm/configs/pxa_defconfig
+++ b/arch/arm/configs/pxa_defconfig
@@ -97,7 +97,7 @@ CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_CMDLINE="root=/dev/ram0 ro"
 CONFIG_KEXEC=y
 CONFIG_CPU_FREQ=y
-CONFIG_CPU_FREQ_STAT_DETAILS=y
+CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=m
 CONFIG_CPU_FREQ_GOV_USERSPACE=m
diff --git a/arch/arm/configs/shmobile_defconfig 
b/arch/arm/configs/shmobile_defconfig
index 1b0f8ae36fb3..adeaecd831a4 100644
--- a/arch/arm/configs/shmobile_defconfig
+++ b/arch/arm/configs/shmobile_defconfig
@@ -38,7 +38,7 @@ CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_ARM_APPENDED_DTB=y
 CONFIG_KEXEC=y
 CONFIG_CPU_FREQ=y
-CONFIG_CPU_FREQ_STAT_DETAILS=y
+CONFIG_CPU_FREQ_STAT=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=y
 CONFIG_CPU_FREQ_GOV_USERSPACE=y
 CONFIG_CPU_FREQ_GOV_ONDEMAND=y
diff --git a/arch/mips/configs/lemote2f_defconfig 
b/arch/mips/configs/lemote2f_defconfig
index 5da76e0e120f..bed745596d86 100644
--- a/arch/mips/configs/lemote2f_defconfig
+++ b/arch/mips/configs/lemote2f_defconfig
@@ -40,7 +40,6 @@ CONFIG_PM_STD_PARTITION="/dev/hda3"
 CONFIG_CPU_FREQ=y
 CONFIG_CPU_FREQ_DEBUG=y
 CONFIG_CPU_FREQ_STAT=m
-CONFIG_CPU_FREQ_STAT_DETAILS=y
 CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=m
 CONFIG_CPU_FREQ_GOV_USERSPACE=m
diff --git a/arch/powerpc/configs/ppc6xx_defconfig 
b/arch/powerpc/configs/ppc6xx_defconfig
index 3ce91a3df27f..1d2d69dd6409 100644
--- a/arch/powerpc/configs/ppc6xx_defconfig
+++ b/arch/powerpc/configs/ppc6xx_defconfig
@@ -62,7 +62,6 @@ CONFIG_MPC8610_HPCD=y
 CONFIG_GEF_SBC610=y
 CONFIG_CPU_FREQ=y
 CONFIG_CPU_FREQ_STAT=m
-CONFIG_CPU_FREQ_STAT_DETAILS=y
 CONFIG_CPU_FREQ_DEFAULT_GOV_USERSPACE=y
 CONFIG_CPU_FREQ_GOV_PERFORMANCE=y
 CONFIG_CPU_FREQ_GOV_POWERSAVE=m
diff --git a/arch/sh/configs/

Re: [PATCH V2] cpufreq: powernv: Add boost files to export ultra-turbo frequencies

2017-01-03 Thread Viresh Kumar
On 03-01-17, 16:36, Shilpasri G Bhat wrote:
> In P8+, Workload Optimized Frequency(WOF) provides the capability to
> boost the cpu frequency based on the utilization of the other cpus
> running in the chip. The On-Chip-Controller(OCC) firmware will control
> the achievability of these frequencies depending on the power headroom
> available in the chip. Currently the ultra-turbo frequencies provided
> by this feature are exported along with the turbo and sub-turbo
> frequencies as scaling_available_frequencies. This patch will export
> the ultra-turbo frequencies separately as scaling_boost_frequencies in
> WOF enabled systems. This patch will add the boost sysfs file which
> can be used to disable/enable ultra-turbo frequencies.
> 
> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
> Reviewed-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
> ---
> Changes from v1:
> - Print if WOF is enabled
> - s/clean_notifiers/cleanup_notifiers
> 
>  drivers/cpufreq/powernv-cpufreq.c | 50 
> ++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [PATCH] cpufreq, powernv: Disable preemption while checking CPU throttling state throttling state

2016-11-08 Thread Viresh Kumar
+some more folks from IBM.

On 08-11-16, 03:35, Denis Kirjanov wrote:
> [   67.700897] BUG: using smp_processor_id() in preemptible [] 
> code: cat/7343
> [   67.700988] caller is .powernv_cpufreq_throttle_check+0x2c/0x710
> [   67.700998] CPU: 13 PID: 7343 Comm: cat Not tainted 4.8.0-rc5-dirty #1
> [   67.701038] Call Trace:
> [   67.701066] [c007d25b75b0] [c0971378] 
> .dump_stack+0xe4/0x150 (unreliable)
> [   67.701153] [c007d25b7640] [c05162e4] 
> .check_preemption_disabled+0x134/0x150
> [   67.701238] [c007d25b76e0] [c07b63ac] 
> .powernv_cpufreq_throttle_check+0x2c/0x710
> [   67.701322] [c007d25b7790] [c07b6d18] 
> .powernv_cpufreq_target_index+0x288/0x360
> [   67.701407] [c007d25b7870] [c07acee4] 
> .__cpufreq_driver_target+0x394/0x8c0
> [   67.701491] [c007d25b7920] [c07b22ac] 
> .cpufreq_set+0x7c/0xd0
> [   67.701565] [c007d25b79b0] [c07adf50] 
> .store_scaling_setspeed+0x80/0xc0
> [   67.701650] [c007d25b7a40] [c07ae270] .store+0xa0/0x100
> [   67.701723] [c007d25b7ae0] [c03566e8] 
> .sysfs_kf_write+0x88/0xb0
> [   67.701796] [c007d25b7b70] [c03553b8] 
> .kernfs_fop_write+0x178/0x260
> [   67.701881] [c007d25b7c10] [c02ac3cc] 
> .__vfs_write+0x3c/0x1c0
> [   67.701954] [c007d25b7cf0] [c02ad584] .vfs_write+0xc4/0x230
> [   67.702027] [c007d25b7d90] [c02aeef8] .SyS_write+0x58/0x100
> [   67.702101] [c007d25b7e30] [c000bfec] system_call+0x38/0xfc
> 

We expect some description of the problem here, not just the OPPs
dump.

> Fixes: 09a972d16209 ("cpufreq: powernv: Report cpu frequency throttling")
> Reviewed-by: Gautham R. Shenoy 
> Signed-off-by: Denis Kirjanov 
> ---
> 
> drivers/cpufreq/powernv-cpufreq.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index d3ffde8..a84724e 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -647,8 +647,14 @@ static int powernv_cpufreq_target_index(struct 
> cpufreq_policy *policy,
>   if (unlikely(rebooting) && new_index != get_nominal_index())
>   return 0;
>  
> - if (!throttled)
> + if (!throttled) {
> + /* we don't want to be preempted while
> +  * checking if the CPU frequency has been throttled
> +  */
> + preempt_disable();
>   powernv_cpufreq_throttle_check(NULL);
> + preempt_enable();
> + }
>  
>   cur_msec = jiffies_to_msecs(get_jiffies_64());
>  
> -- 
> 1.8.3.1

-- 
viresh


Re: [PATCH v2 1/2] cpufreq: powernv: Adding fast_switch for schedutil

2016-11-07 Thread Viresh Kumar
On 07-11-16, 13:09, Akshay Adiga wrote:
> Adding fast_switch which does light weight operation to set the desired
> pstate. Both global and local pstates are set to the same desired pstate.
> 
> Signed-off-by: Akshay Adiga <akshay.ad...@linux.vnet.ibm.com>
> ---
> Changes from v1 :
> - Removed unnecessary check for index out of bound.

Provided Gautham/Shilpa find these patches fine..

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [PATCH 2/2] cpufreq: powernv: Use PMSR to verify global and local pstate

2016-11-04 Thread Viresh Kumar
On 04-11-16, 10:57, Akshay Adiga wrote:
> As fast_switch may get called in interrupt disable mode, it does not

s/in interrupt disable mode/with interrupts disabled
s/it does/it may

> update the global_pstate_info data structure. Hence the global_pstate_info
> has stale data whenever pstate is updated through fast_swtich().

s/has/may have
s/swtich/switch

> 
> So the gpstate_timer can fire after a fast_switch() call has update

s/So the/The
s/a fast_swtich() call has update/the fast_switch() call has updated

> the pstates to a different value. Hence the timer handler cannot rely
> on the cached values of local and global pstate and needs to read it
> from the PMSR. 
> 
> Signed-off-by: Akshay Adiga 
> 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 32 ++--
>  1 file changed, 22 insertions(+), 10 deletions(-)

I am not the best guy to judge the code changes here. Can you please include
Shilpa and Gautham to the mail chain and get there feedback.

-- 
viresh


Re: [PATCH 1/2] cpufreq: powernv: Adding fast_switch for schedutil

2016-11-04 Thread Viresh Kumar
On 04-11-16, 10:57, Akshay Adiga wrote:
> Adding fast_switch which does light weight operation to
> set the desired pstate.
> 
> Signed-off-by: Akshay Adiga 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 22 +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index d3ffde8..09a0496 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -752,9 +752,12 @@ static int powernv_cpufreq_cpu_init(struct 
> cpufreq_policy *policy)
>   spin_lock_init(>gpstate_lock);
>   ret = cpufreq_table_validate_and_show(policy, powernv_freqs);
>  
> - if (ret < 0)
> + if (ret < 0) {
>   kfree(policy->driver_data);
> + return ret;
> + }
>  
> + policy->fast_switch_possible = true;
>   return ret;
>  }
>  
> @@ -897,6 +900,22 @@ static void powernv_cpufreq_stop_cpu(struct 
> cpufreq_policy *policy)
>   del_timer_sync(>timer);
>  }
>  
> +static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + int index;
> + struct powernv_smp_call_data freq_data;
> +
> + index = cpufreq_table_find_index_dl(policy, target_freq);
> + if (index < 0 || index >= powernv_pstate_info.nr_pstates)
> + return CPUFREQ_ENTRY_INVALID;

I don't think such a check is required at all. It wouldn't happen without a BUG
in kernel.

> + freq_data.pstate_id = powernv_freqs[index].driver_data;
> + freq_data.gpstate_id = powernv_freqs[index].driver_data;
> + set_pstate(_data);
> +
> + return powernv_freqs[index].frequency;
> +}
> +
>  static struct cpufreq_driver powernv_cpufreq_driver = {
>   .name   = "powernv-cpufreq",
>   .flags  = CPUFREQ_CONST_LOOPS,
> @@ -904,6 +923,7 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
>   .exit   = powernv_cpufreq_cpu_exit,
>   .verify = cpufreq_generic_frequency_table_verify,
>   .target_index   = powernv_cpufreq_target_index,
> + .fast_switch= powernv_fast_switch,
>   .get= powernv_cpufreq_get,
>   .stop_cpu   = powernv_cpufreq_stop_cpu,
>   .attr   = powernv_cpu_freq_attr,
> -- 
> 2.7.4

-- 
viresh


Re: [PATCH] cpufreq: powernv: Use node ID in init_chip_info

2016-10-19 Thread Viresh Kumar
+ few IBM guys who have been working on this.

On 19-10-16, 15:02, Emily Shaffer wrote:
> Fixed assumption that node_id==chip_id in powernv-cpufreq.c:init_chip_info;
> explicitly use node ID where necessary.
> 
> Tested: All CPUs report in /sys/devices/system/cpu*/cpufreq/throttle_stats
> 
> Effort: platforms/arch-powerpc
> Google-Bug-Id: 26979978

Is this relevant upstream?

> 
> Signed-off-by: Emily Shaffer 
> Change-Id: I22eb626b32fbb8053b3bbb9c75e677c700d0c2fb

Gerrit id isn't required for upstream..

> ---
>  drivers/cpufreq/powernv-cpufreq.c | 27 +--
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c
> b/drivers/cpufreq/powernv-cpufreq.c
> index d3ffde8..3750b58 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -911,32 +911,47 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
> 
>  static int init_chip_info(void)
>  {
> -   unsigned int chip[256];
> +   int rc = 0;
> unsigned int cpu, i;
> unsigned int prev_chip_id = UINT_MAX;
> +   unsigned int *chip, *node;
> +
> +   chip = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL);
> +   node = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL);
> +   if (!chip || !node) {
> +   rc = -ENOMEM;
> +   goto out;
> +   }
> 
> for_each_possible_cpu(cpu) {
> unsigned int id = cpu_to_chip_id(cpu);
> 
> if (prev_chip_id != id) {
> prev_chip_id = id;
> -   chip[nr_chips++] = id;
> +   node[nr_chips] = cpu_to_node(cpu);
> +   chip[nr_chips] = id;
> +   nr_chips++;
> }
> }
> 
> chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL);
> -   if (!chips)
> -   return -ENOMEM;
> +   if (!chips) {
> +   rc = -ENOMEM;
> +   goto out;
> +   }
> 
> for (i = 0; i < nr_chips; i++) {
> chips[i].id = chip[i];
> -   cpumask_copy([i].mask, cpumask_of_node(chip[i]));
> +   cpumask_copy([i].mask, cpumask_of_node(node[i]));
> INIT_WORK([i].throttle, powernv_cpufreq_work_fn);
> for_each_cpu(cpu, [i].mask)
> per_cpu(chip_info, cpu) =  [i];
> }
> 
> -   return 0;
> +out:
> +   kfree(node);
> +   kfree(chip);
> +   return rc;
>  }
> 
>  static inline void clean_chip_info(void)
> -- 
> 2.8.0.rc3.226.g39d4020

-- 
viresh


Re: [PATCH] cpufreq: powernv: Fix crash in gpstate_timer_handler

2016-08-04 Thread Viresh Kumar
On 04-08-16, 20:59, Akshay Adiga wrote:
> 'commit 09ca4c9b5958 ("cpufreq: powernv: Replacing pstate_id with
> frequency table index")' changes calc_global_pstate() to use
> cpufreq_table index instead of pstate_id.
> 
> But in gpstate_timer_handler() pstate_id was being passed instead
> of cpufreq_table index, which caused the index_to_pstate() to access
> out of bound indices, leading to this crash.
> 
> Adding sanity check for index and pstate, to ensure only valid pstate
> and index values are returned.
> 
> Call Trace:
> [c0078d66b130] [c011d224] __free_irq+0x234/0x360
> (unreliable)
> [c0078d66b1c0] [c011d44c] free_irq+0x6c/0xa0
> [c0078d66b1f0] [c006c4f8] opal_event_shutdown+0x88/0xd0
> [c0078d66b230] [c0067a4c] opal_shutdown+0x1c/0x90
> [c0078d66b260] [c0063a00] pnv_shutdown+0x20/0x40
> [c0078d66b280] [c0021538] machine_restart+0x38/0x90
> [c78d66b310] [c0965ea0] panic+0x284/0x300
> [c0078d66b3a0] [c001f508] die+0x388/0x450
> [c0078d66b430] [c0045a50] bad_page_fault+0xd0/0x140
> [c0078d66b4a0] [c0008964] handle_page_fault+0x2c/0x30
>interrupt: 300 at gpstate_timer_handler+0x150/0x260
> LR = gpstate_timer_handler+0x130/0x260
> [c0078d66b7f0] [c0132b58] call_timer_fn+0x58/0x1c0
> [c0078d66b880] [c0132e20] expire_timers+0x130/0x1d0
> [c0078d66b8f0] [c0133068] run_timer_softirq+0x1a8/0x230
> [c0078d66b980] [c00b535c] __do_softirq+0x18c/0x400
> [c0078d66ba70] [c00b5828] irq_exit+0xc8/0x100
> [c0078d66ba90] [c001e214] timer_interrupt+0xa4/0xe0
> [c0078d66bac0] [c00027d0] decrementer_common+0x150/0x180
>interrupt: 901 at arch_local_irq_restore+0x74/0x90
>   0] [c0106b34] call_cpuidle+0x44/0x90
> [c0078d66be50] [c010708c] cpu_startup_entry+0x38c/0x460
> [c0078d66bf20] [c003d930] start_secondary+0x330/0x380
> [c0078d66bf90] [c0008e6c] start_secondary_prolog+0x10/0x14
> 
> Fixes: 08d27eb ("cpufreq: powernv: Replacing pstate_id with
> frequency table index")
> Reported-by: Madhavan Srinivasan <ma...@linux.vnet.ibm.com>
> Signed-off-by: Akshay Adiga <akshay.ad...@linux.vnet.ibm.com>
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh


Re: [PATCH v2] cpufreq: powernv: Replacing pstate_id with frequency table index

2016-07-11 Thread Viresh Kumar
On 30-06-16, 11:53, Akshay Adiga wrote:
> Refactoring code to use frequency table index instead of pstate_id.
> This abstraction will make the code independent of the pstate values.
> 
> - No functional changes
> - The highest frequency is at frequency table index 0 and the frequency
>   decreases as the index increases.
> - Macros pstates_to_idx() and idx_to_pstate() can be used for conversion
>   between pstate_id and index.
> - powernv_pstate_info now contains frequency table index to min, max and
>   nominal frequency (instead of pstate_ids)
> - global_pstate_info new stores index values instead pstate ids.
> - variables renamed as *_idx which now store index instead of pstate
> 
> Signed-off-by: Akshay Adiga <akshay.ad...@linux.vnet.ibm.com>
> Reviewed-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
> ---
> Changes from v1:
>   - changed macro names from get_pstate()/ get_index() to 
> idx_to_pstate()/ pstate_to_idx()
>   - Renamed variables that store index instead of pstate_id to *_idx
>   - Retained previous printk's 
> 
> v1 : http://marc.info/?l=linux-pm=146677701501225=1
> 
>  drivers/cpufreq/powernv-cpufreq.c | 177 
> ++
>  1 file changed, 102 insertions(+), 75 deletions(-)

Haven't done in-depth review, but I trust that Gautham has done it :)

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] cpufreq: powernv: Replacing pstate_id with frequency table index

2016-06-27 Thread Viresh Kumar
Hi Akshay,

Did you try running checkpatch for this?

On 24-06-16, 19:33, Akshay Adiga wrote:
> diff --git a/drivers/cpufreq/powernv-cpufreq.c 
> b/drivers/cpufreq/powernv-cpufreq.c
> index b29c5c2..f6ce6f0 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -43,6 +43,7 @@
>  #define PMSR_SPR_EM_DISABLE  (1UL << 31)
>  #define PMSR_MAX(x)  ((x >> 32) & 0xFF)
>  
> +

?

>  #define MAX_RAMP_DOWN_TIME   5120
>  /*
>   * On an idle system we want the global pstate to ramp-down from max value to
> @@ -124,20 +125,29 @@ static int nr_chips;
>  static DEFINE_PER_CPU(struct chip *, chip_info);
>  
>  /*
> - * Note: The set of pstates consists of contiguous integers, the
> - * smallest of which is indicated by powernv_pstate_info.min, the
> - * largest of which is indicated by powernv_pstate_info.max.
> + * Note: The set of pstates consists of contiguous integers,
> + *
> + * powernv_pstate_info stores the index of the frequency table
> + *  instead of pstate itself for each of the pstates referred
>   *
>   * The nominal pstate is the highest non-turbo pstate in this
>   * platform. This is indicated by powernv_pstate_info.nominal.
>   */
>  static struct powernv_pstate_info {
> - int min;
> - int max;
> - int nominal;
> - int nr_pstates;
> + unsigned int min;
> + unsigned int max;
> + unsigned int nominal;
> + unsigned int nr_pstates;
>  } powernv_pstate_info;
>  
> +/* Use following macros for conversions between pstate_id and index */
> +static inline int get_pstate(unsigned int i) {

Read coding-styles please on how to write functions.

> + return powernv_freqs[i].driver_data;
> +}

Add a blank line here please.

> +static inline unsigned int get_index(int pstate) {
> + return abs(pstate - get_pstate(powernv_pstate_info.max));
> +}
> +
>  static inline void reset_gpstates(struct cpufreq_policy *policy)
>  {
>   struct global_pstate_info *gpstates = policy->driver_data;
> @@ -208,23 +218,28 @@ static int init_powernv_pstates(void)
>   return -ENODEV;
>   }
>  
> + powernv_pstate_info.nr_pstates = nr_pstates;
>   pr_debug("NR PStates %d\n", nr_pstates);
>   for (i = 0; i < nr_pstates; i++) {
>   u32 id = be32_to_cpu(pstate_ids[i]);
>   u32 freq = be32_to_cpu(pstate_freqs[i]);
>  
> - pr_debug("PState id %d freq %d MHz\n", id, freq);

?

>   powernv_freqs[i].frequency = freq * 1000; /* kHz */
>   powernv_freqs[i].driver_data = id;

Will it be possible for Shilpa who was earlier on this to review this patch? As
we don't really have great knowledge of the internals of this driver.

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH V5 2/2] cpufreq: Reuse new freq-table helpers

2016-06-26 Thread Viresh Kumar
This patch migrates few users of cpufreq tables to the new helpers that
work on sorted freq-tables.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c | 14 --
 drivers/cpufreq/amd_freq_sensitivity.c |  4 ++--
 drivers/cpufreq/cpufreq_ondemand.c |  6 ++
 drivers/cpufreq/powernv-cpufreq.c  |  3 +--
 drivers/cpufreq/s5pv210-cpufreq.c  |  3 +--
 5 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 32a15052f363..11c9a078e0fd 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -468,20 +468,14 @@ unsigned int acpi_cpufreq_fast_switch(struct 
cpufreq_policy *policy,
struct acpi_cpufreq_data *data = policy->driver_data;
struct acpi_processor_performance *perf;
struct cpufreq_frequency_table *entry;
-   unsigned int next_perf_state, next_freq, freq;
+   unsigned int next_perf_state, next_freq, index;
 
/*
 * Find the closest frequency above target_freq.
-*
-* The table is sorted in the reverse order with respect to the
-* frequency and all of the entries are valid (see the initialization).
 */
-   entry = policy->freq_table;
-   do {
-   entry++;
-   freq = entry->frequency;
-   } while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
-   entry--;
+   index = cpufreq_table_find_index_dl(policy, target_freq);
+
+   entry = >freq_table[index];
next_freq = entry->frequency;
next_perf_state = entry->driver_data;
 
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index 6d5dc04c3a37..042023bbbf62 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -91,8 +91,8 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
else {
unsigned int index;
 
-   index = cpufreq_frequency_table_target(policy,
-   policy->cur - 1, CPUFREQ_RELATION_H);
+   index = cpufreq_table_find_index_h(policy,
+  policy->cur - 1);
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 0c93cd9dee99..3a1f49f5f4c6 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -85,11 +85,9 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
-   index = cpufreq_frequency_table_target(policy, freq_avg,
-  CPUFREQ_RELATION_H);
+   index = cpufreq_table_find_index_h(policy, freq_avg);
freq_lo = freq_table[index].frequency;
-   index = cpufreq_frequency_table_target(policy, freq_avg,
-  CPUFREQ_RELATION_L);
+   index = cpufreq_table_find_index_l(policy, freq_avg);
freq_hi = freq_table[index].frequency;
 
/* Find out how long we have to be in hi and lo freqs */
diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index b29c5c20c3a1..2a2920c4fdf9 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -760,8 +760,7 @@ void powernv_cpufreq_work_fn(struct work_struct *work)
struct cpufreq_policy policy;
 
cpufreq_get_policy(, cpu);
-   index = cpufreq_frequency_table_target(, policy.cur,
-  CPUFREQ_RELATION_C);
+   index = cpufreq_table_find_index_c(, policy.cur);
powernv_cpufreq_target_index(, index);
cpumask_andnot(, , policy.cpus);
}
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c 
b/drivers/cpufreq/s5pv210-cpufreq.c
index 4f4e9df9b7fc..9e07588ea9f5 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -246,8 +246,7 @@ static int s5pv210_target(struct cpufreq_policy *policy, 
unsigned int index)
new_freq = s5pv210_freq_table[index].frequency;
 
/* Finding current running level index */
-   priv_index = cpufreq_frequency_table_target(policy, old_freq,
-   CPUFREQ_RELATION_H);
+   priv_index = cpufreq_table_find_index_h(policy, old_freq);
 
arm_volt = dvs_conf[index].arm_volt;
int_volt = dvs_conf[index].int_volt;
-- 
2.7.4

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

2016-06-07 Thread Viresh Kumar
On 08-06-16, 02:38, Rafael J. Wysocki wrote:
> On Tuesday, June 07, 2016 09:58:07 AM Viresh Kumar wrote:
> > On 06-06-16, 23:56, Rafael J. Wysocki wrote:
> > > Since you are adding new code, you can write it so it doesn't do
> > > unnecessary checks from the start.
> > 
> > Hmm, I will do all that in this series only now.
> > 
> > > While at it, the "if ((freq < policy->min) || (freq > policy->max))"
> > > checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look
> > > good to me, because they very well may cause those function to return
> > > -EINVAL even when there's a valid table and that may cause
> > > acpi_cpufreq_fast_switch() to do bad things.
> > 
> > Hmm. So, the checks are for sure required here, otherwise we may end up
> > returning a frequency which we aren't allowed to. Also note that 'freq' here
> > isn't the target-freq, but the entry in the freq-table.
> > 
> > This routine should be returning a valid freq within the ranges specified by
> > policy->min/max.
> 
> Which in principle may not be possible if the range doesn't include any
> frequency in the table, eg. min == max and between the table entries.

By within ranges I meant, policy->min <= freq <= policy->max, and that's how all
our checks are. So even if the table will have a single valid frequency, we will
return that only.

> However, the CPU has to run at *some* frequency, even if there's none in the
> min/max range.

I completely agree. But the error will be fired only if there is no frequency
within ranges we can switch to. And that's a bug somewhere else then.

> And if we are sure that there is at least one valid frequency between min
> and max, please note that target_freq has already been clamped between them,

Yeah, its already clamped by the freq-change helpers in cpufreq core, but others
may not be doing it properly.

> > Also note that these routines shall *never* return -EINVAL, otherwise it is
> > mostly a bug we are hitting.
> 
> So make them explicitly return a valid frequency every time.

I thought about return Index 0 on such errors, will that be fine ? Anyway the
new patches have added a WARN() for such cases.

> > We have enough checks in place to make sure that there is at least one valid
> > entry in the freq-table which is >= policy->min and <= policy->max.
> 
> That assuming that the driver will always do the right thing in its ->verify
> callback.

Yeah.

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH V4 2/2] cpufreq: Reuse new freq-table helpers

2016-06-07 Thread Viresh Kumar
This patch migrates few users of cpufreq tables to the new helpers that
work on sorted freq-tables.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c | 14 --
 drivers/cpufreq/amd_freq_sensitivity.c |  4 ++--
 drivers/cpufreq/cpufreq_ondemand.c |  6 ++
 drivers/cpufreq/powernv-cpufreq.c  |  3 +--
 drivers/cpufreq/s5pv210-cpufreq.c  |  3 +--
 5 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 32a15052f363..11c9a078e0fd 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -468,20 +468,14 @@ unsigned int acpi_cpufreq_fast_switch(struct 
cpufreq_policy *policy,
struct acpi_cpufreq_data *data = policy->driver_data;
struct acpi_processor_performance *perf;
struct cpufreq_frequency_table *entry;
-   unsigned int next_perf_state, next_freq, freq;
+   unsigned int next_perf_state, next_freq, index;
 
/*
 * Find the closest frequency above target_freq.
-*
-* The table is sorted in the reverse order with respect to the
-* frequency and all of the entries are valid (see the initialization).
 */
-   entry = policy->freq_table;
-   do {
-   entry++;
-   freq = entry->frequency;
-   } while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
-   entry--;
+   index = cpufreq_table_find_index_dl(policy, target_freq);
+
+   entry = >freq_table[index];
next_freq = entry->frequency;
next_perf_state = entry->driver_data;
 
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index 6d5dc04c3a37..042023bbbf62 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -91,8 +91,8 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
else {
unsigned int index;
 
-   index = cpufreq_frequency_table_target(policy,
-   policy->cur - 1, CPUFREQ_RELATION_H);
+   index = cpufreq_table_find_index_h(policy,
+  policy->cur - 1);
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 0c93cd9dee99..3a1f49f5f4c6 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -85,11 +85,9 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
-   index = cpufreq_frequency_table_target(policy, freq_avg,
-  CPUFREQ_RELATION_H);
+   index = cpufreq_table_find_index_h(policy, freq_avg);
freq_lo = freq_table[index].frequency;
-   index = cpufreq_frequency_table_target(policy, freq_avg,
-  CPUFREQ_RELATION_L);
+   index = cpufreq_table_find_index_l(policy, freq_avg);
freq_hi = freq_table[index].frequency;
 
/* Find out how long we have to be in hi and lo freqs */
diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index b29c5c20c3a1..2a2920c4fdf9 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -760,8 +760,7 @@ void powernv_cpufreq_work_fn(struct work_struct *work)
struct cpufreq_policy policy;
 
cpufreq_get_policy(, cpu);
-   index = cpufreq_frequency_table_target(, policy.cur,
-  CPUFREQ_RELATION_C);
+   index = cpufreq_table_find_index_c(, policy.cur);
powernv_cpufreq_target_index(, index);
cpumask_andnot(, , policy.cpus);
}
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c 
b/drivers/cpufreq/s5pv210-cpufreq.c
index 4f4e9df9b7fc..9e07588ea9f5 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -246,8 +246,7 @@ static int s5pv210_target(struct cpufreq_policy *policy, 
unsigned int index)
new_freq = s5pv210_freq_table[index].frequency;
 
/* Finding current running level index */
-   priv_index = cpufreq_frequency_table_target(policy, old_freq,
-   CPUFREQ_RELATION_H);
+   priv_index = cpufreq_table_find_index_h(policy, old_freq);
 
arm_volt = dvs_conf[index].arm_volt;
int_volt = dvs_conf[index].int_volt;
-- 
2.7.1.410.g6faf27b

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

2016-06-06 Thread Viresh Kumar
On 06-06-16, 23:56, Rafael J. Wysocki wrote:
> Since you are adding new code, you can write it so it doesn't do
> unnecessary checks from the start.

Hmm, I will do all that in this series only now.

> While at it, the "if ((freq < policy->min) || (freq > policy->max))"
> checks in cpufreq_find_index_l() and cpufreq_find_index_h() don't look
> good to me, because they very well may cause those function to return
> -EINVAL even when there's a valid table and that may cause
> acpi_cpufreq_fast_switch() to do bad things.

Hmm. So, the checks are for sure required here, otherwise we may end up
returning a frequency which we aren't allowed to. Also note that 'freq' here
isn't the target-freq, but the entry in the freq-table.

This routine should be returning a valid freq within the ranges specified by
policy->min/max.

Also note that these routines shall *never* return -EINVAL, otherwise it is
mostly a bug we are hitting.

We have enough checks in place to make sure that there is at least one valid
entry in the freq-table which is >= policy->min and <= policy->max.

I will take care of rest of the comments though. Thanks.

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

2016-06-06 Thread Viresh Kumar
On 6 June 2016 at 18:27, Rafael J. Wysocki <raf...@kernel.org> wrote:
> On Mon, Jun 6, 2016 at 2:24 PM, Viresh Kumar <viresh.ku...@linaro.org> wrote:
>> On 6 June 2016 at 17:40, Rafael J. Wysocki <r...@rjwysocki.net> wrote:
>>> On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:
>>
>>>> I agree with that, though that requires larger changes across multiple
>>>> sites.
>>>
>>> What changes and where?
>>
>> s/larger/some :)
>>
>> So we can change all the callers of cpufreq_frequency_table_target(),
>
> But why?
>
> It just works as a static inline wrapper around cpufreq_find_index_l()
> for the code in question after this patch, doesn't it?
>
> So if the caller knows it will always ask for RELATION_L, why bother
> with using the wrapper?

Sorry, I got a bit confused. Are you saying that we should do that change
right in the patch?

Because I am also saying that yes, there is no point calling the wrapper.

I can update this patch to make direct calls to the relation specific routines
if you want.

> Also I'm wondering about the cpufreq_for_each_valid_entry() used all
> over.  Can't the things be arranged so all of the entries are valid?

Yeah, there would be multiple opportunities available to optimize code
after this series is in. The policy->table after this series is all sorted
properly and all the entries are valid as well.

But surely that should be done in a separate series
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

2016-06-06 Thread Viresh Kumar
On 6 June 2016 at 17:40, Rafael J. Wysocki <r...@rjwysocki.net> wrote:
> On Monday, June 06, 2016 09:22:31 AM Viresh Kumar wrote:

>> I agree with that, though that requires larger changes across multiple
>> sites.
>
> What changes and where?

s/larger/some :)

So we can change all the callers of cpufreq_frequency_table_target(),
like the governors, ondemand-bias stub drivers, core, etc and call
the dedicated 'relation' specific routines directly, as we mostly know
in advance the 'relation' in which we want to update the freq.

And, so doing that with a dedicated patch might be better.

--
vireshj
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

2016-06-05 Thread Viresh Kumar
On 03-06-16, 16:48, Steve Muckle wrote:
> On Fri, Jun 03, 2016 at 07:05:14PM +0530, Viresh Kumar wrote:
> ...
> > @@ -468,20 +469,15 @@ unsigned int acpi_cpufreq_fast_switch(struct 
> > cpufreq_policy *policy,
> > struct acpi_cpufreq_data *data = policy->driver_data;
> > struct acpi_processor_performance *perf;
> > struct cpufreq_frequency_table *entry;
> > -   unsigned int next_perf_state, next_freq, freq;
> > +   unsigned int next_perf_state, next_freq, index;
> >  
> > /*
> >  * Find the closest frequency above target_freq.
> > -*
> > -* The table is sorted in the reverse order with respect to the
> > -* frequency and all of the entries are valid (see the initialization).
> >  */
> > -   entry = policy->freq_table;
> > -   do {
> > -   entry++;
> > -   freq = entry->frequency;
> > -   } while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
> > -   entry--;
> > +   index = cpufreq_frequency_table_target(policy, target_freq,
> > +  CPUFREQ_RELATION_L);
> 
> Can we call cpufreq_find_index_l directly here? Seems like we could
> phase out cpufreq_frequency_table_target() for the most part and call
> the helpers directly. It would avoid some code bloat, an unnecessary
> switch statement and an error check for an invalid frequency table which
> seems unnecessary for every frequency table lookup.

I agree with that, though that requires larger changes across multiple
sites. I hope it will be fine if I do it in a separate patch on top of
all this. Right ?

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH V3 8/9] cpufreq: Keep policy->freq_table sorted in ascending order

2016-06-03 Thread Viresh Kumar
The drivers aren't required to provide a sorted frequency table today,
and its not optimal to work on an unsorted frequency tables.

To simplify and improve code performance, always keep policy->freq_table
sorted in ascending order.

Now that freq_table is sorted, update cpufreq_frequency_table_target()
to make it more efficient with help of new helpers.

As these helpers will be used by scheduler hotpath later on, keep them
in a new header cpufreq_table.h to avoid unnecessary function calls.

Also update acpi-cpufreq driver to use the efficient
cpufreq_frequency_table_target() routine, as we can't assume the
descending order of frequencies in freq_table anymore.

Tested on Exynos board with both ondemand and schedutil governor and
confirmed with help of various print messages that we are eventually
switching to the desired frequency based on a target frequency.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 MAINTAINERS|   1 +
 drivers/cpufreq/acpi-cpufreq.c |  16 ++--
 drivers/cpufreq/cpufreq.c  |  20 +++--
 drivers/cpufreq/cpufreq_ondemand.h |   1 +
 drivers/cpufreq/freq_table.c   | 163 +++--
 drivers/cpufreq/powernv-cpufreq.c  |   1 +
 drivers/cpufreq/s3c24xx-cpufreq.c  |   1 +
 drivers/cpufreq/s5pv210-cpufreq.c  |   1 +
 include/linux/cpufreq.h|   3 -
 include/linux/cpufreq_table.h  | 139 +++
 10 files changed, 227 insertions(+), 119 deletions(-)
 create mode 100644 include/linux/cpufreq_table.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 7304d2e37a98..315d49d68500 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3205,6 +3205,7 @@ T:git 
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
 T: git git://git.linaro.org/people/vireshk/linux.git (For ARM Updates)
 F: drivers/cpufreq/
 F: include/linux/cpufreq.h
+F: include/linux/cpufreq_table.h
 
 CPU FREQUENCY DRIVERS - ARM BIG LITTLE
 M:     Viresh Kumar <viresh.ku...@linaro.org>
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 32a15052f363..364b86119f3f 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -468,20 +469,15 @@ unsigned int acpi_cpufreq_fast_switch(struct 
cpufreq_policy *policy,
struct acpi_cpufreq_data *data = policy->driver_data;
struct acpi_processor_performance *perf;
struct cpufreq_frequency_table *entry;
-   unsigned int next_perf_state, next_freq, freq;
+   unsigned int next_perf_state, next_freq, index;
 
/*
 * Find the closest frequency above target_freq.
-*
-* The table is sorted in the reverse order with respect to the
-* frequency and all of the entries are valid (see the initialization).
 */
-   entry = policy->freq_table;
-   do {
-   entry++;
-   freq = entry->frequency;
-   } while (freq >= target_freq && freq != CPUFREQ_TABLE_END);
-   entry--;
+   index = cpufreq_frequency_table_target(policy, target_freq,
+  CPUFREQ_RELATION_L);
+
+   entry = >freq_table[index];
next_freq = entry->frequency;
next_perf_state = entry->driver_data;
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9ae58a18ccb9..91f33bc28fa4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1137,6 +1138,16 @@ static void cpufreq_policy_free(struct cpufreq_policy 
*policy, bool notify)
kfree(policy);
 }
 
+static void cpufreq_policy_exit(struct cpufreq_policy *policy)
+{
+   if (!cpufreq_driver->exit)
+   return;
+
+   cpufreq_driver->exit(policy);
+   kfree(policy->freq_table);
+   policy->freq_table = NULL;
+}
+
 static int cpufreq_online(unsigned int cpu)
 {
struct cpufreq_policy *policy;
@@ -1291,9 +1302,7 @@ static int cpufreq_online(unsigned int cpu)
 
 out_exit_policy:
up_write(>rwsem);
-
-   if (cpufreq_driver->exit)
-   cpufreq_driver->exit(policy);
+   cpufreq_policy_exit(policy);
 out_free_policy:
cpufreq_policy_free(policy, !new_policy);
return ret;
@@ -1378,10 +1387,7 @@ static void cpufreq_offline(unsigned int cpu)
 * since this is a core component, and is essential for the
 * subsequent light-weight ->init() to succeed.
 */
-   if (cpufreq_driver->exit) {
-   cpufreq_driver->exit(policy);
-   policy->freq_table = NULL;
-   }
+   cpufreq_policy_exit(policy);
 
 unlock:
up_write(>rwsem);
diff --git a/drivers/cpufreq/cpufreq_ondemand.h 
b/drivers/cpufreq/cpufreq_ondemand.h

[PATCH V3 7/8] cpufreq: Return index from cpufreq_frequency_table_target()

2016-06-02 Thread Viresh Kumar
This routine can't fail unless the frequency table is invalid and
doesn't contain any valid entries.

Make it return the index and WARN() in case it is used for an invalid
table.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt |  7 +++
 drivers/cpufreq/amd_freq_sensitivity.c |  4 ++--
 drivers/cpufreq/cpufreq.c  |  9 ++---
 drivers/cpufreq/cpufreq_ondemand.c | 14 ++
 drivers/cpufreq/freq_table.c   | 24 +---
 drivers/cpufreq/powernv-cpufreq.c  |  4 ++--
 drivers/cpufreq/s3c24xx-cpufreq.c  | 26 ++
 drivers/cpufreq/s5pv210-cpufreq.c  |  7 ++-
 include/linux/cpufreq.h|  3 +--
 9 files changed, 37 insertions(+), 61 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt 
b/Documentation/cpu-freq/cpu-drivers.txt
index 74e812f0719c..772b94fde264 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -245,12 +245,11 @@ policy->max, and all other criteria are met. This is 
helpful for the
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
unsigned int target_freq,
-   unsigned int relation,
-   unsigned int *index);
+   unsigned int relation);
 
 is the corresponding frequency table helper for the ->target
-stage. Just pass the values to this function, and the unsigned int
-index returns the number of the frequency table entry which contains
+stage. Just pass the values to this function, and this function
+returns the number of the frequency table entry which contains
 the frequency the CPU shall be set to.
 
 The following macros can be used as iterators over cpufreq_frequency_table:
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index 3bea1bb791a9..6d5dc04c3a37 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -91,8 +91,8 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
else {
unsigned int index;
 
-   cpufreq_frequency_table_target(policy,
-   policy->cur - 1, CPUFREQ_RELATION_H, );
+   index = cpufreq_frequency_table_target(policy,
+   policy->cur - 1, CPUFREQ_RELATION_H);
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 30f05dd5c872..9ae58a18ccb9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1914,7 +1914,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
unsigned int relation)
 {
unsigned int old_target_freq = target_freq;
-   int index, retval;
+   int index;
 
if (cpufreq_disabled())
return -ENODEV;
@@ -1943,12 +1943,7 @@ int __cpufreq_driver_target(struct cpufreq_policy 
*policy,
if (!cpufreq_driver->target_index)
return -EINVAL;
 
-   retval = cpufreq_frequency_table_target(policy, target_freq, relation,
-   );
-   if (unlikely(retval)) {
-   pr_err("%s: Unable to find matching freq\n", __func__);
-   return retval;
-   }
+   index = cpufreq_frequency_table_target(policy, target_freq, relation);
 
return __target_index(policy, index);
 }
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 2ee476f5a2bd..0c93cd9dee99 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -65,7 +65,7 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
 {
unsigned int freq_req, freq_reduc, freq_avg;
unsigned int freq_hi, freq_lo;
-   unsigned int index = 0;
+   unsigned int index;
unsigned int delay_hi_us;
struct policy_dbs_info *policy_dbs = policy->governor_data;
struct od_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
@@ -79,19 +79,17 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
return freq_next;
}
 
-   cpufreq_frequency_table_target(policy, freq_next, relation, );
+   index = cpufreq_frequency_table_target(policy, freq_next, relation);
freq_req = freq_table[index].frequency;
freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
-   index = 0;
-   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_H,
-  

[PATCH V3 5/8] cpufreq: Drop freq-table param to cpufreq_frequency_table_target()

2016-06-02 Thread Viresh Kumar
The policy already has this pointer set, use it instead.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt |  1 -
 drivers/cpufreq/amd_freq_sensitivity.c |  3 +--
 drivers/cpufreq/cpufreq.c  |  4 ++--
 drivers/cpufreq/cpufreq_ondemand.c | 11 +--
 drivers/cpufreq/freq_table.c   |  2 +-
 drivers/cpufreq/powernv-cpufreq.c  |  3 +--
 drivers/cpufreq/s3c24xx-cpufreq.c  | 11 +--
 drivers/cpufreq/s5pv210-cpufreq.c  |  3 +--
 include/linux/cpufreq.h|  1 -
 9 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt 
b/Documentation/cpu-freq/cpu-drivers.txt
index decbb8cb5573..74e812f0719c 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -244,7 +244,6 @@ policy->max, and all other criteria are met. This is 
helpful for the
 ->verify call.
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-   struct cpufreq_frequency_table *table,
unsigned int target_freq,
unsigned int relation,
unsigned int *index);
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index bc86816693a8..3bea1bb791a9 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -92,8 +92,7 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
unsigned int index;
 
cpufreq_frequency_table_target(policy,
-   policy->freq_table, policy->cur - 1,
-   CPUFREQ_RELATION_H, );
+   policy->cur - 1, CPUFREQ_RELATION_H, );
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1833eda1f9d4..e68611a67bd9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1947,8 +1947,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
return -EINVAL;
}
 
-   retval = cpufreq_frequency_table_target(policy, freq_table, target_freq,
-   relation, );
+   retval = cpufreq_frequency_table_target(policy, target_freq, relation,
+   );
if (unlikely(retval)) {
pr_err("%s: Unable to find matching freq\n", __func__);
return retval;
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 528353f204fd..2ee476f5a2bd 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -79,20 +79,19 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
return freq_next;
}
 
-   cpufreq_frequency_table_target(policy, freq_table, freq_next, relation,
-  );
+   cpufreq_frequency_table_target(policy, freq_next, relation, );
freq_req = freq_table[index].frequency;
freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
index = 0;
-   cpufreq_frequency_table_target(policy, freq_table, freq_avg,
-  CPUFREQ_RELATION_H, );
+   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_H,
+  );
freq_lo = freq_table[index].frequency;
index = 0;
-   cpufreq_frequency_table_target(policy, freq_table, freq_avg,
-  CPUFREQ_RELATION_L, );
+   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_L,
+  );
freq_hi = freq_table[index].frequency;
 
/* Find out how long we have to be in hi and lo freqs */
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index f52b5473b1f4..f145b64649ef 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -114,7 +114,6 @@ int cpufreq_generic_frequency_table_verify(struct 
cpufreq_policy *policy)
 EXPORT_SYMBOL_GPL(cpufreq_generic_frequency_table_verify);
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-  struct cpufreq_frequency_table *table,
   unsigned int target_freq,
   unsigned int relation,
   unsigned int *index)
@@ -128,6 +127,7 @@ int cpufreq_frequency_table_target(struct cpufreq_policy 
*policy,
.frequency = 0,
};
  

[PATCH V2 6/6] cpufreq: Return index from cpufreq_frequency_table_target()

2016-06-02 Thread Viresh Kumar
This routine can't fail unless the frequency table is invalid and
doesn't contain any valid entries.

Make it return the index and WARN() in case it is used for an invalid
table.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt |  7 +++
 drivers/cpufreq/amd_freq_sensitivity.c |  4 ++--
 drivers/cpufreq/cpufreq.c  |  9 ++---
 drivers/cpufreq/cpufreq_ondemand.c | 14 ++
 drivers/cpufreq/freq_table.c   | 24 +---
 drivers/cpufreq/powernv-cpufreq.c  |  4 ++--
 drivers/cpufreq/s3c24xx-cpufreq.c  | 26 ++
 drivers/cpufreq/s5pv210-cpufreq.c  |  7 ++-
 include/linux/cpufreq.h|  3 +--
 9 files changed, 37 insertions(+), 61 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt 
b/Documentation/cpu-freq/cpu-drivers.txt
index 74e812f0719c..772b94fde264 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -245,12 +245,11 @@ policy->max, and all other criteria are met. This is 
helpful for the
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
unsigned int target_freq,
-   unsigned int relation,
-   unsigned int *index);
+   unsigned int relation);
 
 is the corresponding frequency table helper for the ->target
-stage. Just pass the values to this function, and the unsigned int
-index returns the number of the frequency table entry which contains
+stage. Just pass the values to this function, and this function
+returns the number of the frequency table entry which contains
 the frequency the CPU shall be set to.
 
 The following macros can be used as iterators over cpufreq_frequency_table:
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index 3bea1bb791a9..6d5dc04c3a37 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -91,8 +91,8 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
else {
unsigned int index;
 
-   cpufreq_frequency_table_target(policy,
-   policy->cur - 1, CPUFREQ_RELATION_H, );
+   index = cpufreq_frequency_table_target(policy,
+   policy->cur - 1, CPUFREQ_RELATION_H);
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 30f05dd5c872..9ae58a18ccb9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1914,7 +1914,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
unsigned int relation)
 {
unsigned int old_target_freq = target_freq;
-   int index, retval;
+   int index;
 
if (cpufreq_disabled())
return -ENODEV;
@@ -1943,12 +1943,7 @@ int __cpufreq_driver_target(struct cpufreq_policy 
*policy,
if (!cpufreq_driver->target_index)
return -EINVAL;
 
-   retval = cpufreq_frequency_table_target(policy, target_freq, relation,
-   );
-   if (unlikely(retval)) {
-   pr_err("%s: Unable to find matching freq\n", __func__);
-   return retval;
-   }
+   index = cpufreq_frequency_table_target(policy, target_freq, relation);
 
return __target_index(policy, index);
 }
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 2ee476f5a2bd..0c93cd9dee99 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -65,7 +65,7 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
 {
unsigned int freq_req, freq_reduc, freq_avg;
unsigned int freq_hi, freq_lo;
-   unsigned int index = 0;
+   unsigned int index;
unsigned int delay_hi_us;
struct policy_dbs_info *policy_dbs = policy->governor_data;
struct od_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
@@ -79,19 +79,17 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
return freq_next;
}
 
-   cpufreq_frequency_table_target(policy, freq_next, relation, );
+   index = cpufreq_frequency_table_target(policy, freq_next, relation);
freq_req = freq_table[index].frequency;
freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
-   index = 0;
-   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_H,
-  

[PATCH V2 4/6] cpufreq: Drop freq-table param to cpufreq_frequency_table_target()

2016-06-02 Thread Viresh Kumar
The policy already has this pointer set, use it instead.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt |  1 -
 drivers/cpufreq/amd_freq_sensitivity.c |  3 +--
 drivers/cpufreq/cpufreq.c  |  4 ++--
 drivers/cpufreq/cpufreq_ondemand.c | 11 +--
 drivers/cpufreq/freq_table.c   |  2 +-
 drivers/cpufreq/powernv-cpufreq.c  |  3 +--
 drivers/cpufreq/s3c24xx-cpufreq.c  | 11 +--
 drivers/cpufreq/s5pv210-cpufreq.c  |  3 +--
 include/linux/cpufreq.h|  1 -
 9 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt 
b/Documentation/cpu-freq/cpu-drivers.txt
index decbb8cb5573..74e812f0719c 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -244,7 +244,6 @@ policy->max, and all other criteria are met. This is 
helpful for the
 ->verify call.
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-   struct cpufreq_frequency_table *table,
unsigned int target_freq,
unsigned int relation,
unsigned int *index);
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index bc86816693a8..3bea1bb791a9 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -92,8 +92,7 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
unsigned int index;
 
cpufreq_frequency_table_target(policy,
-   policy->freq_table, policy->cur - 1,
-   CPUFREQ_RELATION_H, );
+   policy->cur - 1, CPUFREQ_RELATION_H, );
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1833eda1f9d4..e68611a67bd9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1947,8 +1947,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
return -EINVAL;
}
 
-   retval = cpufreq_frequency_table_target(policy, freq_table, target_freq,
-   relation, );
+   retval = cpufreq_frequency_table_target(policy, target_freq, relation,
+   );
if (unlikely(retval)) {
pr_err("%s: Unable to find matching freq\n", __func__);
return retval;
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 528353f204fd..2ee476f5a2bd 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -79,20 +79,19 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
return freq_next;
}
 
-   cpufreq_frequency_table_target(policy, freq_table, freq_next, relation,
-  );
+   cpufreq_frequency_table_target(policy, freq_next, relation, );
freq_req = freq_table[index].frequency;
freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
index = 0;
-   cpufreq_frequency_table_target(policy, freq_table, freq_avg,
-  CPUFREQ_RELATION_H, );
+   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_H,
+  );
freq_lo = freq_table[index].frequency;
index = 0;
-   cpufreq_frequency_table_target(policy, freq_table, freq_avg,
-  CPUFREQ_RELATION_L, );
+   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_L,
+  );
freq_hi = freq_table[index].frequency;
 
/* Find out how long we have to be in hi and lo freqs */
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index f52b5473b1f4..f145b64649ef 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -114,7 +114,6 @@ int cpufreq_generic_frequency_table_verify(struct 
cpufreq_policy *policy)
 EXPORT_SYMBOL_GPL(cpufreq_generic_frequency_table_verify);
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-  struct cpufreq_frequency_table *table,
   unsigned int target_freq,
   unsigned int relation,
   unsigned int *index)
@@ -128,6 +127,7 @@ int cpufreq_frequency_table_target(struct cpufreq_policy 
*policy,
.frequency = 0,
};
  

Re: [PATCH 3/8] cpufreq: powerenv: Fix memory leak

2016-06-02 Thread Viresh Kumar
On 02-06-16, 21:37, Michael Ellerman wrote:
> On Thu, 2016-06-02 at 16:52 +0530, Viresh Kumar wrote:
> > On 02-06-16, 21:08, Michael Ellerman wrote:
> > > On Wed, 2016-06-01 at 16:04 +0530, Viresh Kumar wrote:
> > > 
> > > > The policy is copied (unnecessarily) and is never freed. Fix it by just
> > > > getting a reference to the existing policy structure and putting it
> > > > back.
> > > > 
> > > > Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> > > 
> > > When was it broken, always?
> > > 
> > > Cc: stable ?
> > 
> > Its a small memory leak and its not that we will fail on something. So
> > didn't bother to add those details, but in case they are required:
> > 
> > Cc: <sta...@vger.kernel.org> # 4.3+
> > Fixes: 227942809b52 ("cpufreq: powernv: Restore cpu frequency to 
> > policy->cur on unthrottling")
> 
> OK. I can't actually see where the copy is?
> 
> But if we are leaking even a small amount of memory in a loop like that, in a
> function that's run semi-regularly, then it's going to add up eventually.

Urg, it wasn't a memory leak actually. I misread.

I somehow thought that cpufreq_get_policy() is also allocating memory
for the policy, but it just memcpy's it into the callers buffer. So,
that's not a problem really.

This patch should be just dropped. Sorry for the noise.

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/8] cpufreq: powerenv: Fix memory leak

2016-06-02 Thread Viresh Kumar
On 02-06-16, 21:08, Michael Ellerman wrote:
> On Wed, 2016-06-01 at 16:04 +0530, Viresh Kumar wrote:
> 
> > The policy is copied (unnecessarily) and is never freed. Fix it by just
> > getting a reference to the existing policy structure and putting it
> > back.
> > 
> > Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> 
> When was it broken, always?
> 
> Cc: stable ?

Its a small memory leak and its not that we will fail on something. So
didn't bother to add those details, but in case they are required:

Cc: <sta...@vger.kernel.org> # 4.3+
Fixes: 227942809b52 ("cpufreq: powernv: Restore cpu frequency to policy->cur on 
unthrottling")

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 8/8] cpufreq: Return index from cpufreq_frequency_table_target()

2016-06-01 Thread Viresh Kumar
This routine can't fail unless the frequency table is invalid and
doesn't contain any valid entries.

Make it return the index and WARN() in case it is used for an invalid
table.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt |  7 +++
 drivers/cpufreq/amd_freq_sensitivity.c |  4 ++--
 drivers/cpufreq/cpufreq.c  |  9 ++---
 drivers/cpufreq/cpufreq_ondemand.c | 14 ++
 drivers/cpufreq/freq_table.c   | 24 +---
 drivers/cpufreq/powernv-cpufreq.c  |  4 ++--
 drivers/cpufreq/s3c24xx-cpufreq.c  | 26 ++
 drivers/cpufreq/s5pv210-cpufreq.c  |  7 ++-
 include/linux/cpufreq.h|  3 +--
 9 files changed, 37 insertions(+), 61 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt 
b/Documentation/cpu-freq/cpu-drivers.txt
index 74e812f0719c..772b94fde264 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -245,12 +245,11 @@ policy->max, and all other criteria are met. This is 
helpful for the
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
unsigned int target_freq,
-   unsigned int relation,
-   unsigned int *index);
+   unsigned int relation);
 
 is the corresponding frequency table helper for the ->target
-stage. Just pass the values to this function, and the unsigned int
-index returns the number of the frequency table entry which contains
+stage. Just pass the values to this function, and this function
+returns the number of the frequency table entry which contains
 the frequency the CPU shall be set to.
 
 The following macros can be used as iterators over cpufreq_frequency_table:
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index 3bea1bb791a9..6d5dc04c3a37 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -91,8 +91,8 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
else {
unsigned int index;
 
-   cpufreq_frequency_table_target(policy,
-   policy->cur - 1, CPUFREQ_RELATION_H, );
+   index = cpufreq_frequency_table_target(policy,
+   policy->cur - 1, CPUFREQ_RELATION_H);
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1685e930770f..07c933c6c29a 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1914,7 +1914,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
unsigned int relation)
 {
unsigned int old_target_freq = target_freq;
-   int index, retval;
+   int index;
 
if (cpufreq_disabled())
return -ENODEV;
@@ -1943,12 +1943,7 @@ int __cpufreq_driver_target(struct cpufreq_policy 
*policy,
if (!cpufreq_driver->target_index)
return -EINVAL;
 
-   retval = cpufreq_frequency_table_target(policy, target_freq, relation,
-   );
-   if (unlikely(retval)) {
-   pr_err("%s: Unable to find matching freq\n", __func__);
-   return retval;
-   }
+   index = cpufreq_frequency_table_target(policy, target_freq, relation);
 
return __target_index(policy, index);
 }
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 2ee476f5a2bd..0c93cd9dee99 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -65,7 +65,7 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
 {
unsigned int freq_req, freq_reduc, freq_avg;
unsigned int freq_hi, freq_lo;
-   unsigned int index = 0;
+   unsigned int index;
unsigned int delay_hi_us;
struct policy_dbs_info *policy_dbs = policy->governor_data;
struct od_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
@@ -79,19 +79,17 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
return freq_next;
}
 
-   cpufreq_frequency_table_target(policy, freq_next, relation, );
+   index = cpufreq_frequency_table_target(policy, freq_next, relation);
freq_req = freq_table[index].frequency;
freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
-   index = 0;
-   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_H,
-  

[PATCH 6/8] cpufreq: Drop freq-table param to cpufreq_frequency_table_target()

2016-06-01 Thread Viresh Kumar
The policy already has this pointer set, use it instead.

Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt |  1 -
 drivers/cpufreq/amd_freq_sensitivity.c |  3 +--
 drivers/cpufreq/cpufreq.c  |  4 ++--
 drivers/cpufreq/cpufreq_ondemand.c | 11 +--
 drivers/cpufreq/freq_table.c   |  2 +-
 drivers/cpufreq/powernv-cpufreq.c  |  3 +--
 drivers/cpufreq/s3c24xx-cpufreq.c  | 11 +--
 drivers/cpufreq/s5pv210-cpufreq.c  |  3 +--
 include/linux/cpufreq.h|  1 -
 9 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt 
b/Documentation/cpu-freq/cpu-drivers.txt
index decbb8cb5573..74e812f0719c 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -244,7 +244,6 @@ policy->max, and all other criteria are met. This is 
helpful for the
 ->verify call.
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-   struct cpufreq_frequency_table *table,
unsigned int target_freq,
unsigned int relation,
unsigned int *index);
diff --git a/drivers/cpufreq/amd_freq_sensitivity.c 
b/drivers/cpufreq/amd_freq_sensitivity.c
index bc86816693a8..3bea1bb791a9 100644
--- a/drivers/cpufreq/amd_freq_sensitivity.c
+++ b/drivers/cpufreq/amd_freq_sensitivity.c
@@ -92,8 +92,7 @@ static unsigned int amd_powersave_bias_target(struct 
cpufreq_policy *policy,
unsigned int index;
 
cpufreq_frequency_table_target(policy,
-   policy->freq_table, policy->cur - 1,
-   CPUFREQ_RELATION_H, );
+   policy->cur - 1, CPUFREQ_RELATION_H, );
freq_next = policy->freq_table[index].frequency;
}
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index cc252eecc45a..a28144697128 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1947,8 +1947,8 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
return -EINVAL;
}
 
-   retval = cpufreq_frequency_table_target(policy, freq_table, target_freq,
-   relation, );
+   retval = cpufreq_frequency_table_target(policy, target_freq, relation,
+   );
if (unlikely(retval)) {
pr_err("%s: Unable to find matching freq\n", __func__);
return retval;
diff --git a/drivers/cpufreq/cpufreq_ondemand.c 
b/drivers/cpufreq/cpufreq_ondemand.c
index 528353f204fd..2ee476f5a2bd 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -79,20 +79,19 @@ static unsigned int generic_powersave_bias_target(struct 
cpufreq_policy *policy,
return freq_next;
}
 
-   cpufreq_frequency_table_target(policy, freq_table, freq_next, relation,
-  );
+   cpufreq_frequency_table_target(policy, freq_next, relation, );
freq_req = freq_table[index].frequency;
freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
freq_avg = freq_req - freq_reduc;
 
/* Find freq bounds for freq_avg in freq_table */
index = 0;
-   cpufreq_frequency_table_target(policy, freq_table, freq_avg,
-  CPUFREQ_RELATION_H, );
+   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_H,
+  );
freq_lo = freq_table[index].frequency;
index = 0;
-   cpufreq_frequency_table_target(policy, freq_table, freq_avg,
-  CPUFREQ_RELATION_L, );
+   cpufreq_frequency_table_target(policy, freq_avg, CPUFREQ_RELATION_L,
+  );
freq_hi = freq_table[index].frequency;
 
/* Find out how long we have to be in hi and lo freqs */
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index f52b5473b1f4..f145b64649ef 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -114,7 +114,6 @@ int cpufreq_generic_frequency_table_verify(struct 
cpufreq_policy *policy)
 EXPORT_SYMBOL_GPL(cpufreq_generic_frequency_table_verify);
 
 int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
-  struct cpufreq_frequency_table *table,
   unsigned int target_freq,
   unsigned int relation,
   unsigned int *index)
@@ -128,6 +127,7 @@ int cpufreq_frequency_table_target(struct cpufreq_policy 
*policy,
.frequency = 0,
};
  

[PATCH 3/8] cpufreq: powerenv: Fix memory leak

2016-06-01 Thread Viresh Kumar
The policy is copied (unnecessarily) and is never freed. Fix it by just
getting a reference to the existing policy structure and putting it
back.

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

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index 54c45368e3f1..96bb4acd366e 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -756,15 +756,18 @@ void powernv_cpufreq_work_fn(struct work_struct *work)
 
chip->restore = false;
for_each_cpu(cpu, ) {
+   struct cpufreq_policy *policy = cpufreq_cpu_get(cpu)
int index;
-   struct cpufreq_policy policy;
 
-   cpufreq_get_policy(, cpu);
-   cpufreq_frequency_table_target(, policy.freq_table,
-  policy.cur,
+   if (!policy)
+   continue;
+
+   cpufreq_frequency_table_target(policy, policy->freq_table,
+  policy->cur,
   CPUFREQ_RELATION_C, );
-   powernv_cpufreq_target_index(, index);
-   cpumask_andnot(, , policy.cpus);
+   powernv_cpufreq_target_index(policy, index);
+   cpumask_andnot(, , policy->cpus);
+   cpufreq_cpu_put(policy);
}
 out:
put_online_cpus();
-- 
2.7.1.410.g6faf27b

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH-next v2 0/2] cpufreq: powernv: Fixes for Global pstate management

2016-05-09 Thread Viresh Kumar
On 03-05-16, 20:49, Akshay Adiga wrote:
> Fixes are based on patch https://patchwork.ozlabs.org/patch/612058/ which
> is in Rafael's linux-next.
> 
> - Patch [1] fixes WARN_ON in powernv_target_index()
> - Patch [2] Deleting any pending timer to saves an unnecessary irq call
>  in powernv_target_index()
> 
> Akshay Adiga (2):
>   cpufreq: powernv: Move smp_call_function_any() out of irq safe block
>   cpufreq: powernv: del_timer_sync when global and local pstate are
> equal

Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

-- 
viresh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

  1   2   3   >