* Re: pseries/le: Fix endiannes issue in RTAS call from xmon
From: Michael Ellerman @ 2014-11-26 3:31 UTC (permalink / raw)
To: Laurent Dufour, linuxppc-dev, linux-kernel; +Cc: Laurent Dufour, paulus
In-Reply-To: <1416838073-16642-1-git-send-email-ldufour@linux.vnet.ibm.com>
On Mon, 2014-24-11 at 14:07:53 UTC, Laurent Dufour wrote:
> On pseries system (LPAR) xmon failed to enter when running in LE mode, system
> is hunging. Inititating xmon will lead to such an output on the console:
OK. You say "LPAR", by which you mean "under phyp" I think. I haven't seen this
under KVM, and it looks like KVM doesn't implement "set-indicator" so that
would explain that.
I'll take this as a bug fix and CC it to stable.
cheers
^ permalink raw reply
* Re: powerpc/powernv: Fix the hmi event version check.
From: Michael Ellerman @ 2014-11-26 3:44 UTC (permalink / raw)
To: Mahesh Salgaonkar, linuxppc-dev, Benjamin Herrenschmidt
In-Reply-To: <20141120041345.3339.6873.stgit@mars.in.ibm.com>
On Thu, 2014-20-11 at 04:14:36 UTC, Mahesh Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> The current HMI event structure is an ABI and carries a version field to
> accommodate future changes without affecting/rearranging current structure
> members that are valid for previous versions. The current version check
> "if (hmi_evt->version != OpalHMIEvt_V1)" seems to consider that version
> will always be V1 which may not be true in future. If we start supporting
> HMI event > V1, this check would fail without printing anything on older
> kernels. This patch fixes this issue.
It's not clear what you mean when you say "this check would fail without
printing anything". The check will fail, and it will print something, ie. the
error message.
What you mean is "the check will fail, and the HMI info will not be printed".
I'll CC this to stable unless you disagree.
cheers
^ permalink raw reply
* Re: [2/3] powerpc/powernv: Replace OPAL_DEASSERT_RESET with EEH_RESET_DEACTIVATE
From: Michael Ellerman @ 2014-11-26 4:07 UTC (permalink / raw)
To: Gavin Shan, linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1416868020-4129-2-git-send-email-gwshan@linux.vnet.ibm.com>
On Mon, 2014-24-11 at 22:26:59 UTC, Gavin Shan wrote:
> The flag passed to ioda_eeh_phb_reset() should be EEH_RESET_DEACTIVATE,
> which is translated to OPAL_DEASSERT_RESET or something else by the
> EEH backend accordingly.
>
> The patch replaces OPAL_DEASSERT_RESET with EEH_RESET_DEACTIVATE for
> ioda_eeh_phb_reset().
What is the symptom? Does it not work at all or is it just a cosmetic issue?
ie. should I send this to Linus ASAP or can it wait for the next release.
Looks like this went in as 361f2a2a1536 ("powrpc/powernv: Reset PHB in kdump
kernel") in 3.16. Should it go to stable?
cheers
^ permalink raw reply
* Re: [3/3] powerpc/eeh: Fix missed PE#0 on P7IOC
From: Michael Ellerman @ 2014-11-26 4:09 UTC (permalink / raw)
To: Gavin Shan, linuxppc-dev; +Cc: Gavin Shan
In-Reply-To: <1416868020-4129-3-git-send-email-gwshan@linux.vnet.ibm.com>
On Mon, 2014-24-11 at 22:27:00 UTC, Gavin Shan wrote:
> PE#0 should be regarded as valid for P7IOC, while it's invalid for
> PHB3. The patch adds flag EEH_VALID_PE_ZERO to differentiate those
> two cases. Without the patch, we possibly see frozen PE#0 state is
> cleared without EEH recovery taken on P7IOC as following kernel logs
> indicate:
This sounds like an urgent fix but I'm not sure.
cheers
^ permalink raw reply
* [PATCH] powerpc/powernv: Add debugfs file to grab opalv3 trace data
From: Benjamin Herrenschmidt @ 2014-11-26 4:10 UTC (permalink / raw)
To: linuxppc-dev
This adds files in debugfs that can be used to retrieve the
OPALv3 firmware "live binary traces" which can then be parsed
using a userspace tool.
Mostly from Rusty with some updates by myself (BenH)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Reworked from whatever ancient version has been rotting in patchwork,
this one is updated to the actual properties exposed by skiboot nowadays,
endian fixed, and has the ability to set the trace mask.
arch/powerpc/platforms/powernv/Makefile | 2 +-
arch/powerpc/platforms/powernv/opal-trace-types.h | 58 +++++++
arch/powerpc/platforms/powernv/opal-trace.c | 202 ++++++++++++++++++++++
3 files changed, 261 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/platforms/powernv/opal-trace-types.h
create mode 100644 arch/powerpc/platforms/powernv/opal-trace.c
diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
index f241acc..315a825 100644
--- a/arch/powerpc/platforms/powernv/Makefile
+++ b/arch/powerpc/platforms/powernv/Makefile
@@ -1,7 +1,7 @@
obj-y += setup.o opal-wrappers.o opal.o opal-async.o
obj-y += opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
obj-y += rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
-obj-y += opal-msglog.o opal-hmi.o
+obj-y += opal-msglog.o opal-hmi.o opal-trace.o
obj-$(CONFIG_SMP) += smp.o subcore.o subcore-asm.o
obj-$(CONFIG_PCI) += pci.o pci-p5ioc2.o pci-ioda.o
diff --git a/arch/powerpc/platforms/powernv/opal-trace-types.h b/arch/powerpc/platforms/powernv/opal-trace-types.h
new file mode 100644
index 0000000..3bd8ac2
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/opal-trace-types.h
@@ -0,0 +1,58 @@
+/* API for kernel to read trace buffer. */
+#ifndef __OPAL_TRACE_TYPES_H
+#define __OPAL_TRACE_TYPES_H
+
+#define TRACE_REPEAT 1
+#define TRACE_OVERFLOW 2
+#define TRACE_OPAL 3
+#define TRACE_FSP 4
+
+/* One per cpu, plus one for NMIs */
+struct tracebuf {
+ /* Mask to apply to get buffer offset. */
+ __be64 mask;
+ /* This where the buffer starts. */
+ __be64 start;
+ /* This is where writer has written to. */
+ __be64 end;
+ /* This is where the writer wrote to previously. */
+ __be64 last;
+ /* This is where the reader is up to. */
+ __be64 rpos;
+ /* If the last one we read was a repeat, this shows how many. */
+ __be32 last_repeat;
+ /* Maximum possible size of a record. */
+ __be32 max_size;
+
+ char buf[/* TBUF_SZ + max_size */];
+};
+
+/* Common header for all trace entries. */
+struct trace_hdr {
+ __be64 timestamp;
+ u8 type;
+ u8 len_div_8;
+ __be16 cpu;
+ u8 unused[4];
+};
+
+/* Note: all other entries must be at least as large as this! */
+struct trace_repeat {
+ __be64 timestamp; /* Last repeat happened at this timestamp */
+ u8 type; /* == TRACE_REPEAT */
+ u8 len_div_8;
+ __be16 cpu;
+ __be16 prev_len;
+ __be16 num; /* Starts at 1, ie. 1 repeat, or two traces. */
+ /* Note that the count can be one short, if read races a repeat. */
+};
+
+struct trace_overflow {
+ __be64 unused64; /* Timestamp is unused */
+ u8 type; /* == TRACE_OVERFLOW */
+ u8 len_div_8;
+ u8 unused[6]; /* ie. hdr.cpu is indeterminate */
+ __be64 bytes_missed;
+};
+
+#endif /* __OPAL_TRACE_TYPES_H */
diff --git a/arch/powerpc/platforms/powernv/opal-trace.c b/arch/powerpc/platforms/powernv/opal-trace.c
new file mode 100644
index 0000000..6529756
--- /dev/null
+++ b/arch/powerpc/platforms/powernv/opal-trace.c
@@ -0,0 +1,202 @@
+/*
+ * Copyright (C) 2013 Rusty Russell, IBM Corporation
+ *
+ * Simple debugfs file firmware_trace to read out OPALv3 trace
+ * ringbuffers.
+ */
+#include <linux/mutex.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <asm/debug.h>
+#include <asm/opal.h>
+
+#include "opal-trace-types.h"
+
+static DEFINE_MUTEX(tracelock);
+static struct tracebuf **opal_tb;
+static size_t opal_num_tb;
+static __be64 *opal_tmask_p;
+
+/* Maximum possible size of record (since len is 8 bits). */
+union max_trace {
+ struct trace_hdr hdr;
+ struct trace_overflow overflow;
+ struct trace_repeat repeat;
+ char buf[255 * 8];
+};
+static union max_trace trace;
+
+static bool trace_empty(const struct tracebuf *tb)
+{
+ const struct trace_repeat *rep;
+
+ if (tb->rpos == tb->end)
+ return true;
+
+ /*
+ * If we have a single element only, and it's a repeat buffer
+ * we've already seen every repeat for (yet which may be
+ * incremented in future), we're also empty.
+ */
+ rep = (void *)tb->buf + (be64_to_cpu(tb->rpos & tb->mask));
+ if (be64_to_cpu(tb->end) != be64_to_cpu(tb->rpos) + sizeof(*rep))
+ return false;
+
+ if (rep->type != TRACE_REPEAT)
+ return false;
+
+ if (be16_to_cpu(rep->num) != be32_to_cpu(tb->last_repeat))
+ return false;
+
+ return true;
+}
+
+/* You can't read in parallel, so some locking required in caller. */
+static bool trace_get(union max_trace *t, struct tracebuf *tb)
+{
+ u64 start, rpos;
+
+ if (trace_empty(tb))
+ return false;
+
+again:
+ /*
+ * The actual buffer is slightly larger than tbsize, so this
+ * memcpy is always valid.
+ */
+ memcpy(t, tb->buf + be64_to_cpu(tb->rpos & tb->mask),
+ be32_to_cpu(tb->max_size));
+
+ rmb(); /* read barrier, so we read tb->start after copying record. */
+
+ start = be64_to_cpu(tb->start);
+ rpos = be64_to_cpu(tb->rpos);
+
+ /* Now, was that overwritten? */
+ if (rpos < start) {
+ /* Create overflow record. */
+ t->overflow.unused64 = 0;
+ t->overflow.type = TRACE_OVERFLOW;
+ t->overflow.len_div_8 = sizeof(t->overflow) / 8;
+ t->overflow.bytes_missed = cpu_to_be64(start - rpos);
+ tb->rpos = cpu_to_be64(start);
+ return true;
+ }
+
+ /* Repeat entries need special handling */
+ if (t->hdr.type == TRACE_REPEAT) {
+ u32 num = be16_to_cpu(t->repeat.num);
+
+ /* In case we've read some already... */
+ t->repeat.num = cpu_to_be16(num - be32_to_cpu(tb->last_repeat));
+
+ /* Record how many repeats we saw this time. */
+ tb->last_repeat = cpu_to_be32(num);
+
+ /* Don't report an empty repeat buffer. */
+ if (t->repeat.num == 0) {
+ /*
+ * This can't be the last buffer, otherwise
+ * trace_empty would have returned true.
+ */
+ BUG_ON(be64_to_cpu(tb->end) <= rpos + t->hdr.len_div_8 * 8);
+ /* Skip to next entry. */
+ tb->rpos = cpu_to_be64(rpos + t->hdr.len_div_8 * 8);
+ goto again;
+ }
+ } else {
+ tb->last_repeat = 0;
+ tb->rpos = cpu_to_be64(rpos + t->hdr.len_div_8 * 8);
+ }
+
+ return true;
+}
+
+/* Horrible polling interface, designed for dumping. */
+static ssize_t opal_trace_read(struct file *file, char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+ ssize_t err;
+ unsigned int i;
+
+ err = mutex_lock_interruptible(&tracelock);
+ if (err)
+ return err;
+
+ for (i = 0; i < opal_num_tb; i++) {
+ if (trace_get(&trace, opal_tb[i])) {
+ size_t len = trace.hdr.len_div_8 * 8;
+ if (len > count)
+ len = count;
+ if (copy_to_user(ubuf, &trace, len) != 0)
+ err = -EFAULT;
+ else
+ err = len;
+ break;
+ }
+ }
+
+ mutex_unlock(&tracelock);
+ return err;
+}
+
+static const struct file_operations opal_trace_fops = {
+ .read = opal_trace_read,
+ .open = simple_open,
+ .llseek = noop_llseek,
+};
+
+static int opal_tmask_set(void *data, u64 val)
+{
+ *(__be64 *)data = cpu_to_be64(val);
+ return 0;
+}
+static int opal_tmask_get(void *data, u64 *val)
+{
+ *val = be64_to_cpup((__be64 *)data);
+ return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(opal_tmask, opal_tmask_get, opal_tmask_set, "%llx\n");
+
+static int opal_trace_init(void)
+{
+ const __be64 *traces;
+ int len, i, rc;
+ u64 tmask_phys;
+
+ if (!opal_node)
+ return -ENODEV;
+
+ traces = of_get_property(opal_node, "ibm,opal-traces", &len);
+ if (!traces) {
+ pr_warning("%s: OPAL node property \"ibm,opal-traces\""
+ " not found\n", __func__);
+ return -ENODEV;
+ }
+
+ opal_num_tb = len / (sizeof(__be64) * 2);
+ if (!opal_num_tb) {
+ pr_warning("%s: OPAL traces property has invalid length %i\n",
+ __func__, len);
+ return -EINVAL;
+ }
+ opal_tb = kmalloc(sizeof(*opal_tb) * opal_num_tb, GFP_KERNEL);
+ for (i = 0; i < opal_num_tb; i++)
+ opal_tb[i] = __va(be64_to_cpu(traces[i*2]));
+
+ debugfs_create_file("opal-trace", S_IRUSR, powerpc_debugfs_root,
+ NULL, &opal_trace_fops);
+ rc = of_property_read_u64(opal_node, "ibm,opal-trace-mask",
+ &tmask_phys);
+ if (!rc)
+ opal_tmask_p = __va(tmask_phys);
+ if (opal_tmask_p)
+ debugfs_create_file("opal-trace-mask", S_IRUSR | S_IWUSR,
+ powerpc_debugfs_root, opal_tmask_p,
+ &opal_tmask);
+ return 0;
+}
+module_init(opal_trace_init);
+
^ permalink raw reply related
* Re: [V5 7/7] selftests, powerpc: Add test case for TM related ptrace interface
From: Anshuman Khandual @ 2014-11-26 5:10 UTC (permalink / raw)
To: Shuah Khan, linux-kernel, linuxppc-dev
Cc: mikey, james.hogan, avagin, Paul.Clothier, peterz, palves, oleg,
davem, dhowells, kirjanov, davej, akpm, sukadev, tglx,
sam.bobroff
In-Reply-To: <5474B72A.7040403@osg.samsung.com>
On 11/25/2014 10:36 PM, Shuah Khan wrote:
> On 11/25/2014 01:05 AM, Anshuman Khandual wrote:
>> > This patch adds one more test case called 'tm-ptrace' targeting TM
>> > related ptrace interface. This test creates one child process to
>> > run some basic TM transactions and the parent process attaches the
>> > child to do some ptrace probing using the recently added regset
>> > interfaces. The parent process then compares the received values
>> > against the expected values to verify whether it has passed the
>> > given test or not.
>> >
>> > Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> > ---
>> > tools/testing/selftests/powerpc/tm/Makefile | 2 +-
>> > tools/testing/selftests/powerpc/tm/tm-ptrace.c | 542 +++++++++++++++++++++++++
>> > 2 files changed, 543 insertions(+), 1 deletion(-)
>> > create mode 100644 tools/testing/selftests/powerpc/tm/tm-ptrace.c
>> >
>> > diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
>> > index 2cede23..71d400a 100644
>> > --- a/tools/testing/selftests/powerpc/tm/Makefile
>> > +++ b/tools/testing/selftests/powerpc/tm/Makefile
>> > @@ -1,4 +1,4 @@
>> > -PROGS := tm-resched-dscr
>> > +PROGS := tm-resched-dscr tm-ptrace
>> >
> Could you please add .gitignore for the binaries in this directory
> to avoid git status including the binaries it in its output.
Sure, will add one.
^ permalink raw reply
* Re: [PATCH v2 0/4] powernv: cpuidle: Redesign idle states management
From: Preeti U Murthy @ 2014-11-26 5:15 UTC (permalink / raw)
To: Shreyas B. Prabhu, linux-kernel, Benjamin Herrenschmidt
Cc: linux-pm, Rafael J. Wysocki, Paul Mackerras, linuxppc-dev
In-Reply-To: <1416914279-30384-1-git-send-email-shreyas@linux.vnet.ibm.com>
Hi,
I ran hackbench to evaluate this patchset and found good improvements in
the results.
I modified hackbench to take in a 'loops' parameter along with
num_groups which ensures that the test runs long enough to observe and
debug issues. The idea was to find out how latency sensitive workloads
can get affected by modification in cpuidle heuristics since it is easy
to measure the impact on these workloads.
The experiment was conducted on a Power8 system with 1 socket and 6
cores on it.
The first experiment was carried out by pinning hackbench to the first
thread in each core while the rest of the smt threads were idle and
below are the results. This would ensure the core entered deep idle
states more often.
num_grps %improvement with patchset
3 3.6
6 10.6
12 5.0
24 5.0
The second experiment was carried out by allowing hackbench to run on
the smt threads of two cores and % improvement with the patchset was in
range of 4-7%.
I ran the experiments on the vanilla kernel. This means the performance
improvements is primarily due to avoiding having to do a timebase sync
by every thread in the core. The power numbers have very little
variation between the runs with and without the patchset.
Thanks
Regards
Preeti U Murthy
On 11/25/2014 04:47 PM, Shreyas B. Prabhu wrote:
> Deep idle states like sleep and winkle are per core idle states. A core
> enters these states only when all the threads enter either the particular
> idle state or a deeper one. There are tasks like fastsleep hardware bug
> workaround and hypervisor core state save which have to be done only by
> the last thread of the core entering deep idle state and similarly tasks
> like timebase resync, hypervisor core register restore that have to be
> done only by the first thread waking up from these states.
>
> The current idle state management does not have a way to distinguish the
> first/last thread of the core waking/entering idle states. Tasks like
> timebase resync are done for all the threads. This is not only is suboptimal,
> but can cause functionality issues when subcores are involved.
>
> Winkle is deeper idle state compared to fastsleep. In this state the power
> supply to the chiplet, i.e core, private L2 and private L3 is turned off.
> This results in a total hypervisor state loss. This patch set adds support
> for winkle and provides a way to track the idle states of the threads of the
> core and use it for idle state management of idle states sleep and winkle.
>
>
> Changes in v2:
> --------------
> -Using PNV_THREAD_NAP/SLEEP defines while calling power7_powersave_common
> -Comment changes based on review
> -Rebased on top of 3.18-rc6
>
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: linux-pm@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>
> Paul Mackerras (1):
> powerpc: powernv: Switch off MMU before entering nap/sleep/rvwinkle
> mode
>
> Preeti U. Murthy (1):
> powerpc/powernv: Enable Offline CPUs to enter deep idle states
>
> Shreyas B. Prabhu (2):
> powernv: cpuidle: Redesign idle states management
> powernv: powerpc: Add winkle support for offline cpus
>
> arch/powerpc/include/asm/cpuidle.h | 14 ++
> arch/powerpc/include/asm/opal.h | 13 +
> arch/powerpc/include/asm/paca.h | 6 +
> arch/powerpc/include/asm/ppc-opcode.h | 2 +
> arch/powerpc/include/asm/processor.h | 1 +
> arch/powerpc/include/asm/reg.h | 4 +
> arch/powerpc/kernel/asm-offsets.c | 6 +
> arch/powerpc/kernel/cpu_setup_power.S | 4 +
> arch/powerpc/kernel/exceptions-64s.S | 30 ++-
> arch/powerpc/kernel/idle_power7.S | 332 +++++++++++++++++++++----
> arch/powerpc/platforms/powernv/opal-wrappers.S | 39 +++
> arch/powerpc/platforms/powernv/powernv.h | 2 +
> arch/powerpc/platforms/powernv/setup.c | 160 ++++++++++++
> arch/powerpc/platforms/powernv/smp.c | 10 +-
> arch/powerpc/platforms/powernv/subcore.c | 34 +++
> arch/powerpc/platforms/powernv/subcore.h | 1 +
> drivers/cpuidle/cpuidle-powernv.c | 10 +-
> 17 files changed, 608 insertions(+), 60 deletions(-)
> create mode 100644 arch/powerpc/include/asm/cpuidle.h
>
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 7:02 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev
In-Reply-To: <1416915806-24757-1-git-send-email-dahi@linux.vnet.ibm.com>
On Tue, Nov 25, 2014 at 12:43:24PM +0100, David Hildenbrand wrote:
> I recently discovered that commit 662bbcb2747c2422cf98d3d97619509379eee466
> removed/skipped all might_sleep checks for might_fault() calls when in atomic.
Yes. You can add e.g. might_sleep in your code if, for some reason, it is
illegal to call it in an atomic context.
But faults are legal in atomic if you handle the possible
errors, and faults do not necessary cause caller to sleep, so might_fault
should not call might_sleep.
> Reason was to allow calling copy_(to|from)_user while in pagefault_disabled(),
> because otherwise, CONFIG_DEBUG_ATOMIC_SLEEP would find false positives.
That wasn't the only reason BTW.
Andi Kleen also showed that compiler did a terrible job optimizing
get/put user when might_sleep was called.
See e.g. this thread:
https://lkml.org/lkml/2013/8/14/652
There was even an lwn.net article about it, that I don't seem to be
able to locate.
So might_sleep is not appropriate for lightweight operations like __get_user,
which people literally expect to be a single instruction.
I also have a project going which handles very short packets by copying
them into guest memory directly without waking up a thread.
I do it by calling recvmsg on a socket, then switching to
a thread if I get back EFAULT.
Not yet clean enough to upstream but it does seem to cut
the latency down quite a bit, so I'd like to have the option.
Generally, a caller that does something like this under a spinlock:
preempt_disable
pagefault_disable
error = copy_to_user
pagefault_enable
preempt_enable_no_resched
is not doing anything wrong and should not get a warning,
as long as error is handled correctly later.
You can also find the discussion around the patches
educational:
http://article.gmane.org/gmane.comp.emulators.kvm.devel/109928
> However
> we have the inatomic variants of these function for this purpose.
Does inatomic install fixups now?
Last I checked, it didn't so it did not satisfy this purpose.
See this comment from x86:
* Copy data from kernel space to user space. Caller must check
* the specified block with access_ok() before calling this function.
* The caller should also make sure he pins the user space address
* so that we don't result in page fault and sleep.
Also - switching to inatomic would scatter if (atomic) all
over code. It's much better to simply call the same
function (e.g. recvmsg) and have it fail gracefully:
after all, we have code to handle get/put user errors
anyway.
> The result of this change was that all guest access (that correctly uses
> might_fault()) doesn't perform atomic checks when CONFIG_DEBUG_ATOMIC_SLEEP is
> enabled. We lost a mighty debugging feature for user access.
What's the path you are trying to debug?
If your code can faults, then it's safe to call
from atomic context.
If it can't, it must pin the page. You can not do access_ok checks and
then assume access won't fault.
> As I wasn't able to come up with any other reason why this should be
> necessary, I suggest turning the might_sleep() checks on again in the
> might_fault() code.
Faults triggered with pagefault_disabled do not cause
caller to sleep, merely to fail and get an error,
so might_sleep is simply wrong.
>
> pagefault_disable/pagefault_enable seems to be used mainly for futex, perf event
> and kmap.
>
> Going over all changes since that commit, it seems like most code already uses the
> inatomic variants of copy_(to|from)_user. Code relying on (get|put)_user during
> pagefault_disable() don't make use of any might_fault() in their (get|put)_user
> implementation. Examples:
> - arch/m68k/include/asm/futex.h
> - arch/parisc/include/asm/futex.h
> - arch/sh/include/asm/futex-irq.h
> - arch/tile/include/asm/futex.h
> So changing might_fault() back to trigger might_sleep() won't change a thing for
> them. Hope I haven't missed anything.
Did you check the generated code?
On x86 it seems to me this patchset is definitely going to
slow things down, in fact putting back in a performance regression
that Andi found.
> I only identified one might_fault() that would get triggered by get_user() on
> powerpc and fixed it by using the inatomic variant (not tested). I am not sure
> if we need some non-sleeping access_ok() prior to __get_user_inatomic().
>
> By looking at the code I was wondering where the correct place for might_fault()
> calls is? Doesn't seem to be very consistent. Examples:
>
> | asm-generic | arm | arm64 | frv | m32r | x86 and s390
> ---------------------------------------------------------------------------
> get_user() | Yes | Yes | Yes | No | Yes | Yes
> __get_user() | No | Yes | No | No | Yes | No
> put_user() | Yes | Yes | Yes | No | Yes | Yes
> __put_user() | No | Yes | No | No | Yes | No
> copy_to_user() | Yes | No | No | Yes | Yes | Yes
> __copy_to_user() | No | No | No | Yes | No | No
> copy_from_user() | Yes | No | No | Yes | Yes | Yes
> __copy_from_user() | No | No | No | Yes | No | No
>
I think it would be a mistake to make this change.
Most call-sites handle faults in atomic just fine by
returning error to caller.
> So I would have assume that the way asm-generic, x86 and s390 (+ propably
> others) do this is the right way?
> So we can speed up multiple calls to e.g. copy_to_user() by doing the access
> check manually (and also the might_fault() if relevant), then calling
> __copy_to_user().
>
> So in general, I conclude that the concept is:
> 1. __.* variants perform no checking and don't call might_fault()
> 2. [a-z].* variants perform access checking (access_ok() if implemented)
> 3. [a-z].* variants call might_fault()
> 4. .*_inatomic variants usually don't perform access checks
> 5. .*_inatomic variants don't call might_fault()
> 6. If common code uses the __.* variants, it has to trigger access_ok() and
> call might_fault()
> 7. For pagefault_disable(), the inatomic variants are to be used
inatomic variants don't seem to handle faults, so you
must pin any memory you pass to them.
> Comments? Opinions?
>
If the same address is accessed multiple times, access_ok + __
variant can be used to speed access up a bit.
This is rarely the case, but this is the case for e.g. vhost.
But access_ok does not guarantee that no fault will trigger:
there's really no way to do that ATM except pinning the page.
> David Hildenbrand (2):
> powerpc/fsl-pci: atomic get_user when pagefault_disabled
> mm, sched: trigger might_sleep() in might_fault() when atomic
>
> arch/powerpc/sysdev/fsl_pci.c | 2 +-
> include/linux/kernel.h | 8 ++++++--
> mm/memory.c | 11 ++++-------
> 3 files changed, 11 insertions(+), 10 deletions(-)
>
> --
> 1.8.5.5
^ permalink raw reply
* Re: pseries/le: Fix endiannes issue in RTAS call from xmon
From: Laurent Dufour @ 2014-11-26 8:19 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, linux-kernel; +Cc: paulus
In-Reply-To: <20141126033136.977771401AC@ozlabs.org>
On 26/11/2014 04:31, Michael Ellerman wrote:
> OK. You say "LPAR", by which you mean "under phyp" I think. I haven't seen this
> under KVM, and it looks like KVM doesn't implement "set-indicator" so that
> would explain that.
Yes LPAR implies phyp, and KVM don't implement "set-indicator" so this
doesn't happen in that case.
> I'll take this as a bug fix and CC it to stable.
That's a good point.
Thanks,
Laurent.
^ permalink raw reply
* Re: [RESEND, V3] powerpc, xmon: Enable HW instruction breakpoint on POWER8
From: Michael Ellerman @ 2014-11-26 8:25 UTC (permalink / raw)
To: Anshuman Khandual, linuxppc-dev; +Cc: mikey
In-Reply-To: <1416910128-23466-1-git-send-email-khandual@linux.vnet.ibm.com>
On Tue, 2014-25-11 at 10:08:48 UTC, Anshuman Khandual wrote:
> This patch enables support for hardware instruction breakpoints
> on POWER8 with the help of a new register CIABR (Completed
> Instruction Address Breakpoint Register). With this patch, single
> hardware instruction breakpoint can be added and cleared during
> any active xmon debug session. This hardware based instruction
> breakpoint mechanism works correctly along with the existing TRAP
> based instruction breakpoints available on xmon.
Hi Anshuman,
> diff --git a/arch/powerpc/include/asm/xmon.h b/arch/powerpc/include/asm/xmon.h
> index 5eb8e59..5d17aec 100644
> --- a/arch/powerpc/include/asm/xmon.h
> +++ b/arch/powerpc/include/asm/xmon.h
> @@ -29,5 +29,11 @@ static inline void xmon_register_spus(struct list_head *list) { };
> extern int cpus_are_in_xmon(void);
> #endif
This file is the exported interface *of xmon*, it's not the place to put things
that xmon needs internally.
For now just put it in xmon.c
> +#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_PPC_SPLPAR)
> +#include <asm/plpar_wrappers.h>
> +#else
> +static inline long plapr_set_ciabr(unsigned long ciabr) {return 0; };
> +#endif
Also the ifdef is overly verbose, CONFIG_PPC_SPLPAR essentially depends on
CONFIG_PPC_BOOK3S_64. So you can just use #ifdef CONFIG_PPC_SPLPAR.
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index b988b5a..c2f601a 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -271,6 +273,55 @@ static inline void cinval(void *p)
> }
>
> /*
> + * write_ciabr
> + *
> + * This function writes a value to the
> + * CIARB register either directly through
> + * mtspr instruction if the kernel is in HV
> + * privilege mode or call a hypervisor function
> + * to achieve the same in case the kernel is in
> + * supervisor privilege mode.
> + */
I'm not really sure a function this small needs a documentation block.
But if you're going to add one, PLEASE make sure it's an actual kernel-doc
style comment.
You can check with:
$ ./scripts/kernel-doc -text arch/powerpc/xmon/xmon.c
Which you'll notice prints:
Warning(arch/powerpc/xmon/xmon.c): no structured comments found
You need something like:
/**
* write_ciabr() - write the CIABR SPR
* @ciabr: The value to write.
*
* This function writes a value to the CIABR register either directly through
* mtspr instruction if the kernel is in HV privilege mode or calls a
* hypervisor function to achieve the same in case the kernel is in supervisor
* privilege mode.
*/
The rest of the patch is OK. But I was hoping you'd notice that we no longer
support any cpus that implement CPU_FTR_IABR. And so you can just repurpose all
the iabr logic for ciabr.
Something like this, untested:
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index b988b5addf86..6894650bff7f 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -51,6 +51,12 @@
#include <asm/paca.h>
#endif
+#ifdef CONFIG_PPC_SPLPAR
+#include <asm/plpar_wrappers.h>
+#else
+static inline long plapr_set_ciabr(unsigned long ciabr) { return 0; };
+#endif
+
#include "nonstdio.h"
#include "dis-asm.h"
@@ -270,6 +276,31 @@ static inline void cinval(void *p)
asm volatile ("dcbi 0,%0; icbi 0,%0" : : "r" (p));
}
+static void write_ciabr(unsigned long ciabr)
+{
+ if (!cpu_has_feature(CPU_FTR_ARCH_207S))
+ return;
+
+ if (cpu_has_feature(CPU_FTR_HVMODE)) {
+ mtspr(SPRN_CIABR, ciabr);
+ return;
+ }
+
+ plapr_set_ciabr(ciabr);
+}
+
+static void set_ciabr(unsigned long addr)
+{
+ addr &= ~CIABR_PRIV;
+
+ if (cpu_has_feature(CPU_FTR_HVMODE))
+ addr |= CIABR_PRIV_HYPER;
+ else
+ addr |= CIABR_PRIV_SUPER;
+
+ write_ciabr(addr);
+}
+
/*
* Disable surveillance (the service processor watchdog function)
* while we are in xmon.
@@ -764,9 +795,9 @@ static void insert_cpu_bpts(void)
brk.len = 8;
__set_breakpoint(&brk);
}
- if (iabr && cpu_has_feature(CPU_FTR_IABR))
- mtspr(SPRN_IABR, iabr->address
- | (iabr->enabled & (BP_IABR|BP_IABR_TE)));
+
+ if (iabr)
+ set_ciabr(iabr->address);
}
static void remove_bpts(void)
@@ -792,8 +823,7 @@ static void remove_bpts(void)
static void remove_cpu_bpts(void)
{
hw_breakpoint_disable();
- if (cpu_has_feature(CPU_FTR_IABR))
- mtspr(SPRN_IABR, 0);
+ write_ciabr(0);
}
/* Command interpreting routine */
@@ -1127,7 +1157,7 @@ static char *breakpoint_help_string =
"b <addr> [cnt] set breakpoint at given instr addr\n"
"bc clear all breakpoints\n"
"bc <n/addr> clear breakpoint number n or at addr\n"
- "bi <addr> [cnt] set hardware instr breakpoint (POWER3/RS64 only)\n"
+ "bi <addr> [cnt] set hardware instr breakpoint (POWER8 only)\n"
"bd <addr> [cnt] set hardware data breakpoint\n"
"";
@@ -1166,7 +1196,7 @@ bpt_cmds(void)
break;
case 'i': /* bi - hardware instr breakpoint */
- if (!cpu_has_feature(CPU_FTR_IABR)) {
+ if (!cpu_has_feature(CPU_FTR_ARCH_207S)) {
printf("Hardware instruction breakpoint "
"not supported on this cpu\n");
break;
cheers
^ permalink raw reply related
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-26 10:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126070258.GA25523@redhat.com>
Hi Michael,
thanks for your reply! some general thought:
This change was introduced mid 2013 but we don't have a single user relying
on this code change yet, right?
Disabling might_sleep() for functions that clearly state that they may sleep to
get some special case running feels wrong to me.
> On Tue, Nov 25, 2014 at 12:43:24PM +0100, David Hildenbrand wrote:
> > I recently discovered that commit 662bbcb2747c2422cf98d3d97619509379eee466
> > removed/skipped all might_sleep checks for might_fault() calls when in atomic.
>
> Yes. You can add e.g. might_sleep in your code if, for some reason, it is
> illegal to call it in an atomic context.
> But faults are legal in atomic if you handle the possible
> errors, and faults do not necessary cause caller to sleep, so might_fault
> should not call might_sleep.
My point is that and (almost at) everywhere where we use pagefault_disable, we
are using the inatomic variants. Also the documentation of copy_to_user()
clearly states at various points that this function "may sleep":
-> git grep "This function may sleep" yields
"Context: User context only. This function may sleep." e.g. s390, x86, mips
Patching out the might_sleep() from these functions seems more to be a hack to
solve another problem - not using the inatomic variants or finding them not to
be sufficient for a task?
So calling might_sleep() in these functions seems very right to me.
>
> > Reason was to allow calling copy_(to|from)_user while in pagefault_disabled(),
> > because otherwise, CONFIG_DEBUG_ATOMIC_SLEEP would find false positives.
>
> That wasn't the only reason BTW.
> Andi Kleen also showed that compiler did a terrible job optimizing
> get/put user when might_sleep was called.
> See e.g. this thread:
> https://lkml.org/lkml/2013/8/14/652
> There was even an lwn.net article about it, that I don't seem to be
> able to locate.
Thanks, I'll try to look it up! but:
might_sleep() will only be called when lock debugging / sleep in atomic is in,
so this doesn't seem to be a problem for me in a production environment. or am
I missing something?
> So might_sleep is not appropriate for lightweight operations like __get_user,
> which people literally expect to be a single instruction.
I agree that __.* variants should not call might_fault() (like I mentioned
after the table below).
>
>
> I also have a project going which handles very short packets by copying
> them into guest memory directly without waking up a thread.
> I do it by calling recvmsg on a socket, then switching to
> a thread if I get back EFAULT.
> Not yet clean enough to upstream but it does seem to cut
> the latency down quite a bit, so I'd like to have the option.
>
>
> Generally, a caller that does something like this under a spinlock:
> preempt_disable
> pagefault_disable
> error = copy_to_user
So why can't we use the inatomic variant? This seems to be
atomic environment to me. Calling a function that states that it may sleep
doesn't feel right to me.
> pagefault_enable
> preempt_enable_no_resched
>
> is not doing anything wrong and should not get a warning,
> as long as error is handled correctly later.
>
> You can also find the discussion around the patches
> educational:
> http://article.gmane.org/gmane.comp.emulators.kvm.devel/109928
>
> > However
> > we have the inatomic variants of these function for this purpose.
>
> Does inatomic install fixups now?
If not, I think this would rather be the way to go.
>
> Last I checked, it didn't so it did not satisfy this purpose.
> See this comment from x86:
>
> * Copy data from kernel space to user space. Caller must check
> * the specified block with access_ok() before calling this function.
> * The caller should also make sure he pins the user space address
> * so that we don't result in page fault and sleep.
>
>
> Also - switching to inatomic would scatter if (atomic) all
> over code. It's much better to simply call the same
> function (e.g. recvmsg) and have it fail gracefully:
> after all, we have code to handle get/put user errors
> anyway.
>
> > The result of this change was that all guest access (that correctly uses
> > might_fault()) doesn't perform atomic checks when CONFIG_DEBUG_ATOMIC_SLEEP is
> > enabled. We lost a mighty debugging feature for user access.
>
> What's the path you are trying to debug?
Well, we had a problem where we held a spin_lock and called
copy_(from|to)_user(). We experienced very random deadlocks that took some guy
almost a week to debug. The simple might_sleep() check would have showed this
error immediately.
I would have said that in 99,9% of all copy_to_user() calls we want to check
might_sleep(). That pagefault_disable() is a special case that should be
handled differently - in my opinion.
>
> If your code can faults, then it's safe to call
> from atomic context.
> If it can't, it must pin the page. You can not do access_ok checks and
> then assume access won't fault.
>
> > As I wasn't able to come up with any other reason why this should be
> > necessary, I suggest turning the might_sleep() checks on again in the
> > might_fault() code.
>
> Faults triggered with pagefault_disabled do not cause
> caller to sleep, merely to fail and get an error,
> so might_sleep is simply wrong.
>
> >
> > pagefault_disable/pagefault_enable seems to be used mainly for futex, perf event
> > and kmap.
> >
> > Going over all changes since that commit, it seems like most code already uses the
> > inatomic variants of copy_(to|from)_user. Code relying on (get|put)_user during
> > pagefault_disable() don't make use of any might_fault() in their (get|put)_user
> > implementation. Examples:
> > - arch/m68k/include/asm/futex.h
> > - arch/parisc/include/asm/futex.h
> > - arch/sh/include/asm/futex-irq.h
> > - arch/tile/include/asm/futex.h
> > So changing might_fault() back to trigger might_sleep() won't change a thing for
> > them. Hope I haven't missed anything.
>
> Did you check the generated code?
Nope not yet. But as I said, if lock debugging is not enabled, this should
remain as is - without any might_sleep() checks.
> On x86 it seems to me this patchset is definitely going to
> slow things down, in fact putting back in a performance regression
> that Andi found.
>
>
> > I only identified one might_fault() that would get triggered by get_user() on
> > powerpc and fixed it by using the inatomic variant (not tested). I am not sure
> > if we need some non-sleeping access_ok() prior to __get_user_inatomic().
> >
> > By looking at the code I was wondering where the correct place for might_fault()
> > calls is? Doesn't seem to be very consistent. Examples:
> >
> > | asm-generic | arm | arm64 | frv | m32r | x86 and s390
> > ---------------------------------------------------------------------------
> > get_user() | Yes | Yes | Yes | No | Yes | Yes
> > __get_user() | No | Yes | No | No | Yes | No
> > put_user() | Yes | Yes | Yes | No | Yes | Yes
> > __put_user() | No | Yes | No | No | Yes | No
> > copy_to_user() | Yes | No | No | Yes | Yes | Yes
> > __copy_to_user() | No | No | No | Yes | No | No
> > copy_from_user() | Yes | No | No | Yes | Yes | Yes
> > __copy_from_user() | No | No | No | Yes | No | No
> >
>
> I think it would be a mistake to make this change.
>
> Most call-sites handle faults in atomic just fine by
> returning error to caller.
>
> > So I would have assume that the way asm-generic, x86 and s390 (+ propably
> > others) do this is the right way?
> > So we can speed up multiple calls to e.g. copy_to_user() by doing the access
> > check manually (and also the might_fault() if relevant), then calling
> > __copy_to_user().
> >
> > So in general, I conclude that the concept is:
> > 1. __.* variants perform no checking and don't call might_fault()
> > 2. [a-z].* variants perform access checking (access_ok() if implemented)
> > 3. [a-z].* variants call might_fault()
> > 4. .*_inatomic variants usually don't perform access checks
> > 5. .*_inatomic variants don't call might_fault()
> > 6. If common code uses the __.* variants, it has to trigger access_ok() and
> > call might_fault()
> > 7. For pagefault_disable(), the inatomic variants are to be used
>
> inatomic variants don't seem to handle faults, so you
> must pin any memory you pass to them.
>
Would that be an option for your special case?
>
> > Comments? Opinions?
> >
>
> If the same address is accessed multiple times, access_ok + __
> variant can be used to speed access up a bit.
> This is rarely the case, but this is the case for e.g. vhost.
> But access_ok does not guarantee that no fault will trigger:
> there's really no way to do that ATM except pinning the page.
>
>
> > David Hildenbrand (2):
> > powerpc/fsl-pci: atomic get_user when pagefault_disabled
> > mm, sched: trigger might_sleep() in might_fault() when atomic
> >
> > arch/powerpc/sysdev/fsl_pci.c | 2 +-
> > include/linux/kernel.h | 8 ++++++--
> > mm/memory.c | 11 ++++-------
> > 3 files changed, 11 insertions(+), 10 deletions(-)
> >
> > --
> > 1.8.5.5
>
^ permalink raw reply
* Re: powerpc/powernv: Fix the hmi event version check.
From: Mahesh Jagannath Salgaonkar @ 2014-11-26 10:26 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, Benjamin Herrenschmidt
In-Reply-To: <20141126034419.0917A1401B1@ozlabs.org>
On 11/26/2014 09:14 AM, Michael Ellerman wrote:
> On Thu, 2014-20-11 at 04:14:36 UTC, Mahesh Salgaonkar wrote:
>> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>
>> The current HMI event structure is an ABI and carries a version field to
>> accommodate future changes without affecting/rearranging current structure
>> members that are valid for previous versions. The current version check
>> "if (hmi_evt->version != OpalHMIEvt_V1)" seems to consider that version
>> will always be V1 which may not be true in future. If we start supporting
>> HMI event > V1, this check would fail without printing anything on older
>> kernels. This patch fixes this issue.
>
> It's not clear what you mean when you say "this check would fail without
> printing anything". The check will fail, and it will print something, ie. the
> error message.
>
> What you mean is "the check will fail, and the HMI info will not be printed".
My Bad, Yes. I meant 'HMI info will not be printed'. Do you want me to
re spin the patch with correction.
>
> I'll CC this to stable unless you disagree.
Yes. This patch needs to go to stable.
Thanks,
-Mahesh.
^ permalink raw reply
* Disabled LocalPlus Controller (LPC) clock on MPC512x
From: Alexander Popov @ 2014-11-26 11:49 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
Hello.
My Freescale TWR-MPC5125 board instantly reboots if I touch
any physical address on the LocalPlus Bus (LPB) for the first time
when Linux has already booted.
This effect is reproduced by using /dev/mem or loading a kernel module
which works with any peripherals on LPB.
It took me some time to find out that such crash is caused by
clk_disable_unused() in drivers/clk/clk.c, which disables
LocalPlus Controller (LPC) clock if I don't touch LPB addresses in the
previous initcalls. So starting Linux with clk_ignore_unused bootparam
or inserting dummy LPB reading to some initcall is a temporary fix.
Is it correct to gate LPC clock? If yes, how to avoid the mentioned
crashes properly?
There's a piece of code in arch/powerpc/platforms/512x/clock-commonclk.c
which is doubtful for me:
/*
* pre-enable those "internal" clock items which never get
* claimed by any peripheral driver, to not have the clock
* subsystem disable them late at startup
*/
clk_prepare_enable(clks[MPC512x_CLK_DUMMY]);
clk_prepare_enable(clks[MPC512x_CLK_E300]); /* PowerPC CPU */
clk_prepare_enable(clks[MPC512x_CLK_DDR]); /* DRAM */
clk_prepare_enable(clks[MPC512x_CLK_MEM]); /* SRAM */
clk_prepare_enable(clks[MPC512x_CLK_IPS]); /* SoC periph */
clk_prepare_enable(clks[MPC512x_CLK_LPC]); /* boot media */
Does it mean that these clocks should be registered with
CLK_IGNORE_UNUSED flag?
Thanks a lot.
Best regards,
Alexander
^ permalink raw reply
* Re: [PATCH v2 07/14] of/reconfig: Always use the same structure for notifiers
From: Grant Likely @ 2014-11-26 13:16 UTC (permalink / raw)
To: Nathan Fontenot, Benjamin Herrenschmidt
Cc: devicetree, Wolfram Sang, Pantelis Antoniou, linux-kernel,
Rob Herring, Mark Brown, linuxppc-dev
In-Reply-To: <547544FE.5020706@linux.vnet.ibm.com>
On Tue, 25 Nov 2014 21:11:58 -0600
, Nathan Fontenot <nfont@linux.vnet.ibm.com>
wrote:
> On 11/25/2014 05:07 PM, Benjamin Herrenschmidt wrote:
> > On Mon, 2014-11-24 at 22:33 +0000, Grant Likely wrote:
> >> The OF_RECONFIG notifier callback uses a different structure depending
> >> on whether it is a node change or a property change. This is silly, and
> >> not very safe. Rework the code to use the same data structure regardless
> >> of the type of notifier.
> >
> > I fell pretty good about this one except...
> >
> >> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> >> index b9d1dfdbe5bb..9fe6002c1d5a 100644
> >> --- a/arch/powerpc/mm/numa.c
> >> +++ b/arch/powerpc/mm/numa.c
> >> @@ -1711,12 +1711,11 @@ static void stage_topology_update(int core_id)
> >> static int dt_update_callback(struct notifier_block *nb,
> >> unsigned long action, void *data)
> >> {
> >> - struct of_prop_reconfig *update;
> >> + struct of_reconfig_data *update = data;
> >> int rc = NOTIFY_DONE;
> >>
> >> switch (action) {
> >> case OF_RECONFIG_UPDATE_PROPERTY:
> >> - update = (struct of_prop_reconfig *)data;
> >
> > Should we assert/bug on !update->dn / update->prop ?
> >
> > (Same for the rest of the patch)
> >
> > Or do you reckon it's pointless ?
> >
>
> I'm not sure it's worth it, if those are NULL pointers the drivers/of
> code would have tried to use them before invoking the notifier chain.
> We won't make it this far if they're NULL.
Agreed. I'm going to merge it as-is.
g.
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 15:17 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126110504.511b733a@thinkpad-w530>
On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
> > What's the path you are trying to debug?
>
> Well, we had a problem where we held a spin_lock and called
> copy_(from|to)_user(). We experienced very random deadlocks that took some guy
> almost a week to debug. The simple might_sleep() check would have showed this
> error immediately.
This must have been a very old kernel.
A modern kernel will return an error from copy_to_user.
Which is really the point of the patch you are trying to revert.
^ permalink raw reply
* [RFC PATCH v1 1/1] powerpc/85xx: Add support for Emerson/Artesyn MVME2500.
From: Alessio Igor Bogani @ 2014-11-26 14:17 UTC (permalink / raw)
To: Scott Wood, Kumar Gala; +Cc: Alessio Igor Bogani, linuxppc-dev
Add support for the Artesyn MVME2500 Single Board Computer.
The MVME2500 is a 6U form factor VME64 computer with:
- A single Freescale QorIQ P2010 CPU
- 1 GB of DDR3 onboard memory
- Three Gigabit Ethernets
- Five 16550 compatible UARTS
- One USB 2.0 port, one SHDC socket and one SATA connector
- One PCI/PCI eXpress Mezzanine Card (PMC/XMC) Slot
- MultiProcessor Interrupt Controller (MPIC)
- A DS1375T Real Time Clock (RTC) and 512 KB of Non-Volatile Memory
- Two 64 KB EEPROMs
- U-Boot in 16 SPI Flash
This patch is based on linux-3.18-rc6 and has been boot tested.
Signed-off-by: Alessio Igor Bogani <alessio.bogani@elettra.eu>
---
arch/powerpc/boot/dts/mvme2500.dts | 324 +++++++++++++++++++++++++++
arch/powerpc/boot/dts/mvme2500.dtsi | 28 +++
arch/powerpc/configs/85xx/mvme2500_defconfig | 222 ++++++++++++++++++
arch/powerpc/platforms/85xx/Kconfig | 8 +
arch/powerpc/platforms/85xx/Makefile | 1 +
arch/powerpc/platforms/85xx/mvme2500.c | 95 ++++++++
6 files changed, 678 insertions(+)
create mode 100644 arch/powerpc/boot/dts/mvme2500.dts
create mode 100644 arch/powerpc/boot/dts/mvme2500.dtsi
create mode 100644 arch/powerpc/configs/85xx/mvme2500_defconfig
create mode 100644 arch/powerpc/platforms/85xx/mvme2500.c
diff --git a/arch/powerpc/boot/dts/mvme2500.dts b/arch/powerpc/boot/dts/mvme2500.dts
new file mode 100644
index 0000000..fca05fc
--- /dev/null
+++ b/arch/powerpc/boot/dts/mvme2500.dts
@@ -0,0 +1,324 @@
+/*
+ * Device tree source for the Emerson/Artesyn MVME2500
+ *
+ * Copyright 2014 Elettra-Sincrotrone Trieste S.C.p.A.
+ *
+ * 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.
+ *
+ * Based on: P2020 DS Device Tree Source
+ * Copyright 2009 Freescale Semiconductor Inc.
+ */
+
+/include/ "fsl/p2020si-pre.dtsi"
+
+/ {
+ model = "MVME2500";
+ compatible = "artesyn,MVME2500";
+
+ aliases {
+ serial2 = &serial2;
+ serial3 = &serial3;
+ serial4 = &serial4;
+ serial5 = &serial5;
+ };
+
+ memory {
+ device_type = "memory";
+ };
+
+ board_soc: soc: soc@ffe00000 {
+ ranges = <0x0 0 0xffe00000 0x100000>;
+
+ i2c@3000 {
+ hwmon@4c {
+ compatible = "adi,adt7461";
+ reg = <0x4c>;
+ };
+
+ rtc@68 {
+ compatible = "dallas,ds1337";
+ reg = <0x68>;
+ interrupts = <8 1 0 0>;
+ };
+
+ eeprom-vpd@54 {
+ compatible = "atmel,24c64";
+ reg = <0x54>;
+ };
+
+ eeprom@52 {
+ compatible = "atmel,24c512";
+ reg = <0x52>;
+ };
+
+ eeprom@53 {
+ compatible = "atmel,24c512";
+ reg = <0x53>;
+ };
+
+ spd@50 {
+ compatible = "atmel,24c02";
+ reg = <0x50>;
+ };
+
+ };
+
+ spi0: spi@7000 {
+ fsl,espi-num-chipselects = <2>;
+
+ flash@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "atmel,at25df641";
+ reg = <0>;
+ spi-max-frequency = <10000000>;
+ partition@u-boot {
+ label = "u-boot";
+ reg = <0x00000000 0x000A0000>;
+ read-only;
+ };
+ partition@dtb {
+ label = "dtb";
+ reg = <0x000A0000 0x00020000>;
+ };
+ partition@misc {
+ label = "misc";
+ reg = <0x000C0000 0x00040000>;
+ };
+ partition@u-boot-env {
+ label = "u-boot-env";
+ reg = <0x00100000 0x00020000>;
+ };
+ partition@kernel {
+ label = "kernel";
+ reg = <0x00120000 0x004E0000>;
+ };
+ partition@fs {
+ label = "file system";
+ reg = <0x00600000 0x00200000>;
+ };
+ };
+ flash@1 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "atmel,at25df641";
+ reg = <1>;
+ spi-max-frequency = <10000000>;
+ partition@u-boot {
+ label = "u-boot";
+ reg = <0x00000000 0x000A0000>;
+ read-only;
+ };
+ partition@dtb {
+ label = "dtb";
+ reg = <0x000A0000 0x00020000>;
+ };
+ partition@misc {
+ label = "misc";
+ reg = <0x000C0000 0x00040000>;
+ };
+ partition@u-boot-env {
+ label = "u-boot-env";
+ reg = <0x00100000 0x00020000>;
+ };
+ partition@kernel {
+ label = "kernel";
+ reg = <0x00120000 0x004E0000>;
+ };
+ partition@fs {
+ label = "file system";
+ reg = <0x00600000 0x00200000>;
+ };
+ };
+ };
+
+ usb@22000 {
+ dr_mode = "host";
+ phy_type = "ulpi";
+ };
+
+ enet0: ethernet@24000 {
+ tbi-handle = <&tbi0>;
+ phy-handle = <&phy1>;
+ phy-connection-type = "rgmii-id";
+ };
+
+ mdio@24520 {
+ phy1: ethernet-phy@1 {
+ compatible = "brcm,bcm54616S";
+ interrupts = <6 1 0 0>;
+ reg = <0x1>;
+ };
+
+ phy2: ethernet-phy@2 {
+ compatible = "brcm,bcm54616S";
+ interrupts = <6 1 0 0>;
+ reg = <0x2>;
+ };
+
+ phy3: ethernet-phy@3 {
+ compatible = "brcm,bcm54616S";
+ interrupts = <5 1 0 0>;
+ reg = <0x3>;
+ };
+
+ phy7: ethernet-phy@7 {
+ compatible = "brcm,bcm54616S";
+ interrupts = <7 1 0 0>;
+ reg = <0x7>;
+ };
+
+ tbi0: tbi-phy@11 {
+ reg = <0x11>;
+ device_type = "tbi-phy";
+ };
+ };
+
+ enet1: ethernet@25000 {
+ tbi-handle = <&tbi1>;
+ phy-handle = <&phy7>;
+ phy-connection-type = "rgmii-id";
+ };
+
+ mdio@25520 {
+ tbi1: tbi-phy@11 {
+ reg = <0x11>;
+ device_type = "tbi-phy";
+ };
+ };
+
+ enet2: ethernet@26000 {
+ tbi-handle = <&tbi2>;
+ phy-handle = <&phy3>;
+ phy-connection-type = "rgmii-id";
+ };
+
+ mdio@26520 {
+ tbi2: tbi-phy@11 {
+ reg = <0x11>;
+ device_type = "tbi-phy";
+ };
+ };
+
+ sdhc@2e000 {
+ non-removable;
+ };
+ };
+
+ lbc: localbus@ffe05000 {
+ reg = <0 0xffe05000 0 0x1000>;
+
+ ranges = <0x0 0x0 0x0 0xfff00000 0x00080000
+ 0x1 0x0 0x0 0xffc40000 0x00010000
+ 0x2 0x0 0x0 0xffc50000 0x00010000
+ 0x3 0x0 0x0 0xffc60000 0x00010000
+ 0x4 0x0 0x0 0xffc70000 0x00010000
+ 0x6 0x0 0x0 0xffc80000 0x00010000
+ 0x5 0x0 0x0 0xffdf0000 0x00001000>;
+
+ serial2: serial@1,0 {
+ #cell-index = <2>;
+ device_type = "serial";
+ compatible = "ns16550";
+ reg = <0x1 0x0 0x100>;
+ clock-frequency = <1843200>;
+ interrupts = <11 2 0 0>;
+ };
+
+ serial3: serial@2,0 {
+ #cell-index = <3>;
+ device_type = "serial";
+ compatible = "ns16550";
+ reg = <0x2 0x0 0x100>;
+ clock-frequency = <1843200>;
+ interrupts = <1 2 0 0>;
+ };
+
+ serial4: serial@3,0 {
+ #cell-index = <4>;
+ device_type = "serial";
+ compatible = "ns16550";
+ reg = <0x3 0x0 0x100>;
+ clock-frequency = <1843200>;
+ interrupts = <2 2 0 0>;
+ };
+
+ serial5: serial@4,0 {
+ #cell-index = <5>;
+ device_type = "serial";
+ compatible = "ns16550";
+ reg = <0x4 0x0 0x100>;
+ clock-frequency = <1843200>;
+ interrupts = <3 2 0 0>;
+ };
+
+ nvram@0,0 {
+ device_type = "nvram";
+ compatible = "everspin,mram";
+ reg = <0x0 0x0 0x80000>;
+ };
+
+ board-control@5,0 {
+ compatible = "artesyn,mvme2500-fpga";
+ reg = <0x5 0x0 0x01000>;
+ };
+
+ cpld@6,0 {
+ compatible = "artesyn,mvme2500-cpld";
+ reg = <0x6 0x0 0x10000>;
+ interrupts = <9 1 0 0 >;
+ };
+ };
+
+ pci0: pcie@ffe08000 {
+ reg = <0 0xffe08000 0 0x1000>;
+ ranges = <0x2000000 0x0 0x80000000 0 0x80000000 0x0 0x20000000
+ 0x1000000 0x0 0x00000000 0 0xffc00000 0x0 0x10000>;
+ pcie@0 {
+ ranges = <0x2000000 0x0 0x80000000
+ 0x2000000 0x0 0x80000000
+ 0x0 0x20000000
+
+ 0x1000000 0x0 0x0
+ 0x1000000 0x0 0x0
+ 0x0 0x10000>;
+ };
+ };
+
+ pci1: pcie@ffe09000 {
+ reg = <0 0xffe09000 0 0x1000>;
+ ranges = <0x2000000 0x0 0xa0000000 0 0xa0000000 0x0 0x20000000
+ 0x1000000 0x0 0x00000000 0 0xffc10000 0x0 0x10000>;
+ pcie@0 {
+ ranges = <0x2000000 0x0 0xa0000000
+ 0x2000000 0x0 0xa0000000
+ 0x0 0x20000000
+
+ 0x1000000 0x0 0x0
+ 0x1000000 0x0 0x0
+ 0x0 0x10000>;
+ };
+
+ };
+
+ pci2: pcie@ffe0a000 {
+ reg = <0 0xffe0a000 0 0x1000>;
+ ranges = <0x2000000 0x0 0xc0000000 0 0xc0000000 0x0 0x20000000
+ 0x1000000 0x0 0x00000000 0 0xffc20000 0x0 0x10000>;
+ pcie@0 {
+ ranges = <0x2000000 0x0 0xc0000000
+ 0x2000000 0x0 0xc0000000
+ 0x0 0x20000000
+
+ 0x1000000 0x0 0x0
+ 0x1000000 0x0 0x0
+ 0x0 0x10000>;
+ };
+ };
+};
+
+/include/ "fsl/p2020si-post.dtsi"
+/include/ "mvme2500.dtsi"
diff --git a/arch/powerpc/boot/dts/mvme2500.dtsi b/arch/powerpc/boot/dts/mvme2500.dtsi
new file mode 100644
index 0000000..6966f13
--- /dev/null
+++ b/arch/powerpc/boot/dts/mvme2500.dtsi
@@ -0,0 +1,28 @@
+/*
+ * Device tree source stub for the Emerson/Artesyn MVME2500
+ *
+ * Copyright 2014 Elettra-Sincrotrone Trieste S.C.p.A.
+ *
+ * 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.
+ */
+
+&board_soc {
+ // Not connected
+ i2c@3000 {
+ status = "disabled";
+ };
+
+ // Note connected
+ serial@4600 {
+ status = "disabled";
+ };
+
+ gpio-controller@f000 {
+ reg = <0xfc00 0x100>;
+ };
+
+/include/ "fsl/pq3-mpic-message-B.dtsi"
+};
diff --git a/arch/powerpc/configs/85xx/mvme2500_defconfig b/arch/powerpc/configs/85xx/mvme2500_defconfig
new file mode 100644
index 0000000..06fe629
--- /dev/null
+++ b/arch/powerpc/configs/85xx/mvme2500_defconfig
@@ -0,0 +1,222 @@
+CONFIG_PPC_85xx=y
+# CONFIG_LOCALVERSION_AUTO is not set
+CONFIG_SYSVIPC=y
+CONFIG_POSIX_MQUEUE=y
+CONFIG_FHANDLE=y
+CONFIG_AUDIT=y
+CONFIG_HIGH_RES_TIMERS=y
+CONFIG_BSD_PROCESS_ACCT=y
+CONFIG_IKCONFIG=y
+CONFIG_IKCONFIG_PROC=y
+CONFIG_CGROUPS=y
+CONFIG_NAMESPACES=y
+CONFIG_USER_NS=y
+CONFIG_BLK_DEV_INITRD=y
+CONFIG_CC_OPTIMIZE_FOR_SIZE=y
+CONFIG_SYSCTL_SYSCALL=y
+CONFIG_KALLSYMS_ALL=y
+CONFIG_EMBEDDED=y
+CONFIG_PERF_EVENTS=y
+CONFIG_SLAB=y
+CONFIG_MODULES=y
+CONFIG_MODULE_UNLOAD=y
+CONFIG_MODULE_FORCE_UNLOAD=y
+CONFIG_MODVERSIONS=y
+# CONFIG_BLK_DEV_BSG is not set
+CONFIG_PARTITION_ADVANCED=y
+# CONFIG_EFI_PARTITION is not set
+# CONFIG_IOSCHED_DEADLINE is not set
+CONFIG_MVME2500=y
+CONFIG_HIGHMEM=y
+CONFIG_HZ_1000=y
+# CONFIG_CORE_DUMP_DEFAULT_ELF_HEADERS is not set
+CONFIG_MATH_EMULATION=y
+CONFIG_MATH_EMULATION_HW_UNIMPLEMENTED=y
+# CONFIG_COMPACTION is not set
+# CONFIG_MIGRATION is not set
+# CONFIG_BOUNCE is not set
+CONFIG_FORCE_MAX_ZONEORDER=13
+# CONFIG_SUSPEND is not set
+# CONFIG_SECCOMP is not set
+CONFIG_FSL_LBC=y
+CONFIG_PCI=y
+CONFIG_PCIEPORTBUS=y
+# CONFIG_PCIEASPM is not set
+CONFIG_PCI_MSI=y
+CONFIG_ADVANCED_OPTIONS=y
+CONFIG_LOWMEM_SIZE_BOOL=y
+CONFIG_LOWMEM_SIZE=0x40000000
+CONFIG_PAGE_OFFSET_BOOL=y
+CONFIG_PAGE_OFFSET=0x80000000
+CONFIG_KERNEL_START_BOOL=y
+CONFIG_TASK_SIZE_BOOL=y
+CONFIG_TASK_SIZE=0x80000000
+CONFIG_NET=y
+CONFIG_PACKET=y
+CONFIG_UNIX=y
+CONFIG_INET=y
+CONFIG_IP_PNP=y
+CONFIG_IP_PNP_DHCP=y
+CONFIG_IP_PNP_BOOTP=y
+CONFIG_IP_PNP_RARP=y
+# CONFIG_INET_XFRM_MODE_TRANSPORT is not set
+# CONFIG_INET_XFRM_MODE_TUNNEL is not set
+# CONFIG_INET_XFRM_MODE_BEET is not set
+# CONFIG_INET_LRO is not set
+# CONFIG_INET_DIAG is not set
+CONFIG_IPV6=y
+# CONFIG_INET6_XFRM_MODE_TRANSPORT is not set
+# CONFIG_INET6_XFRM_MODE_TUNNEL is not set
+# CONFIG_INET6_XFRM_MODE_BEET is not set
+# CONFIG_IPV6_SIT is not set
+# CONFIG_WIRELESS is not set
+CONFIG_DEVTMPFS=y
+CONFIG_DEVTMPFS_MOUNT=y
+CONFIG_MTD=y
+CONFIG_MTD_M25P80=y
+CONFIG_MTD_SPI_NOR=y
+CONFIG_BLK_DEV_RAM=y
+CONFIG_BLK_DEV_RAM_SIZE=262144
+CONFIG_EEPROM_AT24=y
+# CONFIG_SCSI_PROC_FS is not set
+CONFIG_BLK_DEV_SD=y
+CONFIG_CHR_DEV_SG=y
+# CONFIG_SCSI_LOWLEVEL is not set
+CONFIG_ATA=y
+# CONFIG_ATA_VERBOSE_ERROR is not set
+CONFIG_SATA_AHCI=y
+CONFIG_MD=y
+CONFIG_BLK_DEV_MD=y
+CONFIG_BLK_DEV_DM=y
+CONFIG_NETDEVICES=y
+# CONFIG_NET_VENDOR_3COM is not set
+# CONFIG_NET_VENDOR_ADAPTEC is not set
+# CONFIG_NET_VENDOR_AGERE is not set
+# CONFIG_NET_VENDOR_ALTEON is not set
+# CONFIG_NET_VENDOR_AMD is not set
+# CONFIG_NET_VENDOR_ARC is not set
+# CONFIG_NET_VENDOR_ATHEROS is not set
+# CONFIG_NET_VENDOR_BROADCOM is not set
+# CONFIG_NET_VENDOR_BROCADE is not set
+# CONFIG_NET_VENDOR_CHELSIO is not set
+# CONFIG_NET_VENDOR_CISCO is not set
+# CONFIG_NET_VENDOR_DEC is not set
+# CONFIG_NET_VENDOR_DLINK is not set
+# CONFIG_NET_VENDOR_EMULEX is not set
+# CONFIG_NET_VENDOR_EXAR is not set
+CONFIG_GIANFAR=y
+# CONFIG_NET_VENDOR_HP is not set
+# CONFIG_NET_VENDOR_INTEL is not set
+# CONFIG_NET_VENDOR_MARVELL is not set
+# CONFIG_NET_VENDOR_MELLANOX is not set
+# CONFIG_NET_VENDOR_MICREL is not set
+# CONFIG_NET_VENDOR_MICROCHIP is not set
+# CONFIG_NET_VENDOR_MYRI is not set
+# CONFIG_NET_VENDOR_NATSEMI is not set
+# CONFIG_NET_VENDOR_NVIDIA is not set
+# CONFIG_NET_VENDOR_OKI is not set
+# CONFIG_NET_PACKET_ENGINE is not set
+# CONFIG_NET_VENDOR_QLOGIC is not set
+# CONFIG_NET_VENDOR_QUALCOMM is not set
+# CONFIG_NET_VENDOR_REALTEK is not set
+# CONFIG_NET_VENDOR_RDC is not set
+# CONFIG_NET_VENDOR_SAMSUNG is not set
+# CONFIG_NET_VENDOR_SEEQ is not set
+# CONFIG_NET_VENDOR_SILAN is not set
+# CONFIG_NET_VENDOR_SIS is not set
+# CONFIG_NET_VENDOR_SMSC is not set
+# CONFIG_NET_VENDOR_STMICRO is not set
+# CONFIG_NET_VENDOR_SUN is not set
+# CONFIG_NET_VENDOR_TEHUTI is not set
+# CONFIG_NET_VENDOR_TI is not set
+# CONFIG_NET_VENDOR_VIA is not set
+# CONFIG_NET_VENDOR_WIZNET is not set
+# CONFIG_NET_VENDOR_XILINX is not set
+CONFIG_BROADCOM_PHY=y
+CONFIG_FIXED_PHY=y
+# CONFIG_USB_NET_DRIVERS is not set
+# CONFIG_WLAN is not set
+# CONFIG_INPUT_MOUSEDEV is not set
+# CONFIG_INPUT_KEYBOARD is not set
+# CONFIG_INPUT_MOUSE is not set
+# CONFIG_SERIO_I8042 is not set
+CONFIG_SERIO_LIBPS2=y
+CONFIG_SERIAL_8250=y
+CONFIG_SERIAL_8250_CONSOLE=y
+CONFIG_SERIAL_8250_NR_UARTS=6
+CONFIG_SERIAL_8250_RUNTIME_UARTS=6
+CONFIG_SERIAL_8250_RSA=y
+CONFIG_NVRAM=y
+CONFIG_I2C=y
+# CONFIG_I2C_COMPAT is not set
+CONFIG_I2C_CHARDEV=y
+CONFIG_I2C_MPC=y
+CONFIG_SPI=y
+CONFIG_SPI_BITBANG=y
+CONFIG_SPI_FSL_ESPI=y
+CONFIG_GPIOLIB=y
+CONFIG_GPIO_GENERIC_PLATFORM=y
+CONFIG_GPIO_MPC8XXX=y
+CONFIG_SENSORS_LM90=y
+CONFIG_THERMAL=y
+# CONFIG_VGA_ARB is not set
+CONFIG_USB=y
+CONFIG_USB_ANNOUNCE_NEW_DEVICES=y
+CONFIG_USB_EHCI_HCD=y
+# CONFIG_USB_EHCI_TT_NEWSCHED is not set
+CONFIG_USB_EHCI_FSL=y
+# CONFIG_USB_EHCI_HCD_PPC_OF is not set
+CONFIG_USB_STORAGE=y
+CONFIG_MMC=y
+CONFIG_MMC_SDHCI=y
+CONFIG_MMC_SDHCI_PLTFM=y
+CONFIG_MMC_SDHCI_OF_ESDHC=y
+CONFIG_EDAC=y
+# CONFIG_EDAC_LEGACY_SYSFS is not set
+CONFIG_EDAC_MM_EDAC=y
+CONFIG_EDAC_MPC85XX=y
+CONFIG_RTC_CLASS=y
+CONFIG_RTC_DRV_DS1307=y
+CONFIG_DMADEVICES=y
+CONFIG_FSL_DMA=y
+CONFIG_ASYNC_TX_DMA=y
+CONFIG_STAGING=y
+CONFIG_VME_BUS=y
+CONFIG_VME_TSI148=y
+CONFIG_VME_USER=y
+CONFIG_GENERIC_PHY=y
+CONFIG_EXT2_FS=y
+CONFIG_EXT3_FS=y
+# CONFIG_EXT3_DEFAULTS_TO_ORDERED is not set
+CONFIG_EXT4_FS=y
+CONFIG_FANOTIFY=y
+CONFIG_AUTOFS4_FS=y
+CONFIG_ISO9660_FS=y
+CONFIG_JOLIET=y
+CONFIG_ZISOFS=y
+CONFIG_UDF_FS=y
+CONFIG_MSDOS_FS=y
+CONFIG_VFAT_FS=y
+CONFIG_NTFS_FS=y
+CONFIG_PROC_KCORE=y
+CONFIG_TMPFS=y
+CONFIG_TMPFS_POSIX_ACL=y
+# CONFIG_MISC_FILESYSTEMS is not set
+CONFIG_NFS_FS=y
+CONFIG_NFS_V4=y
+CONFIG_ROOT_NFS=y
+CONFIG_NFSD=y
+CONFIG_NFSD_V3=y
+CONFIG_NLS_CODEPAGE_437=y
+CONFIG_NLS_ASCII=y
+CONFIG_NLS_ISO8859_1=y
+CONFIG_NLS_UTF8=y
+CONFIG_DEBUG_INFO=y
+CONFIG_MAGIC_SYSRQ=y
+CONFIG_DETECT_HUNG_TASK=y
+# CONFIG_SCHED_DEBUG is not set
+# CONFIG_FTRACE is not set
+# CONFIG_STRICT_DEVMEM is not set
+# CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
+# CONFIG_CRYPTO_ANSI_CPRNG is not set
+CONFIG_CRYPTO_DEV_TALITOS=y
diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
index f22635a..b92674a 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -241,6 +241,14 @@ config SGY_CTS1000
help
Enable this to support functionality in Servergy's CTS-1000 systems.
+config MVME2500
+ bool "Artesyn MVME2500"
+ select DEFAULT_UIMAGE
+ select SWIOTLB
+ select MMIO_NVRAM
+ help
+ This option enables support for the Emerson/Artesyn MVME2500 board.
+
endif # PPC32
config PPC_QEMU_E500
diff --git a/arch/powerpc/platforms/85xx/Makefile b/arch/powerpc/platforms/85xx/Makefile
index 7303260..1fe7fb9 100644
--- a/arch/powerpc/platforms/85xx/Makefile
+++ b/arch/powerpc/platforms/85xx/Makefile
@@ -31,3 +31,4 @@ obj-$(CONFIG_XES_MPC85xx) += xes_mpc85xx.o
obj-$(CONFIG_GE_IMP3A) += ge_imp3a.o
obj-$(CONFIG_PPC_QEMU_E500) += qemu_e500.o
obj-$(CONFIG_SGY_CTS1000) += sgy_cts1000.o
+obj-$(CONFIG_MVME2500) += mvme2500.o
diff --git a/arch/powerpc/platforms/85xx/mvme2500.c b/arch/powerpc/platforms/85xx/mvme2500.c
new file mode 100644
index 0000000..af8c4eb
--- /dev/null
+++ b/arch/powerpc/platforms/85xx/mvme2500.c
@@ -0,0 +1,95 @@
+/*
+ * Board setup routines for the Emerson/Artesyn MVME2500
+ *
+ * Copyright 2014 Elettra-Sincrotrone Trieste S.C.p.A.
+ *
+ * Based on earlier code by:
+ *
+ * Xianghua Xiao (x.xiao@freescale.com)
+ * Tom Armistead (tom.armistead@emerson.com)
+ * Copyright 2012 Emerson
+ *
+ * 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.
+ *
+ * Author Alessio Igor Bogani <alessio.bogani@elettra.eu>
+ *
+ */
+
+#include <linux/stddef.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/kdev_t.h>
+#include <linux/delay.h>
+#include <linux/seq_file.h>
+#include <linux/interrupt.h>
+#include <linux/of_platform.h>
+
+#include <asm/time.h>
+#include <asm/machdep.h>
+#include <asm/pci-bridge.h>
+#include <mm/mmu_decl.h>
+#include <asm/prom.h>
+#include <asm/udbg.h>
+#include <asm/mpic.h>
+#include <asm/swiotlb.h>
+#include <asm/nvram.h>
+
+#include <sysdev/fsl_soc.h>
+#include <sysdev/fsl_pci.h>
+
+#include "mpc85xx.h"
+
+void __init mvme2500_pic_init(void)
+{
+ struct mpic *mpic = mpic_alloc(NULL, 0,
+ MPIC_BIG_ENDIAN | MPIC_SINGLE_DEST_CPU,
+ 0, 256, " OpenPIC ");
+ BUG_ON(mpic == NULL);
+ mpic_init(mpic);
+}
+
+/*
+ * Setup the architecture
+ */
+static void __init mvme2500_setup_arch(void)
+{
+ if (ppc_md.progress)
+ ppc_md.progress("mvme2500_setup_arch()", 0);
+ fsl_pci_assign_primary();
+ swiotlb_detect_4g();
+#if defined(CONFIG_MMIO_NVRAM)
+ mmio_nvram_init();
+#endif
+ printk(KERN_INFO "MVME2500 board from Artesyn\n");
+}
+
+machine_arch_initcall(mvme2500, mpc85xx_common_publish_devices);
+machine_arch_initcall(mvme2500, swiotlb_setup_bus_notifier);
+
+/*
+ * Called very early, device-tree isn't unflattened
+ */
+static int __init mvme2500_probe(void)
+{
+ unsigned long root = of_get_flat_dt_root();
+
+ return of_flat_dt_is_compatible(root, "Artesyn,MVME2500");
+}
+
+define_machine(mvme2500) {
+ .name = "MVME2500",
+ .probe = mvme2500_probe,
+ .setup_arch = mvme2500_setup_arch,
+ .init_IRQ = mvme2500_pic_init,
+#ifdef CONFIG_PCI
+ .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
+ .pcibios_fixup_phb = fsl_pcibios_fixup_phb,
+#endif
+ .get_irq = mpic_get_irq,
+ .restart = fsl_rstcr_restart,
+ .calibrate_decr = generic_calibrate_decr,
+ .progress = udbg_progress,
+};
--
2.1.3
^ permalink raw reply related
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 15:22 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126110504.511b733a@thinkpad-w530>
Hmm you sent same mail to me off-list, and I replied there.
Now there's a copy on list - I'm just going to assume
it's exactly identical, pasting my response here as well.
If there are more questions I missed, let me know.
On Wed, Nov 26, 2014 at 09:23:31AM +0100, David Hildenbrand wrote:
> Sorry I haven't put you on cc, must have lost you while packing my list :)
> Thanks for your answer!
>
> This change was introduced in 2013, and I haven't seen an instance making use
> of your described scenario, right?
My work is still out of tree. RHEL6 shipped some patches that use
this. I don't know whether any instances in-tree use this capability.
But it just doesn't make sense for might_fault to call might_sleep
because a fault does not imply sleep.
> > On Tue, Nov 25, 2014 at 12:43:24PM +0100, David Hildenbrand wrote:
> > > I recently discovered that commit 662bbcb2747c2422cf98d3d97619509379eee466
> > > removed/skipped all might_sleep checks for might_fault() calls when in atomic.
> >
> > Yes. You can add e.g. might_sleep in your code if, for some reason, it is
> > illegal to call it in an atomic context.
> > But faults are legal in atomic if you handle the possible
> > errors, and faults do not necessary cause caller to sleep, so might_fault
> > should not call might_sleep.
>
> I think we should use in_atomic variants for this purpose (as done in the code
> for now) - especially as pagefault_disable has been relying on the preempt
> count for a long time. But see my comment below about existing documentation.
IIUC they are not interchangeable.
*in_atomic seems to require that page is pinned.
*user does not: it installs a fixup so you get an error code if you try
to access an invalid address.
Besides, this would just lead to a ton of
if (atomic) return inatomic else return user
in code, for no good purpose.
> >
> > > Reason was to allow calling copy_(to|from)_user while in pagefault_disabled(),
> > > because otherwise, CONFIG_DEBUG_ATOMIC_SLEEP would find false positives.
> >
> > That wasn't the only reason BTW.
> > Andi Kleen also showed that compiler did a terrible job optimizing
> > get/put user when might_sleep was called.
>
> might_fault() should never call might_sleep() when lock debugging is off, so
> there should be no performance problem or am I missing something?
CONFIG_DEBUG_ATOMIC_SLEEP too, no?
> > See e.g. this thread:
> > https://lkml.org/lkml/2013/8/14/652
> > There was even an lwn.net article about it, that I don't seem to be
> > able to locate.
>
> Thanks, will see if I can find it.
>
> > So might_sleep is not appropriate for lightweight operations like __get_user,
> > which people literally expect to be a single instruction.
>
> Yes, as discussed below, __.* variants should never call it.
So that would be even more inconsistent. They fault exactly the same as
the non __ variants.
> >
> >
> > I also have a project going which handles very short packets by copying
> > them into guest memory directly without waking up a thread.
> > I do it by calling recvmsg on a socket, then switching to
> > a thread if I get back EFAULT.
> > Not yet clean enough to upstream but it does seem to cut
> > the latency down quite a bit, so I'd like to have the option.
> >
> >
> > Generally, a caller that does something like this under a spinlock:
> > preempt_disable
> > pagefault_disable
> > error = copy_to_user
> > pagefault_enable
> > preempt_enable_no_resched
> >
> > is not doing anything wrong and should not get a warning,
> > as long as error is handled correctly later.
>
> I think this would be a perfect fit for an inatomic variant, no?
No because inatomic does not handle faults.
> I mean even the copy_to_user documentation of e.g. s390, x86, mips
> clearly says:
> "Context: User context only.>-This function may sleep."
So the comment is incomplete - I didn't get around fixing that.
It may sleep but not in atomic context.
> So calling it from your described scenario is wrong. And as the interface says,
> it might_sleep() and therefore also call the check in might_fault().
Exactly the reverse.
There's no way for it to sleep except on fault and that only if
preempttion is on.
> >
> > You can also find the discussion around the patches
> > educational:
> > http://article.gmane.org/gmane.comp.emulators.kvm.devel/109928
> >
> > > However
> > > we have the inatomic variants of these function for this purpose.
> >
> > Does inatomic install fixups now?
>
> I think this varies between architectures but I am no expert. But as 99,9% of
> all pagefault_disable code uses inatomic, I would have guessed that this is
> rather the way to got than simply using the non atomic variant that clearly
> states on the interface that it might sleep?
Let's go ahead and make the comment more exact then.
> >
> > Last I checked, it didn't so it did not satisfy this purpose.
> > See this comment from x86:
> >
> > * Copy data from kernel space to user space. Caller must check
> > * the specified block with access_ok() before calling this function.
> > * The caller should also make sure he pins the user space address
> > * so that we don't result in page fault and sleep.
> >
> >
> > Also - switching to inatomic would scatter if (atomic) all
> > over code. It's much better to simply call the same
> > function (e.g. recvmsg) and have it fail gracefully:
> > after all, we have code to handle get/put user errors
> > anyway.
> >
> > > The result of this change was that all guest access (that correctly uses
> > > might_fault()) doesn't perform atomic checks when CONFIG_DEBUG_ATOMIC_SLEEP is
> > > enabled. We lost a mighty debugging feature for user access.
> >
> > What's the path you are trying to debug?
> >
>
> Well, if you are holding a spin_lock and call copy_to_guest() you would love to
> see why you get deadlocks, such bugs are really hard to find (... and might
> take people almost a week to identify ...)
Is copy_to_guest same as copy_to_user?
I was unable to find it in tree.
If yes, you will not get deadlocks - it will simply fail.
> > If your code can faults, then it's safe to call
> > from atomic context.
> > If it can't, it must pin the page. You can not do access_ok checks and
> > then assume access won't fault.
> >
> > > As I wasn't able to come up with any other reason why this should be
> > > necessary, I suggest turning the might_sleep() checks on again in the
> > > might_fault() code.
> >
> > Faults triggered with pagefault_disabled do not cause
> > caller to sleep, merely to fail and get an error,
> > so might_sleep is simply wrong.
>
> And my point is, that we need a separate function for this scenario
To me it looks like you want to add a bunch of code for the sole purpose
of then making it easier to debug it.
> (in my
> opinion inatomic) - I mean the caller knows what he is doing, so he can handle
> it properly with inatomic, or am I missing something?
No, the caller gets pointer from userspace so it still must be
validated, inatomic does not do this.
> >
> > >
> > > pagefault_disable/pagefault_enable seems to be used mainly for futex, perf event
> > > and kmap.
> > >
> > > Going over all changes since that commit, it seems like most code already uses the
> > > inatomic variants of copy_(to|from)_user. Code relying on (get|put)_user during
> > > pagefault_disable() don't make use of any might_fault() in their (get|put)_user
> > > implementation. Examples:
> > > - arch/m68k/include/asm/futex.h
> > > - arch/parisc/include/asm/futex.h
> > > - arch/sh/include/asm/futex-irq.h
> > > - arch/tile/include/asm/futex.h
> > > So changing might_fault() back to trigger might_sleep() won't change a thing for
> > > them. Hope I haven't missed anything.
> >
> > Did you check the generated code?
> > On x86 it seems to me this patchset is definitely going to
> > slow things down, in fact putting back in a performance regression
> > that Andi found.
>
> Should be optimized out without lock debugging, right?
You can compile it out, but CONFIG_DEBUG_ATOMIC_SLEEP
is pretty common.
> >
> >
> > > I only identified one might_fault() that would get triggered by get_user() on
> > > powerpc and fixed it by using the inatomic variant (not tested). I am not sure
> > > if we need some non-sleeping access_ok() prior to __get_user_inatomic().
> > >
> > > By looking at the code I was wondering where the correct place for might_fault()
> > > calls is? Doesn't seem to be very consistent. Examples:
> > >
> > > | asm-generic | arm | arm64 | frv | m32r | x86 and s390
> > > ---------------------------------------------------------------------------
> > > get_user() | Yes | Yes | Yes | No | Yes | Yes
> > > __get_user() | No | Yes | No | No | Yes | No
> > > put_user() | Yes | Yes | Yes | No | Yes | Yes
> > > __put_user() | No | Yes | No | No | Yes | No
> > > copy_to_user() | Yes | No | No | Yes | Yes | Yes
> > > __copy_to_user() | No | No | No | Yes | No | No
> > > copy_from_user() | Yes | No | No | Yes | Yes | Yes
> > > __copy_from_user() | No | No | No | Yes | No | No
> > >
> >
> > I think it would be a mistake to make this change.
> >
> > Most call-sites handle faults in atomic just fine by
> > returning error to caller.
> >
> > > So I would have assume that the way asm-generic, x86 and s390 (+ propably
> > > others) do this is the right way?
> > > So we can speed up multiple calls to e.g. copy_to_user() by doing the access
> > > check manually (and also the might_fault() if relevant), then calling
> > > __copy_to_user().
> > >
> > > So in general, I conclude that the concept is:
> > > 1. __.* variants perform no checking and don't call might_fault()
> > > 2. [a-z].* variants perform access checking (access_ok() if implemented)
> > > 3. [a-z].* variants call might_fault()
> > > 4. .*_inatomic variants usually don't perform access checks
> > > 5. .*_inatomic variants don't call might_fault()
> > > 6. If common code uses the __.* variants, it has to trigger access_ok() and
> > > call might_fault()
> > > 7. For pagefault_disable(), the inatomic variants are to be used
> >
> > inatomic variants don't seem to handle faults, so you
> > must pin any memory you pass to them.
>
> And we don't have a single code part in the system that relies on this, right?
Hmm relies on what?
Do you want to change *inatomic to actually handle faults?
That's fine by me, but if you do, won't you need might_fault there.
And then your patch will be wrong, won't it?
> >
> >
> > > Comments? Opinions?
> > >
> >
> > If the same address is accessed multiple times, access_ok + __
> > variant can be used to speed access up a bit.
> > This is rarely the case, but this is the case for e.g. vhost.
> > But access_ok does not guarantee that no fault will trigger:
> > there's really no way to do that ATM except pinning the page.
> >
> >
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 15:23 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126151729.GB9612@redhat.com>
On Wed, Nov 26, 2014 at 05:17:29PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
> > > What's the path you are trying to debug?
> >
> > Well, we had a problem where we held a spin_lock and called
> > copy_(from|to)_user(). We experienced very random deadlocks that took some guy
> > almost a week to debug. The simple might_sleep() check would have showed this
> > error immediately.
>
> This must have been a very old kernel.
> A modern kernel will return an error from copy_to_user.
> Which is really the point of the patch you are trying to revert.
That's assuming you disabled preemption. If you didn't, and take
a spinlock, you have deadlocks even without userspace access.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Christian Borntraeger @ 2014-11-26 15:30 UTC (permalink / raw)
To: Michael S. Tsirkin, David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, paulus, schwidefsky,
akpm, linuxppc-dev, mingo
In-Reply-To: <20141126151729.GB9612@redhat.com>
Am 26.11.2014 um 16:17 schrieb Michael S. Tsirkin:
> On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
>>> What's the path you are trying to debug?
>>
>> Well, we had a problem where we held a spin_lock and called
>> copy_(from|to)_user(). We experienced very random deadlocks that took some guy
>> almost a week to debug. The simple might_sleep() check would have showed this
>> error immediately.
>
> This must have been a very old kernel.
> A modern kernel will return an error from copy_to_user.
I disagree. copy_to_user will not return while holding a spinlock, because it does not know! How should it?
See: spin_lock will call preempt_disable, but thats a no-op for a non-preempt kernel. So the mere fact that we hold a spin_lock is not known by any user access function. (or others). No?
Christian
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-26 15:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126152334.GA9648@redhat.com>
> On Wed, Nov 26, 2014 at 05:17:29PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
> > > > What's the path you are trying to debug?
> > >
> > > Well, we had a problem where we held a spin_lock and called
> > > copy_(from|to)_user(). We experienced very random deadlocks that took some guy
> > > almost a week to debug. The simple might_sleep() check would have showed this
> > > error immediately.
> >
> > This must have been a very old kernel.
> > A modern kernel will return an error from copy_to_user.
> > Which is really the point of the patch you are trying to revert.
>
> That's assuming you disabled preemption. If you didn't, and take
> a spinlock, you have deadlocks even without userspace access.
>
(Thanks for your resent, my first email was sent directly to you ... grml)
This is what happened on our side (very recent kernel):
spin_lock(&lock)
copy_to_user(...)
spin_unlock(&lock)
1. s390 locks/unlocks a spin lock with a compare and swap, using the _cpu id_
as "old value"
2. we slept during copy_to_user()
3. the thread got scheduled onto another cpu
4. spin_unlock failed as the _cpu id_ didn't match (another cpu that locked
the spinlock tried to unlocked it).
5. lock remained locked -> deadlock
Christian came up with the following explanation:
Without preemption, spin_lock() will not touch the preempt counter.
disable_pfault() will always touch it.
Therefore, with preemption disabled, copy_to_user() has no idea that it is
running in atomic context - and will therefore try to sleep.
So copy_to_user() will on s390:
1. run "as atomic" while spin_lock() with preemption enabled.
2. run "as not atomic" while spin_lock() with preemption disabled.
3. run "as atomic" while pagefault_disabled() with preemption enabled or
disabled.
4. run "as not atomic" when really not atomic.
And exactly nr 2. is the thing that produced the deadlock in our scenario and
the reason why I want a might_sleep() :)
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 15:37 UTC (permalink / raw)
To: Christian Borntraeger
Cc: linux-arch, heiko.carstens, linux-kernel, David Hildenbrand,
paulus, schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <5475F218.4050207@de.ibm.com>
On Wed, Nov 26, 2014 at 04:30:32PM +0100, Christian Borntraeger wrote:
> Am 26.11.2014 um 16:17 schrieb Michael S. Tsirkin:
> > On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
> >>> What's the path you are trying to debug?
> >>
> >> Well, we had a problem where we held a spin_lock and called
> >> copy_(from|to)_user(). We experienced very random deadlocks that took some guy
> >> almost a week to debug. The simple might_sleep() check would have showed this
> >> error immediately.
> >
>
> > This must have been a very old kernel.
> > A modern kernel will return an error from copy_to_user.
>
> I disagree. copy_to_user will not return while holding a spinlock, because it does not know! How should it?
> See: spin_lock will call preempt_disable, but thats a no-op for a non-preempt kernel. So the mere fact that we hold a spin_lock is not known by any user access function. (or others). No?
>
> Christian
>
>
Well might_sleep() merely checks preempt count and irqs_disabled too.
If you want debugging things to trigger, you need to enable
a bunch of config options. That's not new.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Michael S. Tsirkin @ 2014-11-26 15:47 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126163207.63810fcb@thinkpad-w530>
On Wed, Nov 26, 2014 at 04:32:07PM +0100, David Hildenbrand wrote:
> > On Wed, Nov 26, 2014 at 05:17:29PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
> > > > > What's the path you are trying to debug?
> > > >
> > > > Well, we had a problem where we held a spin_lock and called
> > > > copy_(from|to)_user(). We experienced very random deadlocks that took some guy
> > > > almost a week to debug. The simple might_sleep() check would have showed this
> > > > error immediately.
> > >
> > > This must have been a very old kernel.
> > > A modern kernel will return an error from copy_to_user.
> > > Which is really the point of the patch you are trying to revert.
> >
> > That's assuming you disabled preemption. If you didn't, and take
> > a spinlock, you have deadlocks even without userspace access.
> >
>
> (Thanks for your resent, my first email was sent directly to you ... grml)
>
> This is what happened on our side (very recent kernel):
>
> spin_lock(&lock)
> copy_to_user(...)
> spin_unlock(&lock)
That's a deadlock even without copy_to_user - it's
enough for the thread to be preempted and another one
to try taking the lock.
> 1. s390 locks/unlocks a spin lock with a compare and swap, using the _cpu id_
> as "old value"
> 2. we slept during copy_to_user()
> 3. the thread got scheduled onto another cpu
> 4. spin_unlock failed as the _cpu id_ didn't match (another cpu that locked
> the spinlock tried to unlocked it).
> 5. lock remained locked -> deadlock
>
> Christian came up with the following explanation:
> Without preemption, spin_lock() will not touch the preempt counter.
> disable_pfault() will always touch it.
>
> Therefore, with preemption disabled, copy_to_user() has no idea that it is
> running in atomic context - and will therefore try to sleep.
>
> So copy_to_user() will on s390:
> 1. run "as atomic" while spin_lock() with preemption enabled.
> 2. run "as not atomic" while spin_lock() with preemption disabled.
> 3. run "as atomic" while pagefault_disabled() with preemption enabled or
> disabled.
> 4. run "as not atomic" when really not atomic.
>
> And exactly nr 2. is the thing that produced the deadlock in our scenario and
> the reason why I want a might_sleep() :)
IMHO it's not copy to user that causes the problem.
It's the misuse of spinlocks with preemption on.
So might_sleep would make you think copy_to_user is
the problem, and e.g. let you paper over it by
moving copy_to_user out.
Enable lock prover and you will see what the real
issue is, which is you didn't disable preempt.
and if you did, copy_to_user would be okay.
--
MST
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: David Hildenbrand @ 2014-11-26 16:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, heiko.carstens, linux-kernel, borntraeger, paulus,
schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126154717.GB10568@redhat.com>
> > This is what happened on our side (very recent kernel):
> >
> > spin_lock(&lock)
> > copy_to_user(...)
> > spin_unlock(&lock)
>
> That's a deadlock even without copy_to_user - it's
> enough for the thread to be preempted and another one
> to try taking the lock.
>
>
> > 1. s390 locks/unlocks a spin lock with a compare and swap, using the _cpu id_
> > as "old value"
> > 2. we slept during copy_to_user()
> > 3. the thread got scheduled onto another cpu
> > 4. spin_unlock failed as the _cpu id_ didn't match (another cpu that locked
> > the spinlock tried to unlocked it).
> > 5. lock remained locked -> deadlock
> >
> > Christian came up with the following explanation:
> > Without preemption, spin_lock() will not touch the preempt counter.
> > disable_pfault() will always touch it.
> >
> > Therefore, with preemption disabled, copy_to_user() has no idea that it is
> > running in atomic context - and will therefore try to sleep.
> >
> > So copy_to_user() will on s390:
> > 1. run "as atomic" while spin_lock() with preemption enabled.
> > 2. run "as not atomic" while spin_lock() with preemption disabled.
> > 3. run "as atomic" while pagefault_disabled() with preemption enabled or
> > disabled.
> > 4. run "as not atomic" when really not atomic.
should have been more clear at that point:
preemption enabled == kernel compiled with preemption support
preemption disabled == kernel compiled without preemption support
> >
> > And exactly nr 2. is the thing that produced the deadlock in our scenario and
> > the reason why I want a might_sleep() :)
>
> IMHO it's not copy to user that causes the problem.
> It's the misuse of spinlocks with preemption on.
As I said, preemption was off.
>
> So might_sleep would make you think copy_to_user is
> the problem, and e.g. let you paper over it by
> moving copy_to_user out.
Actually implementing different way of locking easily fixed the problem for us.
The old might_sleep() checks would have given us the problem within a few
seconds (I tested it).
>
> Enable lock prover and you will see what the real
> issue is, which is you didn't disable preempt.
> and if you did, copy_to_user would be okay.
>
Our kernel is compiled without preemption and we turned on all lock/atomic
sleep debugging aid. No problem was detected.
----
But the question is if we shouldn't rather provide a:
copy_to_user_nosleep() implementation that can be called from
pagefault_disable() because it won't sleep.
and a
copy_to_user_sleep() implementation that cannot be called from
pagefault_disable().
Another way to fix it would be a reworked pagefault_disable() function that
somehow sets "a flag", so copy_to_user() knows that it is in fact called from a
valid context, not just from "some atomic" context. So we could trigger
might_sleep() when detecting a !pagefault_disable context.
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Christian Borntraeger @ 2014-11-26 16:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-arch, heiko.carstens, linux-kernel, David Hildenbrand,
paulus, schwidefsky, akpm, linuxppc-dev, mingo
In-Reply-To: <20141126153732.GA10568@redhat.com>
Am 26.11.2014 um 16:37 schrieb Michael S. Tsirkin:
> On Wed, Nov 26, 2014 at 04:30:32PM +0100, Christian Borntraeger wrote:
>> Am 26.11.2014 um 16:17 schrieb Michael S. Tsirkin:
>>> On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
>>>>> What's the path you are trying to debug?
>>>>
>>>> Well, we had a problem where we held a spin_lock and called
>>>> copy_(from|to)_user(). We experienced very random deadlocks that took some guy
>>>> almost a week to debug. The simple might_sleep() check would have showed this
>>>> error immediately.
>>>
>>
>>> This must have been a very old kernel.
>>> A modern kernel will return an error from copy_to_user.
>>
>> I disagree. copy_to_user will not return while holding a spinlock, because it does not know! How should it?
>> See: spin_lock will call preempt_disable, but thats a no-op for a non-preempt kernel. So the mere fact that we hold a spin_lock is not known by any user access function. (or others). No?
>>
>> Christian
>>
>>
>
> Well might_sleep() merely checks preempt count and irqs_disabled too.
> If you want debugging things to trigger, you need to enable
> a bunch of config options. That's not new.
You miss the point of the whole thread: The problem is that even with debug options enabled, holding a spinlock would not trigger a bug on copy_to_user. So the problem is not the good path, the problem is that a debugging aid for detecting a broken case was lost. Even with all kernel debugging enabled.
That is because CONFIG_DEBUG_ATOMIC_SLEEP selects PREEMPT_COUNT. That means: spin_lock will then be considered as in_atomic and no message comes. Without CONFIG_DEBUG_ATOMIC_SLEEP spin_lock will not touch the preempt_count but we also dont see a message because might_fault is now a nop
I understand that you dont like Davids changes due to other side effects that you have mentioned. So lets focus on how we can fix the debug option. Ok?
Christian
^ permalink raw reply
* Re: [RFC 0/2] Reenable might_sleep() checks for might_fault() when atomic
From: Christian Borntraeger @ 2014-11-26 16:07 UTC (permalink / raw)
To: Michael S. Tsirkin, David Hildenbrand
Cc: linux-arch, heiko.carstens, linux-kernel, paulus, schwidefsky,
akpm, linuxppc-dev, mingo
In-Reply-To: <20141126154717.GB10568@redhat.com>
Am 26.11.2014 um 16:47 schrieb Michael S. Tsirkin:
> On Wed, Nov 26, 2014 at 04:32:07PM +0100, David Hildenbrand wrote:
>>> On Wed, Nov 26, 2014 at 05:17:29PM +0200, Michael S. Tsirkin wrote:
>>>> On Wed, Nov 26, 2014 at 11:05:04AM +0100, David Hildenbrand wrote:
>>>>>> What's the path you are trying to debug?
>>>>>
>>>>> Well, we had a problem where we held a spin_lock and called
>>>>> copy_(from|to)_user(). We experienced very random deadlocks that took some guy
>>>>> almost a week to debug. The simple might_sleep() check would have showed this
>>>>> error immediately.
>>>>
>>>> This must have been a very old kernel.
>>>> A modern kernel will return an error from copy_to_user.
>>>> Which is really the point of the patch you are trying to revert.
>>>
>>> That's assuming you disabled preemption. If you didn't, and take
>>> a spinlock, you have deadlocks even without userspace access.
>>>
>>
>> (Thanks for your resent, my first email was sent directly to you ... grml)
>>
>> This is what happened on our side (very recent kernel):
>>
>> spin_lock(&lock)
>> copy_to_user(...)
>> spin_unlock(&lock)
>
> That's a deadlock even without copy_to_user - it's
> enough for the thread to be preempted and another one
> to try taking the lock.
Huh? With CONFIG_PREEMPT spin_lock will disable preemption. (we had preempt = server anyway).
But please: One step back. The problem is not the good path. The problem is that we lost a debugging aid for a known to be broken case. In other words: Our code had a bug. Older kernels detected that kind of bug. With your change we no longer saw the sleeping while atomic. Thats it. See my other mail.
Christian
^ 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