Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Oct 2022 18:04:49 +0100
From:   Mark Rutland <mark.rutland@arm.com>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, catalin.marinas@arm.com,
        linux-arm-kernel@lists.infradead.org, mhiramat@kernel.org,
        revest@chromium.org, will@kernel.org
Subject: Re: [PATCH 1/4] ftrace: pass fregs to arch_ftrace_set_direct_caller()
Message-ID: <Y1bFsXt5njFNa01Q@FVFF77S0Q05N>
References: <20221024140846.3555435-1-mark.rutland@arm.com>
 <20221024140846.3555435-2-mark.rutland@arm.com>
 <20221024104845.3c898d85@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20221024104845.3c898d85@gandalf.local.home>
Precedence: bulk

On Mon, Oct 24, 2022 at 10:48:45AM -0400, Steven Rostedt wrote:
> On Mon, 24 Oct 2022 15:08:43 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -429,6 +429,7 @@ static inline int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsi
> >  }
> >  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> >  
> > +#ifdef CONFIG_FUNCTION_TRACER
> 
> Instead of adding the above preprocessor check, the below chunk should be
> moved into the CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS block above.

Sure; but note that doing that naively means 'struct ftrace_regs' won't always
be declared (e.g. if !CONFIG_FUNCTION_TRACER), and will result in warnings, e.g.

|   CC      arch/x86/kernel/asm-offsets.s
| In file included from ./include/linux/kvm_host.h:32,
|                  from arch/x86/kernel/../kvm/vmx/vmx.h:5,
|                  from arch/x86/kernel/asm-offsets.c:22:
| ./include/linux/ftrace.h:444:57: error: ‘struct ftrace_regs’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
|   444 | static inline void arch_ftrace_set_direct_caller(struct ftrace_regs *fregs,
|       |                                                         ^~~~~~~~~~~
| cc1: all warnings being treated as errors
| make[1]: *** [scripts/Makefile.build:118: arch/x86/kernel/asm-offsets.s] Error 1
| make: *** [Makefile:1270: prepare0] Error 2

... so I'll either need to add some ifdeffery, for CONFIG_FUNCTION_TRACER, or I
can hoist the declaration of 'struct ftrace_regs' to not depend on
CONFIG_FUNCTION_TRACER.

I guess the latter is preferable, e.g.

| diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
| index 2b34fec40a39..f201fcbfffb0 100644
| --- a/include/linux/ftrace.h
| +++ b/include/linux/ftrace.h
| @@ -37,9 +37,10 @@ extern void ftrace_boot_snapshot(void);
|  static inline void ftrace_boot_snapshot(void) { }
|  #endif
|  
| -#ifdef CONFIG_FUNCTION_TRACER
|  struct ftrace_ops;
|  struct ftrace_regs;
| +
| +#ifdef CONFIG_FUNCTION_TRACER
|  /*
|   * If the arch's mcount caller does not support all of ftrace's
|   * features, then it must call an indirect function that

... so I've done that locally for now.

Thanks,
Mark.