Re: [PATCH 01/31] Add hard/soft lockup debugger entry points

From: Chris Metcalf
Date: Thu Jan 28 2016 - 17:14:52 EST

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

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