Re: [PATCH v3 2/5] powernv, cpufreq:Add per-core locking to serialize frequency transitions
On Thu, Mar 20, 2014 at 05:40:57PM +0530, Gautham R. Shenoy wrote:
From: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
On POWER systems, the CPU frequency is controlled at a core-level and
hence we need to serialize so that only one of the threads in the core
switches the core's frequency at a time.
Using a global mutex lock would needlessly serialize _all_ frequency
transitions in the system (across all cores). So introduce per-core
locking to enable finer-grained synchronization and thereby enhance
the speed and responsiveness of the cpufreq driver to varying workload
demands.
The design of per-core locking is very simple and straight-forward: we
first define a Per-CPU lock and use the ones that belongs to the first
thread sibling of the core.
cpu_first_thread_sibling() macro is used to find the *common* lock for
all thread siblings belonging to a core.
Forgot to add the following line.
Reviewed-by: Preeti U Murthy pre...@linux.vnet.ibm.com
Signed-off-by: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
Signed-off-by: Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com
Signed-off-by: Gautham R. Shenoy e...@linux.vnet.ibm.com
---
drivers/cpufreq/powernv-cpufreq.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
index ab1551f..66dae0d 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -24,8 +24,15 @@
#include linux/of.h
#include asm/cputhreads.h
-/* FIXME: Make this per-core */
-static DEFINE_MUTEX(freq_switch_mutex);
+/* Per-Core locking for frequency transitions */
+static DEFINE_PER_CPU(struct mutex, freq_switch_lock);
+
+#define lock_core_freq(cpu) \
+ mutex_lock(per_cpu(freq_switch_lock,\
+ cpu_first_thread_sibling(cpu)));
+#define unlock_core_freq(cpu)\
+ mutex_unlock(per_cpu(freq_switch_lock,\
+ cpu_first_thread_sibling(cpu)));
#define POWERNV_MAX_PSTATES 256
@@ -221,7 +228,7 @@ static int powernv_cpufreq_target(struct cpufreq_policy
*policy,
freqs.new = powernv_freqs[new_index].frequency;
freqs.cpu = policy-cpu;
- mutex_lock(freq_switch_mutex);
+ lock_core_freq(policy-cpu);
cpufreq_notify_transition(policy, freqs, CPUFREQ_PRECHANGE);
pr_debug(setting frequency for cpu %d to %d kHz index %d pstate %d,
@@ -233,7 +240,7 @@ static int powernv_cpufreq_target(struct cpufreq_policy
*policy,
rc = powernv_set_freq(policy-cpus, new_index);
cpufreq_notify_transition(policy, freqs, CPUFREQ_POSTCHANGE);
- mutex_unlock(freq_switch_mutex);
+ unlock_core_freq(policy-cpu);
return rc;
}
@@ -250,7 +257,7 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
static int __init powernv_cpufreq_init(void)
{
- int rc = 0;
+ int cpu, rc = 0;
/* Discover pstates from device tree and init */
@@ -260,6 +267,10 @@ static int __init powernv_cpufreq_init(void)
pr_info(powernv-cpufreq disabled\n);
return rc;
}
+ /* Init per-core mutex */
+ for_each_possible_cpu(cpu) {
+ mutex_init(per_cpu(freq_switch_lock, cpu));
+ }
rc = cpufreq_register_driver(powernv_cpufreq_driver);
return rc;
--
1.8.3.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/5] powernv:cpufreq: Create pstate_id_to_freq() helper
On Thu, Mar 20, 2014 at 05:40:58PM +0530, Gautham R. Shenoy wrote:
From: Gautham R. Shenoy e...@linux.vnet.ibm.com
Create a helper routine that can return the cpu-frequency for the
corresponding pstate_id.
Also, cache the values of the pstate_max, pstate_min and
pstate_nominal and nr_pstates in a static structure so that they can
be reused in the future to perform any validations.
Forgot to add the following line:
Reviewed-by: Preeti U Murthy pre...@linux.vnet.ibm.com
Signed-off-by: Gautham R. Shenoy e...@linux.vnet.ibm.com
---
drivers/cpufreq/powernv-cpufreq.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
index 66dae0d..e7b0292 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -39,6 +39,14 @@ static DEFINE_PER_CPU(struct mutex, freq_switch_lock);
static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
static int powernv_pstate_ids[POWERNV_MAX_PSTATES+1];
+struct powernv_pstate_info {
+ int pstate_min_id;
+ int pstate_max_id;
+ int pstate_nominal_id;
+ int nr_pstates;
+};
+static struct powernv_pstate_info powernv_pstate_info;
+
/*
* Initialize the freq table based on data obtained
* from the firmware passed via device-tree
@@ -112,9 +120,28 @@ static int init_powernv_pstates(void)
for (i = 0; powernv_freqs[i].frequency != CPUFREQ_TABLE_END; i++)
pr_debug(%d: %d\n, i, powernv_freqs[i].frequency);
+ powernv_pstate_info.pstate_min_id = pstate_min;
+ powernv_pstate_info.pstate_max_id = pstate_max;
+ powernv_pstate_info.pstate_nominal_id = pstate_nominal;
+ powernv_pstate_info.nr_pstates = nr_pstates;
+
return 0;
}
+/**
+ * Returns the cpu frequency corresponding to the pstate_id.
+ */
+static unsigned int pstate_id_to_freq(int pstate_id)
+{
+ int i;
+
+ i = powernv_pstate_info.pstate_max_id - pstate_id;
+
+ BUG_ON(i = powernv_pstate_info.nr_pstates || i 0);
+ WARN_ON(powernv_pstate_ids[i] != pstate_id);
+ return powernv_freqs[i].frequency;
+}
+
static struct freq_attr *powernv_cpu_freq_attr[] = {
cpufreq_freq_attr_scaling_available_freqs,
NULL,
--
1.8.3.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On Thu, Mar 20, 2014 at 05:41:00PM +0530, Gautham R. Shenoy wrote:
From: Gautham R. Shenoy e...@linux.vnet.ibm.com
The current frequency of a cpu is reported through the sysfs file
cpuinfo_cur_freq. This requires the driver to implement a
-get(unsigned int cpu) method which will return the current
operating frequency.
Implement a function named powernv_cpufreq_get() which reads the local
pstate from the PMSR and returns the corresponding frequency.
Set the powernv_cpufreq_driver.get hook to powernv_cpufreq_get().
Forgot to add
Reviewed-by: Preeti U Murthy pre...@linux.vnet.ibm.com
Signed-off-by: Gautham R. Shenoy e...@linux.vnet.ibm.com
---
drivers/cpufreq/powernv-cpufreq.c | 38 ++
1 file changed, 38 insertions(+)
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
index 46bee8a..ef6ed8c 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -206,6 +206,43 @@ static inline void set_pmspr(unsigned long sprn,
unsigned long val)
BUG();
}
+/*
+ * Computes the current frequency on this cpu
+ * and stores the result in *ret_freq.
+ */
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
+ pmspr_val = get_pmspr(SPRN_PMSR);
+
+ /* The local pstate id corresponds bits 48..55 in the PMSR.
+ * Note: Watch out for the sign! */
+ local_pstate_id = (pmspr_val 48) 0xFF;
+ pstate_id = local_pstate_id;
+
+ freq = pstate_id_to_freq(pstate_id);
+ pr_debug(cpu %d pmsr %lx pstate_id %d frequency %d \n,
+ smp_processor_id(), pmspr_val, pstate_id, freq);
+ *cur_freq = freq;
+}
+
+/*
+ * Returns the cpu frequency as reported by the firmware for 'cpu'.
+ * This value is reported through the sysfs file cpuinfo_cur_freq.
+ */
+unsigned int powernv_cpufreq_get(unsigned int cpu)
+{
+ int ret_freq;
+
+ smp_call_function_any(cpu_sibling_mask(cpu), powernv_read_cpu_freq,
+ ret_freq, 1);
+ return ret_freq;
+}
+
static void set_pstate(void *pstate)
{
unsigned long val;
@@ -297,6 +334,7 @@ static int powernv_cpufreq_target(struct cpufreq_policy
*policy,
static struct cpufreq_driver powernv_cpufreq_driver = {
.verify = powernv_cpufreq_verify,
.target = powernv_cpufreq_target,
+ .get= powernv_cpufreq_get,
.init = powernv_cpufreq_cpu_init,
.exit = powernv_cpufreq_cpu_exit,
.name = powernv-cpufreq,
--
1.8.3.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 0/5] powernv: Enable Dynamic Frequency
On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy e...@linux.vnet.ibm.com wrote: This is the v3 of the consolidated patchset consisting patches for enabling cpufreq on IBM POWERNV platforms along with some enhancements. This is the first time I saw them. Looks like you never Cc'd linux-pm list. Also, would be better to keep Maintainers in --to field so that they can review them as soon as possible. Otherwise they might stay on lists for a long time.. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 0/5] powernv: Enable Dynamic Frequency
Hi Viresh, On Fri, Mar 21, 2014 at 01:16:03PM +0530, Viresh Kumar wrote: On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy e...@linux.vnet.ibm.com wrote: This is the v3 of the consolidated patchset consisting patches for enabling cpufreq on IBM POWERNV platforms along with some enhancements. This is the first time I saw them. Looks like you never Cc'd linux-pm list. Also, would be better to keep Maintainers in --to field so that they can review them as soon as possible. Otherwise they might stay on lists for a long time.. Yes, I hadn't Cc'ed linux-pm list last time around. Sorry about that. Will keep this in mind the next time around. Also, your suggestion regarding keeping the Maintainers in --to field is well taken. -- Thanks and Regards gautham. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper
Hi Geoff, Do you have these in a git branch somewhere so I can merge and test them? No, sorry, I don't have a public git repo. Let me see how I can fix that, it might be useful for future patchsets. Thanks, C. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy
e...@linux.vnet.ibm.com wrote:
From: Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com
Hi Vaidy,
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 4b029c0..4ba1632 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -48,6 +48,7 @@ config CPU_FREQ_STAT_DETAILS
choice
prompt Default CPUFreq governor
default CPU_FREQ_DEFAULT_GOV_USERSPACE if ARM_SA1100_CPUFREQ ||
ARM_SA1110_CPUFREQ
+ default CPU_FREQ_DEFAULT_GOV_ONDEMAND if POWERNV_CPUFREQ
Probably we should remove SA1100's entry as well from here. This is
not the right way of doing it. Imagine 100 platforms having entries here.
If you want it, then select it from your platforms Kconfig.
diff --git a/drivers/cpufreq/Kconfig.powerpc b/drivers/cpufreq/Kconfig.powerpc
index ca0021a..93f8689 100644
--- a/drivers/cpufreq/Kconfig.powerpc
+++ b/drivers/cpufreq/Kconfig.powerpc
@@ -54,3 +54,16 @@ config PPC_PASEMI_CPUFREQ
help
This adds the support for frequency switching on PA Semi
PWRficient processors.
+
+config POWERNV_CPUFREQ
+ tristate CPU frequency scaling for IBM POWERNV platform
+ depends on PPC_POWERNV
+ select CPU_FREQ_GOV_PERFORMANCE
+ select CPU_FREQ_GOV_POWERSAVE
+ select CPU_FREQ_GOV_USERSPACE
+ select CPU_FREQ_GOV_ONDEMAND
+ select CPU_FREQ_GOV_CONSERVATIVE
Nice :)
People normally do this from the defconfigs instead. And logically speaking
we might better have only dependencies here, isn't it? And obviously
governors aren't a dependency for this driver. :)
+ default y
+ help
+This adds support for CPU frequency switching on IBM POWERNV
+platform
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
new file mode 100644
index 000..ab1551f
--- /dev/null
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -0,0 +1,277 @@
+/*
+ * POWERNV cpufreq driver for the IBM POWER processors
+ *
+ * (C) Copyright IBM 2014
+ *
+ * Author: Vaidyanathan Srinivasan svaidy at linux.vnet.ibm.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#define pr_fmt(fmt)powernv-cpufreq: fmt
+
+#include linux/module.h
+#include linux/cpufreq.h
+#include linux/of.h
+#include asm/cputhreads.h
That's it? Sure?
Even if things compile for you, you must explicitly include all the
files on which
you depend.
+/* FIXME: Make this per-core */
+static DEFINE_MUTEX(freq_switch_mutex);
+
+#define POWERNV_MAX_PSTATES256
+
+static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
+static int powernv_pstate_ids[POWERNV_MAX_PSTATES+1];
+
+/*
+ * Initialize the freq table based on data obtained
+ * from the firmware passed via device-tree
+ */
+
+static int init_powernv_pstates(void)
+{
+ struct device_node *power_mgt;
+ int nr_pstates = 0;
+ int pstate_min, pstate_max, pstate_nominal;
+ const __be32 *pstate_ids, *pstate_freqs;
+ int i;
+ u32 len_ids, len_freqs;
+
+ power_mgt = of_find_node_by_path(/ibm,opal/power-mgt);
+ if (!power_mgt) {
+ pr_warn(power-mgt node not found\n);
+ return -ENODEV;
+ }
+
+ if (of_property_read_u32(power_mgt, ibm,pstate-min, pstate_min)) {
+ pr_warn(ibm,pstate-min node not found\n);
+ return -ENODEV;
+ }
+
+ if (of_property_read_u32(power_mgt, ibm,pstate-max, pstate_max)) {
+ pr_warn(ibm,pstate-max node not found\n);
+ return -ENODEV;
+ }
+
+ if (of_property_read_u32(power_mgt, ibm,pstate-nominal,
+pstate_nominal)) {
+ pr_warn(ibm,pstate-nominal not found\n);
+ return -ENODEV;
+ }
+ pr_info(cpufreq pstate min %d nominal %d max %d\n, pstate_min,
+ pstate_nominal, pstate_max);
+
+ pstate_ids = of_get_property(power_mgt, ibm,pstate-ids, len_ids);
+ if (!pstate_ids) {
+ pr_warn(ibm,pstate-ids not found\n);
+ return -ENODEV;
+ }
+
+ pstate_freqs = of_get_property(power_mgt,
ibm,pstate-frequencies-mhz,
+ len_freqs);
+ if (!pstate_freqs) {
+ pr_warn(ibm,pstate-frequencies-mhz not found\n);
+ return -ENODEV;
+ }
+
+ WARN_ON(len_ids != len_freqs);
+
Re: [PATCH v3 2/5] powernv, cpufreq:Add per-core locking to serialize frequency transitions
On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy e...@linux.vnet.ibm.com wrote: From: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com On POWER systems, the CPU frequency is controlled at a core-level and hence we need to serialize so that only one of the threads in the core switches the core's frequency at a time. Probably you don't need this anymore. https://lkml.org/lkml/2014/3/21/23 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 9/9] powerpc/pm: support deep sleep feature on T1040
From: Scott Wood [mailto:scottw...@freescale.com] On Thu, 2014-03-20 at 11:59 +, David Laight wrote: I tried to work out what the 'twi, isync' instructions were for (in in_le32()). The best I could come up with was to ensure a synchronous bus-fault. But bus faults are probably only expected during device probing - not normal operation, and the instructions will have a significant cost. Additionally in_le32() and out_le32() both start with a 'sync' instruction. In many cases that isn't needed either - an explicit iosync() can be used after groups of instructions. The idea is that it's better to be maximally safe by default, and let performance critical sections be optimized using raw accessors and explicit synchronization if needed, than to have hard-to-debug bugs due to missing/wrong sync. A lot of I/O is slow enough that the performance impact doesn't really matter, but the brain-time cost of getting the sync right is still there. Hmmm That might be an excuse for the 'sync', but not the twi and isync. I was setting up a dma request (for the ppc 83xx PCIe bridge) and was doing back to back little-endian writes into memory. I had difficulty finding and including header files containing the definitions for byteswapped accesses I needed. arch/powerpc/include/asm/swab.h contains some - but I couldn't work out how to get it included (apart from giving the full path). In any case you need to understand when synchronisation is required - otherwise you will get it wrong. Especially since non-byteswapped accesses are done by direct access. David ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On Thu, Mar 20, 2014 at 5:41 PM, Gautham R. Shenoy
e...@linux.vnet.ibm.com wrote:
From: Gautham R. Shenoy e...@linux.vnet.ibm.com
The current frequency of a cpu is reported through the sysfs file
cpuinfo_cur_freq. This requires the driver to implement a
-get(unsigned int cpu) method which will return the current
operating frequency.
Implement a function named powernv_cpufreq_get() which reads the local
pstate from the PMSR and returns the corresponding frequency.
Set the powernv_cpufreq_driver.get hook to powernv_cpufreq_get().
Signed-off-by: Gautham R. Shenoy e...@linux.vnet.ibm.com
---
drivers/cpufreq/powernv-cpufreq.c | 38 ++
1 file changed, 38 insertions(+)
Please merge these fixups with the first patch which is creating the driver.
I understand that a different guy has created this patch and so wanted
to have a patch on his name but its really difficult to review this way.
Better add your signed-off in the first patch instead. Sending such changes
once the driver is mainlined looks fine.
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
index 46bee8a..ef6ed8c 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -206,6 +206,43 @@ static inline void set_pmspr(unsigned long sprn,
unsigned long val)
BUG();
}
+/*
+ * Computes the current frequency on this cpu
+ * and stores the result in *ret_freq.
+ */
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
You don't need cur_freq variable at all..
+ pmspr_val = get_pmspr(SPRN_PMSR);
+
+ /* The local pstate id corresponds bits 48..55 in the PMSR.
+ * Note: Watch out for the sign! */
+ local_pstate_id = (pmspr_val 48) 0xFF;
+ pstate_id = local_pstate_id;
similarly local_pstate_id
+
+ freq = pstate_id_to_freq(pstate_id);
+ pr_debug(cpu %d pmsr %lx pstate_id %d frequency %d \n,
+ smp_processor_id(), pmspr_val, pstate_id, freq);
+ *cur_freq = freq;
Move above print here after a blank line. Also remove freq variable as
well and use *cur_freq directly.. And then you can rename it to freq as well.
+}
+
+/*
+ * Returns the cpu frequency as reported by the firmware for 'cpu'.
+ * This value is reported through the sysfs file cpuinfo_cur_freq.
+ */
+unsigned int powernv_cpufreq_get(unsigned int cpu)
+{
+ int ret_freq;
+
+ smp_call_function_any(cpu_sibling_mask(cpu), powernv_read_cpu_freq,
+ ret_freq, 1);
+ return ret_freq;
+}
+
static void set_pstate(void *pstate)
{
unsigned long val;
@@ -297,6 +334,7 @@ static int powernv_cpufreq_target(struct cpufreq_policy
*policy,
static struct cpufreq_driver powernv_cpufreq_driver = {
.verify = powernv_cpufreq_verify,
.target = powernv_cpufreq_target,
+ .get= powernv_cpufreq_get,
.init = powernv_cpufreq_cpu_init,
.exit = powernv_cpufreq_cpu_exit,
.name = powernv-cpufreq,
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-pm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper
Hi Geoff, Do you have these in a git branch somewhere so I can merge and test them? No, sorry, I don't have a public git repo. Let me see how I can fix that, it might be useful for future patchsets. Please try that : git pull git://github.com/legoater/linux.git zimage Thanks, C. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 4/5] powernv:cpufreq: Export nominal frequency via sysfs.
On Fri, Mar 21, 2014 at 02:17:28PM +0530, Viresh Kumar wrote:
+static ssize_t show_cpuinfo_nominal_freq(struct cpufreq_policy *policy,
+ char *buf)
+{
+ int nominal_freq;
+ nominal_freq =
pstate_id_to_freq(powernv_pstate_info.pstate_nominal_id);
+ return sprintf(buf, %u\n, nominal_freq);
return sprintf(buf, %u\n,
pstate_id_to_freq(powernv_pstate_info.pstate_nominal_id));
Sure. Will fix this.
??
+}
+
+
remove extra blank line.
+struct freq_attr cpufreq_freq_attr_cpuinfo_nominal_freq = {
+ .attr = { .name = cpuinfo_nominal_freq,
+ .mode = 0444,
+ },
Align {}
Probably the use of ATTR_RO(cpuinfo_nominal_freq) and renaming
show_cpuinfo_nominal_freq to cpuinfo_nominal_freq_show() would be even
better. What do you think ?
+ .show = show_cpuinfo_nominal_freq,
+};
+
+
static struct freq_attr *powernv_cpu_freq_attr[] = {
cpufreq_freq_attr_scaling_available_freqs,
+ cpufreq_freq_attr_cpuinfo_nominal_freq,
NULL,
};
This needs to be rewritten to include the entries of cpufreq_generic_attr.
Thanks for the review.
--
Regards
gautham.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/5] powernv, cpufreq:Add per-core locking to serialize frequency transitions
On 03/21/2014 02:12 PM, Viresh Kumar wrote: On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy e...@linux.vnet.ibm.com wrote: From: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com On POWER systems, the CPU frequency is controlled at a core-level and hence we need to serialize so that only one of the threads in the core switches the core's frequency at a time. Probably you don't need this anymore. https://lkml.org/lkml/2014/3/21/23 Agreed. Regards, Srivatsa S. Bhat ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 4/5] powernv:cpufreq: Export nominal frequency via sysfs.
On 21 March 2014 15:25, Gautham R Shenoy e...@linux.vnet.ibm.com wrote: Probably the use of ATTR_RO(cpuinfo_nominal_freq) and renaming show_cpuinfo_nominal_freq to cpuinfo_nominal_freq_show() would be even better. What do you think ? +1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
Hi Viresh,
On Fri, Mar 21, 2014 at 02:11:32PM +0530, Viresh Kumar wrote:
On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy
e...@linux.vnet.ibm.com wrote:
From: Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com
Hi Vaidy,
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 4b029c0..4ba1632 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -48,6 +48,7 @@ config CPU_FREQ_STAT_DETAILS
choice
prompt Default CPUFreq governor
default CPU_FREQ_DEFAULT_GOV_USERSPACE if ARM_SA1100_CPUFREQ ||
ARM_SA1110_CPUFREQ
+ default CPU_FREQ_DEFAULT_GOV_ONDEMAND if POWERNV_CPUFREQ
Probably we should remove SA1100's entry as well from here. This is
not the right way of doing it. Imagine 100 platforms having entries here.
If you want it, then select it from your platforms Kconfig.
Sure. Will move these bits and the other governor related bits to the
Powernv Kconfig.
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
new file mode 100644
index 000..ab1551f
--- /dev/null
+
+#define pr_fmt(fmt)powernv-cpufreq: fmt
+
+#include linux/module.h
+#include linux/cpufreq.h
+#include linux/of.h
+#include asm/cputhreads.h
That's it? Sure?
Even if things compile for you, you must explicitly include all the
files on which
you depend.
Ok.
+
+ WARN_ON(len_ids != len_freqs);
+ nr_pstates = min(len_ids, len_freqs) / sizeof(u32);
+ WARN_ON(!nr_pstates);
Why do you want to continue here?
Good point. We might be better off exiting at this point.
+ 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].driver_data = i;
I don't think you are using this field at all and this is the field you can
use for driver_data and so you can get rid of powernv_pstate_ids[ ].
Using driver_data to record powernv_pstate_ids won't work since
powernv_pstate_ids can be negative. So a pstate_id -3 can be confused
with CPUFREQ_BOOST_FREQ thereby not displaying the frequency
corresponding to pstate id -3. So for now I think we will be retaining
powernv_pstate_ids.
+ powernv_freqs[i].frequency = freq * 1000; /* kHz */
+ powernv_pstate_ids[i] = id;
+ }
+ /* End of list marker entry */
+ powernv_freqs[i].driver_data = 0;
Not required.
Ok.
+ powernv_freqs[i].frequency = CPUFREQ_TABLE_END;
+
+ /* Print frequency table */
+ for (i = 0; powernv_freqs[i].frequency != CPUFREQ_TABLE_END; i++)
+ pr_debug(%d: %d\n, i, powernv_freqs[i].frequency);
You have already printed this table earlier..
Fair enough.
+
+ return 0;
+}
+
+static struct freq_attr *powernv_cpu_freq_attr[] = {
+ cpufreq_freq_attr_scaling_available_freqs,
+ NULL,
+};
Can use this instead: cpufreq_generic_attr?
In this patch yes. But later patch introduces an additional attribute
for displaying the nominal frequency. Will handle that part in a clean
way in the next version.
+/* Helper routines */
+
+/* Access helpers to power mgt SPR */
+
+static inline unsigned long get_pmspr(unsigned long sprn)
Looks big enough not be inlined?
It is called from one function. It has been defined separately for
readability.
+{
+ switch (sprn) {
+ case SPRN_PMCR:
+ return mfspr(SPRN_PMCR);
+
+ case SPRN_PMICR:
+ return mfspr(SPRN_PMICR);
+
+ case SPRN_PMSR:
+ return mfspr(SPRN_PMSR);
+ }
+ BUG();
+}
+
+static inline void set_pmspr(unsigned long sprn, unsigned long val)
+{
Same here..
Same reason as above.
+ switch (sprn) {
+ case SPRN_PMCR:
+ mtspr(SPRN_PMCR, val);
+ return;
+
+ case SPRN_PMICR:
+ mtspr(SPRN_PMICR, val);
+ return;
+
+ case SPRN_PMSR:
+ mtspr(SPRN_PMSR, val);
+ return;
+ }
+ BUG();
+}
+
+static void set_pstate(void *pstate)
+{
+ unsigned long val;
+ unsigned long pstate_ul = *(unsigned long *) pstate;
Why not sending value only to this routine instead of pointer?
Well this function is called via an smp_call_function. so, cannot send
a value :(
+
+ val = get_pmspr(SPRN_PMCR);
+ val = val 0xULL;
Maybe a blank line here?
Ok.
+ /* Set both global(bits 56..63) and local(bits 48..55) PStates */
+ val = val | (pstate_ul 56) | (pstate_ul 48);
here as well?
Ok.
+ pr_debug(Setting cpu %d pmcr to %016lX\n, smp_processor_id(),
val);
+
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On 21 March 2014 16:13, Gautham R Shenoy e...@linux.vnet.ibm.com wrote:
On Fri, Mar 21, 2014 at 02:11:32PM +0530, Viresh Kumar wrote:
+ pr_debug(PState id %d freq %d MHz\n, id, freq);
+ powernv_freqs[i].driver_data = i;
I don't think you are using this field at all and this is the field you can
use for driver_data and so you can get rid of powernv_pstate_ids[ ].
Using driver_data to record powernv_pstate_ids won't work since
powernv_pstate_ids can be negative. So a pstate_id -3 can be confused
with CPUFREQ_BOOST_FREQ thereby not displaying the frequency
corresponding to pstate id -3. So for now I think we will be retaining
powernv_pstate_ids.
As I said earlier, this field is only used by cpufreq drivers and cpufreq core
isn't supposed to touch it. CPUFREQ_BOOST_FREQ and other macros
are there for .frequency field and not this one.
+static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+ int base, i;
+
+#ifdef CONFIG_SMP
What will break if you don't have this ifdef here? Without that as well
below code should work?
Missed this one?
+ base = cpu_first_thread_sibling(policy-cpu);
+
+ for (i = 0; i threads_per_core; i++)
+ cpumask_set_cpu(base + i, policy-cpus);
+#endif
+ policy-cpuinfo.transition_latency = 25000;
+
+ policy-cur = powernv_freqs[0].frequency;
+ cpufreq_frequency_table_get_attr(powernv_freqs, policy-cpu);
This doesn't exist anymore.
Didn't get this comment!
cpufreq_frequency_table_get_attr() routine doesn't exist anymore.
+static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+ cpufreq_frequency_table_put_attr(policy-cpu);
+ return 0;
+}
You don't need this..
Why not ?
Because cpufreq_frequency_table_get_attr() and
cpufreq_frequency_table_put_attr() don't exist anymore :)
+module_init(powernv_cpufreq_init);
+module_exit(powernv_cpufreq_exit);
Place these right after the routines without any blank lines in
between.
Is this the new convention ?
Don't know I have been following this since long time, probably from the
time I started with Mainline stuff.. I have seen many maintainers advising this
as it make code more readable, specially if these routines are quite big..
Probably it isn't mentioned in coding guidelines but people follow it most of
the times.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On Fri, Mar 21, 2014 at 03:01:29PM +0530, Viresh Kumar wrote:
On Thu, Mar 20, 2014 at 5:41 PM, Gautham R. Shenoy
e...@linux.vnet.ibm.com wrote:
From: Gautham R. Shenoy e...@linux.vnet.ibm.com
The current frequency of a cpu is reported through the sysfs file
cpuinfo_cur_freq. This requires the driver to implement a
-get(unsigned int cpu) method which will return the current
operating frequency.
Implement a function named powernv_cpufreq_get() which reads the local
pstate from the PMSR and returns the corresponding frequency.
Set the powernv_cpufreq_driver.get hook to powernv_cpufreq_get().
Signed-off-by: Gautham R. Shenoy e...@linux.vnet.ibm.com
---
drivers/cpufreq/powernv-cpufreq.c | 38
++
1 file changed, 38 insertions(+)
Please merge these fixups with the first patch which is creating the driver.
I understand that a different guy has created this patch and so wanted
to have a patch on his name but its really difficult to review this
way.
Heh! Well, that wasn't the reason why this was sent out as a separate
patch, but never mind. Though I don't understand why it would be
difficult to review the patch though.
Better add your signed-off in the first patch instead. Sending such changes
once the driver is mainlined looks fine.
Sure, this makes sense.
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
index 46bee8a..ef6ed8c 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -206,6 +206,43 @@ static inline void set_pmspr(unsigned long sprn,
unsigned long val)
BUG();
}
+/*
+ * Computes the current frequency on this cpu
+ * and stores the result in *ret_freq.
+ */
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
You don't need cur_freq variable at all..
I don't like it either. But the compiler complains without this hack
:-(
+ pmspr_val = get_pmspr(SPRN_PMSR);
+
+ /* The local pstate id corresponds bits 48..55 in the PMSR.
+ * Note: Watch out for the sign! */
+ local_pstate_id = (pmspr_val 48) 0xFF;
+ pstate_id = local_pstate_id;
similarly local_pstate_id
well, I am interested in the bits 48..55 of pmspr_val. That's the
pstate_id which can be negative. So I'll like to keep
local_pstate_id.
+
+ freq = pstate_id_to_freq(pstate_id);
+ pr_debug(cpu %d pmsr %lx pstate_id %d frequency %d \n,
+ smp_processor_id(), pmspr_val, pstate_id, freq);
+ *cur_freq = freq;
Move above print here after a blank line. Also remove freq variable as
well and use *cur_freq directly.. And then you can rename it to freq
as well.
We can get rid of freq and use *cur_freq in its place.
Thanks for the detailed review.
--
Regards
gautham.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] mtd: m25p80: Modify the name of mtd_info
To specify spi flash layouts by mtdparts=... in cmdline, we must
give mtd_info a fixed name,because the cmdlinepart's parser will
match the name of mtd_info given in cmdline.
Now, if use DT, the mtd_info's name will be spi-dev-name. It
consists of spi_master-bus_num, and the spi_master-bus_num maybe
dynamically fetched. So, in this case, replace the component bus_num
with thei physical address of spi master.
Signed-off-by: Hou Zhiqiang b48...@freescale.com
---
drivers/mtd/devices/m25p80.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7eda71d..64450a2 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -30,6 +30,7 @@
#include linux/mtd/cfi.h
#include linux/mtd/mtd.h
#include linux/mtd/partitions.h
+#include linux/of_address.h
#include linux/of_platform.h
#include linux/spi/spi.h
@@ -934,9 +935,11 @@ static int m25p_probe(struct spi_device *spi)
struct flash_platform_data *data;
struct m25p *flash;
struct flash_info *info;
- unsignedi;
+ unsignedi, ret;
struct mtd_part_parser_data ppdata;
struct device_node *np = spi-dev.of_node;
+ struct resource res;
+ struct device_node *mnp = spi-master-dev.of_node;
/* Platform data helps sort out which chip type we have, as
* well as how this board partitions it. If we don't have
@@ -1009,8 +1012,17 @@ static int m25p_probe(struct spi_device *spi)
if (data data-name)
flash-mtd.name = data-name;
- else
- flash-mtd.name = dev_name(spi-dev);
+ else{
+ ret = of_address_to_resource(mnp, 0, res);
+ if (ret) {
+ dev_err(spi-dev, failed to get spi master
resource\n);
+ return ret;
+ }
+ flash-mtd.name = kasprintf(GFP_KERNEL, spi%x.%d,
+ (unsigned)res.start, spi-chip_select);
+ if (!flash-mtd.name)
+ return -ENOMEM;
+ }
flash-mtd.type = MTD_NORFLASH;
flash-mtd.writesize = 1;
--
1.8.5
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On 03/21/2014 04:24 PM, Viresh Kumar wrote:
On 21 March 2014 16:13, Gautham R Shenoy e...@linux.vnet.ibm.com wrote:
On Fri, Mar 21, 2014 at 02:11:32PM +0530, Viresh Kumar wrote:
+ pr_debug(PState id %d freq %d MHz\n, id, freq);
+ powernv_freqs[i].driver_data = i;
[...]
+static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+ int base, i;
+
+#ifdef CONFIG_SMP
What will break if you don't have this ifdef here? Without that as well
below code should work?
Missed this one?
Nothing will break, its just that the code size will be a tiny bit
lesser on UP configs, that's all :-) Anyway, I think removing the ifdef
improves the readability (and doesn't add any noticeable overhead on UP
kernels), so let's get rid of it.
Regards,
Srivatsa S. Bhat
+ base = cpu_first_thread_sibling(policy-cpu);
+
+ for (i = 0; i threads_per_core; i++)
+ cpumask_set_cpu(base + i, policy-cpus);
+#endif
+ policy-cpuinfo.transition_latency = 25000;
+
+ policy-cur = powernv_freqs[0].frequency;
+ cpufreq_frequency_table_get_attr(powernv_freqs, policy-cpu);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On 03/21/2014 02:11 PM, Viresh Kumar wrote:
On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy
e...@linux.vnet.ibm.com wrote:
From: Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com
Hi Vaidy,
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 4b029c0..4ba1632 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -48,6 +48,7 @@ config CPU_FREQ_STAT_DETAILS
choice
[...]
+static struct cpufreq_driver powernv_cpufreq_driver = {
+ .verify = powernv_cpufreq_verify,
+ .target = powernv_cpufreq_target,
I think you have Srivatsa there who has seen lots of cpufreq code and
could have helped you a lot :)
:-)
I have followed the locking and synchronization aspects of cpufreq
closely, but unfortunately, I haven't been able to keep up with some of
the other details that well :-( Besides, its hard to keep up with the
rate of your changes, you are way too fast! ;-)
Regards,
Srivatsa S. Bhat
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On 21 March 2014 16:34, Gautham R Shenoy e...@linux.vnet.ibm.com wrote:
Heh! Well, that wasn't the reason why this was sent out as a separate
patch, but never mind. Though I don't understand why it would be
difficult to review the patch though.
Because the initial driver wasn't complete earlier. There were 2-3 patches
after the first one which are changing what the first patch has added.
Nothing else :)
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
You don't need cur_freq variable at all..
I don't like it either. But the compiler complains without this hack
:-(
Why would the compiler warn for doing this?:
*(int *)ret_freq = freq;
+ pmspr_val = get_pmspr(SPRN_PMSR);
+
+ /* The local pstate id corresponds bits 48..55 in the PMSR.
+ * Note: Watch out for the sign! */
+ local_pstate_id = (pmspr_val 48) 0xFF;
+ pstate_id = local_pstate_id;
similarly local_pstate_id
well, I am interested in the bits 48..55 of pmspr_val. That's the
pstate_id which can be negative. So I'll like to keep
local_pstate_id.
Can you please explain at bit level how getting the value in a s8
first and then into a s32 will help you here? Instead of getting it
directly into s32.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On 21 March 2014 17:15, Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com wrote: I think you have Srivatsa there who has seen lots of cpufreq code and could have helped you a lot :) :-) I was waiting for your reply here :) I have followed the locking and synchronization aspects of cpufreq closely, but unfortunately, I haven't been able to keep up with some of the other details that well :-( Besides, its hard to keep up with the rate of your changes, you are way too fast! ;-) Haha.. I am a very slow guy, really :) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
From: Viresh Kumar
On 21 March 2014 16:34, Gautham R Shenoy e...@linux.vnet.ibm.com wrote:
Heh! Well, that wasn't the reason why this was sent out as a separate
patch, but never mind. Though I don't understand why it would be
difficult to review the patch though.
Because the initial driver wasn't complete earlier. There were 2-3 patches
after the first one which are changing what the first patch has added.
Nothing else :)
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
You don't need cur_freq variable at all..
I don't like it either. But the compiler complains without this hack
:-(
Why would the compiler warn for doing this?:
*(int *)ret_freq = freq;
Because it is very likely to be wrong.
In general casts of pointers to integer types are dangerous.
In this case why not make the function return the value?
David
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure
Hi Anshuman, On 21/03/14 03:44, Anshuman Khandual wrote: On 03/10/2014 04:42 PM, Sudeep Holla wrote: Hi Anshuman, On 07/03/14 06:14, Anshuman Khandual wrote: On 03/07/2014 09:36 AM, Anshuman Khandual wrote: On 02/19/2014 09:36 PM, Sudeep Holla wrote: From: Sudeep Holla sudeep.ho...@arm.com This patch removes the redundant sysfs cacheinfo code by making use of the newly introduced generic cacheinfo infrastructure. [...] When it is UNIFIED we return index 0, which is correct. But the index for instruction and data cache seems to be swapped which wrong. This will fetch invalid properties for any given cache type. Ah, that's silly mistake on my side, will fix it. I have done some initial review and testing for this patch's impact on PowerPC (ppc64 POWER specifically). I am trying to do some code clean-up and re-arrangements. Will post out soon. Thanks ! Thanks for taking time for testing and reviewing these patches. Now that you got some of the problems to work on and resend the patches, I will hold on to the clean up patches I had. I have done most of the changes but still unable to find why the shared_cpu_map is getting incorrect on PPC. All the other wrong entries are fixed. Any clue on shared_cpu_map ? Regards, Sudeep ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On Fri, Mar 21, 2014 at 12:01:07PM +, David Laight wrote:
From: Viresh Kumar
On 21 March 2014 16:34, Gautham R Shenoy e...@linux.vnet.ibm.com wrote:
Heh! Well, that wasn't the reason why this was sent out as a separate
patch, but never mind. Though I don't understand why it would be
difficult to review the patch though.
Because the initial driver wasn't complete earlier. There were 2-3 patches
after the first one which are changing what the first patch has added.
Nothing else :)
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
You don't need cur_freq variable at all..
I don't like it either. But the compiler complains without this hack
:-(
Why would the compiler warn for doing this?:
*(int *)ret_freq = freq;
Because it is very likely to be wrong.
In general casts of pointers to integer types are dangerous.
In this case why not make the function return the value?
Because this function is called via an smp_call_function(). And we
need a way of returning the value to the caller.
David
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On 21 March 2014 17:31, David Laight david.lai...@aculab.com wrote: *(int *)ret_freq = freq; Because it is very likely to be wrong. In general casts of pointers to integer types are dangerous. Where are we converting pointers to integers? We are doing a cast from 'void * ' to 'int *' and then using indirection operator to set its value. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On Fri, Mar 21, 2014 at 05:15:34PM +0530, Viresh Kumar wrote:
On 21 March 2014 16:34, Gautham R Shenoy e...@linux.vnet.ibm.com wrote:
Heh! Well, that wasn't the reason why this was sent out as a separate
patch, but never mind. Though I don't understand why it would be
difficult to review the patch though.
Because the initial driver wasn't complete earlier. There were 2-3 patches
after the first one which are changing what the first patch has added.
Nothing else :)
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
You don't need cur_freq variable at all..
I don't like it either. But the compiler complains without this hack
:-(
Why would the compiler warn for doing this?:
*(int *)ret_freq = freq;
The compiler doesn't complain if I do this.
+ pmspr_val = get_pmspr(SPRN_PMSR);
+
+ /* The local pstate id corresponds bits 48..55 in the PMSR.
+ * Note: Watch out for the sign! */
+ local_pstate_id = (pmspr_val 48) 0xFF;
+ pstate_id = local_pstate_id;
similarly local_pstate_id
well, I am interested in the bits 48..55 of pmspr_val. That's the
pstate_id which can be negative. So I'll like to keep
local_pstate_id.
Can you please explain at bit level how getting the value in a s8
first and then into a s32 will help you here? Instead of getting it
directly into s32.
Consider the case when pmspr = 0x00fe;
We are interested in extracting the value 'fe'. And ensure that when
we store this value into an int, we get the sign extension right.
So the following doesn't work:
pstate_id = (pmspr_val 48) 0x;
Since we get pstate_id = 0x00fe, which is not the same as
0xfffe.
Of course, we could do the following, but I wasn't sure if
two levels of type casting helped on the readability front.
pstate_id = (int) ((s8)((pmspr_val 48) 0xFF));
--
Thanks and Regards
gautham.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On 21 March 2014 18:34, Gautham R Shenoy e...@linux.vnet.ibm.com wrote: Consider the case when pmspr = 0x00fe; We are interested in extracting the value 'fe'. And ensure that when we store this value into an int, we get the sign extension right. So the following doesn't work: pstate_id = (pmspr_val 48) 0x; What about pstate_id = (pmspr_val 48) 0xFF; ?? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
Hi Viresh,
On Fri, Mar 21, 2014 at 04:24:27PM +0530, Viresh Kumar wrote:
On 21 March 2014 16:13, Gautham R Shenoy e...@linux.vnet.ibm.com wrote:
On Fri, Mar 21, 2014 at 02:11:32PM +0530, Viresh Kumar wrote:
+ pr_debug(PState id %d freq %d MHz\n, id, freq);
+ powernv_freqs[i].driver_data = i;
I don't think you are using this field at all and this is the field you can
use for driver_data and so you can get rid of powernv_pstate_ids[ ].
Using driver_data to record powernv_pstate_ids won't work since
powernv_pstate_ids can be negative. So a pstate_id -3 can be confused
with CPUFREQ_BOOST_FREQ thereby not displaying the frequency
corresponding to pstate id -3. So for now I think we will be retaining
powernv_pstate_ids.
As I said earlier, this field is only used by cpufreq drivers and cpufreq core
isn't supposed to touch it. CPUFREQ_BOOST_FREQ and other macros
are there for .frequency field and not this one.
Ok, I had based my code on linus's git tree. I checked the 'pm-cpufreq'
branch of Rafael's 'linux-pm' tree and freq_table.c contains the
following code snippet in show_available_frequencies:
if (show_boost ^ (table[i].driver_data == CPUFREQ_BOOST_FREQ))
continue;
I suspect we're talking about different code bases. So could you
please tell me which one should I be looking at ?
+static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+ int base, i;
+
+#ifdef CONFIG_SMP
What will break if you don't have this ifdef here? Without that as well
below code should work?
Missed this one?
+ base = cpu_first_thread_sibling(policy-cpu);
+
+ for (i = 0; i threads_per_core; i++)
+ cpumask_set_cpu(base + i, policy-cpus);
+#endif
+ policy-cpuinfo.transition_latency = 25000;
+
+ policy-cur = powernv_freqs[0].frequency;
+ cpufreq_frequency_table_get_attr(powernv_freqs, policy-cpu);
This doesn't exist anymore.
Didn't get this comment!
cpufreq_frequency_table_get_attr() routine doesn't exist anymore.
+static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+ cpufreq_frequency_table_put_attr(policy-cpu);
+ return 0;
+}
You don't need this..
Why not ?
Because cpufreq_frequency_table_get_attr() and
cpufreq_frequency_table_put_attr() don't exist anymore :)
Well, it does in the codebases that I was looking at. May be I've been
looking at the wrong place.
+module_init(powernv_cpufreq_init);
+module_exit(powernv_cpufreq_exit);
Place these right after the routines without any blank lines in
between.
Is this the new convention ?
Don't know I have been following this since long time, probably from the
time I started with Mainline stuff.. I have seen many maintainers advising
this
as it make code more readable, specially if these routines are quite big..
Probably it isn't mentioned in coding guidelines but people follow it most of
the times.
Ok. I was not aware of this. Good to know :-)
--
Thanks and Regards
gautham.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On 21 March 2014 18:53, Gautham R Shenoy e...@linux.vnet.ibm.com wrote: Ok, I had based my code on linus's git tree. I checked the 'pm-cpufreq' Always check his bleeding-edge or linux-next branch. branch of Rafael's 'linux-pm' tree and freq_table.c contains the following code snippet in show_available_frequencies: if (show_boost ^ (table[i].driver_data == CPUFREQ_BOOST_FREQ)) continue; I suspect we're talking about different code bases. So could you please tell me which one should I be looking at ? Okay, there is some problem here. I will check how can I simplify this.. And yes, you were correct. Because cpufreq_frequency_table_get_attr() and cpufreq_frequency_table_put_attr() don't exist anymore :) Well, it does in the codebases that I was looking at. May be I've been looking at the wrong place. Check the one I mentioned. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On Fri, Mar 21, 2014 at 06:42:50PM +0530, Viresh Kumar wrote: On 21 March 2014 18:34, Gautham R Shenoy e...@linux.vnet.ibm.com wrote: Consider the case when pmspr = 0x00fe; We are interested in extracting the value 'fe'. And ensure that when we store this value into an int, we get the sign extension right. So the following doesn't work: pstate_id = (pmspr_val 48) 0x; What about pstate_id = (pmspr_val 48) 0xFF; ?? Nope. Suppose the pmspr_val is contained in the register %rax, and pstate_id corresponds to the address -20(%rbp) then: pstate_id = (pmspr_val 48) 0xFF; would corrspond to shrq$48, %rax// Left shift by 48 bits andl$255, %eax // Mask the lower 32 bits of %rax with 0x00FF movl%eax, -20(%rbp) // Store the lower 32 bits of %rax into pstate_id On the other hand, pstate_id = (int)((s8)((pmspr_val 48) 0xFF)); would correspond to: shrq$48, %rax // Left shift by 48 bits. movsbl %al, %eax // Move the lower 8 bits of %rax to %eax with sign-extension. movl%eax, -20(%rbp) // store the result in pstate_id; So with this, we are getting the sign extension due to the use of movsbl. And if local_pstate_id corresponds to the address -1(%rbp) then: local_pstate_id = (pmspr_val 48) 0xFF; pstate_id = local_pstate_id; would correspond to: shrq$48, %rax// Left shift by 48 bits movb%al, -1(%rbp)//copy the lower 8 bits to local_pstate_id movsbl -1(%rbp), %eax //move the contents of local_pstate_id to %eax with sign extension. movl%eax, -20(%rbp) // Store the result in pstate_id Hope this helps :-) -- Thanks and Regards gautham. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
* Gautham R Shenoy e...@linux.vnet.ibm.com [2014-03-21 16:13:17]:
Hi Viresh,
On Fri, Mar 21, 2014 at 02:11:32PM +0530, Viresh Kumar wrote:
On Thu, Mar 20, 2014 at 5:40 PM, Gautham R. Shenoy
e...@linux.vnet.ibm.com wrote:
From: Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com
Hi Vaidy,
Hi Viresh,
Thanks for the detailed review.
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 4b029c0..4ba1632 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -48,6 +48,7 @@ config CPU_FREQ_STAT_DETAILS
choice
prompt Default CPUFreq governor
default CPU_FREQ_DEFAULT_GOV_USERSPACE if ARM_SA1100_CPUFREQ ||
ARM_SA1110_CPUFREQ
+ default CPU_FREQ_DEFAULT_GOV_ONDEMAND if POWERNV_CPUFREQ
Probably we should remove SA1100's entry as well from here. This is
not the right way of doing it. Imagine 100 platforms having entries here.
If you want it, then select it from your platforms Kconfig.
Sure. Will move these bits and the other governor related bits to the
Powernv Kconfig.
diff --git a/drivers/cpufreq/powernv-cpufreq.c
b/drivers/cpufreq/powernv-cpufreq.c
new file mode 100644
index 000..ab1551f
--- /dev/null
+
+#define pr_fmt(fmt)powernv-cpufreq: fmt
+
+#include linux/module.h
+#include linux/cpufreq.h
+#include linux/of.h
+#include asm/cputhreads.h
That's it? Sure?
Even if things compile for you, you must explicitly include all the
files on which
you depend.
Ok.
+
+ WARN_ON(len_ids != len_freqs);
+ nr_pstates = min(len_ids, len_freqs) / sizeof(u32);
+ WARN_ON(!nr_pstates);
Why do you want to continue here?
Good point. We might be better off exiting at this point.
Yes, we could return here. We do not generally come till this point
if the platform has a problem discovering PStates from device tree.
But there is no useful debug info after this point if nr_pstates is 0.
+ 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].driver_data = i;
I don't think you are using this field at all and this is the field you can
use for driver_data and so you can get rid of powernv_pstate_ids[ ].
Using driver_data to record powernv_pstate_ids won't work since
powernv_pstate_ids can be negative. So a pstate_id -3 can be confused
with CPUFREQ_BOOST_FREQ thereby not displaying the frequency
corresponding to pstate id -3. So for now I think we will be retaining
powernv_pstate_ids.
Yeah, I had the driver written using driver_data to store pstates.
Gautham found the bug that we are missing one PState when we match the
ID with CPUFREQ_BOOST_FREQ!
We did not know that you have taken care of those issues. Ideally
I did expect that driver_data should not be touched by the framework.
Thanks for fixing that and allowing the back-end driver to use
driver_data.
+ powernv_freqs[i].frequency = freq * 1000; /* kHz */
+ powernv_pstate_ids[i] = id;
+ }
+ /* End of list marker entry */
+ powernv_freqs[i].driver_data = 0;
Not required.
Ok.
+ powernv_freqs[i].frequency = CPUFREQ_TABLE_END;
+
+ /* Print frequency table */
+ for (i = 0; powernv_freqs[i].frequency != CPUFREQ_TABLE_END; i++)
+ pr_debug(%d: %d\n, i, powernv_freqs[i].frequency);
You have already printed this table earlier..
Fair enough.
+
+ return 0;
+}
+
+static struct freq_attr *powernv_cpu_freq_attr[] = {
+ cpufreq_freq_attr_scaling_available_freqs,
+ NULL,
+};
Can use this instead: cpufreq_generic_attr?
In this patch yes. But later patch introduces an additional attribute
for displaying the nominal frequency. Will handle that part in a clean
way in the next version.
+/* Helper routines */
+
+/* Access helpers to power mgt SPR */
+
+static inline unsigned long get_pmspr(unsigned long sprn)
Looks big enough not be inlined?
It is called from one function. It has been defined separately for
readability.
Let the compiler decide. The code is straight forward :)
I wanted to keep this SPR operations in a separate function to
facilitate debug and also introduce any timing/sequence change here if
required. Keeping this separate is helpful, if inlining is
successful, we get a bonus :)
+{
+ switch (sprn) {
+ case SPRN_PMCR:
+ return mfspr(SPRN_PMCR);
+
+ case SPRN_PMICR:
+ return mfspr(SPRN_PMICR);
+
+ case SPRN_PMSR:
+ return mfspr(SPRN_PMSR);
+ }
+ BUG();
+}
+
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On Fri, Mar 21, 2014 at 04:24:27PM +0530, Viresh Kumar wrote:
+static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+ int base, i;
+
+ base = cpu_first_thread_sibling(policy-cpu);
+
+ for (i = 0; i threads_per_core; i++)
+ cpumask_set_cpu(base + i, policy-cpus);
+ policy-cpuinfo.transition_latency = 25000;
+
+ policy-cur = powernv_freqs[0].frequency;
+ cpufreq_frequency_table_get_attr(powernv_freqs, policy-cpu);
This doesn't exist anymore.
Ok, I guess the right thing to do at this point is call
cpufreq_table_validate_and_show(policy, powernv_freqs);
Will fix the code to take care of this.
--
Thanks and Regards
gautham.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
From: Viresh Kumar [mailto:viresh.ku...@linaro.org] On 21 March 2014 17:31, David Laight david.lai...@aculab.com wrote: *(int *)ret_freq = freq; Because it is very likely to be wrong. In general casts of pointers to integer types are dangerous. Where are we converting pointers to integers? We are doing a cast from 'void * ' to 'int *' and then using indirection operator to set its value. You mis-parsed what I wrote, try: In general casts of 'pointer to integer' types are dangerous. Somewhere, much higher up the call stack, the address of an integer variable is being taken and then passed as the 'void *' parameter. The 'problem' is that it is quite easily to pass the address of a 'long' instead. On 32bit and LE systems this won't always be a problem. On 64bit BE it all goes wrong. David ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper
Hi Cédric, On Thu, 2014-03-20 at 16:09 +0100, Cédric Le Goater wrote: The following patchset adds support for 64bit little endian boot wrapper. It is based on original code from Andrew Tauferner. I tested this on PS3 (64 bit BE) and found no regression. Tested by: Geoff Levand ge...@infradead.org ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] mtd: m25p80: Modify the name of mtd_info
On Fri, 2014-03-21 at 19:16 +0800, Hou Zhiqiang wrote:
@@ -1009,8 +1012,17 @@ static int m25p_probe(struct spi_device *spi)
if (data data-name)
flash-mtd.name = data-name;
- else
- flash-mtd.name = dev_name(spi-dev);
+ else{
Whitespace
+ ret = of_address_to_resource(mnp, 0, res);
+ if (ret) {
+ dev_err(spi-dev, failed to get spi master
resource\n);
+ return ret;
+ }
+ flash-mtd.name = kasprintf(GFP_KERNEL, spi%x.%d,
+ (unsigned)res.start, spi-chip_select);
Don't use unsigned by itself. Don't cast physical addresses to
unsigned int -- use unsigned long long.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper
On 03/21/2014 06:28 PM, Geoff Levand wrote: Hi Cédric, On Thu, 2014-03-20 at 16:09 +0100, Cédric Le Goater wrote: The following patchset adds support for 64bit little endian boot wrapper. It is based on original code from Andrew Tauferner. I tested this on PS3 (64 bit BE) and found no regression. Tested by: Geoff Levand ge...@infradead.org Thanks ! C. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/4] ARCH: AUDIT: implement syscall_get_arch for all arches
On 14/03/19, Eric Paris wrote:
For all arches which support audit implement syscall_get_arch()
They are all pretty easy and straight forward, stolen from how the call
to audit_syscall_entry() determines the arch.
Signed-off-by: Eric Paris epa...@redhat.com
Cc: linux-i...@vger.kernel.org
Cc: microblaze-ucli...@itee.uq.edu.au
Cc: linux-m...@linux-mips.org
Cc: li...@lists.openrisc.net
Cc: linux-par...@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: sparcli...@vger.kernel.org
Acked-by: Richard Guy Briggs r...@redhat.com
---
arch/ia64/include/asm/syscall.h | 6 ++
arch/microblaze/include/asm/syscall.h | 5 +
arch/mips/include/asm/syscall.h | 2 +-
arch/openrisc/include/asm/syscall.h | 5 +
arch/parisc/include/asm/syscall.h | 11 +++
arch/powerpc/include/asm/syscall.h| 12
arch/sparc/include/asm/syscall.h | 8
include/uapi/linux/audit.h| 1 +
8 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/arch/ia64/include/asm/syscall.h b/arch/ia64/include/asm/syscall.h
index a7ff1c6..1d0b875 100644
--- a/arch/ia64/include/asm/syscall.h
+++ b/arch/ia64/include/asm/syscall.h
@@ -13,6 +13,7 @@
#ifndef _ASM_SYSCALL_H
#define _ASM_SYSCALL_H 1
+#include uapi/linux/audit.h
#include linux/sched.h
#include linux/err.h
@@ -79,4 +80,9 @@ static inline void syscall_set_arguments(struct task_struct
*task,
ia64_syscall_get_set_arguments(task, regs, i, n, args, 1);
}
+
+static inline int syscall_get_arch(void)
+{
+ return AUDIT_ARCH_IA64;
+}
#endif /* _ASM_SYSCALL_H */
diff --git a/arch/microblaze/include/asm/syscall.h
b/arch/microblaze/include/asm/syscall.h
index 9bc4317..53cfaf3 100644
--- a/arch/microblaze/include/asm/syscall.h
+++ b/arch/microblaze/include/asm/syscall.h
@@ -1,6 +1,7 @@
#ifndef __ASM_MICROBLAZE_SYSCALL_H
#define __ASM_MICROBLAZE_SYSCALL_H
+#include uapi/linux/audit.h
#include linux/kernel.h
#include linux/sched.h
#include asm/ptrace.h
@@ -99,4 +100,8 @@ static inline void syscall_set_arguments(struct
task_struct *task,
asmlinkage long do_syscall_trace_enter(struct pt_regs *regs);
asmlinkage void do_syscall_trace_leave(struct pt_regs *regs);
+static inline int syscall_get_arch(void)
+{
+ return AUDIT_ARCH_MICROBLAZE;
+}
#endif /* __ASM_MICROBLAZE_SYSCALL_H */
diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
index fc556d8..992b6ab 100644
--- a/arch/mips/include/asm/syscall.h
+++ b/arch/mips/include/asm/syscall.h
@@ -103,7 +103,7 @@ extern const unsigned long sysn32_call_table[];
static inline int syscall_get_arch(void)
{
- int arch = EM_MIPS;
+ int arch = AUDIT_ARCH_MIPS;
#ifdef CONFIG_64BIT
arch |= __AUDIT_ARCH_64BIT;
#endif
diff --git a/arch/openrisc/include/asm/syscall.h
b/arch/openrisc/include/asm/syscall.h
index b752bb6..2db9f1c 100644
--- a/arch/openrisc/include/asm/syscall.h
+++ b/arch/openrisc/include/asm/syscall.h
@@ -19,6 +19,7 @@
#ifndef __ASM_OPENRISC_SYSCALL_H__
#define __ASM_OPENRISC_SYSCALL_H__
+#include uapi/linux/audit.h
#include linux/err.h
#include linux/sched.h
@@ -71,4 +72,8 @@ syscall_set_arguments(struct task_struct *task, struct
pt_regs *regs,
memcpy(regs-gpr[3 + i], args, n * sizeof(args[0]));
}
+static inline int syscall_get_arch(void)
+{
+ return AUDIT_ARCH_OPENRISC;
+}
#endif
diff --git a/arch/parisc/include/asm/syscall.h
b/arch/parisc/include/asm/syscall.h
index 8bdfd2c..a5eba95 100644
--- a/arch/parisc/include/asm/syscall.h
+++ b/arch/parisc/include/asm/syscall.h
@@ -3,6 +3,8 @@
#ifndef _ASM_PARISC_SYSCALL_H_
#define _ASM_PARISC_SYSCALL_H_
+#include uapi/linux/audit.h
+#include linux/compat.h
#include linux/err.h
#include asm/ptrace.h
@@ -37,4 +39,13 @@ static inline void syscall_get_arguments(struct
task_struct *tsk,
}
}
+static inline int syscall_get_arch(void)
+{
+ int arch = AUDIT_ARCH_PARISC;
+#ifdef CONFIG_64BIT
+ if (!is_compat_task())
+ arch = AUDIT_ARCH_PARISC64;
+#endif
+ return arch;
+}
#endif /*_ASM_PARISC_SYSCALL_H_*/
diff --git a/arch/powerpc/include/asm/syscall.h
b/arch/powerpc/include/asm/syscall.h
index b54b2ad..4271544 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -13,6 +13,8 @@
#ifndef _ASM_SYSCALL_H
#define _ASM_SYSCALL_H 1
+#include uapi/linux/audit.h
+#include linux/compat.h
#include linux/sched.h
/* ftrace syscalls requires exporting the sys_call_table */
@@ -86,4 +88,14 @@ static inline void syscall_set_arguments(struct
task_struct *task,
memcpy(regs-gpr[3 + i], args, n * sizeof(args[0]));
}
+static inline int syscall_get_arch(void)
+{
+ int arch = AUDIT_ARCH_PPC;
+
+#ifdef CONFIG_PPC64
+ if (!is_32bit_task())
+ arch = AUDIT_ARCH_PPC64;
+#endif
Re: [PATCH 4/4] ARCH: AUDIT: audit_syscall_entry() should not require the arch
On 14/03/19, Eric Paris wrote: We have a function where the arch can be queried, syscall_get_arch(). So rather than have every single piece of arch specific code use and/or duplicate syscall_get_arch(), just have the audit code use the syscall_get_arch() code. Signed-off-by: Eric Paris epa...@redhat.com Cc: linux-al...@vger.kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-i...@vger.kernel.org Cc: microblaze-ucli...@itee.uq.edu.au Cc: linux-m...@linux-mips.org Cc: li...@lists.openrisc.net Cc: linux-par...@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-s...@vger.kernel.org Cc: linux...@vger.kernel.org Cc: sparcli...@vger.kernel.org Cc: user-mode-linux-de...@lists.sourceforge.net Cc: linux-xte...@linux-xtensa.org Cc: x...@kernel.org Acked-by: Richard Guy Briggs r...@redhat.com --- arch/alpha/kernel/ptrace.c | 2 +- arch/arm/kernel/ptrace.c| 4 ++-- arch/ia64/kernel/ptrace.c | 2 +- arch/microblaze/kernel/ptrace.c | 3 +-- arch/mips/kernel/ptrace.c | 4 +--- arch/openrisc/kernel/ptrace.c | 3 +-- arch/parisc/kernel/ptrace.c | 9 +++-- arch/powerpc/kernel/ptrace.c| 7 ++- arch/s390/kernel/ptrace.c | 4 +--- arch/sh/kernel/ptrace_32.c | 14 +- arch/sh/kernel/ptrace_64.c | 17 + arch/sparc/kernel/ptrace_64.c | 9 ++--- arch/um/kernel/ptrace.c | 3 +-- arch/x86/kernel/ptrace.c| 8 ++-- arch/x86/um/asm/ptrace.h| 4 arch/xtensa/kernel/ptrace.c | 2 +- include/linux/audit.h | 7 --- 17 files changed, 25 insertions(+), 77 deletions(-) diff --git a/arch/alpha/kernel/ptrace.c b/arch/alpha/kernel/ptrace.c index 86d8351..d9ee817 100644 --- a/arch/alpha/kernel/ptrace.c +++ b/arch/alpha/kernel/ptrace.c @@ -321,7 +321,7 @@ asmlinkage unsigned long syscall_trace_enter(void) if (test_thread_flag(TIF_SYSCALL_TRACE) tracehook_report_syscall_entry(current_pt_regs())) ret = -1UL; - audit_syscall_entry(AUDIT_ARCH_ALPHA, regs-r0, regs-r16, regs-r17, regs-r18, regs-r19); + audit_syscall_entry(regs-r0, regs-r16, regs-r17, regs-r18, regs-r19); return ret ?: current_pt_regs()-r0; } diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c index 0dd3b79..c9d2b34 100644 --- a/arch/arm/kernel/ptrace.c +++ b/arch/arm/kernel/ptrace.c @@ -943,8 +943,8 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno) if (test_thread_flag(TIF_SYSCALL_TRACEPOINT)) trace_sys_enter(regs, scno); - audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs-ARM_r0, regs-ARM_r1, - regs-ARM_r2, regs-ARM_r3); + audit_syscall_entry(scno, regs-ARM_r0, regs-ARM_r1, regs-ARM_r2, + regs-ARM_r3); return scno; } diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c index b7a5fff..6f54d51 100644 --- a/arch/ia64/kernel/ptrace.c +++ b/arch/ia64/kernel/ptrace.c @@ -1219,7 +1219,7 @@ syscall_trace_enter (long arg0, long arg1, long arg2, long arg3, ia64_sync_krbs(); - audit_syscall_entry(AUDIT_ARCH_IA64, regs.r15, arg0, arg1, arg2, arg3); + audit_syscall_entry(regs.r15, arg0, arg1, arg2, arg3); return 0; } diff --git a/arch/microblaze/kernel/ptrace.c b/arch/microblaze/kernel/ptrace.c index 39cf508..bb10637 100644 --- a/arch/microblaze/kernel/ptrace.c +++ b/arch/microblaze/kernel/ptrace.c @@ -147,8 +147,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) */ ret = -1L; - audit_syscall_entry(EM_MICROBLAZE, regs-r12, regs-r5, regs-r6, - regs-r7, regs-r8); + audit_syscall_entry(regs-r12, regs-r5, regs-r6, regs-r7, regs-r8); return ret ?: regs-r12; } diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c index 65ba622..c06bb82 100644 --- a/arch/mips/kernel/ptrace.c +++ b/arch/mips/kernel/ptrace.c @@ -671,9 +671,7 @@ asmlinkage void syscall_trace_enter(struct pt_regs *regs) if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT))) trace_sys_enter(regs, regs-regs[2]); - audit_syscall_entry(syscall_get_arch(), - regs-regs[2], - regs-regs[4], regs-regs[5], + audit_syscall_entry(regs-regs[2], regs-regs[4], regs-regs[5], regs-regs[6], regs-regs[7]); } diff --git a/arch/openrisc/kernel/ptrace.c b/arch/openrisc/kernel/ptrace.c index 71a2a0c..4f59fa4 100644 --- a/arch/openrisc/kernel/ptrace.c +++ b/arch/openrisc/kernel/ptrace.c @@ -187,8 +187,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs) */ ret = -1L; - audit_syscall_entry(AUDIT_ARCH_OPENRISC, regs-gpr[11], - regs-gpr[3], regs-gpr[4], +
Re: [PATCH 9/9] powerpc/pm: support deep sleep feature on T1040
On Fri, 2014-03-21 at 09:21 +, David Laight wrote: From: Scott Wood [mailto:scottw...@freescale.com] On Thu, 2014-03-20 at 11:59 +, David Laight wrote: I tried to work out what the 'twi, isync' instructions were for (in in_le32()). The best I could come up with was to ensure a synchronous bus-fault. But bus faults are probably only expected during device probing - not normal operation, and the instructions will have a significant cost. Additionally in_le32() and out_le32() both start with a 'sync' instruction. In many cases that isn't needed either - an explicit iosync() can be used after groups of instructions. The idea is that it's better to be maximally safe by default, and let performance critical sections be optimized using raw accessors and explicit synchronization if needed, than to have hard-to-debug bugs due to missing/wrong sync. A lot of I/O is slow enough that the performance impact doesn't really matter, but the brain-time cost of getting the sync right is still there. Hmmm That might be an excuse for the 'sync', but not the twi and isync. That might be true if I/O is always cache inhibited and guarded, in which case I think we can rely on that to ensure that the load has completed before we do things like wrtee or rfi. In any case, I'd want to hear Ben's explanation. I was setting up a dma request (for the ppc 83xx PCIe bridge) and was doing back to back little-endian writes into memory. I had difficulty finding and including header files containing the definitions for byteswapped accesses I needed. arch/powerpc/include/asm/swab.h contains some - but I couldn't work out how to get it included (apart from giving the full path). In any case you need to understand when synchronisation is required - otherwise you will get it wrong. Especially since non-byteswapped accesses are done by direct access. Yes, it's bad that rawness combines the lack of byteswapping with the lack of synchronization. Ideally the raw accessors would also come in big and little endian form, plus a native endian form if it's really needed. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 5/5] powernv:cpufreq: Implement the driver-get() method
On Fri, 2014-03-21 at 16:34 +0530, Gautham R Shenoy wrote:
+/*
+ * Computes the current frequency on this cpu
+ * and stores the result in *ret_freq.
+ */
+static void powernv_read_cpu_freq(void *ret_freq)
+{
+ unsigned long pmspr_val;
+ s8 local_pstate_id;
+ int *cur_freq, freq, pstate_id;
+
+ cur_freq = (int *)ret_freq;
You don't need cur_freq variable at all..
I don't like it either. But the compiler complains without this hack
:-(
Casting integers into void * is a recipe for disaster... what is that
supposed to be about ? We lose all type checking and get exposed
to endian issues etc... the day somebody uses a different type on both
sides.
Also is freq a frequency ? In this case an int isn't big enough.
Cheers,
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[RFC][PATCH] perf: Add 'merge-recursive' callchain option
From 9ad9432dab2bf4d1c8e6ff9201e88d5ae9f3994a Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu suka...@linux.vnet.ibm.com
Date: Wed, 19 Mar 2014 20:24:22 -0500
Subject: [PATCH 1/1] perf: Add 'merge-recursive' callchain option
Powerpc saves the link register (LR) with each sample to help resolve
callchains for programs involving tail calls. Sometimes the value in the
LR is same as the NIP register. Other times it is not. This results in
callchains samples like these:
3547953334790 0x1ec0 [0x78]: PERF_RECORD_SAMPLE(IP, 2): 4667/4667: 0x80a7be3c58
period: 1773985 addr: 0
... chain: nr:9
. 0: fe00
. 1: 0080a7be3c58 __random
. 2: 0080a7be3c40 __random
. 3: 0080a7be4270 rand
. 4: 1784 do_my_sprintf
. 5: 19d8 main
. 6: 0080a7bc482c
. 7: 0080a7bc4a34
. 8:
... thread: sprintft:4667
.. dso: /usr/lib64/libc-2.18.so
67470723107802 0x2f10 [0x78]: PERF_RECORD_SAMPLE(IP, 0x2): 5706/5706:
0x80a7be3c20 period: 872629 addr: 0
... chain: nr:9
. 0: fe00
. 1: 0080a7be3c20 __random
. 2: 0080a7be4270 rand
. 3: 0080a7be4270 rand
. 4: 1784
. 5: 19d8
. 6: 0080a7bc482c
. 7: 0080a7bc4a34
. 8:
... thread: sprintft:5706
.. dso: /usr/lib64/libc-2.18.so
67470738362072 0x4cf8 [0x78]: PERF_RECORD_SAMPLE(IP, 0x2): 5706/5706:
0x80a7be3c14 period: 869774 addr: 0
... chain: nr:9
. 0: fe00
. 1: 0080a7be3c14 __random
. 2: 0080a7be4270 rand
. 3: 0080a7be4270 rand
. 4: 1784 do_my_sprintf
. 5: 19d8 main
. 6: 0080a7bc482c
. 7: 0080a7bc4a34
. 8:
... thread: sprintft:5706
.. dso: /usr/lib64/libc-2.18.so
In the perf tool, these samples end up on different branches of the RB-Tree
resulting in duplicated arcs in the final call-graph.
15.02% sprintft libc-2.18.so [.] __random
|
--- __random
|
|--58.45%-- rand
| rand
| do_my_sprintf
| main
| generic_start_main.isra.0
| __libc_start_main
| 0x0
|
--41.55%-- __random
rand
do_my_sprintf
main
generic_start_main.isra.0
__libc_start_main
0x0
This is an RFC for adding a 'merge-recursive' call-graph option that would
discard the duplicate entries resulting in a more compact call-graph. The
duplicates can be either identical instruction pointer values or IP values
within the same function.
perf report --call-graph=fractal,0.5,callee,function,merge-recursive
15.02% sprintft libc-2.18.so [.] __random
|
---__random
rand
do_my_sprintf
main
generic_start_main.isra.0
__libc_start_main
(nil)
This option could also be used to collapse call-graphs of recursive functions.
AFAICT, the existing CCKEY_FUNCTION does not address this case because
the callchain lengths can vary across samples.
Signed-off-by: Sukadev Bhattiprolu suka...@linux.vnet.ibm.com
---
tools/perf/builtin-report.c | 14 --
tools/perf/util/callchain.h |1 +
tools/perf/util/machine.c | 39 +++
3 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index d882b6f..0b68749 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -647,6 +647,16 @@ parse_callchain_opt(const struct option *opt, const char
*arg, int unset)
callchain_param.key = CCKEY_ADDRESS;
else
return -1;
+
+ tok2 = strtok(NULL, ,);
+ if (!tok2)
+ goto setup;
+
+ if (!strncmp(tok2, merge-recursive, strlen(merge-recursive)))
+ callchain_param.merge_recursive = 1;
+ else
+ return -1;
+
setup:
if (callchain_register_param(callchain_param) 0) {
pr_err(Can't register callchain params\n);
@@ -762,8 +772,8 @@ int cmd_report(int argc, const char **argv, const char
*prefix __maybe_unused)
regex filter to identify parent, see: '--sort parent'),
OPT_BOOLEAN('x', exclude-other, symbol_conf.exclude_other,
Only display entries with parent-match),
- OPT_CALLBACK_DEFAULT('g', call-graph, report,
output_type,min_percent[,print_limit],call_order,
-Display callchains using output_type (graph, flat,
Re: Tasks stuck in futex code (in 3.14-rc6)
So reverting and applying v3 3/4 and 4/4 patches works for me. Ok, I verified that the above endds up resulting in the same tree as the minimal patch I sent out, modulo (a) some comments and (b) an #ifdef CONFIG_SMP in futex_get_mm() that doesn't really matter. So I committed the minimal patch with your tested-by. Just to be sure, I have verified with latest mainline with HEAD having commit 08edb33c4 (Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux) which also has the commit 11d4616bd0 futex:( revert back to the explicit waiter counting code). -- Thanks and Regards Srikar Dronamraju ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Tasks stuck in futex code (in 3.14-rc6)
On Sat, 2014-03-22 at 07:57 +0530, Srikar Dronamraju wrote: So reverting and applying v3 3/4 and 4/4 patches works for me. Ok, I verified that the above endds up resulting in the same tree as the minimal patch I sent out, modulo (a) some comments and (b) an #ifdef CONFIG_SMP in futex_get_mm() that doesn't really matter. So I committed the minimal patch with your tested-by. Just to be sure, I have verified with latest mainline with HEAD having commit 08edb33c4 (Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux) which also has the commit 11d4616bd0 futex:( revert back to the explicit waiter counting code). Thanks for double checking. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On 21 March 2014 20:18, Vaidyanathan Srinivasan sva...@linux.vnet.ibm.com wrote: Yeah, I had the driver written using driver_data to store pstates. Gautham found the bug that we are missing one PState when we match the ID with CPUFREQ_BOOST_FREQ! I see.. We did not know that you have taken care of those issues. Ideally I did expect that driver_data should not be touched by the framework. Thanks for fixing that and allowing the back-end driver to use driver_data. No, I haven't fixed anything yet. And this piece of code still exists. I will see if I can get this fixed, by that time you can continue the way your code is there in this version. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/5] powernv: cpufreq driver for powernv platform
On 21 March 2014 20:24, Gautham R Shenoy e...@linux.vnet.ibm.com wrote: Ok, I guess the right thing to do at this point is call cpufreq_table_validate_and_show(policy, powernv_freqs); Will fix the code to take care of this. Yes. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
