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

Will Deacon
2022-08-08T11:39:39Z

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
"/**".

Will