Re: [PATCH v2 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record()

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Aug 08, 2022 at 12:38:10PM +0100, Will Deacon wrote:
> On Fri, Aug 05, 2022 at 01:45:16PM +0100, Mark Rutland wrote:
> > The unwind_next_common() function unwinds a single frame record. There
> > are other unwind steps (e.g. unwinding through trampolines) which are
> > handled in the regular kernel unwinder, and in future there may be other
> > common unwind helpers.
> > 
> > Clarify the purpose of unwind_next_common() by renaming it to
> > unwind_next_frame_record(). At the same time, add commentary, and delete
> > the redundant comment at the top of asm/stacktrace/common.h.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Reviewed-by: Kalesh Singh <kaleshsingh@google.com>
> > Reviewed-by: Mark Brown <broonie@kernel.org>
> > Cc: Fuad Tabba <tabba@google.com>
> > Cc: Kalesh Singh <kaleshsingh@google.com>
> > Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/stacktrace/common.h | 22 ++++++++++++----------
> >  arch/arm64/kernel/stacktrace.c             |  2 +-
> >  arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  2 +-
> >  arch/arm64/kvm/stacktrace.c                |  4 ++--
> >  4 files changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> > index 01dc9f44a24a7..676002d7d333c 100644
> > --- a/arch/arm64/include/asm/stacktrace/common.h
> > +++ b/arch/arm64/include/asm/stacktrace/common.h
> > @@ -2,13 +2,6 @@
> >  /*
> >   * Common arm64 stack unwinder code.
> >   *
> > - * To implement a new arm64 stack unwinder:
> > - *     1) Include this header
> > - *
> > - *     2) Call into unwind_next_common() from your top level unwind
> > - *        function, passing it the validation and translation callbacks
> > - *        (though the later can be NULL if no translation is required).
> > - *
> >   * See: arch/arm64/kernel/stacktrace.c for the reference implementation.
> >   *
> >   * Copyright (C) 2012 ARM Ltd.
> > @@ -139,9 +132,18 @@ typedef bool (*on_accessible_stack_fn)(const struct task_struct *tsk,
> >  				       unsigned long sp, unsigned long size,
> >  				       struct stack_info *info);
> >  
> > -static inline int unwind_next_common(struct unwind_state *state,
> > -				     on_accessible_stack_fn accessible,
> > -				     stack_trace_translate_fp_fn translate_fp)
> > +/*
> > + * unwind_next_frame_record() - Unwind to the next frame record indicated by
> > + * @state->fp.
> > + *
> > + * @state:        the current unwind state.
> > + * @accessible:   determines whether the frame record is accessible
> > + * @translate_fp: translates the fp prior to access (may be NULL)
> > + */
> 
> This looks like kerneldoc, so you I think you should make the opening "/*" a
> "/**".

I agree; that's also true for all the other comments in this file.

Would you mind if I did that as a follow-up fixing all of them?

Otherwise, I can spin a v3 with a preparatory patch for the existing instances.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel