On 1/28/16, Chris Metcalf <cmetc...@ezchip.com> wrote: > On 01/28/2016 02:46 PM, 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. > > I'm concerned that you are duplicating the breakpoint instructions > for all the platforms. Could you make kgdb.h include kdebug.h and > just move the arch_kgdb_breakpoint() implementations in kgdb.h to > arch_breakpoint() in kdebug.h? Then each platform can just put an > appropriate define in kgdb.h, e.g. "#define arch_kgdb_breakpoint > arch_breakpoint", unless (like mips) they have a more complicated > requirement. > > I'm concerned that in some cases (e.g. arm64) there is a perfectly good > breakpoint defined in kgdb.h but you are providing a no-op in kdebug.h. > You should probably do another check across all the architectures for > this case. > > You should probably add your no-op implementation of arch_breakpoint() > to asm-generic/kdebug.h, and then add "generic-y" lines to the Kbuild > files for the architectures that you are creating new empty files for. > I'm a little ambivalent about the "silent no-op" implementation, but I'm > not really sure there's a better option. > > For mips, I'm pretty sure you don't want to create a global "breakinst" > symbol every time you insert a breakpoint into code. I think this is an > example of where you need to have a different implementation of > arch_breakpoint() and arch_kgdb_breakpoint(), since mips makes its > breakpoint magical by knowing what the address used for that specific > instruction is, and you can't do that for arch_breakpoint(). > > As a general rule, you probably want to provide header guards in new > headers that you create, but if you just use asm-generic instead, it > actually won't matter for this case. > > I should add that I didn't do a thorough review of the patch series, > just a quick skim of a few of the architectures. > > -- > Chris Metcalf, EZChip Semiconductor > http://www.ezchip.com > >
todo: * Adding header guards (if file does not exist) * reimplemeting arch_breakpoint * move arch_breakpoint to asm-generic/kdebug.h * implement as a macro if possible * study MIPS breakpoint instruction. Ask for help is not sure. * fix syntax for INT3 instruction for x86 to use proper gas semantics * fix kbuild test robot error for SPARC (move to asm-generic fixes) List so far. Jeff
