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;
MIME-Version: 1.0
References: <20230829-missingvardecl-efi-v1-1-13d055a55176@google.com>
In-Reply-To: <20230829-missingvardecl-efi-v1-1-13d055a55176@google.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 30 Aug 2023 01:03:46 +0200
Message-ID: <CAMj1kXE_xs+wWno4G4U+6CFCOwO7JWcmNv4et5=W=ZRrQatWnQ@mail.gmail.com>
Subject: Re: [PATCH] efi: fix -Wmissing-variable-declarations warning
To:     Justin Stitt <justinstitt@google.com>
Cc:     Darren Hart <dvhart@infradead.org>,
        Andy Shevchenko <andy@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-efi@vger.kernel.org,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Justin,

On Wed, 30 Aug 2023 at 00:54, Justin Stitt <justinstitt@google.com> wrote:
>
> When building x86/defconfig with Clang-18 I encounter the following warnings:
> | arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
> |   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> | arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
> |   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> | arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
> |   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>
> These variables are not externally declared anywhere (AFAIK)

They are:

drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
efi_attr_fw_vendor;
drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute efi_attr_runtime;
drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
efi_attr_config_table;

> so let's
> add the static keyword and ensure we follow the ODR.
>

One Definition Rule, right? Better to spell that out.

> Link: https://github.com/ClangBuiltLinux/linux/issues/1920
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> When building x86/defconfig with Clang-18 I encounter the following warnings:
> | arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
> |   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> | arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
> |   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> | arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
> |   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);

This is duplicated. Is this a b4 fail?

> ---
> Note: build-tested.
> ---
>  arch/x86/platform/efi/efi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index e9f99c56f3ce..30c354c52ad4 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -931,9 +931,9 @@ EFI_ATTR_SHOW(fw_vendor);
>  EFI_ATTR_SHOW(runtime);
>  EFI_ATTR_SHOW(config_table);
>
> -struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> -struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> -struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> +static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> +static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> +static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>
>  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
>  {
>

This won't work.

Those variables are referenced via weak references in generic code.
The idea is that the weak references resolve to NULL pointers on
architectures other than x86, terminating the array early and hiding
the non-existent variables.

Making them static in arch/x86/platform/efi/efi.c means that these
references will remain unsatisfied, and so the variables will no
longer be exposed on x86 either.