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

[Chris Metcalf <cmetcalf@ezchip.com>]

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