[PATCH 1/2] alpha: run the remote RTC access in a worker, not an IPI callback

[PATCH 1/2] alpha: run the remote RTC access in a worker, not an IPI callback

[Matt Turner]

On Marvel the CMOS clock is only reachable from the boot cpu, so
remote_read_time() and remote_set_time() bounce the access there with
smp_call_function_single(), whose callback runs in hard interrupt
context.

alpha_rtc_read_time() calls mc146818_get_time() with a 10 ms timeout.
That waits out the RTC update cycle in mc146818_avoid_UIP(), which drops
rtc_lock and udelay()s 100 us at a time until the update completes or
the timeout expires:

	for (i = 0; UIP_RECHECK_LOOPS_MS(i) < timeout; i++) {
		spin_lock_irqsave(&rtc_lock, flags);
		...
		if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
			spin_unlock_irqrestore(&rtc_lock, flags);
			udelay(UIP_RECHECK_DELAY);
			continue;
		}

So a clock read from a non-boot cpu can spin for up to 10 ms in hard
interrupt context on the boot cpu, while the cpu that sent the request
spins in smp_call_function_single() waiting for it to finish.

mc146818_set_time() does not poll, but it takes rtc_lock too, and
rtc_lock is a spinlock_t.  Only raw spinlocks may be taken in hard
interrupt context, so lockdep reports the write path as soon as a
non-boot cpu sets the clock:

  [ BUG: Invalid wait context ]
  -----------------------------
  swapper/0/0 is trying to lock:
  fffffc0003690470 (rtc_lock){....}-{3:3}, at: mc146818_set_time+0x74/0x450
  other info that might help us debug this:
  context-{2:2}
  no locks held by swapper/0/0.
  stack backtrace:
  CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 7.2.0-rc1 #1 NONE
  Trace:
  [<fffffc000102ebb0>] dump_stack+0x28/0x44
  [<fffffc000110efcc>] __lock_acquire+0xb0c/0x1060
  [<fffffc000110f5f0>] lock_acquire.part.0+0xd0/0x300
  [...]
  [<fffffc0001b0d834>] mc146818_set_time+0x74/0x450
  [<fffffc0001f090cc>] _raw_spin_lock_irqsave+0x7c/0xc0
  [<fffffc0001042f90>] do_remote_set+0x90/0xc0
  [<fffffc000119c1a4>] __flush_smp_call_function_queue+0x314/0x5c0
  [<fffffc000119c474>] generic_smp_call_function_single_interrupt+0x24/0x40
  [<fffffc000103d984>] handle_ipi+0xa4/0x230
  [<fffffc0001037044>] do_entInt+0x1a4/0x2e0

The rtc class ops are always called from process context, so there is no
reason to run the access from an interrupt at all.  Use work_on_cpu() to
run it in a worker on the boot cpu.  Alpha does not support cpu hotplug,
so the boot cpu cannot go offline while the work is pending.

Tested on an AlphaServer ES47 (Marvel/EV7): hwclock read and write
pinned to a non-boot cpu, twenty times, with no splat.

Signed-off-by: Matt Turner <mattst88@gmail.com>
---
 arch/alpha/kernel/rtc.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/arch/alpha/kernel/rtc.c b/arch/alpha/kernel/rtc.c
index cfdf90bc8b3f..9e7d714ef6f8 100644
--- a/arch/alpha/kernel/rtc.c
+++ b/arch/alpha/kernel/rtc.c
@@ -15,6 +15,7 @@
 #include <linux/bcd.h>
 #include <linux/rtc.h>
 #include <linux/platform_device.h>
+#include <linux/workqueue.h>
 
 #include "proto.h"
 
@@ -142,54 +143,40 @@ static const struct rtc_class_ops alpha_rtc_ops = {
 };
 
 /*
- * Similarly, except do the actual CMOS access on the boot cpu only.
- * This requires marshalling the data across an interprocessor call.
+ * Similarly, except do the actual CMOS access on the boot cpu only.  The
+ * access polls for the RTC update cycle and takes rtc_lock, so run it in a
+ * worker on that cpu rather than from an interprocessor interrupt.
  */
 
 #if defined(CONFIG_SMP) && \
     (defined(CONFIG_ALPHA_GENERIC) || defined(CONFIG_ALPHA_MARVEL))
 # define HAVE_REMOTE_RTC 1
 
-union remote_data {
-	struct rtc_time *tm;
-	long retval;
-};
-
-static void
+static long
 do_remote_read(void *data)
 {
-	union remote_data *x = data;
-	x->retval = alpha_rtc_read_time(NULL, x->tm);
+	return alpha_rtc_read_time(NULL, data);
 }
 
 static int
 remote_read_time(struct device *dev, struct rtc_time *tm)
 {
-	union remote_data x;
-	if (smp_processor_id() != boot_cpuid) {
-		x.tm = tm;
-		smp_call_function_single(boot_cpuid, do_remote_read, &x, 1);
-		return x.retval;
-	}
+	if (smp_processor_id() != boot_cpuid)
+		return work_on_cpu(boot_cpuid, do_remote_read, tm);
 	return alpha_rtc_read_time(NULL, tm);
 }
 
-static void
+static long
 do_remote_set(void *data)
 {
-	union remote_data *x = data;
-	x->retval = alpha_rtc_set_time(NULL, x->tm);
+	return alpha_rtc_set_time(NULL, data);
 }
 
 static int
 remote_set_time(struct device *dev, struct rtc_time *tm)
 {
-	union remote_data x;
-	if (smp_processor_id() != boot_cpuid) {
-		x.tm = tm;
-		smp_call_function_single(boot_cpuid, do_remote_set, &x, 1);
-		return x.retval;
-	}
+	if (smp_processor_id() != boot_cpuid)
+		return work_on_cpu(boot_cpuid, do_remote_set, tm);
 	return alpha_rtc_set_time(NULL, tm);
 }
 
-- 
2.54.0

[PATCH 2/2] alpha: annotate hardirqs-off on IPL 7 interrupt entry

[Matt Turner]

do_entInt() opens with local_irq_disable(), which with
CONFIG_TRACE_IRQFLAGS only calls trace_hardirqs_off() if interrupts were
not already off:

	#define local_irq_disable()				\
		do {						\
			bool was_disabled = raw_irqs_disabled();\
			raw_local_irq_disable();		\
			if (!was_disabled)			\
				trace_hardirqs_off();		\
		} while (0)

On alpha raw_irqs_disabled() is (rdps() & 7) == IPL_MAX, i.e. IPL 7.
PALcode raises PS.IPL to the level of the interrupt before entInt runs,
so for an IPL 7 entry - a processor machine check (vector 0x660) or a
system event (vector 0x680), both IPL_MCHECK == IPL_MAX - the gate is
already true and the annotation is skipped.  lockdep keeps whatever
hardirq state the interrupted context had.  If that context had
interrupts enabled, lockdep believes they are still enabled for the
duration of the handler, and every lockdep_assert_irqs_disabled() in the
interrupt path fires:

  WARNING: kernel/context_tracking.c:346 at ct_irq_enter+0xc4/0xd0, CPU#0: swapper/0/0
  [...]
  [<fffffc0001ef74d4>] ct_irq_enter+0xc4/0xd0
  [<fffffc000105ebd0>] irq_enter+0x20/0x50
  [<fffffc000103707c>] do_entInt+0x1dc/0x2e0
  [<fffffc0001031d60>] ret_from_exception+0x0/0x10

  irq event stamp: 735356346
  hardirqs last  enabled at (735356346): trace_hardirqs_on+0x68/0x220
  hardirqs last disabled at (735356345): do_idle+0xf0/0x270

The stamps show the problem directly: the most recent event is the
enable from the interrupted idle loop, and do_entInt() recorded no
disable at all.  ct_irq_exit() warns the same way on the way out.

Ordinary device interrupts arrive at IPL 3-5 and IPIs and performance
counter interrupts at IPL 6, so was_disabled is false for them and the
annotation happens normally.  Only the two IPL 7 vectors are affected,
which is why this needs an environmental event to show up.

Take the hardware IPL out of the decision and drive the annotation from
lockdep's own state instead.

This corrects the annotation only.  An IPL 7 event can also interrupt a
region that has legitimately disabled interrupts, where irq_enter() and
irq_exit() are not the right primitives and NMI semantics are needed;
that is a larger change and is left alone here.

Tested on an AlphaServer ES47 (Marvel/EV7) by injecting system events
through the system management path: fifteen injections, idle and under
load, with no splat.  The same injection on a freshly booted kernel
without this change reproduces both warnings.

Signed-off-by: Matt Turner <mattst88@gmail.com>
---
 arch/alpha/kernel/irq_alpha.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/alpha/kernel/irq_alpha.c b/arch/alpha/kernel/irq_alpha.c
index cb4d58079d83..014c1e98e922 100644
--- a/arch/alpha/kernel/irq_alpha.c
+++ b/arch/alpha/kernel/irq_alpha.c
@@ -52,8 +52,19 @@ do_entInt(unsigned long type, unsigned long vector,
 	 * Note that there is no matching local_irq_enable() due to
 	 * severe problems with RTI at IPL0 and some MILO PALcode
 	 * (namely LX164).
+	 *
+	 * PALcode has already raised PS.IPL to the level of the interrupt
+	 * being delivered.  For an IPL 7 entry - a machine check or a system
+	 * event - that is IPL_MAX, which is what arch_irqs_disabled() tests
+	 * for, so local_irq_disable() would decide interrupts were already
+	 * off and skip trace_hardirqs_off().  lockdep would then spend the
+	 * whole handler believing interrupts are enabled.  Drive the
+	 * annotation from lockdep's own state rather than the hardware IPL.
 	 */
-	local_irq_disable();
+	raw_local_irq_disable();
+	if (lockdep_hardirqs_enabled())
+		trace_hardirqs_off();
+
 	old_regs = set_irq_regs(regs);
 
 	switch (type) {
-- 
2.54.0