[PATCH] arm64/fpsimd: Avoid warning when sve_to_fpsimd() is unused

[PATCH] arm64/fpsimd: Avoid warning when sve_to_fpsimd() is unused

[Mark Rutland]

Historically fpsimd_to_sve() and sve_to_fpsimd() were (conditionally)
called by functions which were defined regardless of CONFIG_ARM64_SVE.
Hence it was necessary that both fpsimd_to_sve() and sve_to_fpsimd()
were always defined and not guarded by ifdeffery.

As a result of the removal of fpsimd_signal_preserve_current_state() in
commit:

  929fa99b1215966f ("arm64/fpsimd: signal: Always save+flush state early")

... sve_to_fpsimd() has no callers when CONFIG_ARM64_SVE=n, resulting in
a build-time warnign that it is unused:

| arch/arm64/kernel/fpsimd.c:676:13: warning: unused function 'sve_to_fpsimd' [-Wunused-function]
|   676 | static void sve_to_fpsimd(struct task_struct *task)
|       |             ^~~~~~~~~~~~~
| 1 warning generated.

In contrast, fpsimd_to_sve() still has callers which are defined when
CONFIG_ARM64_SVE=n, and it would be awkward to hide this behind
ifdeffery and/or to use stub functions.

For now, suppress the warning by marking both fpsimd_to_sve() and
sve_to_fpsimd() as 'static inline', as we usually do for stub functions.
The compiler will no longer warn if either function is unused.

Aside from suppressing the warning, there should be no functional change
as a result of this patch.

Link: https://lore.kernel.org/linux-arm-kernel/20250429194600.GA26883@willie-the-truck/
Reported-by: Will Deacon <will@kernel.org>
Fixes: 929fa99b1215966f ("arm64/fpsimd: signal: Always save+flush state early")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/fpsimd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Catalin, this is a fix for a patch currently queued up in the arm64
for-next/sme-fixes branch. Are you happy to apply it atop?

