* [PATCH 3/3][RFC] ftrace/ppc: Use patch_instruction instead of probe_kernel_write()
From: Steven Rostedt @ 2012-04-26 18:31 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev
In-Reply-To: <20120426183116.857877522@goodmis.org>
From: Steven Rostedt <srostedt@redhat.com>
The patch_instruction() interface is made to modify kernel text. It is
safer to use that then the probe_kernel_write() when modifying kernel
code.
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
arch/powerpc/kernel/ftrace.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 84a5ddd..4e376bc 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -63,11 +63,9 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
return -EINVAL;
/* replace the text with the new text */
- if (probe_kernel_write((void *)ip, &new, MCOUNT_INSN_SIZE))
+ if (patch_instruction((unsigned int *)ip, new))
return -EPERM;
- flush_icache_range(ip, ip + 8);
-
return 0;
}
@@ -212,12 +210,9 @@ __ftrace_make_nop(struct module *mod,
*/
op = 0x48000008; /* b +8 */
- if (probe_kernel_write((void *)ip, &op, MCOUNT_INSN_SIZE))
+ if (patch_instruction((unsigned int *)ip, op))
return -EPERM;
-
- flush_icache_range(ip, ip + 8);
-
return 0;
}
@@ -286,11 +281,9 @@ __ftrace_make_nop(struct module *mod,
op = PPC_INST_NOP;
- if (probe_kernel_write((void *)ip, &op, MCOUNT_INSN_SIZE))
+ if (patch_instruction((unsigned int *)ip, op))
return -EPERM;
- flush_icache_range(ip, ip + 8);
-
return 0;
}
#endif /* PPC64 */
@@ -426,11 +419,9 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
pr_devel("write to %lx\n", rec->ip);
- if (probe_kernel_write((void *)ip, &op, MCOUNT_INSN_SIZE))
+ if (patch_instruction((unsigned int *)ip, op))
return -EPERM;
- flush_icache_range(ip, ip + 8);
-
return 0;
}
#endif /* CONFIG_PPC64 */
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/3][RFC] powerpc: Have patch_instruction detect faults
From: Steven Rostedt @ 2012-04-26 18:31 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev
In-Reply-To: <20120426183116.857877522@goodmis.org>
From: Steven Rostedt <srostedt@redhat.com>
For ftrace to use the patch_instruction code, it needs to check for
faults on write. Ftrace updates code all over the kernel, and we need to
know if code is updated or not due to protections that are placed on
some portions of the kernel. If ftrace does not detect a fault, it will
error later on, and it will be much more difficult to find the problem.
By changing patch_instruction() to detect faults, then ftrace will be
able to make use of it too.
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
arch/powerpc/include/asm/code-patching.h | 4 ++--
arch/powerpc/lib/code-patching.c | 14 ++++++++++----
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 37c32aba..a6f8c7a 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -26,8 +26,8 @@ unsigned int create_branch(const unsigned int *addr,
unsigned long target, int flags);
unsigned int create_cond_branch(const unsigned int *addr,
unsigned long target, int flags);
-void patch_branch(unsigned int *addr, unsigned long target, int flags);
-void patch_instruction(unsigned int *addr, unsigned int instr);
+int patch_branch(unsigned int *addr, unsigned long target, int flags);
+int patch_instruction(unsigned int *addr, unsigned int instr);
int instr_is_relative_branch(unsigned int instr);
int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr);
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 7c975d4..dd223b3 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -13,17 +13,23 @@
#include <linux/mm.h>
#include <asm/page.h>
#include <asm/code-patching.h>
+#include <asm/uaccess.h>
-void patch_instruction(unsigned int *addr, unsigned int instr)
+int patch_instruction(unsigned int *addr, unsigned int instr)
{
- *addr = instr;
+ int err;
+
+ err = __put_user(instr, addr);
+ if (err)
+ return err;
asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (addr));
+ return 0;
}
-void patch_branch(unsigned int *addr, unsigned long target, int flags)
+int patch_branch(unsigned int *addr, unsigned long target, int flags)
{
- patch_instruction(addr, create_branch(addr, target, flags));
+ return patch_instruction(addr, create_branch(addr, target, flags));
}
unsigned int create_branch(const unsigned int *addr,
--
1.7.9.5
^ permalink raw reply related
* [PATCH 0/3][RFC] powerpc/ftrace: Removal of stop machine (and other goodies)
From: Steven Rostedt @ 2012-04-26 18:31 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev
Benjamin,
You once told me on IRC that powerpc has no problem with modifying
code on one CPU that may be executing on another CPU. With the tests I
made on my PPC64 (2 CPUs) box, it seems to be the case.
The first patch removes stop_machine from powerpc. The other patches
add some error handling if ftrace detects an update didn't occur
with 'patch_instruction'.
This is just an RFC, but if it's fine, feel free to pull them into
your tree.
-- Steve
Steven Rostedt (3):
ftrace/ppc: Have PPC skip updating with stop_machine()
powerpc: Have patch_instruction detect faults
ftrace/ppc: Use patch_instruction instead of probe_kernel_write()
----
arch/powerpc/include/asm/code-patching.h | 4 +-
arch/powerpc/kernel/ftrace.c | 69 ++++++++++++++++++++++++------
arch/powerpc/lib/code-patching.c | 14 ++++--
3 files changed, 68 insertions(+), 19 deletions(-)
^ permalink raw reply
* [PATCH 1/3][RFC] ftrace/ppc: Have PPC skip updating with stop_machine()
From: Steven Rostedt @ 2012-04-26 18:31 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev
In-Reply-To: <20120426183116.857877522@goodmis.org>
From: Steven Rostedt <srostedt@redhat.com>
PPC does not have the synchronization issues that x86 has with
modifying code on one CPU while another CPU is executing it.
The other CPU will either see the old or new code without any
issues, unlike x86 which may issue a GPF.
Instead of calling the heavy stop_machine, just update the code.
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
arch/powerpc/kernel/ftrace.c | 52 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index bf99cfa..84a5ddd 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -484,6 +484,58 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
return ret;
}
+static int __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
+{
+ unsigned long ftrace_addr = (unsigned long)FTRACE_ADDR;
+ int ret;
+
+ ret = ftrace_update_record(rec, enable);
+
+ switch (ret) {
+ case FTRACE_UPDATE_IGNORE:
+ return 0;
+ case FTRACE_UPDATE_MAKE_CALL:
+ return ftrace_make_call(rec, ftrace_addr);
+ case FTRACE_UPDATE_MAKE_NOP:
+ return ftrace_make_nop(NULL, rec, ftrace_addr);
+ }
+
+ return 0;
+}
+
+static void ftrace_replace_code(int enable)
+{
+ struct ftrace_rec_iter *iter;
+ struct dyn_ftrace *rec;
+ int ret;
+
+ for (iter = ftrace_rec_iter_start(); iter;
+ iter = ftrace_rec_iter_next(iter)) {
+ rec = ftrace_rec_iter_record(iter);
+ ret = __ftrace_replace_code(rec, enable);
+ if (ret) {
+ ftrace_bug(ret, rec->ip);
+ return;
+ }
+ }
+}
+
+void arch_ftrace_update_code(int command)
+{
+ if (command & FTRACE_UPDATE_CALLS)
+ ftrace_replace_code(1);
+ else if (command & FTRACE_DISABLE_CALLS)
+ ftrace_replace_code(0);
+
+ if (command & FTRACE_UPDATE_TRACE_FUNC)
+ ftrace_update_ftrace_func(ftrace_trace_function);
+
+ if (command & FTRACE_START_FUNC_RET)
+ ftrace_enable_ftrace_graph_caller();
+ else if (command & FTRACE_STOP_FUNC_RET)
+ ftrace_disable_ftrace_graph_caller();
+}
+
int __init ftrace_dyn_arch_init(void *data)
{
/* caller expects data to be zero */
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2] powerpc/85xx: Enable MTD/NOR/NAND options by default in defconfig
From: Scott Wood @ 2012-04-26 19:43 UTC (permalink / raw)
To: Shengzhou Liu; +Cc: linux-mtd, linuxppc-dev
In-Reply-To: <1335436983-21360-1-git-send-email-Shengzhou.Liu@freescale.com>
On 04/26/2012 05:43 AM, Shengzhou Liu wrote:
> +CONFIG_MTD_NAND_VERIFY_WRITE=y
I don't think we want this on by default.
-Scott
^ permalink raw reply
* Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates
From: Benjamin Herrenschmidt @ 2012-04-26 22:06 UTC (permalink / raw)
To: Andreas Schwab; +Cc: khali, linuxppc-dev
In-Reply-To: <m2ehra67p5.fsf@igel.home>
On Thu, 2012-04-26 at 13:41 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>
> > In fact, can you change that define around and see if it makes it behave
> > more like therm_pm72 overall ? IE That's the only -known- difference
> > between the old and new driver (+/- a bug / typo / etc..)
>
> Yes, that's make the difference for the cpu fan control. Note that
> MacOS (at least 10.3, which is the only one I have) drives the fans the
> same way as the old driver.
Ok, I'll switch that back then. It seemed more sensible to read the
actual fan values rather than the programmed ones (in fact I wonder if I
can just skip the read alltogether then and use a cached value but that
means I won't be able to detect failed fans...), but if you say it
behaves better, let's keep it the way it was.
As for the tickle, I'm not sure yet how to proceed. I'll look into it,
try various things. We can maybe just remove the tickle but that means
that a completely idle machine might start ramping up as the FCU times
out.
Finally, setting the slot fan from sysfs should be doable reasonably
easily. Stay tuned and thanks a lot for testing ! :-)
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/9] cpu: Introduce clear_tasks_mm_cpumask() helper
From: Andrew Morton @ 2012-04-26 23:59 UTC (permalink / raw)
To: Anton Vorontsov
Cc: linaro-kernel, Mike Frysinger, Peter Zijlstra,
user-mode-linux-devel, linux-sh, Richard Weinberger, linuxppc-dev,
Oleg Nesterov, linux-kernel, linux-mm, Paul Mundt, John Stultz,
patches, KOSAKI Motohiro, Russell King, uclinux-dist-devel,
linux-arm-kernel
In-Reply-To: <20120423070736.GA30752@lizard>
On Mon, 23 Apr 2012 00:07:36 -0700
Anton Vorontsov <anton.vorontsov@linaro.org> wrote:
> Many architectures clear tasks' mm_cpumask like this:
>
> read_lock(&tasklist_lock);
> for_each_process(p) {
> if (p->mm)
> cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
> }
> read_unlock(&tasklist_lock);
>
> Depending on the context, the code above may have several problems,
> such as:
>
> 1. Working with task->mm w/o getting mm or grabing the task lock is
> dangerous as ->mm might disappear (exit_mm() assigns NULL under
> task_lock(), so tasklist lock is not enough).
>
> 2. Checking for process->mm is not enough because process' main
> thread may exit or detach its mm via use_mm(), but other threads
> may still have a valid mm.
>
> This patch implements a small helper function that does things
> correctly, i.e.:
>
> 1. We take the task's lock while whe handle its mm (we can't use
> get_task_mm()/mmput() pair as mmput() might sleep);
>
> 2. To catch exited main thread case, we use find_lock_task_mm(),
> which walks up all threads and returns an appropriate task
> (with task lock held).
>
> Also, Per Peter Zijlstra's idea, now we don't grab tasklist_lock in
> the new helper, instead we take the rcu read lock. We can do this
> because the function is called after the cpu is taken down and marked
> offline, so no new tasks will get this cpu set in their mm mask.
>
Seems reasonable.
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -179,6 +179,7 @@ extern void put_online_cpus(void);
> #define hotcpu_notifier(fn, pri) cpu_notifier(fn, pri)
> #define register_hotcpu_notifier(nb) register_cpu_notifier(nb)
> #define unregister_hotcpu_notifier(nb) unregister_cpu_notifier(nb)
> +void clear_tasks_mm_cpumask(int cpu);
> int cpu_down(unsigned int cpu);
>
> #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 2060c6e..ecdf499 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -10,6 +10,8 @@
> #include <linux/sched.h>
> #include <linux/unistd.h>
> #include <linux/cpu.h>
> +#include <linux/oom.h>
> +#include <linux/rcupdate.h>
> #include <linux/export.h>
> #include <linux/kthread.h>
> #include <linux/stop_machine.h>
> @@ -171,6 +173,30 @@ void __ref unregister_cpu_notifier(struct notifier_block *nb)
> }
> EXPORT_SYMBOL(unregister_cpu_notifier);
>
> +void clear_tasks_mm_cpumask(int cpu)
The operation of this function was presumably obvious to you at the
time you wrote it, but that isn't true of other people at later times.
Please document it?
> +{
> + struct task_struct *p;
> +
> + /*
> + * This function is called after the cpu is taken down and marked
> + * offline,
hm, well. Who said that this function will only ever be called
after that CPU was taken down? There is nothing in the function name
nor in the (absent) documentation which enforces this precondition.
If someone tries to use this function for a different purpose, or
copies-and-modifies it for a different purpose, we just shot them in
the foot.
They'd be pretty dumb to do that without reading the local comment,
but still...
> so its not like new tasks will ever get this cpu set in
> + * their mm mask. -- Peter Zijlstra
> + * Thus, we may use rcu_read_lock() here, instead of grabbing
> + * full-fledged tasklist_lock.
> + */
> + rcu_read_lock();
> + for_each_process(p) {
> + struct task_struct *t;
> +
> + t = find_lock_task_mm(p);
> + if (!t)
> + continue;
> + cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
> + task_unlock(t);
> + }
> + rcu_read_unlock();
> +}
It is good that this code exists under CONFIG_HOTPLUG_CPU. Did you
check that everything works correctly with CONFIG_HOTPLUG_CPU=n?
^ permalink raw reply
* RE: [PATCH][2/3][RFC] TDM Framework
From: Aggrwal Poonam-B10812 @ 2012-04-27 2:07 UTC (permalink / raw)
To: Wood Scott-B07421; +Cc: linuxppc-dev@lists.ozlabs.org, Singh Sandeep-B37400
In-Reply-To: <4F95F523.50400@freescale.com>
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, April 24, 2012 6:05 AM
> To: Aggrwal Poonam-B10812
> Cc: linuxppc-dev@lists.ozlabs.org; Singh Sandeep-B37400
> Subject: Re: [PATCH][2/3][RFC] TDM Framework
>=20
Thanks Scott for the comments, we will incorporate them and send the next v=
ersion.
Regards
Poonam
> On 03/10/2012 06:57 AM, Poonam Aggrwal wrote:
> > diff --git a/drivers/Kconfig b/drivers/Kconfig index ad6c1eb..25f7f5b
> > 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -130,4 +130,5 @@ source "drivers/virt/Kconfig"
> >
> > source "drivers/net/dpa/NetCommSw/Kconfig"
> >
> > +source "drivers/tdm/Kconfig"
> > endmenu
>=20
> When posting patches to this list, please ensure that they are based on
> an upstream Linux kernel and not a Freescale BSP kernel.
Sure, but this was RFC patch set where the primary intention was to get des=
ign level comments.
But we will send the updated patches rebased on upstream head.
>=20
> > +if TDM
> > +
> > +config TDM_DEBUG_CORE
> > + bool "TDM Core debugging messages"
> > + help
> > + Say Y here if you want the TDM core to produce a bunch of debug
> > + messages to the system log. Select this if you are having a
> > + problem with TDM support and want to see more of what is going
> on.
> > +
> > +endif # TDM
>=20
> Please use the normal kernel mechanisms for controlling debug messages.
>=20
okay
> > diff --git a/drivers/tdm/tdm-core.c b/drivers/tdm/tdm-core.c new file
> > mode 100644 index 0000000..cdda260
> > --- /dev/null
> > +++ b/drivers/tdm/tdm-core.c
> > @@ -0,0 +1,1146 @@
> > +/* driver/tdm/tdm-core.c
> > + *
> > + * Copyright (C) 2012 Freescale Semiconductor, Inc, All rights
> reserved.
> > + *
> > + * TDM core is the interface between TDM clients and TDM devices.
> > + * It is also intended to serve as an interface for line controld
> > + * devices later on.
> > + *
> > + * Author:Hemant Agrawal <hemant@freescale.com>
> > + * Rajesh Gumasta <rajesh.gumasta@freescale.com>
> > + *
> > + * Modified by Sandeep Kr Singh <sandeep@freescale.com>
> > + * Poonam Aggarwal <poonam.aggarwal@freescale.com>
> > + * 1. Added framework based initialization of device.
> > + * 2. All the init/run time configuration is now done by framework.
> > + * 3. Added channel level operations.
> > + *
> > + * 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 of the License, 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > +along
> > + * with this program; if not, write to the Free Software Foundation,
> > +Inc.,
> > + * 675 Mass Ave, Cambridge, MA 02139, USA.
> > + */
> > +
> > +/* if read write debug required */
> > +#undef TDM_CORE_DEBUG
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/slab.h>
> > +#include <linux/tdm.h>
> > +#include <linux/init.h>
> > +#include <linux/idr.h>
> > +#include <linux/mutex.h>
> > +#include <linux/completion.h>
> > +#include <linux/hardirq.h>
> > +#include <linux/irqflags.h>
> > +#include <linux/list.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/io.h>
> > +#include "device/tdm_fsl.h"
>=20
> What is a reference to tdm_fsl.h doing in the presumably hardware-
> independent tdm-core.c?
Need to check that, ideally this include should not be required
>=20
> > +static DEFINE_MUTEX(tdm_core_lock);
> > +static DEFINE_IDR(tdm_adapter_idr);
> > +/* List of TDM adapters registered with TDM framework */
> > +LIST_HEAD(adapter_list);
> > +
> > +/* List of TDM clients registered with TDM framework */
> > +LIST_HEAD(driver_list);
> > +
> > +/* In case the previous data is not fetched by the client driver, the
> > + * de-interleaving function will discard the old data and rewrite
> > +the
> > + * new data */
> > +static int use_latest_tdm_data =3D 1;
>=20
> Describe when one would want to change this, and provide a better way to
> change it (e.g. module parameter or sysfs knob).
>=20
Ok, description can be added.
Will also explore the sysfs knob option. =
=
=
=20
> /*
> * Linux kernel
> * multi-line comment style
> * is like this.
> */
>=20
Sure
> > +/* this tasklet is created for each adapter instance */ static void
> > +tdm_data_tasklet_fn(unsigned long);
> > +
> > +/* tries to match client driver with the adapter */ static int
> > +tdm_device_match(struct tdm_driver *driver, struct tdm_adapter *adap)
> > +{
> > + /* match on an id table if there is one */
> > + if (driver->id_table && driver->id_table->name[0]) {
> > + if (!(strcmp(driver->id_table->name, adap->name)))
> > + return (int)driver->id_table;
> > + }
> > + return TDM_E_OK;
> > +}
>=20
> s/TDM_E_OK/0/g
>=20
ok
> > +static int tdm_attach_driver_adap(struct tdm_driver *driver,
> > + struct tdm_adapter *adap)
> > +{
> > + /* if driver is already attached to any other adapter, return*/
> > + if (driver->adapter && (driver->adapter !=3D adap))
> > + return TDM_E_OK;
>=20
> Why can't one driver service multiple adapters? How would multiple
> drivers service one adapter? Are there sub-devices that need individual
> controlling?
>=20
Driver is here client driver which would be using=20
> > + driver->adapter =3D adap;
> > +
> > + if (driver->attach_adapter) {
> > + if (driver->attach_adapter(adap) < 0)
> > + /* We ignore the return code; if it fails, too bad */
> > + pr_err("attach_adapter failed for driver [%s]\n",
> > + driver->name);
>=20
> Why ignore errors?
This will be fixed.
>=20
> > +/* Detach client driver and adapter */ static int
> > +tdm_detach_driver_adap(struct tdm_driver *driver,
> > + struct tdm_adapter *adap)
> > +{
> > + int res =3D TDM_E_OK;
> > +
> > + if (!driver->adapter || (driver->adapter !=3D adap))
> > + return TDM_E_OK;
>=20
> Shouldn't this be an error?
Will check this.
>=20
> > + if (!driver->detach_adapter)
> > + return TDM_E_OK;
> > +
> > + adap->drv_count--;
>=20
> If the driver doesn't have a detach_adapter method, you skip decrementing
> the count and leave the tasklet lying around?
>=20
This has been fixed in later version of the patch which we made for FSL BSP=
.
Will fix this here also.
> > +/* TDM adapter Registration/De-registration with TDM framework */
> > +
> > +static int tdm_register_adapter(struct tdm_adapter *adap) {
> > + int res =3D TDM_E_OK;
> > + struct tdm_driver *driver, *next;
> > +
> > + if (!adap) {
> > + pr_err("%s:Invalid handle\n", __func__);
> > + return -EINVAL;
> > + }
>=20
> Pointers aren't handles. The caller should not be passing NULL, and it
> would be more useful to crash and get a backtrace if it does. It's not
> realistic to check every pointer for being NULL when there's no
> legitimate reason it could be NULL, and it doesn't help you if you have
> some other bad value besides NULL.
Okay, we will check this.
>=20
> > + mutex_init(&adap->adap_lock);
> > + INIT_LIST_HEAD(&adap->myports);
> > + spin_lock_init(&adap->portlist_lock);
> > +
> > + adap->drv_count =3D 0;
> > + adap->tasklet_conf =3D 0;
> > +
> > + list_add_tail(&adap->list, &adapter_list);
> > +
> > + /* initialization of driver by framework in default configuration
> */
> > + init_config_adapter(adap);
> > +
> > + /* Notify drivers */
> > + pr_info("adapter [%s] registered\n", adap->name);
>=20
> This is too noisy. You haven't even gotten a match yet.
>=20
It is telling that these adapters have registered to the framework.
> > +/*
> > + * tdm_add_adapter - declare tdm adapter, use dynamic device number
>=20
> Why are there device numbers at all?
>=20
> I suspect there's a fair bit of copy and paste going on of another
> subsystem's quirks (i2c?). I don't see any mention in the copyright
> block of this code having been derived from anything else, though...
>=20
When we initially started, we took i2c as an example. But later not much co=
uld be used as-is.
But yes some initial stuff is from i2c. Will update it in copyright section=
.
> > +
> > +
> > +/**
> > + * tdm_del_adapter - unregister TDM adapter
> > + * @adap: the adapter being unregistered
> > + *
> > + * This unregisters an TDM adapter which was previously registered
> > + * by @tdm_add_adapter.
> > + */
> > +int tdm_del_adapter(struct tdm_adapter *adap) {
> > + int res =3D TDM_E_OK;
> > + struct tdm_adapter *found;
> > + struct tdm_driver *driver, *next;
> > +
> > + if (!adap) {
> > + pr_err("%s:Invalid handle\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + /* First make sure that this adapter was ever added */
> > + mutex_lock(&tdm_core_lock);
> > + found =3D idr_find(&tdm_adapter_idr, adap->id);
> > + mutex_unlock(&tdm_core_lock);
> > + if (found !=3D adap) {
> > + pr_err("tdm-core: attempting to delete unregistered "
> > + "adapter [%s]\n", adap->name);
> > + return -EINVAL;
> > + }
> > +
> > + /*disable and kill the data processing tasklet */
> > + if (adap->tasklet_conf) {
> > + tasklet_disable(&adap->tdm_data_tasklet);
> > + tasklet_kill(&adap->tdm_data_tasklet);
> > + adap->tasklet_conf =3D 0;
> > + }
> > +
> > + /* Detach any active ports. This can't fail, thus we do not
> > + checking the returned value. */
> > + mutex_lock(&tdm_core_lock);
> > + list_for_each_entry_safe(driver, next, &driver_list, list) {
> > + if (tdm_device_match(driver, adap)) {
> > + tdm_detach_driver_adap(driver, adap);
> > + pr_info(
> > + "Driver(ID=3D%d) is detached from Adapter %s(ID =3D %d)\n",
> > + driver->id, adap->name, adap->id);
> > + }
> > + }
> > + mutex_unlock(&tdm_core_lock);
> > +
> > + mutex_lock(&tdm_core_lock);
> > + idr_remove(&tdm_adapter_idr, adap->id);
> > + mutex_unlock(&tdm_core_lock);
>=20
> Why are you dropping the lock then reacquiring it?
>=20
Mistake, will fix it.
> > +/* TDM Client Drivers Registration/De-registration Functions */ int
> > +tdm_register_driver(struct tdm_driver *driver) {
> > + int res =3D TDM_E_OK;
> > + struct tdm_adapter *adap, *next;
> > +
> > + list_add_tail(&driver->list, &driver_list);
> > +
> > + mutex_lock(&tdm_core_lock);
> > + /* Walk the adapters that are already present */
> > + list_for_each_entry_safe(adap, next, &adapter_list, list) {
> > + if (tdm_device_match(driver, adap)) {
> > + res =3D tdm_attach_driver_adap(driver, adap);
> > + pr_info("TDM Driver(ID=3D%d)is attached with Adapter"
> > + "%s(ID =3D %d) drv_count=3D%d", driver->id,
> > + adap->name, adap->id, adap->drv_count);
> > + break;
> > + }
> > + }
>=20
> Indentation.
>=20
> > + mutex_unlock(&tdm_core_lock);
> > +
> > + return res;
> > +}
> > +EXPORT_SYMBOL(tdm_register_driver);
> > +
> > +/*
> > + * tdm_unregister_driver - unregister TDM client driver from TDM
> > +framework
> > + * @driver: the driver being unregistered */ void
> > +tdm_unregister_driver(struct tdm_driver *driver) {
> > + if (!driver) {
> > + pr_err("%s:Invalid handle\n", __func__);
> > + return;
> > + }
> > + /* A driver can register to only one adapter,
> > + * so no need to browse the list */
>=20
> Whitespace.
>=20
> > + mutex_lock(&tdm_core_lock);
> > + tdm_detach_driver_adap(driver, driver->adapter);
> > + mutex_unlock(&tdm_core_lock);
> > +
> > + list_del(&driver->list);
> > +
> > + pr_debug("tdm-core: driver [%s] unregistered\n", driver->name); }
> > +EXPORT_SYMBOL(tdm_unregister_driver);
> > +
> > +/* TDM Framework init and exit */
> > +static int __init tdm_init(void)
> > +{
> > + pr_info("%s\n", __func__);
> > + return TDM_E_OK;
> > +}
> > +
> > +static void __exit tdm_exit(void)
> > +{
> > + pr_info("%s\n", __func__);
> > + return;
> > +}
>=20
> Don't spam the console just because the driver got loaded or unloaded (at
> this point you haven't even found the hardware).
>=20
Ok
> > +/* We must initialize early, because some subsystems register tdm
> > +drivers
> > + * in subsys_initcall() code, but are linked (and initialized) before
> tdm.
> > + */
> > +postcore_initcall(tdm_init);
> > +module_exit(tdm_exit);
>=20
> tdm_init doesn't do anything, so why does it need to run early?
>=20
> > +/* Port Level APIs of TDM Framework */ unsigned int
> > +tdm_port_open(struct tdm_driver *driver, void **h_port)
>=20
> Why is the return unsigned int? You're returning negative numbers.
>=20
> Also consider having the return be a pointer, and use PTR_ERR/ERR_PTR --
> or at least put a proper type on h_port (what is the "h_"?).
>=20
> > +{
> > + struct tdm_port *port;
> > + struct tdm_adapter *adap;
> > + unsigned long flags;
> > + int res =3D TDM_E_OK;
> > +
> > + if (driver =3D=3D NULL) {
> > + pr_err("driver NULL\n");
> > + return -ENODEV;
> > + }
> > + if (driver->adapter =3D=3D NULL) {
> > + pr_err("adapter NULL\n");
> > + return -ENODEV;
> > + }
>=20
> Either make these pr_debug (or remove them), or make the message more
> specific.
>=20
> > + adap =3D tdm_get_adapter(driver->adapter->id);
> > + if (!adap)
> > + return -ENODEV;
> > +
> > + /* This creates an anonymous tdm_port, which may later be
> > + * pointed to some slot.
> > + *
> > + */
> > + port =3D kzalloc(sizeof(*port), GFP_KERNEL);
> > + if (!port) {
> > + res =3D -ENOMEM;
> > + goto out;
> > + }
> > +
> > + init_waitqueue_head(&port->ch_wait_queue);
> > +
> > +
> > + port->rx_max_frames =3D NUM_SAMPLES_PER_FRAME;
> > + port->port_cfg.port_mode =3D e_TDM_PORT_CHANNELIZED;
> > +
> > + port->in_use =3D 1;
>=20
> When would a port have in_use be false, other than this brief window
> where nothing else should be looking at it?
>=20
> > + list_for_each_entry_safe(channel, temp, &port->mychannels, list) {
> > + if (channel)
> > + if (channel->in_use) {
> > + pr_err("%s: Cannot close port. Channel in use\n",
> > + __func__);
> > + res =3D -ENXIO;
> > + goto out;
> > + }
> > + }
>=20
> Broken indentation.
>=20
> > +unsigned int tdm_channel_read(void *h_port, void *h_channel,
> > + void *p_data, u16 *size)
> > +{
> > + struct tdm_port *port;
> > + struct tdm_channel *channel;
> > + struct tdm_bd *rx_bd;
> > + unsigned long flags;
> > + int i, res =3D TDM_E_OK;
> > + unsigned short *buf, *buf1;
> > + port =3D (struct tdm_port *)h_port;
> > + channel =3D (struct tdm_channel *)h_channel;
>=20
> Unnecessary casts.
>=20
> > + if ((port && channel) =3D=3D 0) { /* invalid handle*/
>=20
> This is an odd construct -- how about "if (!port || !channel)"?
>=20
> > + pr_err("%s:Invalid Handle\n", __func__);
> > + return -ENXIO;
> > + }
> > +
> > + if (!port->in_use)
> > + return -EIO;
> > + if (!channel->p_ch_data || !channel->in_use)
> > + return -EIO;
> > +
> > + spin_lock_irqsave(&channel->p_ch_data->rx_channel_lock, flags);
>=20
> Shouldn't you test whether it's in use after you get the lock?
>=20
> > + rx_bd =3D channel->p_ch_data->rx_out_data;
> > +
> > + if (rx_bd->flag) {
> > + *size =3D rx_bd->length;
> > + buf =3D (u16 *) p_data;
> > + buf1 =3D (u16 *)rx_bd->p_data;
> > + for (i =3D 0; i < NUM_SAMPLES_PER_FRAME; i++)
> > + buf[i] =3D buf1[i];
> > + rx_bd->flag =3D 0;
> > + rx_bd->offset =3D 0;
> > + channel->p_ch_data->rx_out_data =3D (rx_bd->wrap) ?
> > + channel->p_ch_data->rx_data_fifo : rx_bd + 1;
> > +
> > + } else {
> > + spin_unlock_irqrestore(&channel->p_ch_data->rx_channel_lock,
> > + flags);
> > + pr_info("No Data Available");
> > + return -EAGAIN;
> > + }
>=20
> That pr_info() is inappropriate. This driver appears to be overly chatty
> in general (and with quite vague messages) -- or is this debug stuff that
> will be removed in a non-RFC patch?
>=20
> > +unsigned int tdm_port_get_stats(void *h_port, struct tdm_port_stats
> > +*portStat) {
> > + struct tdm_port *port;
> > + int port_num;
> > + port =3D (struct tdm_port *)h_port;
> > +
> > + if (port =3D=3D NULL || portStat =3D=3D NULL) { /* invalid handle*/
> > + pr_err("Invalid Handle");
> > + return -ENXIO;
> > + }
> > + port_num =3D port->port_id;
> > +
> > + memcpy(portStat, &port->port_stat, sizeof(struct tdm_port_stats));
> > +
> > + pr_info("TDM Port %d Get Stats", port_num);
> > +
> > + return TDM_E_OK;
> > +}
> > +EXPORT_SYMBOL(tdm_port_get_stats);
> > +
> > +/* Data handling functions */
> > +
> > +static int tdm_data_rx_deinterleave(struct tdm_adapter *adap) {
> > + struct tdm_port *port, *next;
> > + struct tdm_channel *channel, *temp;
> > + struct tdm_bd *ch_bd;
> > +
> > + int i, buf_size, ch_data_len;
> > + u16 *input_tdm_buffer;
> > + u16 *pcm_buffer;
> > + int slot_width;
> > + int frame_ch_data_size;
> > + bool ch_data;
> > + int bytes_in_fifo_per_frame;
> > + int bytes_slot_offset;
> > +
> > + ch_data_len =3D NUM_SAMPLES_PER_FRAME;
> > + frame_ch_data_size =3D NUM_SAMPLES_PER_FRAME;
> > + ch_data =3D 0;
> > +
> > + if (!adap) { /* invalid handle*/
> > + pr_err("%s: Invalid Handle\n", __func__);
> > + return -ENXIO;
> > + }
> > +
> > + slot_width =3D adap->adapt_cfg.slot_width;
> > + buf_size =3D tdm_adap_recv(adap, (void **)&input_tdm_buffer);
> > + if (buf_size <=3D 0 || !input_tdm_buffer)
> > + return -EINVAL;
> > +
> > + bytes_in_fifo_per_frame =3D buf_size/frame_ch_data_size;
> > + bytes_slot_offset =3D bytes_in_fifo_per_frame/slot_width;
> > +
> > + /* de-interleaving for all ports*/
> > + list_for_each_entry_safe(port, next, &adap->myports, list) {
> > +
> > + /* if the port is not open */
> > + if (!port->in_use)
> > + continue;
> > +
> > + list_for_each_entry_safe(channel, temp, &port->mychannels,
> > + list) {
> > + /* if the channel is not open */
> > + if (!channel->in_use || !channel->p_ch_data)
> > + continue;
> > + ch_bd =3D channel->p_ch_data->rx_in_data;
> > + spin_lock(&channel->p_ch_data->rx_channel_lock);
> > + /*if old data is to be discarded */
> > + if (use_latest_tdm_data)
> > + if (ch_bd->flag) {
> > + ch_bd->flag =3D 0;
> > + ch_bd->offset =3D 0;
> > + if (ch_bd =3D=3D channel->p_ch_data->rx_out_data)
> > + channel->p_ch_data->rx_out_data =3D
> > + ch_bd->wrap ?
> > + channel->p_ch_data->rx_data_fifo
> > + : ch_bd+1;
> > + port->port_stat.rx_pkt_drop_count++;
> > + }
> > + /* if the bd is empty */
> > + if (!ch_bd->flag) {
> > + if (ch_bd->offset =3D=3D 0)
> > + ch_bd->length =3D port->rx_max_frames;
> > +
> > + pcm_buffer =3D ch_bd->p_data + ch_bd->offset;
> > + /* De-interleaving the data */
> > + for (i =3D 0; i < ch_data_len; i++) {
> > + pcm_buffer[i]
> > + =3D input_tdm_buffer[i*bytes_slot_offset +
> > + channel->ch_id];
> > + }
> > + ch_bd->offset +=3D ch_data_len * slot_width;
> > +
> > + if (ch_bd->offset >=3D
> > + (ch_bd->length - frame_ch_data_size)*
> > + (adap->adapt_cfg.slot_width)) {
> > + ch_bd->flag =3D 1;
> > + ch_bd->offset =3D 0;
> > + channel->p_ch_data->rx_in_data =3D
> > + ch_bd->wrap ?
> > + channel->p_ch_data->rx_data_fifo
> > + : ch_bd+1;
> > + ch_data =3D 1;
> > + }
> > + } else {
> > + port->port_stat.rx_pkt_drop_count++;
> > + }
> > + spin_unlock(&channel->p_ch_data->rx_channel_lock);
> > + }
>=20
> Broken indentation. Spaces around operators.
>=20
> > +int tdm_channel_close(u16 chanid, u16 ch_width, struct tdm_port *port,
> > + struct tdm_channel *h_channel)
> > +{
> > + struct tdm_channel *channel;
> > + unsigned long flags;
> > + int res =3D TDM_E_OK;
> > + channel =3D h_channel;
> > +
> > + if (!(port && channel)) {
> > + pr_err("%s: Invalid handle\n", __func__);
> > + res =3D -EINVAL;
> > + goto out;
> > + }
> > +
> > + if (ch_width !=3D 1) {
> > + pr_err("%s: Mode not supported\n", __func__);
> > + res =3D -EINVAL;
> > + goto out;
> > + }
>=20
> close() seems an odd time to be checking for supported channel width,
> especially since you don't use that variable anywhere else in this
> function.
>=20
> > +#ifndef _LINUX_TDM_H
> > +#define _LINUX_TDM_H
> > +
> > +#ifdef __KERNEL__
>=20
> Is this supposed to be a userspace header ever? If TDM exposes a
> userspace interface, it needs to be documented.
>=20
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/device.h> /* for struct device */
> > +#include <linux/sched.h> /* for completion */
> > +#include <linux/mutex.h>
> > +#include <linux/interrupt.h>
> > +
> > +#define CHANNEL_8BIT_LIN 0 /* 8 bit linear */
> > +#define CHANNEL_8BIT_ULAW 1 /* 8 bit Mu-law */
> > +#define CHANNEL_8BIT_ALAW 2 /* 8 bit A-law */
> > +#define CHANNEL_16BIT_LIN 3 /* 16 bit Linear */
> > +
> > +#define NUM_CHANNELS 16
> > +#define NUM_SAMPLES_PER_MS 8 /* 8 samples per milli sec per
> > + channel. Req for voice data */
> > +#define NUM_MS 10
> > +#define NUM_SAMPLES_PER_FRAME (NUM_MS * NUM_SAMPLES_PER_MS) /*
> Number of
> > + samples for 1 client buffer */
> > +#define NUM_OF_TDM_BUF 3
>=20
> These need proper namespacing -- plus, should all of these really be
> hardcoded like this? Is this standard stuff that will always be the
> same?
>=20
> > +/* General options */
> > +
> > +struct tdm_adapt_algorithm;
> > +struct tdm_adapter;
> > +struct tdm_port;
> > +struct tdm_driver;
> > +
> > +/* Align addr on a size boundary - adjust address up if needed */
> > +/* returns min value greater than size which is multiple of alignment
> > +*/ static inline int ALIGN_SIZE(u64 size, u32 alignment) {
> > + return (size + alignment - 1) & (~(alignment - 1)); }
>=20
> Use the already existing ALIGN().
>=20
> > +/**
> > + * struct tdm_driver - represent an TDM device driver
> > + * @class: What kind of tdm device we instantiate (for detect)
> > + * @id:Driver id
> > + * @name: Name of the driver
> > + * @attach_adapter: Callback for device addition (for legacy drivers)
> > + * @detach_adapter: Callback for device removal (for legacy drivers)
> > + * @probe: Callback for device binding
> > + * @remove: Callback for device unbinding
> > + * @shutdown: Callback for device shutdown
> > + * @suspend: Callback for device suspend
> > + * @resume: Callback for device resume
> > + * @command: Callback for sending commands to device
> > + * @id_table: List of TDM devices supported by this driver
> > + * @list: List of drivers created (for tdm-core use only) */ struct
> > +tdm_driver {
> > + unsigned int class;
> > + unsigned int id;
> > + char name[TDM_NAME_SIZE];
> > +
> > + int (*attach_adapter)(struct tdm_adapter *);
> > + int (*detach_adapter)(struct tdm_adapter *);
> > +
> > + /* Standard driver model interfaces */
> > + int (*probe)(const struct tdm_device_id *);
> > + int (*remove)(void);
> > +
> > + /* driver model interfaces that don't relate to enumeration */
> > + void (*shutdown)(void);
> > + int (*suspend)(pm_message_t mesg);
> > + int (*resume)(void);
> > +
> > + /* a ioctl like command that can be used to perform specific
> functions
> > + * with the device.
> > + */
> > + int (*command)(unsigned int cmd, void *arg);
>=20
> Please describe what semantics you expect for the non-standard functions.
>=20
> Where are the "ioctl like commands" defined? When would they be used?
> I don't see it used in this patchset.
>=20
> > +/* struct tdm_channel- represent a TDM channel for a port */ struct
> > +tdm_channel {
> > + u16 ch_id; /* logical channel number */
> > + struct list_head list; /* list of channels in a port*/
> > + struct tdm_port *port; /* port for this channel */
> > + u16 in_use; /* channel is enabled? */
> > + struct tdm_ch_cfg ch_cfg; /* channel configuration */
> > + struct tdm_ch_data *p_ch_data; /* data storage space for
> channel */
> > +};
>=20
> Why are ch_id and especially in_use u16?
>=20
> > +/* tdm_adapt_algorithm is for accessing the routines of device */
> > +struct tdm_adapt_algorithm {
> > + u32 (*tdm_read)(struct tdm_adapter *, u16 **);
> > + u32 (*tdm_get_write_buf)(struct tdm_adapter *, u16 **);
> > + u32 (*tdm_write)(struct tdm_adapter *, void * , unsigned int len);
> > + int (*tdm_enable)(struct tdm_adapter *);
> > + int (*tdm_disable)(struct tdm_adapter *); };
> >
>=20
> Provide parameter names and document the semantics you're expecting.
>=20
> > +/* tdm_adapter_mode is to define in mode of the device */ enum
> > +tdm_adapter_mode {
> > + e_TDM_ADAPTER_MODE_NONE =3D 0x00,
> > + e_TDM_ADAPTER_MODE_T1 =3D 0x01,
> > + e_TDM_ADAPTER_MODE_E1 =3D 0x02,
> > + e_TDM_ADAPTER_MODE_T1_RAW =3D 0x10,
> > + e_TDM_ADAPTER_MODE_E1_RAW =3D 0x20,
> > +};
> > +
> > +/* tdm_port_mode defines the mode in which the port is configured to
> > +operate
> > + * It can be channelized/full/fractional.
> > + */
> > +enum tdm_port_mode {
> > + e_TDM_PORT_CHANNELIZED =3D 0 /* Channelized mode */
> > + , e_TDM_PORT_FULL =3D 1 /* Full mode */
> > + , e_TDM_PORT_FRACTIONAL =3D 2 /* Fractional mode */
> > +};
> > +
> > +/* tdm_process_mode used for testing the tdm device in normal mode or
> > +internal
> > + * loopback or external loopback
> > + */
> > +enum tdm_process_mode {
> > + e_TDM_PROCESS_NORMAL =3D 0 /* Normal mode */
> > + , e_TDM_PROCESS_INT_LPB =3D 1 /* Internal loop mode */
> > + , e_TDM_PROCESS_EXT_LPB =3D 2 /* External Loopback mode */
> > +};
>=20
> Commas go at the end of lines, not the beginning.
>=20
> No hungarian notation -- drop the "e_".
>=20
> > +/* TDM configuration parameters */
> > +struct fsl_tdm_adapt_cfg {
> > + u8 num_ch; /* Number of channels in this adpater */
> > + u8 ch_size_type; /* reciever/transmit channel
> > + size for all channels */
> > + u8 slot_width; /* 1 or 2 Is defined by channel type */
> > + u8 frame_len; /* Length of frame in samples */
>=20
> Is u8 really appropriate here? Maybe int?
>=20
> What does "type" mean in "ch_size_type"?
>=20
> > + u8 adap_mode; /* 0=3DNone, 1=3D T1, 2=3D T1-FULL, 3=3DE1,
> > + 4 =3D E1-FULL */
>=20
> Use #defines or an enum.
>=20
> > + int max_num_ports; /* Not Used: Max Number of ports that
> > + can be created on this adapter */
>=20
> If it's not used, why is it here?
>=20
> > +/*
> > + * tdm_adapter is the structure used to identify a physical tdm
> > +device along
> > + * with the access algorithms necessary to access it.
> > + */
> > +struct tdm_adapter {
> > + struct module *owner; /* owner of the adapter module */
> > + unsigned int id; /* Adapter Id */
>=20
> What does the id mean? Why do we need an arbitrary numberspace?
>=20
> > + unsigned int class; /* classes to allow probing for */
>=20
> What sort of values would go here?
>=20
> > + unsigned int drv_count; /* Number of drivers associated with the
> > + adapter */
> > +
> > + const struct tdm_adapt_algorithm *algo; /* the algorithm to
> access the
> > + adapter*/
> > +
> > + char name[TDM_NAME_SIZE]; /* Name of Adapter */
> > + struct mutex adap_lock;
> > + struct device *parent; /*Not Used*/
>=20
> Why is the parent device not used?
>=20
> > + struct tasklet_struct tdm_data_tasklet; /* tasklet handle to
> perform
> > + data processing*/
> > + int tasklet_conf; /* flag for tasklet configuration */
> > + int tdm_rx_flag;
>=20
> What does "tdm_rx_flag" indicate? What about "tasklet_conf"?
>=20
> > + struct list_head myports; /* list of ports, created on this
> > + adapter */
> > + struct list_head list;
>=20
> If you've got more than one list, it's probably a bad idea for any of
> them to be simply called "list".
>=20
> > + spinlock_t portlist_lock; /* Spin Lock for port_list */
>=20
> Comments should add new information, not just restate what the code
> already said.
>=20
> > +static inline void *tdm_get_adapdata(const struct tdm_adapter *dev) {
> > + return dev->data;
> > +}
> > +
> > +static inline void tdm_set_adapdata(struct tdm_adapter *dev, void
> > +*data) {
> > + dev->data =3D data;
> > +}
>=20
> Is this really needed?
>=20
> > +
> > +/* functions exported by tdm.o */
> > +
> > +extern int tdm_add_adapter(struct tdm_adapter *); extern int
> > +tdm_del_adapter(struct tdm_adapter *); extern int
> > +tdm_register_driver(struct tdm_driver *); extern void
> > +tdm_del_driver(struct tdm_driver *); extern void
> > +tdm_unregister_driver(struct tdm_driver *); extern void
> > +init_config_adapter(struct tdm_adapter *);
> > +
> > +extern unsigned int tdm_port_open(struct tdm_driver *, void **);
> > +extern unsigned int tdm_port_close(void *); extern unsigned int
> > +tdm_port_ioctl(void *, unsigned int, unsigned long); extern unsigned
> > +int tdm_channel_read(void *, void *, void *, u16 *); extern unsigned
> > +int tdm_channel_write(void *, void * , void *, u16); extern unsigned
> > +int tdm_port_poll(void *, unsigned int);
> > +
> > +extern int tdm_channel_open(u16, u16, struct tdm_port *, void **);
> > +extern int tdm_channel_close(u16, u16, struct tdm_port *,
> > + struct tdm_channel *);
>=20
> Please provide parameter names with prototypes.
>=20
> The "extern" is unnecessary.
>=20
> > +static inline int tdm_add_driver(struct tdm_driver *driver) {
> > + return tdm_register_driver(driver);
> > +}
>=20
> What is the semantic difference between tdm_add_driver() and
> tdm_register_driver()? Why do they both exist?
>=20
> -Scott
^ permalink raw reply
* [PATCH v3] powerpc/85xx: Enable MTD/NOR/NAND options by default in defconfig
From: Shengzhou Liu @ 2012-04-27 3:01 UTC (permalink / raw)
To: linuxppc-dev; +Cc: dwmw2, linux-mtd, Shengzhou Liu
Enable MTD/NOR/NAND options by default in mpc85xx_defconfig and
mpc85xx_smp_defconfig to support NOR, NAND flash.
Signed-off-by: Shengzhou Liu <Shengzhou.Liu@freescale.com>
---
based on master branch of git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git tree.
v3: remove "CONFIG_MTD_NAND_VERIFY_WRITE=y"
v2: remove typo "CONFIG_MPC8xxx_GPIO=y"
arch/powerpc/configs/mpc85xx_defconfig | 24 ++++++++++++++++++++++++
arch/powerpc/configs/mpc85xx_smp_defconfig | 24 ++++++++++++++++++++++++
2 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/configs/mpc85xx_defconfig b/arch/powerpc/configs/mpc85xx_defconfig
index d6b6df5..8c3da78 100644
--- a/arch/powerpc/configs/mpc85xx_defconfig
+++ b/arch/powerpc/configs/mpc85xx_defconfig
@@ -74,6 +74,30 @@ CONFIG_INET_ESP=y
CONFIG_IPV6=y
CONFIG_IP_SCTP=m
CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
+CONFIG_MTD=y
+CONFIG_MTD_CMDLINE_PARTS=y
+CONFIG_MTD_CHAR=y
+CONFIG_MTD_BLOCK=y
+CONFIG_MTD_CFI=y
+CONFIG_FTL=y
+CONFIG_MTD_GEN_PROBE=y
+CONFIG_MTD_MAP_BANK_WIDTH_1=y
+CONFIG_MTD_MAP_BANK_WIDTH_2=y
+CONFIG_MTD_MAP_BANK_WIDTH_4=y
+CONFIG_MTD_CFI_I1=y
+CONFIG_MTD_CFI_I2=y
+CONFIG_MTD_CFI_INTELEXT=y
+CONFIG_MTD_CFI_AMDSTD=y
+CONFIG_MTD_CFI_UTIL=y
+CONFIG_MTD_PHYSMAP_OF=y
+CONFIG_MTD_PARTITIONS=y
+CONFIG_MTD_OF_PARTS=y
+CONFIG_MTD_NAND=y
+CONFIG_MTD_NAND_FSL_ELBC=y
+CONFIG_MTD_NAND_FSL_IFC=y
+CONFIG_MTD_NAND_IDS=y
+CONFIG_MTD_NAND_ECC=y
+CONFIG_MTD_M25P80=y
CONFIG_PROC_DEVICETREE=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_BLK_DEV_NBD=y
diff --git a/arch/powerpc/configs/mpc85xx_smp_defconfig b/arch/powerpc/configs/mpc85xx_smp_defconfig
index 5b0e292..575c410 100644
--- a/arch/powerpc/configs/mpc85xx_smp_defconfig
+++ b/arch/powerpc/configs/mpc85xx_smp_defconfig
@@ -76,6 +76,30 @@ CONFIG_INET_ESP=y
CONFIG_IPV6=y
CONFIG_IP_SCTP=m
CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
+CONFIG_MTD=y
+CONFIG_MTD_CMDLINE_PARTS=y
+CONFIG_MTD_CHAR=y
+CONFIG_MTD_BLOCK=y
+CONFIG_MTD_CFI=y
+CONFIG_FTL=y
+CONFIG_MTD_GEN_PROBE=y
+CONFIG_MTD_MAP_BANK_WIDTH_1=y
+CONFIG_MTD_MAP_BANK_WIDTH_2=y
+CONFIG_MTD_MAP_BANK_WIDTH_4=y
+CONFIG_MTD_CFI_I1=y
+CONFIG_MTD_CFI_I2=y
+CONFIG_MTD_CFI_INTELEXT=y
+CONFIG_MTD_CFI_AMDSTD=y
+CONFIG_MTD_CFI_UTIL=y
+CONFIG_MTD_PHYSMAP_OF=y
+CONFIG_MTD_PARTITIONS=y
+CONFIG_MTD_OF_PARTS=y
+CONFIG_MTD_NAND=y
+CONFIG_MTD_NAND_FSL_ELBC=y
+CONFIG_MTD_NAND_FSL_IFC=y
+CONFIG_MTD_NAND_IDS=y
+CONFIG_MTD_NAND_ECC=y
+CONFIG_MTD_M25P80=y
CONFIG_PROC_DEVICETREE=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_BLK_DEV_NBD=y
--
1.6.4
^ permalink raw reply related
* Re: [PATCH 00/15] PowerMac i2c API conversions & windfarm updates
From: Andreas Schwab @ 2012-04-27 7:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: khali, linuxppc-dev
In-Reply-To: <1335477999.21961.85.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> Ok, I'll switch that back then. It seemed more sensible to read the
> actual fan values rather than the programmed ones (in fact I wonder if I
> can just skip the read alltogether then and use a cached value but that
> means I won't be able to detect failed fans...), but if you say it
> behaves better, let's keep it the way it was.
I don't actually care too much about this, since the most annoyance came
from the slots fan.
> As for the tickle, I'm not sure yet how to proceed. I'll look into it,
> try various things. We can maybe just remove the tickle but that means
> that a completely idle machine might start ramping up as the FCU times
> out.
The old driver gets away with it probably because it always writes to
the fcu even if the speed didn't change.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-27 13:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <1335291342-14922-1-git-send-email-mchehab@redhat.com>
Btw,
this patch gives
[ 8.278399] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
[ 8.287594] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 1: dimm1 (0:1:0): row 0, chan 1
[ 8.296784] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 2: dimm2 (1:0:0): row 1, chan 0
[ 8.305968] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 3: dimm3 (1:1:0): row 1, chan 1
[ 8.315144] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 4: dimm4 (2:0:0): row 2, chan 0
[ 8.324326] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 5: dimm5 (2:1:0): row 2, chan 1
[ 8.333502] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 6: dimm6 (3:0:0): row 3, chan 0
[ 8.342684] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 7: dimm7 (3:1:0): row 3, chan 1
[ 8.351860] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 8: dimm8 (4:0:0): row 4, chan 0
[ 8.361049] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 9: dimm9 (4:1:0): row 4, chan 1
[ 8.370227] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 10: dimm10 (5:0:0): row 5, chan 0
[ 8.379582] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 11: dimm11 (5:1:0): row 5, chan 1
[ 8.388941] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 12: dimm12 (6:0:0): row 6, chan 0
[ 8.398315] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 13: dimm13 (6:1:0): row 6, chan 1
[ 8.407680] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 14: dimm14 (7:0:0): row 7, chan 0
[ 8.417047] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 15: dimm15 (7:1:0): row 7, chan 1
and the memory controller has the following chip selects
[ 8.137662] EDAC MC: DCT0 chip selects:
[ 8.150291] EDAC amd64: MC: 0: 2048MB 1: 2048MB
[ 8.155349] EDAC amd64: MC: 2: 2048MB 3: 2048MB
[ 8.160408] EDAC amd64: MC: 4: 0MB 5: 0MB
[ 8.165475] EDAC amd64: MC: 6: 0MB 7: 0MB
[ 8.180499] EDAC MC: DCT1 chip selects:
[ 8.184693] EDAC amd64: MC: 0: 2048MB 1: 2048MB
[ 8.189753] EDAC amd64: MC: 2: 2048MB 3: 2048MB
[ 8.194812] EDAC amd64: MC: 4: 0MB 5: 0MB
[ 8.199875] EDAC amd64: MC: 6: 0MB 7: 0MB
Those are 4 dual-ranked DIMMs on this node, DCT0 is one channel and DCT1
is another and I have 4 ranks per channel. Having dimm0-dimm15 is very
misleading and has nothing to do with the reality. So, if this is to use
your nomenclature with layers, I'll have dimm0-dimm7 where each dimm is
a rank.
Or, the most correct thing to do would be to have dimm0-dimm3, each
dual-ranked.
So either tot_dimms is computed wrongly or there's a more serious error
somewhere.
I've reviewed almost the half patch, will review the rest when/if we
sort out the above issue first.
Thanks.
On Tue, Apr 24, 2012 at 03:15:41PM -0300, Mauro Carvalho Chehab wrote:
> Change the EDAC internal representation to work with non-csrow
> based memory controllers.
>
> There are lots of those memory controllers nowadays, and more
> are coming. So, the EDAC internal representation needs to be
> changed, in order to work with those memory controllers, while
> preserving backward compatibility with the old ones.
>
> The edac core were written with the idea that memory controllers
was
> are able to directly access csrows, and that the channels are
> used inside a csrows select.
This sounds funny, simply remove that second part about the channels.
> This is not true for FB-DIMM and RAMBUS memory controllers.
>
> Also, some recent advanced memory controllers don't present a per-csrows
> view. Instead, they view memories as DIMM's, instead of ranks, accessed
DIMMs instead of ranks."
Remove the rest.
> via csrow/channel.
>
> So, change the allocation and error report routines to allow
> them to work with all types of architectures.
>
> This will allow the removal of several hacks on FB-DIMM and RAMBUS
with
> memory controllers on the next patches.
. Remove the rest.
>
> Also, several tests were done on different platforms using different
> x86 drivers.
>
> TODO: a multi-rank DIMM's are currently represented by multiple DIMM
Multi-rank DIMMs
> entries at struct dimm_info. That means that changing a label for one
in
> rank won't change the same label for the other ranks at the same dimm.
of the same DIMM.
> Such bug is there since the beginning of the EDAC, so it is not a big
This bug is present ..
> deal. However, on several drivers, it is possible to fix this issue, but
remove "on"
> it should be a per-driver fix, as the csrow => DIMM arrangement may not
> be equal for all. So, don't try to fix it here yet.
>
> PS.: I tried to make this patch as short as possible, preceding it with
Remove "PS."
> several other patches that simplified the logic here. Yet, as the
> internal API changes, all drivers need changes. The changes are
> generally bigger on the drivers for FB-DIMM's.
in for FB-DIMMs.
>
> FIXME: while the FB-DIMMs are not converted to use the new
> design, uncorrected errors will show just one channel. In
> the past, all changes were on a big patch with about 150K.
> As it needed to be split, in order to be accepted by the
> EDAC ML at vger, we've opted to have this small drawback.
> As an advantage, it is now easier to review the patch series.
This whole paragraph above doesn't have anything to do with what the
patch does, so it can go.
[..]
> ---
>
> v16: Only context changes
>
> drivers/edac/edac_core.h | 92 ++++++-
> drivers/edac/edac_mc.c | 682 ++++++++++++++++++++++++++++------------------
> include/linux/edac.h | 40 ++-
> 3 files changed, 526 insertions(+), 288 deletions(-)
>
> diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
> index e48ab31..7201bb1 100644
> --- a/drivers/edac/edac_core.h
> +++ b/drivers/edac/edac_core.h
> @@ -447,8 +447,13 @@ static inline void pci_write_bits32(struct pci_dev *pdev, int offset,
>
> #endif /* CONFIG_PCI */
>
> -extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> - unsigned nr_chans, int edac_index);
> +struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> + unsigned nr_chans, int edac_index);
Why not "extern"?
> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
> + unsigned n_layers,
> + struct edac_mc_layer *layers,
> + bool rev_order,
> + unsigned sz_pvt);
ditto.
> extern int edac_mc_add_mc(struct mem_ctl_info *mci);
> extern void edac_mc_free(struct mem_ctl_info *mci);
> extern struct mem_ctl_info *edac_mc_find(int idx);
> @@ -467,24 +472,80 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
> * reporting logic and function interface - reduces conditional
> * statement clutter and extra function arguments.
> */
> -extern void edac_mc_handle_ce(struct mem_ctl_info *mci,
> +
> +void edac_mc_handle_error(const enum hw_event_mc_err_type type,
> + struct mem_ctl_info *mci,
> + const unsigned long page_frame_number,
> + const unsigned long offset_in_page,
> + const unsigned long syndrome,
> + const int layer0,
> + const int layer1,
> + const int layer2,
> + const char *msg,
> + const char *other_detail,
> + const void *mcelog);
Why isn't this one "extern" either?
> +
> +static inline void edac_mc_handle_ce(struct mem_ctl_info *mci,
> unsigned long page_frame_number,
> unsigned long offset_in_page,
> unsigned long syndrome, int row, int channel,
> - const char *msg);
Strange alignment, pls do
static inline void edac_mc_handle_ce(struct...,
unsigned...,
...,
...);
> -extern void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
> - const char *msg);
> -extern void edac_mc_handle_ue(struct mem_ctl_info *mci,
> + const char *msg)
> +{
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + page_frame_number, offset_in_page, syndrome,
> + row, channel, -1, msg, NULL, NULL);
> +}
> +
> +static inline void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
> + const char *msg)
ditto.
> +{
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
> +}
> +
> +static inline void edac_mc_handle_ue(struct mem_ctl_info *mci,
> unsigned long page_frame_number,
> unsigned long offset_in_page, int row,
> - const char *msg);
ditto.
> -extern void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
> - const char *msg);
> -extern void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci, unsigned int csrow,
> - unsigned int channel0, unsigned int channel1,
> - char *msg);
> -extern void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci, unsigned int csrow,
> - unsigned int channel, char *msg);
> + const char *msg)
> +{
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + page_frame_number, offset_in_page, 0,
> + row, -1, -1, msg, NULL, NULL);
> +}
> +
> +static inline void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
> + const char *msg)
> +{
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
> +}
> +
> +static inline void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
> + unsigned int csrow,
> + unsigned int channel0,
> + unsigned int channel1,
> + char *msg)
Now this alignment looks correct.
> +{
> + /*
> + *FIXME: The error can also be at channel1 (e. g. at the second
> + * channel of the same branch). The fix is to push
> + * edac_mc_handle_error() call into each driver
> + */
> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> + 0, 0, 0,
> + csrow, channel0, -1, msg, NULL, NULL);
> +}
> +
> +static inline void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
> + unsigned int csrow,
> + unsigned int channel, char *msg)
> +{
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> + 0, 0, 0,
> + csrow, channel, -1, msg, NULL, NULL);
> +}
> +
> +
Two superfluous newlines.
>
> /*
> * edac_device APIs
> @@ -496,6 +557,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
> extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
> int inst_nr, int block_nr, const char *msg);
> extern int edac_device_alloc_index(void);
> +extern const char *edac_layer_name[];
>
> /*
> * edac_pci APIs
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 6ec967a..4d4d8b7 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -44,9 +44,25 @@ static void edac_mc_dump_channel(struct rank_info *chan)
> debugf4("\tchannel = %p\n", chan);
> debugf4("\tchannel->chan_idx = %d\n", chan->chan_idx);
> debugf4("\tchannel->csrow = %p\n\n", chan->csrow);
> - debugf4("\tdimm->ce_count = %d\n", chan->dimm->ce_count);
> - debugf4("\tdimm->label = '%s'\n", chan->dimm->label);
> - debugf4("\tdimm->nr_pages = 0x%x\n", chan->dimm->nr_pages);
> + debugf4("\tchannel->dimm = %p\n", chan->dimm);
> +}
> +
> +static void edac_mc_dump_dimm(struct dimm_info *dimm)
> +{
> + int i;
> +
> + debugf4("\tdimm = %p\n", dimm);
> + debugf4("\tdimm->label = '%s'\n", dimm->label);
> + debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
> + debugf4("\tdimm location ");
> + for (i = 0; i < dimm->mci->n_layers; i++) {
> + printk(KERN_CONT "%d", dimm->location[i]);
> + if (i < dimm->mci->n_layers - 1)
> + printk(KERN_CONT ".");
> + }
> + printk(KERN_CONT "\n");
This looks hacky but I don't have a good suggestion what to do instead
here. Maybe snprintf into a complete string which you can issue with
debugf4()...
> + debugf4("\tdimm->grain = %d\n", dimm->grain);
> + debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
> }
>
> static void edac_mc_dump_csrow(struct csrow_info *csrow)
> @@ -70,6 +86,8 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)
> debugf4("\tmci->edac_check = %p\n", mci->edac_check);
> debugf3("\tmci->nr_csrows = %d, csrows = %p\n",
> mci->nr_csrows, mci->csrows);
> + debugf3("\tmci->nr_dimms = %d, dimns = %p\n",
->tot_dimms dimms
> + mci->tot_dimms, mci->dimms);
> debugf3("\tdev = %p\n", mci->dev);
> debugf3("\tmod_name:ctl_name = %s:%s\n", mci->mod_name, mci->ctl_name);
> debugf3("\tpvt_info = %p\n\n", mci->pvt_info);
> @@ -157,10 +175,25 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
> }
>
> /**
> - * edac_mc_alloc: Allocate a struct mem_ctl_info structure
> - * @size_pvt: size of private storage needed
> - * @nr_csrows: Number of CWROWS needed for this MC
> - * @nr_chans: Number of channels for the MC
> + * edac_mc_alloc: Allocate and partially fills a struct mem_ctl_info structure
fill
> + * @edac_index: Memory controller number
> + * @n_layers: Number of layers at the MC hierarchy
Number of MC hierarchy layers
> + * layers: Describes each layer as seen by the Memory Controller
> + * @rev_order: Fills csrows/cs channels at the reverse order
csrows/channels in reverse order
> + * @size_pvt: size of private storage needed
> + *
> + *
> + * FIXME: drivers handle multi-rank memories on different ways: on some
in in
> + * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
memory stick in
> + * a single multi-rank DIMM would be mapped into several "dimms".
memory stick
> + *
> + * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
> + * such DIMMS properly, but the CSROWS-based ones will likely do the wrong
csrow-based
> + * thing, as two chip select values are used for dual-rank memories (and 4, for
> + * quad-rank ones). I suspect that this issue could be solved inside the EDAC
> + * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
> + *
> + * In summary, solving this issue is not easy, as it requires a lot of testing.
> *
> * Everything is kmalloc'ed as one big chunk - more efficient.
> * Only can be used if all structures have the same lifetime - otherwise
> @@ -172,18 +205,41 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
> * NULL allocation failed
> * struct mem_ctl_info pointer
> */
> -struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> - unsigned nr_chans, int edac_index)
> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
> + unsigned n_layers,
> + struct edac_mc_layer *layers,
> + bool rev_order,
> + unsigned sz_pvt)
strange function argument vertical alignment
> {
> void *ptr = NULL;
> struct mem_ctl_info *mci;
> - struct csrow_info *csi, *csrow;
> + struct edac_mc_layer *lay;
As before, call this "layers" pls.
> + struct csrow_info *csi, *csr;
> struct rank_info *chi, *chp, *chan;
> struct dimm_info *dimm;
> + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
> void *pvt;
> - unsigned size;
> - int row, chn;
> + unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
> + unsigned tot_csrows, tot_cschannels;
No need to call this "tot_cschannels" - "tot_channels" should be enough.
> + int i, j;
> int err;
> + int row, chn;
All those local variables should be sorted in a reverse christmas tree
order:
u32 this_is_the_longest_array_name[LENGTH];
void *shorter_named_variable;
unsigned long size;
int i;
...
> +
> + BUG_ON(n_layers > EDAC_MAX_LAYERS);
Push this BUG_ON up into edac_mc_alloc as the first thing this function
does. Also, is it valid to have n_layers == 0? The memcpy call below
will do nothing.
> + /*
> + * Calculate the total amount of dimms and csrows/cschannels while
> + * in the old API emulation mode
> + */
> + tot_dimms = 1;
> + tot_cschannels = 1;
> + tot_csrows = 1;
Those initializations can be done above at variable declaration time.
> + for (i = 0; i < n_layers; i++) {
> + tot_dimms *= layers[i].size;
> + if (layers[i].is_virt_csrow)
> + tot_csrows *= layers[i].size;
> + else
> + tot_cschannels *= layers[i].size;
> + }
>
> /* Figure out the offsets of the various items from the start of an mc
> * structure. We want the alignment of each item to be at least as
> @@ -191,12 +247,21 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> * hardcode everything into a single struct.
> */
> mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
> - csi = edac_align_ptr(&ptr, sizeof(*csi), nr_csrows);
> - chi = edac_align_ptr(&ptr, sizeof(*chi), nr_csrows * nr_chans);
> - dimm = edac_align_ptr(&ptr, sizeof(*dimm), nr_csrows * nr_chans);
> + lay = edac_align_ptr(&ptr, sizeof(*lay), n_layers);
> + csi = edac_align_ptr(&ptr, sizeof(*csi), tot_csrows);
> + chi = edac_align_ptr(&ptr, sizeof(*chi), tot_csrows * tot_cschannels);
> + dimm = edac_align_ptr(&ptr, sizeof(*dimm), tot_dimms);
> + count = 1;
ditto.
> + for (i = 0; i < n_layers; i++) {
> + count *= layers[i].size;
> + ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
> + ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
> + }
> pvt = edac_align_ptr(&ptr, sz_pvt, 1);
> size = ((unsigned long)pvt) + sz_pvt;
>
> + debugf1("%s(): allocating %u bytes for mci data (%d dimms, %d csrows/channels)\n",
> + __func__, size, tot_dimms, tot_csrows * tot_cschannels);
> mci = kzalloc(size, GFP_KERNEL);
> if (mci == NULL)
> return NULL;
> @@ -204,42 +269,99 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> /* Adjust pointers so they point within the memory we just allocated
> * rather than an imaginary chunk of memory located at address 0.
> */
> + lay = (struct edac_mc_layer *)(((char *)mci) + ((unsigned long)lay));
> csi = (struct csrow_info *)(((char *)mci) + ((unsigned long)csi));
> chi = (struct rank_info *)(((char *)mci) + ((unsigned long)chi));
> dimm = (struct dimm_info *)(((char *)mci) + ((unsigned long)dimm));
> + for (i = 0; i < n_layers; i++) {
> + mci->ce_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ce_per_layer[i]));
> + mci->ue_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ue_per_layer[i]));
> + }
> pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL;
>
> /* setup index and various internal pointers */
> mci->mc_idx = edac_index;
> mci->csrows = csi;
> mci->dimms = dimm;
> + mci->tot_dimms = tot_dimms;
> mci->pvt_info = pvt;
> - mci->nr_csrows = nr_csrows;
> + mci->n_layers = n_layers;
> + mci->layers = lay;
> + memcpy(mci->layers, layers, sizeof(*lay) * n_layers);
> + mci->nr_csrows = tot_csrows;
> + mci->num_cschannel = tot_cschannels;
>
> /*
> - * For now, assumes that a per-csrow arrangement for dimms.
> - * This will be latter changed.
> + * Fills the csrow struct
> */
> - dimm = mci->dimms;
> -
> - for (row = 0; row < nr_csrows; row++) {
> - csrow = &csi[row];
> - csrow->csrow_idx = row;
> - csrow->mci = mci;
> - csrow->nr_channels = nr_chans;
> - chp = &chi[row * nr_chans];
> - csrow->channels = chp;
> -
> - for (chn = 0; chn < nr_chans; chn++) {
> + for (row = 0; row < tot_csrows; row++) {
> + csr = &csi[row];
> + csr->csrow_idx = row;
> + csr->mci = mci;
> + csr->nr_channels = tot_cschannels;
> + chp = &chi[row * tot_cschannels];
> + csr->channels = chp;
> +
> + for (chn = 0; chn < tot_cschannels; chn++) {
> chan = &chp[chn];
> chan->chan_idx = chn;
> - chan->csrow = csrow;
> + chan->csrow = csr;
> + }
> + }
>
> - mci->csrows[row].channels[chn].dimm = dimm;
> - dimm->csrow = row;
> - dimm->csrow_channel = chn;
> - dimm++;
> - mci->nr_dimms++;
> + /*
> + * Fills the dimm struct
> + */
> + memset(&pos, 0, sizeof(pos));
> + row = 0;
> + chn = 0;
> + debugf4("%s: initializing %d dimms\n", __func__, tot_dimms);
> + for (i = 0; i < tot_dimms; i++) {
> + chan = &csi[row].channels[chn];
> + dimm = EDAC_DIMM_PTR(lay, mci->dimms, n_layers,
> + pos[0], pos[1], pos[2]);
> + dimm->mci = mci;
> +
> + debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
> + i, (dimm - mci->dimms),
> + pos[0], pos[1], pos[2], row, chn);
> +
> + /* Copy DIMM location */
> + for (j = 0; j < n_layers; j++)
> + dimm->location[j] = pos[j];
> +
> + /* Link it to the csrows old API data */
> + chan->dimm = dimm;
> + dimm->csrow = row;
> + dimm->cschannel = chn;
> +
> + /* Increment csrow location */
> + if (!rev_order) {
> + for (j = n_layers - 1; j >= 0; j--)
> + if (!layers[j].is_virt_csrow)
> + break;
> + chn++;
> + if (chn == tot_cschannels) {
> + chn = 0;
> + row++;
> + }
> + } else {
> + for (j = n_layers - 1; j >= 0; j--)
> + if (layers[j].is_virt_csrow)
> + break;
> + row++;
> + if (row == tot_csrows) {
> + row = 0;
> + chn++;
> + }
> + }
> +
> + /* Increment dimm location */
> + for (j = n_layers - 1; j >= 0; j--) {
> + pos[j]++;
> + if (pos[j] < layers[j].size)
> + break;
> + pos[j] = 0;
> }
> }
>
> @@ -263,6 +385,57 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> */
> return mci;
> }
> +EXPORT_SYMBOL_GPL(new_edac_mc_alloc);
> +
> +/**
> + * edac_mc_alloc: Allocate and partially fills a struct mem_ctl_info structure
> + * @edac_index: Memory controller number
> + * @n_layers: Nu
> +mber of layers at the MC hierarchy
> + * layers: Describes each layer as seen by the Memory Controller
> + * @rev_order: Fills csrows/cs channels at the reverse order
> + * @size_pvt: size of private storage needed
> + *
> + *
> + * FIXME: drivers handle multi-rank memories on different ways: on some
> + * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
> + * a single multi-rank DIMM would be mapped into several "dimms".
> + *
> + * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
> + * such DIMMS properly, but the CSROWS-based ones will likely do the wrong
> + * thing, as two chip select values are used for dual-rank memories (and 4, for
> + * quad-rank ones). I suspect that this issue could be solved inside the EDAC
> + * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
> + *
> + * In summary, solving this issue is not easy, as it requires a lot of testing.
> + *
> + * Everything is kmalloc'ed as one big chunk - more efficient.
> + * Only can be used if all structures have the same lifetime - otherwise
> + * you have to allocate and initialize your own structures.
> + *
> + * Use edac_mc_free() to free mc structures allocated by this function.
> + *
> + * Returns:
> + * NULL allocation failed
> + * struct mem_ctl_info pointer
> + */
> +
> +struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> + unsigned nr_chans, int edac_index)
> +{
> + unsigned n_layers = 2;
> + struct edac_mc_layer layers[n_layers];
> +
> + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
> + layers[0].size = nr_csrows;
> + layers[0].is_virt_csrow = true;
> + layers[1].type = EDAC_MC_LAYER_CHANNEL;
> + layers[1].size = nr_chans;
> + layers[1].is_virt_csrow = false;
> +
> + return new_edac_mc_alloc(edac_index, ARRAY_SIZE(layers), layers,
> + false, sz_pvt);
> +}
> EXPORT_SYMBOL_GPL(edac_mc_alloc);
>
> /**
> @@ -528,7 +701,6 @@ EXPORT_SYMBOL(edac_mc_find);
> * edac_mc_add_mc: Insert the 'mci' structure into the mci global list and
> * create sysfs entries associated with mci structure
> * @mci: pointer to the mci structure to be added to the list
> - * @mc_idx: A unique numeric identifier to be assigned to the 'mci' structure.
> *
> * Return:
> * 0 Success
> @@ -555,6 +727,8 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
> edac_mc_dump_channel(&mci->csrows[i].
> channels[j]);
> }
> + for (i = 0; i < mci->tot_dimms; i++)
> + edac_mc_dump_dimm(&mci->dimms[i]);
> }
> #endif
> mutex_lock(&mem_ctls_mutex);
> @@ -712,261 +886,249 @@ int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
> }
> EXPORT_SYMBOL_GPL(edac_mc_find_csrow_by_page);
>
> -/* FIXME - setable log (warning/emerg) levels */
> -/* FIXME - integrate with evlog: http://evlog.sourceforge.net/ */
> -void edac_mc_handle_ce(struct mem_ctl_info *mci,
> - unsigned long page_frame_number,
> - unsigned long offset_in_page, unsigned long syndrome,
> - int row, int channel, const char *msg)
> +const char *edac_layer_name[] = {
> + [EDAC_MC_LAYER_BRANCH] = "branch",
> + [EDAC_MC_LAYER_CHANNEL] = "channel",
> + [EDAC_MC_LAYER_SLOT] = "slot",
> + [EDAC_MC_LAYER_CHIP_SELECT] = "csrow",
> +};
> +EXPORT_SYMBOL_GPL(edac_layer_name);
> +
> +static void edac_increment_ce_error(struct mem_ctl_info *mci,
> + bool enable_filter,
> + unsigned pos[EDAC_MAX_LAYERS])
> {
> - unsigned long remapped_page;
> - char *label = NULL;
> - u32 grain;
> + int i, index = 0;
>
> - debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
> + mci->ce_mc++;
>
> - /* FIXME - maybe make panic on INTERNAL ERROR an option */
> - if (row >= mci->nr_csrows || row < 0) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range "
> - "(%d >= %d)\n", row, mci->nr_csrows);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> + if (!enable_filter) {
> + mci->ce_noinfo_count++;
> return;
> }
>
> - if (channel >= mci->csrows[row].nr_channels || channel < 0) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel out of range "
> - "(%d >= %d)\n", channel,
> - mci->csrows[row].nr_channels);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> - return;
> - }
> -
> - label = mci->csrows[row].channels[channel].dimm->label;
> - grain = mci->csrows[row].channels[channel].dimm->grain;
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] < 0)
> + break;
> + index += pos[i];
> + mci->ce_per_layer[i][index]++;
>
> - if (edac_mc_get_log_ce())
> - /* FIXME - put in DIMM location */
> - edac_mc_printk(mci, KERN_WARNING,
> - "CE page 0x%lx, offset 0x%lx, grain %d, syndrome "
> - "0x%lx, row %d, channel %d, label \"%s\": %s\n",
> - page_frame_number, offset_in_page,
> - grain, syndrome, row, channel,
> - label, msg);
> + if (i < mci->n_layers - 1)
> + index *= mci->layers[i + 1].size;
> + }
> +}
>
> - mci->ce_count++;
> - mci->csrows[row].ce_count++;
> - mci->csrows[row].channels[channel].dimm->ce_count++;
> - mci->csrows[row].channels[channel].ce_count++;
> +static void edac_increment_ue_error(struct mem_ctl_info *mci,
> + bool enable_filter,
> + unsigned pos[EDAC_MAX_LAYERS])
> +{
> + int i, index = 0;
>
> - if (mci->scrub_mode & SCRUB_SW_SRC) {
> - /*
> - * Some MC's can remap memory so that it is still available
> - * at a different address when PCI devices map into memory.
> - * MC's that can't do this lose the memory where PCI devices
> - * are mapped. This mapping is MC dependent and so we call
> - * back into the MC driver for it to map the MC page to
> - * a physical (CPU) page which can then be mapped to a virtual
> - * page - which can then be scrubbed.
> - */
> - remapped_page = mci->ctl_page_to_phys ?
> - mci->ctl_page_to_phys(mci, page_frame_number) :
> - page_frame_number;
> + mci->ue_mc++;
>
> - edac_mc_scrub_block(remapped_page, offset_in_page, grain);
> + if (!enable_filter) {
> + mci->ce_noinfo_count++;
> + return;
> }
> -}
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ce);
>
> -void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci, const char *msg)
> -{
> - if (edac_mc_get_log_ce())
> - edac_mc_printk(mci, KERN_WARNING,
> - "CE - no information available: %s\n", msg);
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] < 0)
> + break;
> + index += pos[i];
> + mci->ue_per_layer[i][index]++;
>
> - mci->ce_noinfo_count++;
> - mci->ce_count++;
> + if (i < mci->n_layers - 1)
> + index *= mci->layers[i + 1].size;
> + }
> }
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ce_no_info);
>
> -void edac_mc_handle_ue(struct mem_ctl_info *mci,
> - unsigned long page_frame_number,
> - unsigned long offset_in_page, int row, const char *msg)
> +#define OTHER_LABEL " or "
> +void edac_mc_handle_error(const enum hw_event_mc_err_type type,
> + struct mem_ctl_info *mci,
> + const unsigned long page_frame_number,
> + const unsigned long offset_in_page,
> + const unsigned long syndrome,
> + const int layer0,
> + const int layer1,
> + const int layer2,
> + const char *msg,
> + const char *other_detail,
> + const void *mcelog)
> {
> - int len = EDAC_MC_LABEL_LEN * 4;
> - char labels[len + 1];
> - char *pos = labels;
> - int chan;
> - int chars;
> - char *label = NULL;
> + unsigned long remapped_page;
> + /* FIXME: too much for stack: move it to some pre-alocated area */
> + char detail[80], location[80];
> + char label[(EDAC_MC_LABEL_LEN + 1 + sizeof(OTHER_LABEL)) * mci->tot_dimms];
> + char *p;
> + int row = -1, chan = -1;
> + int pos[EDAC_MAX_LAYERS] = { layer0, layer1, layer2 };
> + int i;
> u32 grain;
> + bool enable_filter = false;
>
> debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
>
> - /* FIXME - maybe make panic on INTERNAL ERROR an option */
> - if (row >= mci->nr_csrows || row < 0) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range "
> - "(%d >= %d)\n", row, mci->nr_csrows);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
> - }
> -
> - grain = mci->csrows[row].channels[0].dimm->grain;
> - label = mci->csrows[row].channels[0].dimm->label;
> - chars = snprintf(pos, len + 1, "%s", label);
> - len -= chars;
> - pos += chars;
> -
> - for (chan = 1; (chan < mci->csrows[row].nr_channels) && (len > 0);
> - chan++) {
> - label = mci->csrows[row].channels[chan].dimm->label;
> - chars = snprintf(pos, len + 1, ":%s", label);
> - len -= chars;
> - pos += chars;
> + /* Check if the event report is consistent */
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] >= (int)mci->layers[i].size) {
> + if (type == HW_EVENT_ERR_CORRECTED) {
> + p = "CE";
> + mci->ce_mc++;
> + } else {
> + p = "UE";
> + mci->ue_mc++;
> + }
> + edac_mc_printk(mci, KERN_ERR,
> + "INTERNAL ERROR: %s value is out of range (%d >= %d)\n",
> + edac_layer_name[mci->layers[i].type],
> + pos[i], mci->layers[i].size);
> + /*
> + * Instead of just returning it, let's use what's
> + * known about the error. The increment routines and
> + * the DIMM filter logic will do the right thing by
> + * pointing the likely damaged DIMMs.
> + */
> + pos[i] = -1;
> + }
> + if (pos[i] >= 0)
> + enable_filter = true;
> }
>
> - if (edac_mc_get_log_ue())
> - edac_mc_printk(mci, KERN_EMERG,
> - "UE page 0x%lx, offset 0x%lx, grain %d, row %d, "
> - "labels \"%s\": %s\n", page_frame_number,
> - offset_in_page, grain, row, labels, msg);
> -
> - if (edac_mc_get_panic_on_ue())
> - panic("EDAC MC%d: UE page 0x%lx, offset 0x%lx, grain %d, "
> - "row %d, labels \"%s\": %s\n", mci->mc_idx,
> - page_frame_number, offset_in_page,
> - grain, row, labels, msg);
> -
> - mci->ue_count++;
> - mci->csrows[row].ue_count++;
> -}
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ue);
> + /*
> + * Get the dimm label/grain that applies to the match criteria.
> + * As the error algorithm may not be able to point to just one memory,
> + * the logic here will get all possible labels that could pottentially
> + * be affected by the error.
> + * On FB-DIMM memory controllers, for uncorrected errors, it is common
> + * to have only the MC channel and the MC dimm (also called as "rank")
> + * but the channel is not known, as the memory is arranged in pairs,
> + * where each memory belongs to a separate channel within the same
> + * branch.
> + * It will also get the max grain, over the error match range
> + */
> + grain = 0;
> + p = label;
> + *p = '\0';
> + for (i = 0; i < mci->tot_dimms; i++) {
> + struct dimm_info *dimm = &mci->dimms[i];
>
> -void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci, const char *msg)
> -{
> - if (edac_mc_get_panic_on_ue())
> - panic("EDAC MC%d: Uncorrected Error", mci->mc_idx);
> + if (layer0 >= 0 && layer0 != dimm->location[0])
> + continue;
> + if (layer1 >= 0 && layer1 != dimm->location[1])
> + continue;
> + if (layer2 >= 0 && layer2 != dimm->location[2])
> + continue;
>
> - if (edac_mc_get_log_ue())
> - edac_mc_printk(mci, KERN_WARNING,
> - "UE - no information available: %s\n", msg);
> - mci->ue_noinfo_count++;
> - mci->ue_count++;
> -}
> -EXPORT_SYMBOL_GPL(edac_mc_handle_ue_no_info);
> + if (dimm->grain > grain)
> + grain = dimm->grain;
>
> -/*************************************************************
> - * On Fully Buffered DIMM modules, this help function is
> - * called to process UE events
> - */
> -void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
> - unsigned int csrow,
> - unsigned int channela,
> - unsigned int channelb, char *msg)
> -{
> - int len = EDAC_MC_LABEL_LEN * 4;
> - char labels[len + 1];
> - char *pos = labels;
> - int chars;
> - char *label;
> -
> - if (csrow >= mci->nr_csrows) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range (%d >= %d)\n",
> - csrow, mci->nr_csrows);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
> + /*
> + * If the error is memory-controller wide, there's no sense
> + * on seeking for the affected DIMMs, as everything may be
> + * affected. Also, don't show errors for non-filled dimm's.
> + */
> + if (enable_filter && dimm->nr_pages) {
> + if (p != label) {
> + strcpy(p, OTHER_LABEL);
> + p += strlen(OTHER_LABEL);
> + }
> + strcpy(p, dimm->label);
> + p += strlen(p);
> + *p = '\0';
> +
> + /*
> + * get csrow/channel of the dimm, in order to allow
> + * incrementing the compat API counters
> + */
> + debugf4("%s: dimm csrows (%d,%d)\n",
> + __func__, dimm->csrow, dimm->cschannel);
> + if (row == -1)
> + row = dimm->csrow;
> + else if (row >= 0 && row != dimm->csrow)
> + row = -2;
> + if (chan == -1)
> + chan = dimm->cschannel;
> + else if (chan >= 0 && chan != dimm->cschannel)
> + chan = -2;
> + }
> }
> -
> - if (channela >= mci->csrows[csrow].nr_channels) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel-a out of range "
> - "(%d >= %d)\n",
> - channela, mci->csrows[csrow].nr_channels);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
> + if (!enable_filter) {
> + strcpy(label, "any memory");
> + } else {
> + debugf4("%s: csrow/channel to increment: (%d,%d)\n",
> + __func__, row, chan);
> + if (p == label)
> + strcpy(label, "unknown memory");
> + if (type == HW_EVENT_ERR_CORRECTED) {
> + if (row >= 0) {
> + mci->csrows[row].ce_count++;
> + if (chan >= 0)
> + mci->csrows[row].channels[chan].ce_count++;
> + }
> + } else
> + if (row >= 0)
> + mci->csrows[row].ue_count++;
> }
>
> - if (channelb >= mci->csrows[csrow].nr_channels) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel-b out of range "
> - "(%d >= %d)\n",
> - channelb, mci->csrows[csrow].nr_channels);
> - edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
> - return;
> + /* Fill the RAM location data */
> + p = location;
> + for (i = 0; i < mci->n_layers; i++) {
> + if (pos[i] < 0)
> + continue;
> + p += sprintf(p, "%s %d ",
> + edac_layer_name[mci->layers[i].type],
> + pos[i]);
> }
>
> - mci->ue_count++;
> - mci->csrows[csrow].ue_count++;
> -
> - /* Generate the DIMM labels from the specified channels */
> - label = mci->csrows[csrow].channels[channela].dimm->label;
> - chars = snprintf(pos, len + 1, "%s", label);
> - len -= chars;
> - pos += chars;
> -
> - chars = snprintf(pos, len + 1, "-%s",
> - mci->csrows[csrow].channels[channelb].dimm->label);
> -
> - if (edac_mc_get_log_ue())
> - edac_mc_printk(mci, KERN_EMERG,
> - "UE row %d, channel-a= %d channel-b= %d "
> - "labels \"%s\": %s\n", csrow, channela, channelb,
> - labels, msg);
> -
> - if (edac_mc_get_panic_on_ue())
> - panic("UE row %d, channel-a= %d channel-b= %d "
> - "labels \"%s\": %s\n", csrow, channela,
> - channelb, labels, msg);
> -}
> -EXPORT_SYMBOL(edac_mc_handle_fbd_ue);
> + /* Memory type dependent details about the error */
> + if (type == HW_EVENT_ERR_CORRECTED)
> + snprintf(detail, sizeof(detail),
> + "page 0x%lx offset 0x%lx grain %d syndrome 0x%lx",
> + page_frame_number, offset_in_page,
> + grain, syndrome);
> + else
> + snprintf(detail, sizeof(detail),
> + "page 0x%lx offset 0x%lx grain %d",
> + page_frame_number, offset_in_page, grain);
> +
> + if (type == HW_EVENT_ERR_CORRECTED) {
> + if (edac_mc_get_log_ce())
> + edac_mc_printk(mci, KERN_WARNING,
> + "CE %s on %s (%s%s %s)\n",
> + msg, label, location,
> + detail, other_detail);
> + edac_increment_ce_error(mci, enable_filter, pos);
> +
> + if (mci->scrub_mode & SCRUB_SW_SRC) {
> + /*
> + * Some MC's can remap memory so that it is still
> + * available at a different address when PCI devices
> + * map into memory.
> + * MC's that can't do this lose the memory where PCI
> + * devices are mapped. This mapping is MC dependent
> + * and so we call back into the MC driver for it to
> + * map the MC page to a physical (CPU) page which can
> + * then be mapped to a virtual page - which can then
> + * be scrubbed.
> + */
> + remapped_page = mci->ctl_page_to_phys ?
> + mci->ctl_page_to_phys(mci, page_frame_number) :
> + page_frame_number;
> +
> + edac_mc_scrub_block(remapped_page,
> + offset_in_page, grain);
> + }
> + } else {
> + if (edac_mc_get_log_ue())
> + edac_mc_printk(mci, KERN_WARNING,
> + "UE %s on %s (%s%s %s)\n",
> + msg, label, location, detail, other_detail);
>
> -/*************************************************************
> - * On Fully Buffered DIMM modules, this help function is
> - * called to process CE events
> - */
> -void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
> - unsigned int csrow, unsigned int channel, char *msg)
> -{
> - char *label = NULL;
> + if (edac_mc_get_panic_on_ue())
> + panic("UE %s on %s (%s%s %s)\n",
> + msg, label, location, detail, other_detail);
>
> - /* Ensure boundary values */
> - if (csrow >= mci->nr_csrows) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: row out of range (%d >= %d)\n",
> - csrow, mci->nr_csrows);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> - return;
> + edac_increment_ue_error(mci, enable_filter, pos);
> }
> - if (channel >= mci->csrows[csrow].nr_channels) {
> - /* something is wrong */
> - edac_mc_printk(mci, KERN_ERR,
> - "INTERNAL ERROR: channel out of range (%d >= %d)\n",
> - channel, mci->csrows[csrow].nr_channels);
> - edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
> - return;
> - }
> -
> - label = mci->csrows[csrow].channels[channel].dimm->label;
> -
> - if (edac_mc_get_log_ce())
> - /* FIXME - put in DIMM location */
> - edac_mc_printk(mci, KERN_WARNING,
> - "CE row %d, channel %d, label \"%s\": %s\n",
> - csrow, channel, label, msg);
> -
> - mci->ce_count++;
> - mci->csrows[csrow].ce_count++;
> - mci->csrows[csrow].channels[channel].dimm->ce_count++;
> - mci->csrows[csrow].channels[channel].ce_count++;
> }
> -EXPORT_SYMBOL(edac_mc_handle_fbd_ce);
> +EXPORT_SYMBOL_GPL(edac_mc_handle_error);
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index 3b8798d..412d5cd 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -412,18 +412,20 @@ struct edac_mc_layer {
> /* FIXME: add the proper per-location error counts */
> struct dimm_info {
> char label[EDAC_MC_LABEL_LEN + 1]; /* DIMM label on motherboard */
> - unsigned memory_controller;
> - unsigned csrow;
> - unsigned csrow_channel;
> +
> + /* Memory location data */
> + unsigned location[EDAC_MAX_LAYERS];
> +
> + struct mem_ctl_info *mci; /* the parent */
>
> u32 grain; /* granularity of reported error in bytes */
> enum dev_type dtype; /* memory device type */
> enum mem_type mtype; /* memory dimm type */
> enum edac_type edac_mode; /* EDAC mode for this dimm */
>
> - u32 nr_pages; /* number of pages in csrow */
> + u32 nr_pages; /* number of pages on this dimm */
>
> - u32 ce_count; /* Correctable Errors for this dimm */
> + unsigned csrow, cschannel; /* Points to the old API data */
> };
>
> /**
> @@ -443,9 +445,10 @@ struct dimm_info {
> */
> struct rank_info {
> int chan_idx;
> - u32 ce_count;
> struct csrow_info *csrow;
> struct dimm_info *dimm;
> +
> + u32 ce_count; /* Correctable Errors for this csrow */
> };
>
> struct csrow_info {
> @@ -497,6 +500,11 @@ struct mcidev_sysfs_attribute {
> ssize_t (*store)(struct mem_ctl_info *, const char *,size_t);
> };
>
> +struct edac_hierarchy {
> + char *name;
> + unsigned nr;
> +};
> +
> /* MEMORY controller information structure
> */
> struct mem_ctl_info {
> @@ -541,13 +549,16 @@ struct mem_ctl_info {
> unsigned long (*ctl_page_to_phys) (struct mem_ctl_info * mci,
> unsigned long page);
> int mc_idx;
> - int nr_csrows;
> struct csrow_info *csrows;
> + unsigned nr_csrows, num_cschannel;
>
> + /* Memory Controller hierarchy */
> + unsigned n_layers;
> + struct edac_mc_layer *layers;
> /*
> * DIMM info. Will eventually remove the entire csrows_info some day
> */
> - unsigned nr_dimms;
> + unsigned tot_dimms;
> struct dimm_info *dimms;
>
> /*
> @@ -562,12 +573,15 @@ struct mem_ctl_info {
> const char *dev_name;
> char proc_name[MC_PROC_NAME_MAX_LEN + 1];
> void *pvt_info;
> - u32 ue_noinfo_count; /* Uncorrectable Errors w/o info */
> - u32 ce_noinfo_count; /* Correctable Errors w/o info */
> - u32 ue_count; /* Total Uncorrectable Errors for this MC */
> - u32 ce_count; /* Total Correctable Errors for this MC */
> + u32 ue_count; /* Total Uncorrectable Errors for this MC */
> + u32 ce_count; /* Total Correctable Errors for this MC */
> unsigned long start_time; /* mci load start time (in jiffies) */
>
> + /* drivers shouldn't access this struct directly */
> + unsigned ce_noinfo_count, ue_noinfo_count;
> + unsigned ce_mc, ue_mc;
> + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
> +
> struct completion complete;
>
> /* edac sysfs device control */
> @@ -580,7 +594,7 @@ struct mem_ctl_info {
> * by the low level driver.
> *
> * Set by the low level driver to provide attributes at the
> - * controller level, same level as 'ue_count' and 'ce_count' above.
> + * controller level.
> * An array of structures, NULL terminated
> *
> * If attributes are desired, then set to array of attributes
> --
> 1.7.8
>
>
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Joe Perches @ 2012-04-27 14:11 UTC (permalink / raw)
To: Borislav Petkov
Cc: Mauro Carvalho Chehab, Jason Uhlenkott, Aristeu Rozanski,
Hitoshi Mitake, Shaohui Xie, Mark Gross, Dmitry Eremin-Solenikov,
Ranganathan Desikan, Egor Martovetsky, Niklas Söderlund,
Tim Small, Arvind R., Chris Metcalf, Doug Thompson, Andrew Morton,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Olof Johansson, linuxppc-dev
In-Reply-To: <20120427133304.GE9626@aftab.osrc.amd.com>
On Fri, 2012-04-27 at 15:33 +0200, Borislav Petkov wrote:
> this patch gives
>
> [ 8.278399] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
One too many __func__'s in some combination of the
pr_fmt and/or dbg call and/or the actual call site?
> > diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
[]
> > @@ -447,8 +447,13 @@ static inline void pci_write_bits32(struct pci_dev *pdev, int offset,
> >
> > #endif /* CONFIG_PCI */
> >
> > -extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> > - unsigned nr_chans, int edac_index);
> > +struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> > + unsigned nr_chans, int edac_index);
>
> Why not "extern"?
Using extern function prototypes in .h files
isn't generally necessary nor is extern the
more common kernel style.
> > +static inline void edac_mc_handle_ce(struct mem_ctl_info *mci,
> > unsigned long page_frame_number,
> > unsigned long offset_in_page,
> > unsigned long syndrome, int row, int channel,
> > - const char *msg);
>
> Strange alignment, pls do
>
> static inline void edac_mc_handle_ce(struct...,
> unsigned...,
> ...,
> ...);
>
or
static inline
void edac_mc_handle_ce(struct ..., etc)
or
static inline void
edac_mc_handle_ce(...)
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-27 15:12 UTC (permalink / raw)
To: Joe Perches
Cc: Mauro Carvalho Chehab, Jason Uhlenkott, Aristeu Rozanski,
Hitoshi Mitake, Shaohui Xie, Mark Gross, Dmitry Eremin-Solenikov,
Ranganathan Desikan, Borislav Petkov, Egor Martovetsky,
Niklas Söderlund, Tim Small, Arvind R., Chris Metcalf,
Doug Thompson, Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Olof Johansson, Andrew Morton,
linuxppc-dev
In-Reply-To: <1335535895.25521.7.camel@joe2Laptop>
On Fri, Apr 27, 2012 at 10:11:35AM -0400, Joe Perches wrote:
> > > -extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> > > - unsigned nr_chans, int edac_index);
> > > +struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
> > > + unsigned nr_chans, int edac_index);
> >
> > Why not "extern"?
>
> Using extern function prototypes in .h files
> isn't generally necessary nor is extern the
> more common kernel style.
Searching for it, there's this discussion, for example:
http://gcc.gnu.org/ml/gcc/2009-04/msg00812.html
Maybe we should put a small note in Documentation/CodingStyle what the
kernel preference is and hold people to it.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-27 15:36 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <20120427133304.GE9626@aftab.osrc.amd.com>
Em 27-04-2012 10:33, Borislav Petkov escreveu:
> Btw,
>
> this patch gives
>
> [ 8.278399] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
> [ 8.287594] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 1: dimm1 (0:1:0): row 0, chan 1
> [ 8.296784] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 2: dimm2 (1:0:0): row 1, chan 0
> [ 8.305968] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 3: dimm3 (1:1:0): row 1, chan 1
> [ 8.315144] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 4: dimm4 (2:0:0): row 2, chan 0
> [ 8.324326] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 5: dimm5 (2:1:0): row 2, chan 1
> [ 8.333502] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 6: dimm6 (3:0:0): row 3, chan 0
> [ 8.342684] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 7: dimm7 (3:1:0): row 3, chan 1
> [ 8.351860] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 8: dimm8 (4:0:0): row 4, chan 0
> [ 8.361049] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 9: dimm9 (4:1:0): row 4, chan 1
> [ 8.370227] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 10: dimm10 (5:0:0): row 5, chan 0
> [ 8.379582] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 11: dimm11 (5:1:0): row 5, chan 1
> [ 8.388941] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 12: dimm12 (6:0:0): row 6, chan 0
> [ 8.398315] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 13: dimm13 (6:1:0): row 6, chan 1
> [ 8.407680] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 14: dimm14 (7:0:0): row 7, chan 0
> [ 8.417047] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 15: dimm15 (7:1:0): row 7, chan 1
>
> and the memory controller has the following chip selects
>
> [ 8.137662] EDAC MC: DCT0 chip selects:
> [ 8.150291] EDAC amd64: MC: 0: 2048MB 1: 2048MB
> [ 8.155349] EDAC amd64: MC: 2: 2048MB 3: 2048MB
> [ 8.160408] EDAC amd64: MC: 4: 0MB 5: 0MB
> [ 8.165475] EDAC amd64: MC: 6: 0MB 7: 0MB
> [ 8.180499] EDAC MC: DCT1 chip selects:
> [ 8.184693] EDAC amd64: MC: 0: 2048MB 1: 2048MB
> [ 8.189753] EDAC amd64: MC: 2: 2048MB 3: 2048MB
> [ 8.194812] EDAC amd64: MC: 4: 0MB 5: 0MB
> [ 8.199875] EDAC amd64: MC: 6: 0MB 7: 0MB
>
> Those are 4 dual-ranked DIMMs on this node, DCT0 is one channel and DCT1
> is another and I have 4 ranks per channel. Having dimm0-dimm15 is very
> misleading and has nothing to do with the reality. So, if this is to use
> your nomenclature with layers, I'll have dimm0-dimm7 where each dimm is
> a rank.
>
> Or, the most correct thing to do would be to have dimm0-dimm3, each
> dual-ranked.
>
> So either tot_dimms is computed wrongly or there's a more serious error
> somewhere.
>
> I've reviewed almost the half patch, will review the rest when/if we
> sort out the above issue first.
>
> Thanks.
The fix for it were in another patch[1], as calling them as "rank" is needed
also at the sysfs API.
[1] http://lists-archives.com/linux-kernel/27623222-edac-add-a-new-per-dimm-api-and-make-the-old-per-virtual-rank-api-obsolete.html
I can just merge the fix on this patch, with the enclosed diff.
Regards,
Mauro
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 4d4d8b7..e0d9481 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -86,7 +86,7 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)
debugf4("\tmci->edac_check = %p\n", mci->edac_check);
debugf3("\tmci->nr_csrows = %d, csrows = %p\n",
mci->nr_csrows, mci->csrows);
- debugf3("\tmci->nr_dimms = %d, dimns = %p\n",
+ debugf3("\tmci->nr_dimms = %d, dimms = %p\n",
mci->tot_dimms, mci->dimms);
debugf3("\tdev = %p\n", mci->dev);
debugf3("\tmod_name:ctl_name = %s:%s\n", mci->mod_name, mci->ctl_name);
@@ -183,10 +183,6 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
* @size_pvt: size of private storage needed
*
*
- * FIXME: drivers handle multi-rank memories on different ways: on some
- * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
- * a single multi-rank DIMM would be mapped into several "dimms".
- *
* Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
* such DIMMS properly, but the CSROWS-based ones will likely do the wrong
* thing, as two chip select values are used for dual-rank memories (and 4, for
@@ -201,6 +197,12 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
*
* Use edac_mc_free() to free mc structures allocated by this function.
*
+ * NOTE: drivers handle multi-rank memories on different ways: on some
+ * drivers, one multi-rank memory is mapped as one entry, while, on others,
+ * a single multi-rank DIMM would be mapped into several entries. Currently,
+ * this function will allocate multiple struct dimm_info on such scenarios,
+ * as grouping the multiple ranks require drivers change.
+ *
* Returns:
* NULL allocation failed
* struct mem_ctl_info pointer
@@ -220,10 +222,11 @@ struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
void *pvt;
unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
- unsigned tot_csrows, tot_cschannels;
+ unsigned tot_csrows, tot_cschannels, tot_errcount = 0;
int i, j;
int err;
int row, chn;
+ bool per_rank = false;
BUG_ON(n_layers > EDAC_MAX_LAYERS);
/*
@@ -239,6 +242,9 @@ struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
tot_csrows *= layers[i].size;
else
tot_cschannels *= layers[i].size;
+
+ if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
+ per_rank = true;
}
/* Figure out the offsets of the various items from the start of an mc
@@ -254,14 +260,21 @@ struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
count = 1;
for (i = 0; i < n_layers; i++) {
count *= layers[i].size;
+ debugf4("%s: errcount layer %d size %d\n", __func__, i, count);
ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+ tot_errcount += 2 * count;
}
+
+ debugf4("%s: allocating %d error counters\n", __func__, tot_errcount);
pvt = edac_align_ptr(&ptr, sz_pvt, 1);
size = ((unsigned long)pvt) + sz_pvt;
- debugf1("%s(): allocating %u bytes for mci data (%d dimms, %d csrows/channels)\n",
- __func__, size, tot_dimms, tot_csrows * tot_cschannels);
+ debugf1("%s(): allocating %u bytes for mci data (%d %s, %d csrows/channels)\n",
+ __func__, size,
+ tot_dimms,
+ per_rank ? "ranks" : "dimms",
+ tot_csrows * tot_cschannels);
mci = kzalloc(size, GFP_KERNEL);
if (mci == NULL)
return NULL;
@@ -290,6 +303,7 @@ struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
memcpy(mci->layers, layers, sizeof(*lay) * n_layers);
mci->nr_csrows = tot_csrows;
mci->num_cschannel = tot_cschannels;
+ mci->mem_is_per_rank = per_rank;
/*
* Fills the csrow struct
@@ -315,15 +329,16 @@ struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
memset(&pos, 0, sizeof(pos));
row = 0;
chn = 0;
- debugf4("%s: initializing %d dimms\n", __func__, tot_dimms);
+ debugf4("%s: initializing %d %s\n", __func__, tot_dimms,
+ per_rank ? "ranks" : "dimms");
for (i = 0; i < tot_dimms; i++) {
chan = &csi[row].channels[chn];
dimm = EDAC_DIMM_PTR(lay, mci->dimms, n_layers,
pos[0], pos[1], pos[2]);
dimm->mci = mci;
- debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
- i, (dimm - mci->dimms),
+ debugf2("%s: %d: %s%zd (%d:%d:%d): row %d, chan %d\n", __func__,
+ i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
pos[0], pos[1], pos[2], row, chn);
/* Copy DIMM location */
@@ -1040,8 +1055,10 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
* get csrow/channel of the dimm, in order to allow
* incrementing the compat API counters
*/
- debugf4("%s: dimm csrows (%d,%d)\n",
- __func__, dimm->csrow, dimm->cschannel);
+ debugf4("%s: %s csrows map: (%d,%d)\n",
+ __func__,
+ mci->mem_is_per_rank ? "rank" : "dimm",
+ dimm->csrow, dimm->cschannel);
if (row == -1)
row = dimm->csrow;
else if (row >= 0 && row != dimm->csrow)
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 412d5cd..2b66109 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -555,6 +555,8 @@ struct mem_ctl_info {
/* Memory Controller hierarchy */
unsigned n_layers;
struct edac_mc_layer *layers;
+ bool mem_is_per_rank;
+
/*
* DIMM info. Will eventually remove the entire csrows_info some day
*/
^ permalink raw reply related
* Re: Regression in 32-bit ppc kernel
From: Larry Finger @ 2012-04-27 15:38 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras, LKML
In-Reply-To: <1335390257.21961.53.camel@pasglop>
On 04/25/2012 04:44 PM, Benjamin Herrenschmidt wrote:
>
> Do we know what the bad interrupt maps to ? Also what is the value of
> NR_IRQ and do you have SPARSE_IRQ enabled ? Can you try with the latter
> disabled and NR_IRQ set to something large, such as 128 ?
>
> (You may be able to check the interrupt mapping in debugfs)
Sorry, I was unable to find anything in debugfs to help me learn about interrupt
mapping. The value of CONFIG_NR_IRQS is already 512. I have not tried reducing
it to 128. The setting for CONFIG_SPARSE_IRQ was on, and changing it to off did
not make any difference.
I finished the bisection, which led to
commit a79dd5ae5a8f49688d65b89a859f2b98a7ee5538
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu Dec 15 11:13:03 2011 +1100
tty/serial/pmac_zilog: Fix suspend & resume
As this seemed to be an improbable result, I did the full test by checking out
the previous commit (43ca5d3). That resulted in a "good" result. Then I used
quilt to add commit a79dd5a as a patch and the fault returned. I then noticed
that you said in the commit message that "I removed some code for handling
unexpected interrupt which should never be hit...". It appears that my box does
indeed hit such an unexpected interrupt.
I could always get rid of the fault by disabling CONFIG_SERIAL_PMACZILOG, but I
would like to fix the problem if possible.
Thanks,
Larry
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-27 16:07 UTC (permalink / raw)
To: Joe Perches
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Borislav Petkov, Egor Martovetsky, Niklas Söderlund,
Tim Small, Arvind R., Chris Metcalf, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Olof Johansson, Andrew Morton,
linuxppc-dev
In-Reply-To: <1335535895.25521.7.camel@joe2Laptop>
Em 27-04-2012 11:11, Joe Perches escreveu:
> On Fri, 2012-04-27 at 15:33 +0200, Borislav Petkov wrote:
>> this patch gives
>>
>> [ 8.278399] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
>
> One too many __func__'s in some combination of the
> pr_fmt and/or dbg call and/or the actual call site?
Yes. This is a common issue at the EDAC core: on several places, it calls the
edac debug macros (DEBUGF0...DEBUGF4) passing a __func__ as an argument, while
the debug macros already handles that. I suspect that, in the past, the __func__
were not at the macros, but some patch added it there, and forgot to fix the
occurrences of its call.
This is something that needs to be reviewed at the entire EDAC core (and likely
at the drivers).
I opted to not touch on this at the existing debug logic, as I think that the
better is to address all those issues on one separate patch, after fixing the
EDAC core bugs.
>
>>> diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
> []
>>> @@ -447,8 +447,13 @@ static inline void pci_write_bits32(struct pci_dev *pdev, int offset,
>>>
>>> #endif /* CONFIG_PCI */
>>>
>>> -extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
>>> - unsigned nr_chans, int edac_index);
>>> +struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
>>> + unsigned nr_chans, int edac_index);
>>
>> Why not "extern"?
>
> Using extern function prototypes in .h files
> isn't generally necessary nor is extern the
> more common kernel style.
Yes. I never add extern on the code I write.
While CodingStyle doesn't explicitly say anything about that, its spirit
seem to indicate to that the right thing is avoid using it, like, for
example:
"Chapter 4: Naming
C is a Spartan language, and so should your naming be."
(also on other places, like avoiding to use {} for single-statement if's).
So, useless clauses like "extern" doesn't seem to be the best choice.
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-27 17:52 UTC (permalink / raw)
To: Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <20120427133304.GE9626@aftab.osrc.amd.com>
Em 27-04-2012 10:33, Borislav Petkov escreveu:
> Btw,
>
> this patch gives
>
> [ 8.278399] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: dimm0 (0:0:0): row 0, chan 0
> [ 8.287594] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 1: dimm1 (0:1:0): row 0, chan 1
> [ 8.296784] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 2: dimm2 (1:0:0): row 1, chan 0
> [ 8.305968] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 3: dimm3 (1:1:0): row 1, chan 1
> [ 8.315144] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 4: dimm4 (2:0:0): row 2, chan 0
> [ 8.324326] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 5: dimm5 (2:1:0): row 2, chan 1
> [ 8.333502] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 6: dimm6 (3:0:0): row 3, chan 0
> [ 8.342684] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 7: dimm7 (3:1:0): row 3, chan 1
> [ 8.351860] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 8: dimm8 (4:0:0): row 4, chan 0
> [ 8.361049] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 9: dimm9 (4:1:0): row 4, chan 1
> [ 8.370227] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 10: dimm10 (5:0:0): row 5, chan 0
> [ 8.379582] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 11: dimm11 (5:1:0): row 5, chan 1
> [ 8.388941] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 12: dimm12 (6:0:0): row 6, chan 0
> [ 8.398315] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 13: dimm13 (6:1:0): row 6, chan 1
> [ 8.407680] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 14: dimm14 (7:0:0): row 7, chan 0
> [ 8.417047] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 15: dimm15 (7:1:0): row 7, chan 1
>
> and the memory controller has the following chip selects
>
> [ 8.137662] EDAC MC: DCT0 chip selects:
> [ 8.150291] EDAC amd64: MC: 0: 2048MB 1: 2048MB
> [ 8.155349] EDAC amd64: MC: 2: 2048MB 3: 2048MB
> [ 8.160408] EDAC amd64: MC: 4: 0MB 5: 0MB
> [ 8.165475] EDAC amd64: MC: 6: 0MB 7: 0MB
> [ 8.180499] EDAC MC: DCT1 chip selects:
> [ 8.184693] EDAC amd64: MC: 0: 2048MB 1: 2048MB
> [ 8.189753] EDAC amd64: MC: 2: 2048MB 3: 2048MB
> [ 8.194812] EDAC amd64: MC: 4: 0MB 5: 0MB
> [ 8.199875] EDAC amd64: MC: 6: 0MB 7: 0MB
>
> Those are 4 dual-ranked DIMMs on this node, DCT0 is one channel and DCT1
> is another and I have 4 ranks per channel. Having dimm0-dimm15 is very
> misleading and has nothing to do with the reality. So, if this is to use
> your nomenclature with layers, I'll have dimm0-dimm7 where each dimm is
> a rank.
>
> Or, the most correct thing to do would be to have dimm0-dimm3, each
> dual-ranked.
>
> So either tot_dimms is computed wrongly or there's a more serious error
> somewhere.
>
> I've reviewed almost the half patch, will review the rest when/if we
> sort out the above issue first.
>
> Thanks.
>
> On Tue, Apr 24, 2012 at 03:15:41PM -0300, Mauro Carvalho Chehab wrote:
>> Change the EDAC internal representation to work with non-csrow
>> based memory controllers.
>>
>> There are lots of those memory controllers nowadays, and more
>> are coming. So, the EDAC internal representation needs to be
>> changed, in order to work with those memory controllers, while
>> preserving backward compatibility with the old ones.
>>
>> The edac core were written with the idea that memory controllers
>
> was
>
>> are able to directly access csrows, and that the channels are
>> used inside a csrows select.
>
> This sounds funny, simply remove that second part about the channels.
>
>> This is not true for FB-DIMM and RAMBUS memory controllers.
>>
>> Also, some recent advanced memory controllers don't present a per-csrows
>> view. Instead, they view memories as DIMM's, instead of ranks, accessed
>
> DIMMs instead of ranks."
>
> Remove the rest.
>
>> via csrow/channel.
>>
>> So, change the allocation and error report routines to allow
>> them to work with all types of architectures.
>>
>> This will allow the removal of several hacks on FB-DIMM and RAMBUS
>
> with
>
>> memory controllers on the next patches.
>
> . Remove the rest.
>
>>
>> Also, several tests were done on different platforms using different
>> x86 drivers.
>>
>> TODO: a multi-rank DIMM's are currently represented by multiple DIMM
>
> Multi-rank DIMMs
>
>> entries at struct dimm_info. That means that changing a label for one
>
> in
>
>> rank won't change the same label for the other ranks at the same dimm.
>
> of the same DIMM.
>
>> Such bug is there since the beginning of the EDAC, so it is not a big
>
> This bug is present ..
>
>> deal. However, on several drivers, it is possible to fix this issue, but
>
> remove "on"
>
>> it should be a per-driver fix, as the csrow => DIMM arrangement may not
>> be equal for all. So, don't try to fix it here yet.
>>
>> PS.: I tried to make this patch as short as possible, preceding it with
>
> Remove "PS."
>
>> several other patches that simplified the logic here. Yet, as the
>> internal API changes, all drivers need changes. The changes are
>> generally bigger on the drivers for FB-DIMM's.
>
> in for FB-DIMMs.
>
>>
>> FIXME: while the FB-DIMMs are not converted to use the new
>> design, uncorrected errors will show just one channel. In
>> the past, all changes were on a big patch with about 150K.
>> As it needed to be split, in order to be accepted by the
>> EDAC ML at vger, we've opted to have this small drawback.
>> As an advantage, it is now easier to review the patch series.
>
> This whole paragraph above doesn't have anything to do with what the
> patch does, so it can go.
>
> [..]
>
>> ---
>>
>> v16: Only context changes
>>
>> drivers/edac/edac_core.h | 92 ++++++-
>> drivers/edac/edac_mc.c | 682 ++++++++++++++++++++++++++++------------------
>> include/linux/edac.h | 40 ++-
>> 3 files changed, 526 insertions(+), 288 deletions(-)
>>
>> diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
>> index e48ab31..7201bb1 100644
>> --- a/drivers/edac/edac_core.h
>> +++ b/drivers/edac/edac_core.h
>> @@ -447,8 +447,13 @@ static inline void pci_write_bits32(struct pci_dev *pdev, int offset,
>>
>> #endif /* CONFIG_PCI */
>>
>> -extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
>> - unsigned nr_chans, int edac_index);
>> +struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
>> + unsigned nr_chans, int edac_index);
>
> Why not "extern"?
>
>> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
>> + unsigned n_layers,
>> + struct edac_mc_layer *layers,
>> + bool rev_order,
>> + unsigned sz_pvt);
>
> ditto.
>
>> extern int edac_mc_add_mc(struct mem_ctl_info *mci);
>> extern void edac_mc_free(struct mem_ctl_info *mci);
>> extern struct mem_ctl_info *edac_mc_find(int idx);
>> @@ -467,24 +472,80 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
>> * reporting logic and function interface - reduces conditional
>> * statement clutter and extra function arguments.
>> */
>> -extern void edac_mc_handle_ce(struct mem_ctl_info *mci,
>> +
>> +void edac_mc_handle_error(const enum hw_event_mc_err_type type,
>> + struct mem_ctl_info *mci,
>> + const unsigned long page_frame_number,
>> + const unsigned long offset_in_page,
>> + const unsigned long syndrome,
>> + const int layer0,
>> + const int layer1,
>> + const int layer2,
>> + const char *msg,
>> + const char *other_detail,
>> + const void *mcelog);
>
> Why isn't this one "extern" either?
>
>> +
>> +static inline void edac_mc_handle_ce(struct mem_ctl_info *mci,
>> unsigned long page_frame_number,
>> unsigned long offset_in_page,
>> unsigned long syndrome, int row, int channel,
>> - const char *msg);
>
> Strange alignment, pls do
>
> static inline void edac_mc_handle_ce(struct...,
> unsigned...,
> ...,
> ...);
>
>
>> -extern void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
>> - const char *msg);
>> -extern void edac_mc_handle_ue(struct mem_ctl_info *mci,
>> + const char *msg)
>> +{
>> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
>> + page_frame_number, offset_in_page, syndrome,
>> + row, channel, -1, msg, NULL, NULL);
>> +}
>> +
>> +static inline void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
>> + const char *msg)
>
> ditto.
>
>> +{
>> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
>> + 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
>> +}
>> +
>> +static inline void edac_mc_handle_ue(struct mem_ctl_info *mci,
>> unsigned long page_frame_number,
>> unsigned long offset_in_page, int row,
>> - const char *msg);
>
> ditto.
>
>> -extern void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
>> - const char *msg);
>> -extern void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci, unsigned int csrow,
>> - unsigned int channel0, unsigned int channel1,
>> - char *msg);
>> -extern void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci, unsigned int csrow,
>> - unsigned int channel, char *msg);
>> + const char *msg)
>> +{
>> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
>> + page_frame_number, offset_in_page, 0,
>> + row, -1, -1, msg, NULL, NULL);
>> +}
>> +
>> +static inline void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
>> + const char *msg)
>> +{
>> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
>> + 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
>> +}
>> +
>> +static inline void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
>> + unsigned int csrow,
>> + unsigned int channel0,
>> + unsigned int channel1,
>> + char *msg)
>
> Now this alignment looks correct.
>
>> +{
>> + /*
>> + *FIXME: The error can also be at channel1 (e. g. at the second
>> + * channel of the same branch). The fix is to push
>> + * edac_mc_handle_error() call into each driver
>> + */
>> + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
>> + 0, 0, 0,
>> + csrow, channel0, -1, msg, NULL, NULL);
>> +}
>> +
>> +static inline void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
>> + unsigned int csrow,
>> + unsigned int channel, char *msg)
>> +{
>> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
>> + 0, 0, 0,
>> + csrow, channel, -1, msg, NULL, NULL);
>> +}
>> +
>> +
>
> Two superfluous newlines.
Fixed all above (except for the "extern").
>
>>
>> /*
>> * edac_device APIs
>> @@ -496,6 +557,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
>> extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
>> int inst_nr, int block_nr, const char *msg);
>> extern int edac_device_alloc_index(void);
>> +extern const char *edac_layer_name[];
>>
>> /*
>> * edac_pci APIs
>> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
>> index 6ec967a..4d4d8b7 100644
>> --- a/drivers/edac/edac_mc.c
>> +++ b/drivers/edac/edac_mc.c
>> @@ -44,9 +44,25 @@ static void edac_mc_dump_channel(struct rank_info *chan)
>> debugf4("\tchannel = %p\n", chan);
>> debugf4("\tchannel->chan_idx = %d\n", chan->chan_idx);
>> debugf4("\tchannel->csrow = %p\n\n", chan->csrow);
>> - debugf4("\tdimm->ce_count = %d\n", chan->dimm->ce_count);
>> - debugf4("\tdimm->label = '%s'\n", chan->dimm->label);
>> - debugf4("\tdimm->nr_pages = 0x%x\n", chan->dimm->nr_pages);
>> + debugf4("\tchannel->dimm = %p\n", chan->dimm);
>> +}
>> +
>> +static void edac_mc_dump_dimm(struct dimm_info *dimm)
>> +{
>> + int i;
>> +
>> + debugf4("\tdimm = %p\n", dimm);
>> + debugf4("\tdimm->label = '%s'\n", dimm->label);
>> + debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
>> + debugf4("\tdimm location ");
>> + for (i = 0; i < dimm->mci->n_layers; i++) {
>> + printk(KERN_CONT "%d", dimm->location[i]);
>> + if (i < dimm->mci->n_layers - 1)
>> + printk(KERN_CONT ".");
>> + }
>> + printk(KERN_CONT "\n");
>
> This looks hacky but I don't have a good suggestion what to do instead
> here. Maybe snprintf into a complete string which you can issue with
> debugf4()...
This is not hacky. There are several places at the Kernel doing loops like
that. Look, for example, at lib/hexdump.c (without KERN_CONT, as this
macro was added later - probably to avoid checkpatch.pl complains).
>> + debugf4("\tdimm->grain = %d\n", dimm->grain);
>> + debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
>> }
>>
>> static void edac_mc_dump_csrow(struct csrow_info *csrow)
>> @@ -70,6 +86,8 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)
>> debugf4("\tmci->edac_check = %p\n", mci->edac_check);
>> debugf3("\tmci->nr_csrows = %d, csrows = %p\n",
>> mci->nr_csrows, mci->csrows);
>> + debugf3("\tmci->nr_dimms = %d, dimns = %p\n",
>
> ->tot_dimms dimms
Fixed.
>
>> + mci->tot_dimms, mci->dimms);
>> debugf3("\tdev = %p\n", mci->dev);
>> debugf3("\tmod_name:ctl_name = %s:%s\n", mci->mod_name, mci->ctl_name);
>> debugf3("\tpvt_info = %p\n\n", mci->pvt_info);
>> @@ -157,10 +175,25 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
>> }
>>
>> /**
>> - * edac_mc_alloc: Allocate a struct mem_ctl_info structure
>> - * @size_pvt: size of private storage needed
>> - * @nr_csrows: Number of CWROWS needed for this MC
>> - * @nr_chans: Number of channels for the MC
>> + * edac_mc_alloc: Allocate and partially fills a struct mem_ctl_info structure
>
> fill
>
>> + * @edac_index: Memory controller number
>> + * @n_layers: Number of layers at the MC hierarchy
>
> Number of MC hierarchy layers
>
>> + * layers: Describes each layer as seen by the Memory Controller
>> + * @rev_order: Fills csrows/cs channels at the reverse order
>
> csrows/channels in reverse order
>
>> + * @size_pvt: size of private storage needed
>> + *
>> + *
>> + * FIXME: drivers handle multi-rank memories on different ways: on some
>
> in in
>
>> + * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
>
> memory stick in
>
>> + * a single multi-rank DIMM would be mapped into several "dimms".
>
> memory stick
>
>> + *
>> + * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
>> + * such DIMMS properly, but the CSROWS-based ones will likely do the wrong
>
> csrow-based
>
>> + * thing, as two chip select values are used for dual-rank memories (and 4, for
>> + * quad-rank ones). I suspect that this issue could be solved inside the EDAC
>> + * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
>> + *
>> + * In summary, solving this issue is not easy, as it requires a lot of testing.
>> *
>> * Everything is kmalloc'ed as one big chunk - more efficient.
>> * Only can be used if all structures have the same lifetime - otherwise
>> @@ -172,18 +205,41 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
>> * NULL allocation failed
>> * struct mem_ctl_info pointer
>> */
>> -struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
>> - unsigned nr_chans, int edac_index)
>> +struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
>> + unsigned n_layers,
>> + struct edac_mc_layer *layers,
>> + bool rev_order,
>> + unsigned sz_pvt)
>
> strange function argument vertical alignment
>
>> {
>> void *ptr = NULL;
>> struct mem_ctl_info *mci;
>> - struct csrow_info *csi, *csrow;
>> + struct edac_mc_layer *lay;
>
> As before, call this "layers" pls.
>
>> + struct csrow_info *csi, *csr;
>> struct rank_info *chi, *chp, *chan;
>> struct dimm_info *dimm;
>> + u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
>> void *pvt;
>> - unsigned size;
>> - int row, chn;
>> + unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
>> + unsigned tot_csrows, tot_cschannels;
>
> No need to call this "tot_cschannels" - "tot_channels" should be enough.
>
>> + int i, j;
>> int err;
>> + int row, chn;
>
> All those local variables should be sorted in a reverse christmas tree
> order:
>
> u32 this_is_the_longest_array_name[LENGTH];
> void *shorter_named_variable;
> unsigned long size;
> int i;
>
> ...
Why? There's nothing at the CodingStyle saying about how the vars should
be ordered. If you want to enforce some particular order, please do it
yourself, but apply it consistently among the entire subsystem.
>
>> +
>> + BUG_ON(n_layers > EDAC_MAX_LAYERS);
>
>
> Push this BUG_ON up into edac_mc_alloc as the first thing this function
> does.
It is already the first thing at the function.
> Also, is it valid to have n_layers == 0? The memcpy call below
> will do nothing.
Changed to:
BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
>> + /*
>> + * Calculate the total amount of dimms and csrows/cschannels while
>> + * in the old API emulation mode
>> + */
>> + tot_dimms = 1;
>> + tot_cschannels = 1;
>> + tot_csrows = 1;
>
> Those initializations can be done above at variable declaration time.
Yes, but the compiled code will be the same anyway, as gcc will optimize
it, either by using registers for those vars or by moving the initialization
to the top of the function.
This function is too complex, so it is better to initialize those vars
just before the loops that are calculating those totals.
>
>> + for (i = 0; i < n_layers; i++) {
>> + tot_dimms *= layers[i].size;
>> + if (layers[i].is_virt_csrow)
>> + tot_csrows *= layers[i].size;
>> + else
>> + tot_cschannels *= layers[i].size;
>> + }
>>
>> /* Figure out the offsets of the various items from the start of an mc
>> * structure. We want the alignment of each item to be at least as
>> @@ -191,12 +247,21 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
>> * hardcode everything into a single struct.
>> */
>> mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
>> - csi = edac_align_ptr(&ptr, sizeof(*csi), nr_csrows);
>> - chi = edac_align_ptr(&ptr, sizeof(*chi), nr_csrows * nr_chans);
>> - dimm = edac_align_ptr(&ptr, sizeof(*dimm), nr_csrows * nr_chans);
>> + lay = edac_align_ptr(&ptr, sizeof(*lay), n_layers);
>> + csi = edac_align_ptr(&ptr, sizeof(*csi), tot_csrows);
>> + chi = edac_align_ptr(&ptr, sizeof(*chi), tot_csrows * tot_cschannels);
>> + dimm = edac_align_ptr(&ptr, sizeof(*dimm), tot_dimms);
>> + count = 1;
>
> ditto.
>> + for (i = 0; i < n_layers; i++) {
>> + tot_dimms *= layers[i].size;
>> + if (layers[i].is_virt_csrow)
>> + tot_csrows *= layers[i].size;
>> + else
>> + tot_cschannels *= layers[i].size;
>> + }
Ditto: let gcc optimize it.
Spreading the 'count' match code will only make harder for a reviewer to
actually see what's there.
At assember, count will likely be optimized as a register anyway.
<removed the rest of the email, as there aren't any comments after that point>
Patches with all the fixes is enclosed.
--
[PATCH EDACv17] edac: Change internal representation to work with layers
Change the EDAC internal representation to work with non-csrow
based memory controllers.
There are lots of those memory controllers nowadays, and more
are coming. So, the EDAC internal representation needs to be
changed, in order to work with those memory controllers, while
preserving backward compatibility with the old ones.
The edac core was written with the idea that memory controllers
are able to directly access csrows.
This is not true for FB-DIMM and RAMBUS memory controllers.
Also, some recent advanced memory controllers don't present a per-csrows
view. Instead, they view memories as DIMMs, instead of ranks.
So, change the allocation and error report routines to allow
them to work with all types of architectures.
This will allow the removal of several hacks with FB-DIMM and RAMBUS
memory controllers.
Also, several tests were done on different platforms using different
x86 drivers.
TODO: a multi-rank DIMMs are currently represented by multiple DIMM
entries in struct dimm_info. That means that changing a label for one
rank won't change the same label for the other ranks at the same DIMM.
This bug is present since the beginning of the EDAC, so it is not a big
deal. However, on several drivers, it is possible to fix this issue, but
it should be a per-driver fix, as the csrow => DIMM arrangement may not
be equal for all. So, don't try to fix it here yet.
I tried to make this patch as short as possible, preceding it with
several other patches that simplified the logic here. Yet, as the
internal API changes, all drivers need changes. The changes are
generally bigger in the drivers for FB-DIMMs.
Cc: Aristeu Rozanski <arozansk@redhat.com>
Cc: Doug Thompson <norsk5@yahoo.com>
Cc: Borislav Petkov <borislav.petkov@amd.com>
Cc: Mark Gross <mark.gross@intel.com>
Cc: Jason Uhlenkott <juhlenko@akamai.com>
Cc: Tim Small <tim@buttersideup.com>
Cc: Ranganathan Desikan <ravi@jetztechnologies.com>
Cc: "Arvind R." <arvino55@gmail.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Egor Martovetsky <egor@pasemi.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Joe Perches <joe@perches.com>
Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hitoshi Mitake <h.mitake@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Niklas Söderlund" <niklas.soderlund@ericsson.com>
Cc: Shaohui Xie <Shaohui.Xie@freescale.com>
Cc: Josh Boyer <jwboyer@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
v17: Several cosmetic changes.
diff --git a/drivers/edac/edac_core.h b/drivers/edac/edac_core.h
index e48ab31..b2dfdf5 100644
--- a/drivers/edac/edac_core.h
+++ b/drivers/edac/edac_core.h
@@ -447,8 +447,13 @@ static inline void pci_write_bits32(struct pci_dev *pdev, int offset,
#endif /* CONFIG_PCI */
-extern struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
- unsigned nr_chans, int edac_index);
+struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
+ unsigned nr_chans, int edac_index);
+struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
+ unsigned n_layers,
+ struct edac_mc_layer *layers,
+ bool rev_order,
+ unsigned sz_pvt);
extern int edac_mc_add_mc(struct mem_ctl_info *mci);
extern void edac_mc_free(struct mem_ctl_info *mci);
extern struct mem_ctl_info *edac_mc_find(int idx);
@@ -467,24 +472,78 @@ extern int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci,
* reporting logic and function interface - reduces conditional
* statement clutter and extra function arguments.
*/
-extern void edac_mc_handle_ce(struct mem_ctl_info *mci,
- unsigned long page_frame_number,
- unsigned long offset_in_page,
- unsigned long syndrome, int row, int channel,
- const char *msg);
-extern void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
- const char *msg);
-extern void edac_mc_handle_ue(struct mem_ctl_info *mci,
- unsigned long page_frame_number,
- unsigned long offset_in_page, int row,
- const char *msg);
-extern void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
- const char *msg);
-extern void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci, unsigned int csrow,
- unsigned int channel0, unsigned int channel1,
- char *msg);
-extern void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci, unsigned int csrow,
- unsigned int channel, char *msg);
+
+void edac_mc_handle_error(const enum hw_event_mc_err_type type,
+ struct mem_ctl_info *mci,
+ const unsigned long page_frame_number,
+ const unsigned long offset_in_page,
+ const unsigned long syndrome,
+ const int layer0,
+ const int layer1,
+ const int layer2,
+ const char *msg,
+ const char *other_detail,
+ const void *mcelog);
+
+static inline void edac_mc_handle_ce(struct mem_ctl_info *mci,
+ unsigned long page_frame_number,
+ unsigned long offset_in_page,
+ unsigned long syndrome, int row, int channel,
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+ page_frame_number, offset_in_page, syndrome,
+ row, channel, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci,
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+ 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ue(struct mem_ctl_info *mci,
+ unsigned long page_frame_number,
+ unsigned long offset_in_page, int row,
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+ page_frame_number, offset_in_page, 0,
+ row, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci,
+ const char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+ 0, 0, 0, -1, -1, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
+ unsigned int csrow,
+ unsigned int channel0,
+ unsigned int channel1,
+ char *msg)
+{
+ /*
+ *FIXME: The error can also be at channel1 (e. g. at the second
+ * channel of the same branch). The fix is to push
+ * edac_mc_handle_error() call into each driver
+ */
+ edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+ 0, 0, 0,
+ csrow, channel0, -1, msg, NULL, NULL);
+}
+
+static inline void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
+ unsigned int csrow,
+ unsigned int channel, char *msg)
+{
+ edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+ 0, 0, 0,
+ csrow, channel, -1, msg, NULL, NULL);
+}
/*
* edac_device APIs
@@ -496,6 +555,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
int inst_nr, int block_nr, const char *msg);
extern int edac_device_alloc_index(void);
+extern const char *edac_layer_name[];
/*
* edac_pci APIs
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 6ec967a..a9f7650 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -44,9 +44,25 @@ static void edac_mc_dump_channel(struct rank_info *chan)
debugf4("\tchannel = %p\n", chan);
debugf4("\tchannel->chan_idx = %d\n", chan->chan_idx);
debugf4("\tchannel->csrow = %p\n\n", chan->csrow);
- debugf4("\tdimm->ce_count = %d\n", chan->dimm->ce_count);
- debugf4("\tdimm->label = '%s'\n", chan->dimm->label);
- debugf4("\tdimm->nr_pages = 0x%x\n", chan->dimm->nr_pages);
+ debugf4("\tchannel->dimm = %p\n", chan->dimm);
+}
+
+static void edac_mc_dump_dimm(struct dimm_info *dimm)
+{
+ int i;
+
+ debugf4("\tdimm = %p\n", dimm);
+ debugf4("\tdimm->label = '%s'\n", dimm->label);
+ debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
+ debugf4("\tdimm location ");
+ for (i = 0; i < dimm->mci->n_layers; i++) {
+ printk(KERN_CONT "%d", dimm->location[i]);
+ if (i < dimm->mci->n_layers - 1)
+ printk(KERN_CONT ".");
+ }
+ printk(KERN_CONT "\n");
+ debugf4("\tdimm->grain = %d\n", dimm->grain);
+ debugf4("\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
}
static void edac_mc_dump_csrow(struct csrow_info *csrow)
@@ -70,6 +86,8 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)
debugf4("\tmci->edac_check = %p\n", mci->edac_check);
debugf3("\tmci->nr_csrows = %d, csrows = %p\n",
mci->nr_csrows, mci->csrows);
+ debugf3("\tmci->nr_dimms = %d, dimms = %p\n",
+ mci->tot_dimms, mci->dimms);
debugf3("\tdev = %p\n", mci->dev);
debugf3("\tmod_name:ctl_name = %s:%s\n", mci->mod_name, mci->ctl_name);
debugf3("\tpvt_info = %p\n\n", mci->pvt_info);
@@ -157,10 +175,21 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
}
/**
- * edac_mc_alloc: Allocate a struct mem_ctl_info structure
- * @size_pvt: size of private storage needed
- * @nr_csrows: Number of CWROWS needed for this MC
- * @nr_chans: Number of channels for the MC
+ * edac_mc_alloc: Allocate and partially fill a struct mem_ctl_info structure
+ * @edac_index: Memory controller number
+ * @n_layers: Number of MC hierarchy layers
+ * layers: Describes each layer as seen by the Memory Controller
+ * @rev_order: Fills csrows/channels at the reverse order
+ * @size_pvt: size of private storage needed
+ *
+ *
+ * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
+ * such DIMMS properly, but the CSROWS-based ones will likely do the wrong
+ * thing, as two chip select values are used for dual-rank memories (and 4, for
+ * quad-rank ones). I suspect that this issue could be solved inside the EDAC
+ * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
+ *
+ * In summary, solving this issue is not easy, as it requires a lot of testing.
*
* Everything is kmalloc'ed as one big chunk - more efficient.
* Only can be used if all structures have the same lifetime - otherwise
@@ -168,22 +197,55 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
*
* Use edac_mc_free() to free mc structures allocated by this function.
*
+ * NOTE: drivers handle multi-rank memories in different ways: in some
+ * drivers, one multi-rank memory stick is mapped as one entry, while, in
+ * others, a single multi-rank memory stick would be mapped into several
+ * entries. Currently, this function will allocate multiple struct dimm_info
+ * on such scenarios, as grouping the multiple ranks require drivers change.
+ *
* Returns:
* NULL allocation failed
* struct mem_ctl_info pointer
*/
-struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
- unsigned nr_chans, int edac_index)
+struct mem_ctl_info *new_edac_mc_alloc(unsigned edac_index,
+ unsigned n_layers,
+ struct edac_mc_layer *layers,
+ bool rev_order,
+ unsigned sz_pvt)
{
void *ptr = NULL;
struct mem_ctl_info *mci;
- struct csrow_info *csi, *csrow;
+ struct edac_mc_layer *layer;
+ struct csrow_info *csi, *csr;
struct rank_info *chi, *chp, *chan;
struct dimm_info *dimm;
+ u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
void *pvt;
- unsigned size;
- int row, chn;
+ unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
+ unsigned tot_csrows, tot_channels, tot_errcount = 0;
+ int i, j;
int err;
+ int row, chn;
+ bool per_rank = false;
+
+ BUG_ON(n_layers > EDAC_MAX_LAYERS);
+ /*
+ * Calculate the total amount of dimms and csrows/cschannels while
+ * in the old API emulation mode
+ */
+ tot_dimms = 1;
+ tot_channels = 1;
+ tot_csrows = 1;
+ for (i = 0; i < n_layers; i++) {
+ tot_dimms *= layers[i].size;
+ if (layers[i].is_virt_csrow)
+ tot_csrows *= layers[i].size;
+ else
+ tot_channels *= layers[i].size;
+
+ if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
+ per_rank = true;
+ }
/* Figure out the offsets of the various items from the start of an mc
* structure. We want the alignment of each item to be at least as
@@ -191,12 +253,28 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
* hardcode everything into a single struct.
*/
mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
- csi = edac_align_ptr(&ptr, sizeof(*csi), nr_csrows);
- chi = edac_align_ptr(&ptr, sizeof(*chi), nr_csrows * nr_chans);
- dimm = edac_align_ptr(&ptr, sizeof(*dimm), nr_csrows * nr_chans);
+ layer = edac_align_ptr(&ptr, sizeof(*layer), n_layers);
+ csi = edac_align_ptr(&ptr, sizeof(*csi), tot_csrows);
+ chi = edac_align_ptr(&ptr, sizeof(*chi), tot_csrows * tot_channels);
+ dimm = edac_align_ptr(&ptr, sizeof(*dimm), tot_dimms);
+ count = 1;
+ for (i = 0; i < n_layers; i++) {
+ count *= layers[i].size;
+ debugf4("%s: errcount layer %d size %d\n", __func__, i, count);
+ ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+ ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+ tot_errcount += 2 * count;
+ }
+
+ debugf4("%s: allocating %d error counters\n", __func__, tot_errcount);
pvt = edac_align_ptr(&ptr, sz_pvt, 1);
size = ((unsigned long)pvt) + sz_pvt;
+ debugf1("%s(): allocating %u bytes for mci data (%d %s, %d csrows/channels)\n",
+ __func__, size,
+ tot_dimms,
+ per_rank ? "ranks" : "dimms",
+ tot_csrows * tot_channels);
mci = kzalloc(size, GFP_KERNEL);
if (mci == NULL)
return NULL;
@@ -204,42 +282,101 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
/* Adjust pointers so they point within the memory we just allocated
* rather than an imaginary chunk of memory located at address 0.
*/
+ layer = (struct edac_mc_layer *)(((char *)mci) + ((unsigned long)layer));
csi = (struct csrow_info *)(((char *)mci) + ((unsigned long)csi));
chi = (struct rank_info *)(((char *)mci) + ((unsigned long)chi));
dimm = (struct dimm_info *)(((char *)mci) + ((unsigned long)dimm));
+ for (i = 0; i < n_layers; i++) {
+ mci->ce_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ce_per_layer[i]));
+ mci->ue_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ue_per_layer[i]));
+ }
pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL;
/* setup index and various internal pointers */
mci->mc_idx = edac_index;
mci->csrows = csi;
mci->dimms = dimm;
+ mci->tot_dimms = tot_dimms;
mci->pvt_info = pvt;
- mci->nr_csrows = nr_csrows;
+ mci->n_layers = n_layers;
+ mci->layers = layer;
+ memcpy(mci->layers, layers, sizeof(*layer) * n_layers);
+ mci->nr_csrows = tot_csrows;
+ mci->num_cschannel = tot_channels;
+ mci->mem_is_per_rank = per_rank;
/*
- * For now, assumes that a per-csrow arrangement for dimms.
- * This will be latter changed.
+ * Fills the csrow struct
*/
- dimm = mci->dimms;
-
- for (row = 0; row < nr_csrows; row++) {
- csrow = &csi[row];
- csrow->csrow_idx = row;
- csrow->mci = mci;
- csrow->nr_channels = nr_chans;
- chp = &chi[row * nr_chans];
- csrow->channels = chp;
-
- for (chn = 0; chn < nr_chans; chn++) {
+ for (row = 0; row < tot_csrows; row++) {
+ csr = &csi[row];
+ csr->csrow_idx = row;
+ csr->mci = mci;
+ csr->nr_channels = tot_channels;
+ chp = &chi[row * tot_channels];
+ csr->channels = chp;
+
+ for (chn = 0; chn < tot_channels; chn++) {
chan = &chp[chn];
chan->chan_idx = chn;
- chan->csrow = csrow;
+ chan->csrow = csr;
+ }
+ }
- mci->csrows[row].channels[chn].dimm = dimm;
- dimm->csrow = row;
- dimm->csrow_channel = chn;
- dimm++;
- mci->nr_dimms++;
+ /*
+ * Fills the dimm struct
+ */
+ memset(&pos, 0, sizeof(pos));
+ row = 0;
+ chn = 0;
+ debugf4("%s: initializing %d %s\n", __func__, tot_dimms,
+ per_rank ? "ranks" : "dimms");
+ for (i = 0; i < tot_dimms; i++) {
+ chan = &csi[row].channels[chn];
+ dimm = EDAC_DIMM_PTR(layer, mci->dimms, n_layers,
+ pos[0], pos[1], pos[2]);
+ dimm->mci = mci;
+
+ debugf2("%s: %d: %s%zd (%d:%d:%d): row %d, chan %d\n", __func__,
+ i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
+ pos[0], pos[1], pos[2], row, chn);
+
+ /* Copy DIMM location */
+ for (j = 0; j < n_layers; j++)
+ dimm->location[j] = pos[j];
+
+ /* Link it to the csrows old API data */
+ chan->dimm = dimm;
+ dimm->csrow = row;
+ dimm->cschannel = chn;
+
+ /* Increment csrow location */
+ if (!rev_order) {
+ for (j = n_layers - 1; j >= 0; j--)
+ if (!layers[j].is_virt_csrow)
+ break;
+ chn++;
+ if (chn == tot_channels) {
+ chn = 0;
+ row++;
+ }
+ } else {
+ for (j = n_layers - 1; j >= 0; j--)
+ if (layers[j].is_virt_csrow)
+ break;
+ row++;
+ if (row == tot_csrows) {
+ row = 0;
+ chn++;
+ }
+ }
+
+ /* Increment dimm location */
+ for (j = n_layers - 1; j >= 0; j--) {
+ pos[j]++;
+ if (pos[j] < layers[j].size)
+ break;
+ pos[j] = 0;
}
}
@@ -263,6 +400,57 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
*/
return mci;
}
+EXPORT_SYMBOL_GPL(new_edac_mc_alloc);
+
+/**
+ * edac_mc_alloc: Allocate and partially fills a struct mem_ctl_info structure
+ * @edac_index: Memory controller number
+ * @n_layers: Nu
+mber of layers at the MC hierarchy
+ * layers: Describes each layer as seen by the Memory Controller
+ * @rev_order: Fills csrows/cs channels at the reverse order
+ * @size_pvt: size of private storage needed
+ *
+ *
+ * FIXME: drivers handle multi-rank memories on different ways: on some
+ * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
+ * a single multi-rank DIMM would be mapped into several "dimms".
+ *
+ * Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
+ * such DIMMS properly, but the csrow-based ones will likely do the wrong
+ * thing, as two chip select values are used for dual-rank memories (and 4, for
+ * quad-rank ones). I suspect that this issue could be solved inside the EDAC
+ * core for SDRAM memories, but it requires further study at JEDEC JESD 21C.
+ *
+ * In summary, solving this issue is not easy, as it requires a lot of testing.
+ *
+ * Everything is kmalloc'ed as one big chunk - more efficient.
+ * Only can be used if all structures have the same lifetime - otherwise
+ * you have to allocate and initialize your own structures.
+ *
+ * Use edac_mc_free() to free mc structures allocated by this function.
+ *
+ * Returns:
+ * NULL allocation failed
+ * struct mem_ctl_info pointer
+ */
+
+struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows,
+ unsigned nr_chans, int edac_index)
+{
+ unsigned n_layers = 2;
+ struct edac_mc_layer layers[n_layers];
+
+ layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+ layers[0].size = nr_csrows;
+ layers[0].is_virt_csrow = true;
+ layers[1].type = EDAC_MC_LAYER_CHANNEL;
+ layers[1].size = nr_chans;
+ layers[1].is_virt_csrow = false;
+
+ return new_edac_mc_alloc(edac_index, ARRAY_SIZE(layers), layers,
+ false, sz_pvt);
+}
EXPORT_SYMBOL_GPL(edac_mc_alloc);
/**
@@ -528,7 +716,6 @@ EXPORT_SYMBOL(edac_mc_find);
* edac_mc_add_mc: Insert the 'mci' structure into the mci global list and
* create sysfs entries associated with mci structure
* @mci: pointer to the mci structure to be added to the list
- * @mc_idx: A unique numeric identifier to be assigned to the 'mci' structure.
*
* Return:
* 0 Success
@@ -555,6 +742,8 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
edac_mc_dump_channel(&mci->csrows[i].
channels[j]);
}
+ for (i = 0; i < mci->tot_dimms; i++)
+ edac_mc_dump_dimm(&mci->dimms[i]);
}
#endif
mutex_lock(&mem_ctls_mutex);
@@ -712,261 +901,251 @@ int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
}
EXPORT_SYMBOL_GPL(edac_mc_find_csrow_by_page);
-/* FIXME - setable log (warning/emerg) levels */
-/* FIXME - integrate with evlog: http://evlog.sourceforge.net/ */
-void edac_mc_handle_ce(struct mem_ctl_info *mci,
- unsigned long page_frame_number,
- unsigned long offset_in_page, unsigned long syndrome,
- int row, int channel, const char *msg)
+const char *edac_layer_name[] = {
+ [EDAC_MC_LAYER_BRANCH] = "branch",
+ [EDAC_MC_LAYER_CHANNEL] = "channel",
+ [EDAC_MC_LAYER_SLOT] = "slot",
+ [EDAC_MC_LAYER_CHIP_SELECT] = "csrow",
+};
+EXPORT_SYMBOL_GPL(edac_layer_name);
+
+static void edac_increment_ce_error(struct mem_ctl_info *mci,
+ bool enable_filter,
+ unsigned pos[EDAC_MAX_LAYERS])
{
- unsigned long remapped_page;
- char *label = NULL;
- u32 grain;
+ int i, index = 0;
- debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
+ mci->ce_mc++;
- /* FIXME - maybe make panic on INTERNAL ERROR an option */
- if (row >= mci->nr_csrows || row < 0) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range "
- "(%d >= %d)\n", row, mci->nr_csrows);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
+ if (!enable_filter) {
+ mci->ce_noinfo_count++;
return;
}
- if (channel >= mci->csrows[row].nr_channels || channel < 0) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel out of range "
- "(%d >= %d)\n", channel,
- mci->csrows[row].nr_channels);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
- return;
- }
-
- label = mci->csrows[row].channels[channel].dimm->label;
- grain = mci->csrows[row].channels[channel].dimm->grain;
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] < 0)
+ break;
+ index += pos[i];
+ mci->ce_per_layer[i][index]++;
- if (edac_mc_get_log_ce())
- /* FIXME - put in DIMM location */
- edac_mc_printk(mci, KERN_WARNING,
- "CE page 0x%lx, offset 0x%lx, grain %d, syndrome "
- "0x%lx, row %d, channel %d, label \"%s\": %s\n",
- page_frame_number, offset_in_page,
- grain, syndrome, row, channel,
- label, msg);
+ if (i < mci->n_layers - 1)
+ index *= mci->layers[i + 1].size;
+ }
+}
- mci->ce_count++;
- mci->csrows[row].ce_count++;
- mci->csrows[row].channels[channel].dimm->ce_count++;
- mci->csrows[row].channels[channel].ce_count++;
+static void edac_increment_ue_error(struct mem_ctl_info *mci,
+ bool enable_filter,
+ unsigned pos[EDAC_MAX_LAYERS])
+{
+ int i, index = 0;
- if (mci->scrub_mode & SCRUB_SW_SRC) {
- /*
- * Some MC's can remap memory so that it is still available
- * at a different address when PCI devices map into memory.
- * MC's that can't do this lose the memory where PCI devices
- * are mapped. This mapping is MC dependent and so we call
- * back into the MC driver for it to map the MC page to
- * a physical (CPU) page which can then be mapped to a virtual
- * page - which can then be scrubbed.
- */
- remapped_page = mci->ctl_page_to_phys ?
- mci->ctl_page_to_phys(mci, page_frame_number) :
- page_frame_number;
+ mci->ue_mc++;
- edac_mc_scrub_block(remapped_page, offset_in_page, grain);
+ if (!enable_filter) {
+ mci->ce_noinfo_count++;
+ return;
}
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ce);
-void edac_mc_handle_ce_no_info(struct mem_ctl_info *mci, const char *msg)
-{
- if (edac_mc_get_log_ce())
- edac_mc_printk(mci, KERN_WARNING,
- "CE - no information available: %s\n", msg);
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] < 0)
+ break;
+ index += pos[i];
+ mci->ue_per_layer[i][index]++;
- mci->ce_noinfo_count++;
- mci->ce_count++;
+ if (i < mci->n_layers - 1)
+ index *= mci->layers[i + 1].size;
+ }
}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ce_no_info);
-void edac_mc_handle_ue(struct mem_ctl_info *mci,
- unsigned long page_frame_number,
- unsigned long offset_in_page, int row, const char *msg)
+#define OTHER_LABEL " or "
+void edac_mc_handle_error(const enum hw_event_mc_err_type type,
+ struct mem_ctl_info *mci,
+ const unsigned long page_frame_number,
+ const unsigned long offset_in_page,
+ const unsigned long syndrome,
+ const int layer0,
+ const int layer1,
+ const int layer2,
+ const char *msg,
+ const char *other_detail,
+ const void *mcelog)
{
- int len = EDAC_MC_LABEL_LEN * 4;
- char labels[len + 1];
- char *pos = labels;
- int chan;
- int chars;
- char *label = NULL;
+ unsigned long remapped_page;
+ /* FIXME: too much for stack: move it to some pre-alocated area */
+ char detail[80], location[80];
+ char label[(EDAC_MC_LABEL_LEN + 1 + sizeof(OTHER_LABEL)) * mci->tot_dimms];
+ char *p;
+ int row = -1, chan = -1;
+ int pos[EDAC_MAX_LAYERS] = { layer0, layer1, layer2 };
+ int i;
u32 grain;
+ bool enable_filter = false;
debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
- /* FIXME - maybe make panic on INTERNAL ERROR an option */
- if (row >= mci->nr_csrows || row < 0) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range "
- "(%d >= %d)\n", row, mci->nr_csrows);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
- }
-
- grain = mci->csrows[row].channels[0].dimm->grain;
- label = mci->csrows[row].channels[0].dimm->label;
- chars = snprintf(pos, len + 1, "%s", label);
- len -= chars;
- pos += chars;
-
- for (chan = 1; (chan < mci->csrows[row].nr_channels) && (len > 0);
- chan++) {
- label = mci->csrows[row].channels[chan].dimm->label;
- chars = snprintf(pos, len + 1, ":%s", label);
- len -= chars;
- pos += chars;
+ /* Check if the event report is consistent */
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] >= (int)mci->layers[i].size) {
+ if (type == HW_EVENT_ERR_CORRECTED) {
+ p = "CE";
+ mci->ce_mc++;
+ } else {
+ p = "UE";
+ mci->ue_mc++;
+ }
+ edac_mc_printk(mci, KERN_ERR,
+ "INTERNAL ERROR: %s value is out of range (%d >= %d)\n",
+ edac_layer_name[mci->layers[i].type],
+ pos[i], mci->layers[i].size);
+ /*
+ * Instead of just returning it, let's use what's
+ * known about the error. The increment routines and
+ * the DIMM filter logic will do the right thing by
+ * pointing the likely damaged DIMMs.
+ */
+ pos[i] = -1;
+ }
+ if (pos[i] >= 0)
+ enable_filter = true;
}
- if (edac_mc_get_log_ue())
- edac_mc_printk(mci, KERN_EMERG,
- "UE page 0x%lx, offset 0x%lx, grain %d, row %d, "
- "labels \"%s\": %s\n", page_frame_number,
- offset_in_page, grain, row, labels, msg);
-
- if (edac_mc_get_panic_on_ue())
- panic("EDAC MC%d: UE page 0x%lx, offset 0x%lx, grain %d, "
- "row %d, labels \"%s\": %s\n", mci->mc_idx,
- page_frame_number, offset_in_page,
- grain, row, labels, msg);
-
- mci->ue_count++;
- mci->csrows[row].ue_count++;
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ue);
+ /*
+ * Get the dimm label/grain that applies to the match criteria.
+ * As the error algorithm may not be able to point to just one memory,
+ * the logic here will get all possible labels that could pottentially
+ * be affected by the error.
+ * On FB-DIMM memory controllers, for uncorrected errors, it is common
+ * to have only the MC channel and the MC dimm (also called as "rank")
+ * but the channel is not known, as the memory is arranged in pairs,
+ * where each memory belongs to a separate channel within the same
+ * branch.
+ * It will also get the max grain, over the error match range
+ */
+ grain = 0;
+ p = label;
+ *p = '\0';
+ for (i = 0; i < mci->tot_dimms; i++) {
+ struct dimm_info *dimm = &mci->dimms[i];
-void edac_mc_handle_ue_no_info(struct mem_ctl_info *mci, const char *msg)
-{
- if (edac_mc_get_panic_on_ue())
- panic("EDAC MC%d: Uncorrected Error", mci->mc_idx);
+ if (layer0 >= 0 && layer0 != dimm->location[0])
+ continue;
+ if (layer1 >= 0 && layer1 != dimm->location[1])
+ continue;
+ if (layer2 >= 0 && layer2 != dimm->location[2])
+ continue;
- if (edac_mc_get_log_ue())
- edac_mc_printk(mci, KERN_WARNING,
- "UE - no information available: %s\n", msg);
- mci->ue_noinfo_count++;
- mci->ue_count++;
-}
-EXPORT_SYMBOL_GPL(edac_mc_handle_ue_no_info);
+ if (dimm->grain > grain)
+ grain = dimm->grain;
-/*************************************************************
- * On Fully Buffered DIMM modules, this help function is
- * called to process UE events
- */
-void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
- unsigned int csrow,
- unsigned int channela,
- unsigned int channelb, char *msg)
-{
- int len = EDAC_MC_LABEL_LEN * 4;
- char labels[len + 1];
- char *pos = labels;
- int chars;
- char *label;
-
- if (csrow >= mci->nr_csrows) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range (%d >= %d)\n",
- csrow, mci->nr_csrows);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
+ /*
+ * If the error is memory-controller wide, there's no sense
+ * on seeking for the affected DIMMs, as everything may be
+ * affected. Also, don't show errors for non-filled dimm's.
+ */
+ if (enable_filter && dimm->nr_pages) {
+ if (p != label) {
+ strcpy(p, OTHER_LABEL);
+ p += strlen(OTHER_LABEL);
+ }
+ strcpy(p, dimm->label);
+ p += strlen(p);
+ *p = '\0';
+
+ /*
+ * get csrow/channel of the dimm, in order to allow
+ * incrementing the compat API counters
+ */
+ debugf4("%s: %s csrows map: (%d,%d)\n",
+ __func__,
+ mci->mem_is_per_rank ? "rank" : "dimm",
+ dimm->csrow, dimm->cschannel);
+ if (row == -1)
+ row = dimm->csrow;
+ else if (row >= 0 && row != dimm->csrow)
+ row = -2;
+ if (chan == -1)
+ chan = dimm->cschannel;
+ else if (chan >= 0 && chan != dimm->cschannel)
+ chan = -2;
+ }
}
-
- if (channela >= mci->csrows[csrow].nr_channels) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel-a out of range "
- "(%d >= %d)\n",
- channela, mci->csrows[csrow].nr_channels);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
+ if (!enable_filter) {
+ strcpy(label, "any memory");
+ } else {
+ debugf4("%s: csrow/channel to increment: (%d,%d)\n",
+ __func__, row, chan);
+ if (p == label)
+ strcpy(label, "unknown memory");
+ if (type == HW_EVENT_ERR_CORRECTED) {
+ if (row >= 0) {
+ mci->csrows[row].ce_count++;
+ if (chan >= 0)
+ mci->csrows[row].channels[chan].ce_count++;
+ }
+ } else
+ if (row >= 0)
+ mci->csrows[row].ue_count++;
}
- if (channelb >= mci->csrows[csrow].nr_channels) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel-b out of range "
- "(%d >= %d)\n",
- channelb, mci->csrows[csrow].nr_channels);
- edac_mc_handle_ue_no_info(mci, "INTERNAL ERROR");
- return;
+ /* Fill the RAM location data */
+ p = location;
+ for (i = 0; i < mci->n_layers; i++) {
+ if (pos[i] < 0)
+ continue;
+ p += sprintf(p, "%s %d ",
+ edac_layer_name[mci->layers[i].type],
+ pos[i]);
}
- mci->ue_count++;
- mci->csrows[csrow].ue_count++;
-
- /* Generate the DIMM labels from the specified channels */
- label = mci->csrows[csrow].channels[channela].dimm->label;
- chars = snprintf(pos, len + 1, "%s", label);
- len -= chars;
- pos += chars;
-
- chars = snprintf(pos, len + 1, "-%s",
- mci->csrows[csrow].channels[channelb].dimm->label);
-
- if (edac_mc_get_log_ue())
- edac_mc_printk(mci, KERN_EMERG,
- "UE row %d, channel-a= %d channel-b= %d "
- "labels \"%s\": %s\n", csrow, channela, channelb,
- labels, msg);
-
- if (edac_mc_get_panic_on_ue())
- panic("UE row %d, channel-a= %d channel-b= %d "
- "labels \"%s\": %s\n", csrow, channela,
- channelb, labels, msg);
-}
-EXPORT_SYMBOL(edac_mc_handle_fbd_ue);
+ /* Memory type dependent details about the error */
+ if (type == HW_EVENT_ERR_CORRECTED)
+ snprintf(detail, sizeof(detail),
+ "page 0x%lx offset 0x%lx grain %d syndrome 0x%lx",
+ page_frame_number, offset_in_page,
+ grain, syndrome);
+ else
+ snprintf(detail, sizeof(detail),
+ "page 0x%lx offset 0x%lx grain %d",
+ page_frame_number, offset_in_page, grain);
+
+ if (type == HW_EVENT_ERR_CORRECTED) {
+ if (edac_mc_get_log_ce())
+ edac_mc_printk(mci, KERN_WARNING,
+ "CE %s on %s (%s%s %s)\n",
+ msg, label, location,
+ detail, other_detail);
+ edac_increment_ce_error(mci, enable_filter, pos);
+
+ if (mci->scrub_mode & SCRUB_SW_SRC) {
+ /*
+ * Some MC's can remap memory so that it is still
+ * available at a different address when PCI devices
+ * map into memory.
+ * MC's that can't do this lose the memory where PCI
+ * devices are mapped. This mapping is MC dependent
+ * and so we call back into the MC driver for it to
+ * map the MC page to a physical (CPU) page which can
+ * then be mapped to a virtual page - which can then
+ * be scrubbed.
+ */
+ remapped_page = mci->ctl_page_to_phys ?
+ mci->ctl_page_to_phys(mci, page_frame_number) :
+ page_frame_number;
+
+ edac_mc_scrub_block(remapped_page,
+ offset_in_page, grain);
+ }
+ } else {
+ if (edac_mc_get_log_ue())
+ edac_mc_printk(mci, KERN_WARNING,
+ "UE %s on %s (%s%s %s)\n",
+ msg, label, location, detail, other_detail);
-/*************************************************************
- * On Fully Buffered DIMM modules, this help function is
- * called to process CE events
- */
-void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
- unsigned int csrow, unsigned int channel, char *msg)
-{
- char *label = NULL;
+ if (edac_mc_get_panic_on_ue())
+ panic("UE %s on %s (%s%s %s)\n",
+ msg, label, location, detail, other_detail);
- /* Ensure boundary values */
- if (csrow >= mci->nr_csrows) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: row out of range (%d >= %d)\n",
- csrow, mci->nr_csrows);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
- return;
+ edac_increment_ue_error(mci, enable_filter, pos);
}
- if (channel >= mci->csrows[csrow].nr_channels) {
- /* something is wrong */
- edac_mc_printk(mci, KERN_ERR,
- "INTERNAL ERROR: channel out of range (%d >= %d)\n",
- channel, mci->csrows[csrow].nr_channels);
- edac_mc_handle_ce_no_info(mci, "INTERNAL ERROR");
- return;
- }
-
- label = mci->csrows[csrow].channels[channel].dimm->label;
-
- if (edac_mc_get_log_ce())
- /* FIXME - put in DIMM location */
- edac_mc_printk(mci, KERN_WARNING,
- "CE row %d, channel %d, label \"%s\": %s\n",
- csrow, channel, label, msg);
-
- mci->ce_count++;
- mci->csrows[csrow].ce_count++;
- mci->csrows[csrow].channels[channel].dimm->ce_count++;
- mci->csrows[csrow].channels[channel].ce_count++;
}
-EXPORT_SYMBOL(edac_mc_handle_fbd_ce);
+EXPORT_SYMBOL_GPL(edac_mc_handle_error);
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 3b8798d..2b66109 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -412,18 +412,20 @@ struct edac_mc_layer {
/* FIXME: add the proper per-location error counts */
struct dimm_info {
char label[EDAC_MC_LABEL_LEN + 1]; /* DIMM label on motherboard */
- unsigned memory_controller;
- unsigned csrow;
- unsigned csrow_channel;
+
+ /* Memory location data */
+ unsigned location[EDAC_MAX_LAYERS];
+
+ struct mem_ctl_info *mci; /* the parent */
u32 grain; /* granularity of reported error in bytes */
enum dev_type dtype; /* memory device type */
enum mem_type mtype; /* memory dimm type */
enum edac_type edac_mode; /* EDAC mode for this dimm */
- u32 nr_pages; /* number of pages in csrow */
+ u32 nr_pages; /* number of pages on this dimm */
- u32 ce_count; /* Correctable Errors for this dimm */
+ unsigned csrow, cschannel; /* Points to the old API data */
};
/**
@@ -443,9 +445,10 @@ struct dimm_info {
*/
struct rank_info {
int chan_idx;
- u32 ce_count;
struct csrow_info *csrow;
struct dimm_info *dimm;
+
+ u32 ce_count; /* Correctable Errors for this csrow */
};
struct csrow_info {
@@ -497,6 +500,11 @@ struct mcidev_sysfs_attribute {
ssize_t (*store)(struct mem_ctl_info *, const char *,size_t);
};
+struct edac_hierarchy {
+ char *name;
+ unsigned nr;
+};
+
/* MEMORY controller information structure
*/
struct mem_ctl_info {
@@ -541,13 +549,18 @@ struct mem_ctl_info {
unsigned long (*ctl_page_to_phys) (struct mem_ctl_info * mci,
unsigned long page);
int mc_idx;
- int nr_csrows;
struct csrow_info *csrows;
+ unsigned nr_csrows, num_cschannel;
+
+ /* Memory Controller hierarchy */
+ unsigned n_layers;
+ struct edac_mc_layer *layers;
+ bool mem_is_per_rank;
/*
* DIMM info. Will eventually remove the entire csrows_info some day
*/
- unsigned nr_dimms;
+ unsigned tot_dimms;
struct dimm_info *dimms;
/*
@@ -562,12 +575,15 @@ struct mem_ctl_info {
const char *dev_name;
char proc_name[MC_PROC_NAME_MAX_LEN + 1];
void *pvt_info;
- u32 ue_noinfo_count; /* Uncorrectable Errors w/o info */
- u32 ce_noinfo_count; /* Correctable Errors w/o info */
- u32 ue_count; /* Total Uncorrectable Errors for this MC */
- u32 ce_count; /* Total Correctable Errors for this MC */
+ u32 ue_count; /* Total Uncorrectable Errors for this MC */
+ u32 ce_count; /* Total Correctable Errors for this MC */
unsigned long start_time; /* mci load start time (in jiffies) */
+ /* drivers shouldn't access this struct directly */
+ unsigned ce_noinfo_count, ue_noinfo_count;
+ unsigned ce_mc, ue_mc;
+ u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
+
struct completion complete;
/* edac sysfs device control */
@@ -580,7 +596,7 @@ struct mem_ctl_info {
* by the low level driver.
*
* Set by the low level driver to provide attributes at the
- * controller level, same level as 'ue_count' and 'ce_count' above.
+ * controller level.
* An array of structures, NULL terminated
*
* If attributes are desired, then set to array of attributes
^ permalink raw reply related
* RE: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Luck, Tony @ 2012-04-27 18:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Borislav Petkov
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Gross, Mark, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <4F9ADCE3.2030506@redhat.com>
>>> + for (i =3D 0; i < dimm->mci->n_layers; i++) {
>>> + printk(KERN_CONT "%d", dimm->location[i]);
>>> + if (i < dimm->mci->n_layers - 1)
>>> + printk(KERN_CONT ".");
>>> + }
>>> + printk(KERN_CONT "\n");
>>=20
>> This looks hacky but I don't have a good suggestion what to do instead
>> here. Maybe snprintf into a complete string which you can issue with
>> debugf4()...
>
> This is not hacky. There are several places at the Kernel doing loops lik=
e
> that. Look, for example, at lib/hexdump.c (without KERN_CONT, as this
> macro was added later - probably to avoid checkpatch.pl complains).
There is some benefit to "one printk =3D=3D one output line" ... it means
that console output will not be (as) jumbled if multiple cpus are
printk'ing at the same time.
-Tony
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Mauro Carvalho Chehab @ 2012-04-27 19:24 UTC (permalink / raw)
To: Luck, Tony
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Gross, Mark, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Borislav Petkov, Egor Martovetsky, Niklas Söderlund,
Tim Small, Arvind R., Chris Metcalf, Olof Johansson,
Doug Thompson, Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <3908561D78D1C84285E8C5FCA982C28F170F4A4C@ORSMSX104.amr.corp.intel.com>
Em 27-04-2012 15:11, Luck, Tony escreveu:
>>>> + for (i = 0; i < dimm->mci->n_layers; i++) {
>>>> + printk(KERN_CONT "%d", dimm->location[i]);
>>>> + if (i < dimm->mci->n_layers - 1)
>>>> + printk(KERN_CONT ".");
>>>> + }
>>>> + printk(KERN_CONT "\n");
>>>
>>> This looks hacky but I don't have a good suggestion what to do instead
>>> here. Maybe snprintf into a complete string which you can issue with
>>> debugf4()...
>>
>> This is not hacky. There are several places at the Kernel doing loops like
>> that. Look, for example, at lib/hexdump.c (without KERN_CONT, as this
>> macro was added later - probably to avoid checkpatch.pl complains).
>
> There is some benefit to "one printk == one output line" ... it means
> that console output will not be (as) jumbled if multiple cpus are
> printk'ing at the same time.
Ok, but this message only appears when all the conditions below are met:
- the driver is compiled with EDAC_DEBUG;
- the edac_core is modprobed with edac_debug_level=4;
- during the driver modprobe, when the EDAC driver is being registered.
Even on several-core machines, those messages won't mangle, in practice.
Let's not over-design a simple debug message.
Regards,
Mauro
^ permalink raw reply
* Re: Regression in 32-bit ppc kernel
From: Benjamin Herrenschmidt @ 2012-04-27 22:26 UTC (permalink / raw)
To: Larry Finger; +Cc: linuxppc-dev, Paul Mackerras, LKML
In-Reply-To: <4F9ABD67.3060704@lwfinger.net>
On Fri, 2012-04-27 at 10:38 -0500, Larry Finger wrote:
> Sorry, I was unable to find anything in debugfs to help me learn about interrupt
> mapping. The value of CONFIG_NR_IRQS is already 512. I have not tried reducing
> it to 128. The setting for CONFIG_SPARSE_IRQ was on, and changing it to off did
> not make any difference.
>
> I finished the bisection, which led to
>
> commit a79dd5ae5a8f49688d65b89a859f2b98a7ee5538
> Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Thu Dec 15 11:13:03 2011 +1100
>
> tty/serial/pmac_zilog: Fix suspend & resume
>
> As this seemed to be an improbable result, I did the full test by checking out
> the previous commit (43ca5d3). That resulted in a "good" result. Then I used
> quilt to add commit a79dd5a as a patch and the fault returned. I then noticed
> that you said in the commit message that "I removed some code for handling
> unexpected interrupt which should never be hit...". It appears that my box does
> indeed hit such an unexpected interrupt.
>
> I could always get rid of the fault by disabling CONFIG_SERIAL_PMACZILOG, but I
> would like to fix the problem if possible.
Right, it should be fixed. I need to understand where the unexpected
interrupt comes from. Can you tell me (or remind me) what specific
machine model you are using ? Are you putting the console on the serial
port ?
Cheers,
Ben.
^ permalink raw reply
* Re: Regression in 32-bit ppc kernel
From: Larry Finger @ 2012-04-28 0:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras, LKML
In-Reply-To: <1335565573.20866.7.camel@pasglop>
On 04/27/2012 05:26 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2012-04-27 at 10:38 -0500, Larry Finger wrote:
>
>> Sorry, I was unable to find anything in debugfs to help me learn about interrupt
>> mapping. The value of CONFIG_NR_IRQS is already 512. I have not tried reducing
>> it to 128. The setting for CONFIG_SPARSE_IRQ was on, and changing it to off did
>> not make any difference.
>>
>> I finished the bisection, which led to
>>
>> commit a79dd5ae5a8f49688d65b89a859f2b98a7ee5538
>> Author: Benjamin Herrenschmidt<benh@kernel.crashing.org>
>> Date: Thu Dec 15 11:13:03 2011 +1100
>>
>> tty/serial/pmac_zilog: Fix suspend& resume
>>
>> As this seemed to be an improbable result, I did the full test by checking out
>> the previous commit (43ca5d3). That resulted in a "good" result. Then I used
>> quilt to add commit a79dd5a as a patch and the fault returned. I then noticed
>> that you said in the commit message that "I removed some code for handling
>> unexpected interrupt which should never be hit...". It appears that my box does
>> indeed hit such an unexpected interrupt.
>>
>> I could always get rid of the fault by disabling CONFIG_SERIAL_PMACZILOG, but I
>> would like to fix the problem if possible.
>
> Right, it should be fixed. I need to understand where the unexpected
> interrupt comes from. Can you tell me (or remind me) what specific
> machine model you are using ? Are you putting the console on the serial
> port ?
It is a 15" Powerbook G4. I think they call it a Titanium. The console is not on
a serial port. In fact, the reason that I did not think this patch was a problem
is because the serial port does not appear to be connected to an external port.
I was unaware that there was a serial port on the motherboard. There is a modem
jack, but no 9 or 25-pin connectors that would indicate a standard serial port.
There are two stack dumps with the same trace. I posted the first, but the
second is preceded by the lines
[<c02adca0] pmz_interrupt
Disabling IRQ #23
ttyPZ1: IrDA setup for 57600 bps, dongle version: 4
ttyPZ1: IrDA setup for 115200 bps, dongle version: 4
irq23: nobody cared (try booting with the "irqpoll" option
As I am not sure how to put options in with yaboot, I have not tried that.
Larry
^ permalink raw reply
* Re: Regression in 32-bit ppc kernel
From: Benjamin Herrenschmidt @ 2012-04-28 0:42 UTC (permalink / raw)
To: Larry Finger; +Cc: linuxppc-dev, Paul Mackerras, LKML
In-Reply-To: <4F9B3398.9060701@lwfinger.net>
On Fri, 2012-04-27 at 19:02 -0500, Larry Finger wrote:
> It is a 15" Powerbook G4. I think they call it a Titanium. The console is not on
> a serial port. In fact, the reason that I did not think this patch was a problem
> is because the serial port does not appear to be connected to an external port.
> I was unaware that there was a serial port on the motherboard. There is a modem
> jack, but no 9 or 25-pin connectors that would indicate a standard serial port.
>
> There are two stack dumps with the same trace. I posted the first, but the
> second is preceded by the lines
>
> [<c02adca0] pmz_interrupt
> Disabling IRQ #23
> ttyPZ1: IrDA setup for 57600 bps, dongle version: 4
> ttyPZ1: IrDA setup for 115200 bps, dongle version: 4
> irq23: nobody cared (try booting with the "irqpoll" option
>
> As I am not sure how to put options in with yaboot, I have not tried that.
Ok, so you do have a serial port, probably two even :-) One of them is
connected to the infra red transceiver and the other one is probably
connected to the internal modem.
(The modem itself might not use it, some of these machines use an
i2s/i2c modem, some use a usb modem, but the serial port is wired to the
connector regardless).
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-28 8:52 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Borislav Petkov, Egor Martovetsky, Niklas Söderlund,
Tim Small, Arvind R., Chris Metcalf, Joe Perches, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Olof Johansson, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9AC44A.5000509@redhat.com>
On Fri, Apr 27, 2012 at 01:07:38PM -0300, Mauro Carvalho Chehab wrote:
> Yes. This is a common issue at the EDAC core: on several places, it calls the
> edac debug macros (DEBUGF0...DEBUGF4) passing a __func__ as an argument, while
> the debug macros already handles that. I suspect that, in the past, the __func__
> were not at the macros, but some patch added it there, and forgot to fix the
> occurrences of its call.
The patch that added it is d357cbb445208 and you reviewed it.
> This is something that needs to be reviewed at the entire EDAC core (and likely
> at the drivers).
Looks like a job for a newbie to get her/his feet wet with kernel work.
> I opted to not touch on this at the existing debug logic, as I think that the
> better is to address all those issues on one separate patch, after fixing the
> EDAC core bugs.
No,
you simply need to remove the __func__ argument in your newly added debug call:
debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
i, (dimm - mci->dimms),
pos[0], pos[1], pos[2], row, chn);
And while you're at it, remove the rest of the __func__ arguments from
your newly added debugfX calls.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-28 8:58 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Gross, Mark, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Luck, Tony, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <4F9AF26C.9030808@redhat.com>
On Fri, Apr 27, 2012 at 04:24:28PM -0300, Mauro Carvalho Chehab wrote:
> Em 27-04-2012 15:11, Luck, Tony escreveu:
> >>>> + for (i = 0; i < dimm->mci->n_layers; i++) {
> >>>> + printk(KERN_CONT "%d", dimm->location[i]);
> >>>> + if (i < dimm->mci->n_layers - 1)
> >>>> + printk(KERN_CONT ".");
> >>>> + }
> >>>> + printk(KERN_CONT "\n");
> >>>
> >>> This looks hacky but I don't have a good suggestion what to do instead
> >>> here. Maybe snprintf into a complete string which you can issue with
> >>> debugf4()...
> >>
> >> This is not hacky. There are several places at the Kernel doing loops like
> >> that. Look, for example, at lib/hexdump.c (without KERN_CONT, as this
> >> macro was added later - probably to avoid checkpatch.pl complains).
> >
> > There is some benefit to "one printk == one output line" ... it means
> > that console output will not be (as) jumbled if multiple cpus are
> > printk'ing at the same time.
>
> Ok, but this message only appears when all the conditions below are met:
> - the driver is compiled with EDAC_DEBUG;
> - the edac_core is modprobed with edac_debug_level=4;
> - during the driver modprobe, when the EDAC driver is being registered.
That means nothing.
> Even on several-core machines, those messages won't mangle, in practice.
>
> Let's not over-design a simple debug message.
No, let's design a simple debug message correctly, regardless of when it
appears.
>
> Regards,
> Mauro
>
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
* Re: [PATCH EDACv16 1/2] edac: Change internal representation to work with layers
From: Borislav Petkov @ 2012-04-28 9:05 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Shaohui Xie, Jason Uhlenkott, Aristeu Rozanski, Hitoshi Mitake,
Mark Gross, Dmitry Eremin-Solenikov, Ranganathan Desikan,
Egor Martovetsky, Niklas Söderlund, Tim Small, Arvind R.,
Chris Metcalf, Olof Johansson, Doug Thompson,
Linux Edac Mailing List, Michal Marek, Jiri Kosina,
Linux Kernel Mailing List, Joe Perches, Andrew Morton,
linuxppc-dev
In-Reply-To: <4F9ABCEC.9090807@redhat.com>
On Fri, Apr 27, 2012 at 12:36:12PM -0300, Mauro Carvalho Chehab wrote:
> The fix for it were in another patch[1], as calling them as "rank" is
> needed also at the sysfs API.
No, this doesn't fix it either:
[ 10.486440] EDAC MC: DCT0 chip selects:
[ 10.486443] EDAC amd64: MC: 0: 2048MB 1: 2048MB
[ 10.486445] EDAC amd64: MC: 2: 2048MB 3: 2048MB
[ 10.486448] EDAC amd64: MC: 4: 0MB 5: 0MB
[ 10.486450] EDAC amd64: MC: 6: 0MB 7: 0MB
[ 10.486453] EDAC DEBUG: amd64_debug_display_dimm_sizes: F2x180 (DRAM Bank Address Mapping): 0x00000088
[ 10.486455] EDAC MC: DCT1 chip selects:
[ 10.486458] EDAC amd64: MC: 0: 2048MB 1: 2048MB
[ 10.486460] EDAC amd64: MC: 2: 2048MB 3: 2048MB
[ 10.486463] EDAC amd64: MC: 4: 0MB 5: 0MB
[ 10.486465] EDAC amd64: MC: 6: 0MB 7: 0MB
[ 10.486467] EDAC amd64: using x8 syndromes.
[ 10.486469] EDAC DEBUG: amd64_dump_dramcfg_low: F2x190 (DRAM Cfg Low): 0x00083100
[ 10.486472] EDAC DEBUG: amd64_dump_dramcfg_low: DIMM type: buffered; all DIMMs support ECC: yes
[ 10.486475] EDAC DEBUG: amd64_dump_dramcfg_low: PAR/ERR parity: enabled
[ 10.486478] EDAC DEBUG: amd64_dump_dramcfg_low: DCT 128bit mode width: 64b
[ 10.486481] EDAC DEBUG: amd64_dump_dramcfg_low: x4 logical DIMMs present: L0: yes L1: yes L2: no L3: no
[ 10.486485] EDAC DEBUG: f1x_early_channel_count: Data width is not 128 bits - need more decoding
[ 10.486488] EDAC amd64: MCT channel count: 2
[ 10.486493] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc(): allocating 3692 bytes for mci data (16 ranks, 16 csrows/channels)
[ 10.486501] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 0: rank0 (0:0:0): row 0, chan 0
[ 10.486506] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 1: rank1 (0:1:0): row 0, chan 1
[ 10.486510] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 2: rank2 (1:0:0): row 1, chan 0
[ 10.486514] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 3: rank3 (1:1:0): row 1, chan 1
[ 10.486518] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 4: rank4 (2:0:0): row 2, chan 0
[ 10.486522] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 5: rank5 (2:1:0): row 2, chan 1
[ 10.486526] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 6: rank6 (3:0:0): row 3, chan 0
[ 10.486530] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 7: rank7 (3:1:0): row 3, chan 1
[ 10.486534] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 8: rank8 (4:0:0): row 4, chan 0
[ 10.486538] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 9: rank9 (4:1:0): row 4, chan 1
[ 10.486542] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 10: rank10 (5:0:0): row 5, chan 0
[ 10.486546] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 11: rank11 (5:1:0): row 5, chan 1
[ 10.486550] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 12: rank12 (6:0:0): row 6, chan 0
[ 10.486554] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 13: rank13 (6:1:0): row 6, chan 1
[ 10.486558] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 14: rank14 (7:0:0): row 7, chan 0
[ 10.486562] EDAC DEBUG: new_edac_mc_alloc: new_edac_mc_alloc: 15: rank15 (7:1:0): row 7, chan 1
DCT0 has 4 ranks + DCT1 also 4 ranks = 8 ranks total.
Now your change is showing 16 ranks. Still b0rked.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox