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

Mark Rutland
2022-08-08T11:47:36Z

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.