Mark.

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index b0874402f7ecc..422b9d43b1e64 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -649,7 +649,7 @@ static void __fpsimd_to_sve(void *sst, struct user_fpsimd_state const *fst,
  * task->thread.uw.fpsimd_state must be up to date before calling this
  * function.
  */
-static void fpsimd_to_sve(struct task_struct *task)
+static inline void fpsimd_to_sve(struct task_struct *task)
 {
 	unsigned int vq;
 	void *sst = task->thread.sve_state;
@@ -673,7 +673,7 @@ static void fpsimd_to_sve(struct task_struct *task)
  * bytes of allocated kernel memory.
  * task->thread.sve_state must be up to date before calling this function.
  */
-static void sve_to_fpsimd(struct task_struct *task)
+static inline void sve_to_fpsimd(struct task_struct *task)
 {
 	unsigned int vq, vl;
 	void const *sst = task->thread.sve_state;
-- 
2.30.2

Re: [PATCH] arm64/fpsimd: Avoid warning when sve_to_fpsimd() is unused

[Catalin Marinas]

On Wed, 30 Apr 2025 18:32:40 +0100, Mark Rutland wrote:
> Historically fpsimd_to_sve() and sve_to_fpsimd() were (conditionally)
> called by functions which were defined regardless of CONFIG_ARM64_SVE.
> Hence it was necessary that both fpsimd_to_sve() and sve_to_fpsimd()
> were always defined and not guarded by ifdeffery.
> 
> As a result of the removal of fpsimd_signal_preserve_current_state() in
> commit:
> 
> [...]

Applied to arm64 (for-next/sme-fixes), thanks!

[1/1] arm64/fpsimd: Avoid warning when sve_to_fpsimd() is unused
      https://git.kernel.org/arm64/c/f699c66691fb

I did not push it to for-kernelci as Will took over the for-next/*
branches (might as well have left this patch to Will).

In general, slight preference for __maybe_unused but I agree with you
that there's already precedent for static inline in this file.

-- 
Catalin

Re: [PATCH] arm64/fpsimd: Avoid warning when sve_to_fpsimd() is unused

[Arnd Bergmann]

On Wed, Apr 30, 2025, at 19:32, Mark Rutland wrote:
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index b0874402f7ecc..422b9d43b1e64 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -649,7 +649,7 @@ static void __fpsimd_to_sve(void *sst, struct 
> user_fpsimd_state const *fst,
>   * task->thread.uw.fpsimd_state must be up to date before calling this
>   * function.
>   */
> -static void fpsimd_to_sve(struct task_struct *task)
> +static inline void fpsimd_to_sve(struct task_struct *task)
>  {
>  	unsigned int vq;
>  	void *sst = task->thread.sve_state;

I just sent a different patch (adding an #ifdef) before I
saw this one is already applied.

Avoiding the #ifdef does make your version nicer, though this
may come back later, since I think this would still be a warning
at W=1 level, see this bit in linux/compiler.h:

/*
 * GCC does not warn about unused static inline functions for -Wunused-function.
 * Suppress the warning in clang as well by using __maybe_unused, but enable it
 * for W=1 build. This will allow clang to find unused functions. Remove the
 * __inline_maybe_unused entirely after fixing most of -Wunused-function warnings.
 */
#ifdef KBUILD_EXTRA_WARN1
#define __inline_maybe_unused
#else
#define __inline_maybe_unused __maybe_unused
#endif
#define inline inline __gnu_inline __inline_maybe_unused notrace

IIRC, gcc never warns for unused inline functions, but clang warns
about them when they are defined in a .c file rather than a header.

    Arnd

Re: [PATCH] arm64/fpsimd: Avoid warning when sve_to_fpsimd() is unused

[Mark Rutland]

On Sat, May 03, 2025 at 04:10:43PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 30, 2025, at 19:32, Mark Rutland wrote:
> >
> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> > index b0874402f7ecc..422b9d43b1e64 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
> > @@ -649,7 +649,7 @@ static void __fpsimd_to_sve(void *sst, struct 
> > user_fpsimd_state const *fst,
> >   * task->thread.uw.fpsimd_state must be up to date before calling this
> >   * function.
> >   */
> > -static void fpsimd_to_sve(struct task_struct *task)
> > +static inline void fpsimd_to_sve(struct task_struct *task)
> >  {
> >  	unsigned int vq;
> >  	void *sst = task->thread.sve_state;
> 
> I just sent a different patch (adding an #ifdef) before I
> saw this one is already applied.
> 
> Avoiding the #ifdef does make your version nicer, though this
> may come back later, since I think this would still be a warning
> at W=1 level, see this bit in linux/compiler.h:
> 
> /*
>  * GCC does not warn about unused static inline functions for -Wunused-function.
>  * Suppress the warning in clang as well by using __maybe_unused, but enable it
>  * for W=1 build. This will allow clang to find unused functions. Remove the
>  * __inline_maybe_unused entirely after fixing most of -Wunused-function warnings.
>  */
> #ifdef KBUILD_EXTRA_WARN1
> #define __inline_maybe_unused
> #else
> #define __inline_maybe_unused __maybe_unused
> #endif
> #define inline inline __gnu_inline __inline_maybe_unused notrace
> 
> IIRC, gcc never warns for unused inline functions, but clang warns
> about them when they are defined in a .c file rather than a header.

Fair, though with W=1 there's an existing issue with sme_free() that no-one has
complained about:

| [mark@lakrids:~/src/linux]% git clean -qfdx
| [mark@lakrids:~/src/linux]% usekorg-llvm 19.1.0 make ARCH=arm64 LLVM=1 -s defconfig             
| [mark@lakrids:~/src/linux]% ./scripts/config -d ARM64_SVE                          
| [mark@lakrids:~/src/linux]% usekorg-llvm 19.1.0 make ARCH=arm64 LLVM=1 -s W=1 arch/arm64/kernel/fpsimd.o
| arch/arm64/kernel/fpsimd.c:208:20: warning: unused function 'sme_free' [-Wunused-function]
|   208 | static inline void sme_free(struct task_struct *t) { }
|       |                    ^~~~~~~~
| arch/arm64/kernel/fpsimd.c:676:20: warning: unused function 'sve_to_fpsimd' [-Wunused-function]
|   676 | static inline void sve_to_fpsimd(struct task_struct *task)
|       |                    ^~~~~~~~~~~~~
| 2 warnings generated.

... so we'd want to do something consistent there (and probably for a
few related functions).

How important is fixing W=1? I note we have a *tonne* of warnings today;
so I assume we can punt that to a future cleanup?

Mark.

Re: [PATCH] arm64/fpsimd: Avoid warning when sve_to_fpsimd() is unused

[Arnd Bergmann]

On Sat, May 3, 2025, at 16:52, Mark Rutland wrote:
> On Sat, May 03, 2025 at 04:10:43PM +0200, Arnd Bergmann wrote:
>> On Wed, Apr 30, 2025, at 19:32, Mark Rutland wrote:
>
> Fair, though with W=1 there's an existing issue with sme_free() that no-one has
> complained about:
>
> | [mark@lakrids:~/src/linux]% git clean -qfdx
> | [mark@lakrids:~/src/linux]% usekorg-llvm 19.1.0 make ARCH=arm64 
> LLVM=1 -s defconfig             
> | [mark@lakrids:~/src/linux]% ./scripts/config -d ARM64_SVE             
>              
> | [mark@lakrids:~/src/linux]% usekorg-llvm 19.1.0 make ARCH=arm64 
> LLVM=1 -s W=1 arch/arm64/kernel/fpsimd.o
> | arch/arm64/kernel/fpsimd.c:208:20: warning: unused function 
> 'sme_free' [-Wunused-function]
> |   208 | static inline void sme_free(struct task_struct *t) { }
> |       |                    ^~~~~~~~
> | arch/arm64/kernel/fpsimd.c:676:20: warning: unused function 
> 'sve_to_fpsimd' [-Wunused-function]
> |   676 | static inline void sve_to_fpsimd(struct task_struct *task)
> |       |                    ^~~~~~~~~~~~~
> | 2 warnings generated.
>
> ... so we'd want to do something consistent there (and probably for a
> few related functions).
>
> How important is fixing W=1? I note we have a *tonne* of warnings today;
> so I assume we can punt that to a future cleanup?

There are usually new warnings coming in, while there are developers
chipping away at the existing ones, either doing one warning throughout
the tree, or fixing all warnings in a particular subsystem.

I've spent some time on fixing the -Wunused-const-variable warnings,
and others have done similar patches, so we can hopefully turn that
on by default soon. I haven't looked at the unused inline functions
in a while, so I don't know much there would be to do on this one. 

     Arnd