Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1549084 > unrolled thread

[PATCH 1/3] m68k/mac: Improve NMI handler

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] m68k/mac: Improve NMI handler Finn Thain <fthain@telegraphics.com.au> - 2017-01-02 11:00 +0100
    Re: [PATCH 1/3] m68k/mac: Improve NMI handler Geert Uytterhoeven <geert@linux-m68k.org> - 2017-01-02 11:30 +0100
      Re: [PATCH 1/3] m68k/mac: Improve NMI handler Finn Thain <fthain@telegraphics.com.au> - 2017-01-03 05:40 +0100
        Re: [PATCH 1/3] m68k/mac: Improve NMI handler Geert Uytterhoeven <geert@linux-m68k.org> - 2017-01-03 09:20 +0100

#1549084 — [PATCH 1/3] m68k/mac: Improve NMI handler

mac_nmi_handler() is useless in its present form and locks up my PowerBook
180. Let's throw out the dead code and make it do something useful: print
a register dump and a stack trace.

mac_debug_handler() is also dead code. Remove it along with its static
data.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/macints.c | 67 ++++++-------------------------------------------
 1 file changed, 8 insertions(+), 59 deletions(-)

diff --git a/arch/m68k/mac/macints.c b/arch/m68k/mac/macints.c
index 9f98c08..f9672bb 100644
--- a/arch/m68k/mac/macints.c
+++ b/arch/m68k/mac/macints.c
@@ -127,12 +127,9 @@
 
 #define SHUTUP_SONIC
 
-/*
- * console_loglevel determines NMI handler function
- */
+extern void show_registers(struct pt_regs *);
 
 irqreturn_t mac_nmi_handler(int, void *);
-irqreturn_t mac_debug_handler(int, void *);
 
 /* #define DEBUG_MACINTS */
 
@@ -276,65 +273,17 @@ static void mac_irq_shutdown(struct irq_data *data)
 		mac_irq_disable(data);
 }
 
-static int num_debug[8];
-
-irqreturn_t mac_debug_handler(int irq, void *dev_id)
-{
-	if (num_debug[irq] < 10) {
-		printk("DEBUG: Unexpected IRQ %d\n", irq);
-		num_debug[irq]++;
-	}
-	return IRQ_HANDLED;
-}
-
-static int in_nmi;
-static volatile int nmi_hold;
+static volatile int in_nmi;
 
 irqreturn_t mac_nmi_handler(int irq, void *dev_id)
 {
-	int i;
-	/*
-	 * generate debug output on NMI switch if 'debug' kernel option given
-	 * (only works with Penguin!)
-	 */
-
-	in_nmi++;
-	for (i=0; i<100; i++)
-		udelay(1000);
-
-	if (in_nmi == 1) {
-		nmi_hold = 1;
-		printk("... pausing, press NMI to resume ...");
-	} else {
-		printk(" ok!\n");
-		nmi_hold = 0;
-	}
+	if (in_nmi)
+		return IRQ_HANDLED;
+	in_nmi = 1;
 
-	barrier();
+	pr_info("Non-Maskable Interrupt\n");
+	show_registers(get_irq_regs());
 
-	while (nmi_hold == 1)
-		udelay(1000);
-
-	if (console_loglevel >= 8) {
-#if 0
-		struct pt_regs *fp = get_irq_regs();
-		show_state();
-		printk("PC: %08lx\nSR: %04x  SP: %p\n", fp->pc, fp->sr, fp);
-		printk("d0: %08lx    d1: %08lx    d2: %08lx    d3: %08lx\n",
-		       fp->d0, fp->d1, fp->d2, fp->d3);
-		printk("d4: %08lx    d5: %08lx    a0: %08lx    a1: %08lx\n",
-		       fp->d4, fp->d5, fp->a0, fp->a1);
-
-		if (STACK_MAGIC != *(unsigned long *)current->kernel_stack_page)
-			printk("Corrupted stack page\n");
-		printk("Process %s (pid: %d, stackpage=%08lx)\n",
-			current->comm, current->pid, current->kernel_stack_page);
-		if (intr_count == 1)
-			dump_stack((struct frame *)fp);
-#else
-		/* printk("NMI "); */
-#endif
-	}
-	in_nmi--;
+	in_nmi = 0;
 	return IRQ_HANDLED;
 }
-- 
2.10.2

[toc] | [next] | [standalone]

#1549099

Hi Finn,

On Mon, Jan 2, 2017 at 10:53 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> mac_nmi_handler() is useless in its present form and locks up my PowerBook
> 180. Let's throw out the dead code and make it do something useful: print
> a register dump and a stack trace.
>
> mac_debug_handler() is also dead code. Remove it along with its static
> data.

Thanks for your patch!

