On Thu, 28 Jan 2016, Jeffrey Merkey wrote: > This patch series adds an export which can be set by system debuggers to > direct the hard lockup and soft lockup detector to trigger a breakpoint > exception and enter a debugger if one is active. It is assumed that if > someone sets this variable, then an breakpoint handler of some sort will > be actively loaded or registered via the notify die handler chain. > > This addition is extremely useful for debugging hard and soft lockups > real time and quickly from a console debugger. What's the intended use case for this hook and what do you call a console debugger? I'm asking because from the debugging perspective the Linux kernel is a bare metal application and for such the BREAK instruction is not the usual choice on the MIPS target. This instruction is normally used for userland debugging, it traps to the kernel and is handled there. Furthermore there are a few other applications of this instruction defined as a part of the MIPS ABI, also handled by the kernel, determined by the breakpoint code embedded with the instruction word, e.g. to trap integer division errors. See arch/mips/include/uapi/asm/break.h for the codes defined so far. All this means the trap may not be appropriate for debugging the kernel itself as you don't want to intercept it or the system won't run correctly. You'd have to hook into the kernel itself to intercept it too. But if you do have a working debug environment already set up around this arrangement, then it might be fine after all. In that case I think using a non-zero breakpoint code would make sense though, as 0 is often treated as a random (spurious) trap (it was used by IRIX though). Or do you detect it by the symbol name somehow? I've got a couple of further notes on the patch itself below. > diff --git a/arch/mips/include/asm/kdebug.h b/arch/mips/include/asm/kdebug.h > index 8e3d08e..af5999e 100644 > --- a/arch/mips/include/asm/kdebug.h > +++ b/arch/mips/include/asm/kdebug.h > @@ -16,4 +16,16 @@ enum die_val { > DIE_UPROBE_XOL, > }; > > + > +void arch_breakpoint(void) > +{ > + __asm__ __volatile__( > + ".globl breakinst\n\t" Please keep formatting consistent -- the rest of code below uses `\t' as the separator, so use it here as well. We don't have an established inline assembly formatting style, so please just keep your chosen one consistent. > + ".set\tnoreorder\n\t" > + "nop\n" > + "breakinst:\tbreak\n\t" > + "nop\n\t" > + ".set\treorder"); > +} > + > #endif /* _ASM_MIPS_KDEBUG_H */ Why do you need these NOPs around the breakpoint? You also need to mark `breakinst' as a function symbol (`.aent' might do) or otherwise you'll get garbled disassembly if this is built as microMIPS code. Maciej