> --- /dev/null > +++ b/arch/riscv/kernel/perf_callchain.c > @@ -0,0 +1,122 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2019 Hangzhou C-SKY Microsystems co.,ltd.
Please use normal /* */ Style comment for everything but the SPDX tags. > +static int unwind_frame_kernel(struct stackframe *frame) > +{ > + int graph = 0; > + > + /* 0x3 means misalignment */ > + if (!kstack_end((void *)frame->fp) && > + !((unsigned long)frame->fp & 0x3) && > + ((unsigned long)frame->fp >= TASK_SIZE)) { > + frame->ra = ((struct stackframe *)frame->fp - 1)->ra; > + frame->fp = ((struct stackframe *)frame->fp - 1)->fp; > + /* make sure CONFIG_FUNCTION_GRAPH_TRACER is turned on */ > + if (__kernel_text_address(frame->ra)) > + frame->ra = ftrace_graph_ret_addr(NULL, &graph, > + frame->ra, NULL); > + return 0; > + } else { > + return -EPERM; > + } Please do early exists for all the error conditions. Also no casts are needed for using ->fp as a scalar value, and we should probably just do a struct copy instead of copying both values individually. The function should look something like: static int unwind_frame_kernel(struct stackframe *frame) { if (!kstack_end((void *)frame->fp)) return -EPERM; if ((frame->fp & 0x3 || frame->fp >= TASK_SIZE) return -EPERM; *frame = *((struct stackframe *)frame->fp - 1); if (__kernel_text_address(frame->ra)) { int graph = 0; frame->ra = ftrace_graph_ret_addr(NULL, &graph, frame->ra, NULL); } return 0; } > +static void notrace walk_stackframe(struct stackframe *fr, > + struct perf_callchain_entry_ctx *entry) > +{ > + while (1) { > + int ret; > + > + perf_callchain_store(entry, fr->ra); > + > + ret = unwind_frame_kernel(fr); > + if (ret < 0) > + break; > + } > +} Why not: do { perf_callchain_store(entry, fr->ra); } while (unwind_frame_kernel(fr) >= 0); > +/* > + * Get the return address for a single stackframe and return a pointer to the > + * next frame tail. > + */ > +static unsigned long user_backtrace(struct perf_callchain_entry_ctx *entry, > + unsigned long fp, unsigned long reg_ra) > +{ > + struct stackframe buftail; > + unsigned long ra = 0; > + unsigned long *user_frame_tail = (unsigned long *)(fp - sizeof(struct > stackframe)); Overly long line. > + fp = user_backtrace(entry, fp, regs->ra); > + while ((entry->nr < entry->max_stack) && > + fp && !((unsigned long)fp & 0x3)) > + fp = user_backtrace(entry, fp, 0); Please don't indent the condition continuation and the loop body by the same amount.