> --- a/arch/m68k/mac/macints.c
> +++ b/arch/m68k/mac/macints.c
> @@ -127,12 +127,9 @@
>
>  #define SHUTUP_SONIC
>
> -/*
> - * console_loglevel determines NMI handler function
> - */
> +extern void show_registers(struct pt_regs *);

Seems like we do have a declaration in ... <linux/kprobes.h>.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1549529

On Mon, 2 Jan 2017, Geert Uytterhoeven wrote:

> >
> > -/*
> > - * console_loglevel determines NMI handler function
> > - */
> > +extern void show_registers(struct pt_regs *);
> 
> Seems like we do have a declaration in ... <linux/kprobes.h>.
> 

Yes, and it would have to be moved outside of the #ifdef CONFIG_KPROBES 
portion before it could be used by m68k, openrisc, cris or mn10300, which 
all lack HAVE_KPROBES.

I can't see why linux/kprobes.h is more appropriate for this declaration 
than, say linux/sched.h.

Despite the checkpatch warning, placing the extern at the call site is 
very popular.

$ egrep -r "extern.*show_registers" arch/
arch/mips/kernel/unaligned.c:extern void show_registers(struct pt_regs *regs);
arch/openrisc/kernel/process.c: extern void show_registers(struct pt_regs *regs);
arch/cris/kernel/traps.c:extern void show_registers(struct pt_regs *regs);
arch/cris/mm/fault.c:extern void show_registers(struct pt_regs *regs);
arch/cris/arch-v32/mach-a3/arbiter.c:extern void show_registers(struct pt_regs *regs);
arch/cris/arch-v32/kernel/time.c:extern void show_registers(struct pt_regs *regs);
arch/cris/arch-v32/mach-fs/arbiter.c:extern void show_registers(struct pt_regs *regs);
arch/mn10300/include/asm/gdb-stub.h:extern void show_registers_only(struct pt_regs *regs);
arch/mn10300/include/asm/processor.h:extern void show_registers(struct pt_regs *regs);
arch/frv/include/asm/gdb-stub.h:extern void show_registers_only(struct pt_regs *regs);
$ 
$ egrep -r "extern.*show_registers" include/
include/linux/kprobes.h:extern void show_registers(struct pt_regs *regs);
$ 

The situation with show_regs(struct pt_regs *) is a bit better in that the 
declaration is not found at multiple call sites (as with cris) but still 
appears in multiple header files.

$ egrep -r "extern.*show_regs" arch/
arch/x86/include/asm/kdebug.h:extern void __show_regs(struct pt_regs *regs, int all);
arch/nios2/include/asm/ptrace.h:extern void show_regs(struct pt_regs *);
arch/arm/include/asm/bug.h:extern void __show_regs(struct pt_regs *);
arch/tile/include/asm/stack.h:extern void tile_show_regs(struct pt_regs *);
arch/arm64/include/asm/system_misc.h:extern void __show_regs(struct pt_regs *);
arch/c6x/include/asm/ptrace.h:extern void show_regs(struct pt_regs *);
arch/avr32/include/asm/processor.h:extern void show_regs_log_lvl(struct pt_regs *regs, const char *log_lvl);
arch/unicore32/kernel/setup.h:extern void __show_regs(struct pt_regs *);
arch/alpha/kernel/proto.h:extern void dik_show_regs(struct pt_regs *regs, unsigned long *r9_15);
$ 
$ egrep -r "extern.*show_regs" include/
include/linux/sched.h:extern void show_regs(struct pt_regs *);
$ 

I guess we could put a show_registers() declaration in 
arch/m68k/include/asm/ptrace.h, as that's where struct pt_regs definition 
comes from (actually uapi/asm/ptrace.h). Both nios2 and c6x do this for 
show_regs().

Or we could just leave the patch as it is, because thus far m68k has no 
need for any declaration except a sole call site in macints.c.

-- 

[toc] | [prev] | [next] | [standalone]

#1549590

Hi Finn,

On Tue, Jan 3, 2017 at 5:31 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Mon, 2 Jan 2017, Geert Uytterhoeven wrote:
>> >
>> > -/*
>> > - * console_loglevel determines NMI handler function
>> > - */
>> > +extern void show_registers(struct pt_regs *);
>>
>> Seems like we do have a declaration in ... <linux/kprobes.h>.
>
> Yes, and it would have to be moved outside of the #ifdef CONFIG_KPROBES
> portion before it could be used by m68k, openrisc, cris or mn10300, which
> all lack HAVE_KPROBES.

Right, seems my manual cpp made a mistake here. It's indeed inside.

> Or we could just leave the patch as it is, because thus far m68k has no
> need for any declaration except a sole call site in macints.c.

Fine for me. Sorry for the noise.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web