Re: [PATCH RFC v4 1/6] arm64: support single-step and breakpointhandler hooks

[Will Deacon]

On Tue, Dec 03, 2013 at 02:33:17PM +0000, Sandeepa Prabhu wrote:
> Hi Will,
> 
> Sorry for responding to this after long-time, I missed this review
> during Linaro connect travels.

No problem.

> >> @@ -215,7 +257,10 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
> >>                */
> >>               user_rewind_single_step(current);
> >>       } else {
> >> -             /* TODO: route to KGDB */
> >> +             /* call registered single step handlers */
> >
> > Don't bother with this comment (it's crystal clear from the code).
> OK, I will remove this unnecessary print.

Thanks.

> >> +static LIST_HEAD(break_hook);
> >> +DEFINE_RWLOCK(break_hook_lock);
> >
> > This guy can be a plain old spinlock. That way, the readers have less
> > overhead but things still work because we only call a single hook function.
> well, kprobes need to support recursive breakpoints (i.e. breakpoint
> handler executing BRK once again)
> so I converted this lock to rw_lock.  I should put this info in commit
> description to be more clearer.

Actually, this is one place where a comment in the code *would* be useful!

> Let me know if you find any issue with re-cursing in breakpoint exception?

Sounds ok to me. With those changes:

  Acked-by: Will Deacon <will.deacon@xxxxxxx>

Cheers,

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/