diff mbox series

[1/5] crypto: arm/aes-ce - enable module autoloading based on CPU feature bits

Message ID 1495362220-30044-2-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show
Series crypto: arm - enable module autoloading | expand

Commit Message

Ard Biesheuvel May 21, 2017, 10:23 a.m. UTC 
Make the module autoloadable by tying it to the CPU feature bit that
describes whether the optional instructions it relies on are implemented
by the current CPU.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/arm/crypto/aes-ce-glue.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
2.7.4

Comments

Stefan Agner Sept. 10, 2018, 6:21 a.m. UTC | #1 
Hi Ard,

On 21.05.2017 03:23, Ard Biesheuvel wrote:
> Make the module autoloadable by tying it to the CPU feature bit that

> describes whether the optional instructions it relies on are implemented

> by the current CPU.

> 


This leads to a compiler warning when compiling multi_v7_defconfig/ARM32
using Clang 6.0.1:

arch/arm/crypto/aes-ce-glue.c:450:1: warning: variable
'cpu_feature_match_AES' is not needed and will not
      be emitted [-Wunneeded-internal-declaration]
module_cpu_feature_match(AES, aes_init);

./include/linux/cpufeature.h:48:33: note: expanded from macro
'module_cpu_feature_match'
static struct cpu_feature const cpu_feature_match_ ## x[] =     \

<scratch space>:83:1: note: expanded from here
cpu_feature_match_AES
^
1 warning generated.

Do you happen to have an idea how to alleviate?

--
Stefan


> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  arch/arm/crypto/aes-ce-glue.c | 6 ++----

>  1 file changed, 2 insertions(+), 4 deletions(-)

> 

> diff --git a/arch/arm/crypto/aes-ce-glue.c b/arch/arm/crypto/aes-ce-glue.c

> index 883b84d828c5..0f966a8ca1ce 100644

> --- a/arch/arm/crypto/aes-ce-glue.c

> +++ b/arch/arm/crypto/aes-ce-glue.c

> @@ -14,6 +14,7 @@

>  #include <crypto/aes.h>

>  #include <crypto/internal/simd.h>

>  #include <crypto/internal/skcipher.h>

> +#include <linux/cpufeature.h>

>  #include <linux/module.h>

>  #include <crypto/xts.h>

>  

> @@ -425,9 +426,6 @@ static int __init aes_init(void)

>  	int err;

>  	int i;

>  

> -	if (!(elf_hwcap2 & HWCAP2_AES))

> -		return -ENODEV;

> -

>  	err = crypto_register_skciphers(aes_algs, ARRAY_SIZE(aes_algs));

>  	if (err)

>  		return err;

> @@ -451,5 +449,5 @@ static int __init aes_init(void)

>  	return err;

>  }

>  

> -module_init(aes_init);

> +module_cpu_feature_match(AES, aes_init);

>  module_exit(aes_exit);
Ard Biesheuvel Sept. 10, 2018, 7:01 a.m. UTC | #2 
On 10 September 2018 at 08:21, Stefan Agner <stefan@agner.ch> wrote:
> Hi Ard,

>

> On 21.05.2017 03:23, Ard Biesheuvel wrote:

>> Make the module autoloadable by tying it to the CPU feature bit that

>> describes whether the optional instructions it relies on are implemented

>> by the current CPU.

>>

>

> This leads to a compiler warning when compiling multi_v7_defconfig/ARM32

> using Clang 6.0.1:

>

> arch/arm/crypto/aes-ce-glue.c:450:1: warning: variable

> 'cpu_feature_match_AES' is not needed and will not

>       be emitted [-Wunneeded-internal-declaration]

> module_cpu_feature_match(AES, aes_init);

>

> ./include/linux/cpufeature.h:48:33: note: expanded from macro

> 'module_cpu_feature_match'

> static struct cpu_feature const cpu_feature_match_ ## x[] =     \

>

> <scratch space>:83:1: note: expanded from here

> cpu_feature_match_AES

> ^

> 1 warning generated.

>

> Do you happen to have an idea how to alleviate?

>


I guess this only happens for modules that are selected as builtin,
and so MODULE_DEVICE_TABLE() resolves to nothing?
Does this only occur for CPU features?
Stefan Agner Sept. 13, 2018, 6:24 a.m. UTC | #3 
On 10.09.2018 00:01, Ard Biesheuvel wrote:
> On 10 September 2018 at 08:21, Stefan Agner <stefan@agner.ch> wrote:

>> Hi Ard,

>>

>> On 21.05.2017 03:23, Ard Biesheuvel wrote:

>>> Make the module autoloadable by tying it to the CPU feature bit that

>>> describes whether the optional instructions it relies on are implemented

>>> by the current CPU.

>>>

>>

>> This leads to a compiler warning when compiling multi_v7_defconfig/ARM32

>> using Clang 6.0.1:

>>

>> arch/arm/crypto/aes-ce-glue.c:450:1: warning: variable

>> 'cpu_feature_match_AES' is not needed and will not

>>       be emitted [-Wunneeded-internal-declaration]

>> module_cpu_feature_match(AES, aes_init);

>>

>> ./include/linux/cpufeature.h:48:33: note: expanded from macro

>> 'module_cpu_feature_match'

>> static struct cpu_feature const cpu_feature_match_ ## x[] =     \

>>

>> <scratch space>:83:1: note: expanded from here

>> cpu_feature_match_AES

>> ^

>> 1 warning generated.

>>

>> Do you happen to have an idea how to alleviate?

>>

> 

> I guess this only happens for modules that are selected as builtin,

> and so MODULE_DEVICE_TABLE() resolves to nothing?

> Does this only occur for CPU features?


So in the above case CONFIG_ARM_CRYPTO=y, CONFIG_CRYPTO_AES_ARM_CE=m...

Right now I only saw it with CPU features... I remember seen similar issues, which got resolved. Digging in the git history I found 1f318a8bafcf ("modules: mark __inittest/__exittest as __maybe_unused"), 

This seems to resolve it:

--- a/include/linux/cpufeature.h
+++ b/include/linux/cpufeature.h
@@ -45,7 +45,7 @@
  * 'asm/cpufeature.h' of your favorite architecture.
  */
 #define module_cpu_feature_match(x, __initfunc)                        \
-static struct cpu_feature const cpu_feature_match_ ## x[] =    \
+static struct cpu_feature const __maybe_unused cpu_feature_match_ ## x[] = \
        { { .feature = cpu_feature(x) }, { } };                 \
 MODULE_DEVICE_TABLE(cpu, cpu_feature_match_ ## x);             \
                                                                \

Also arch/arm/crypto/crc32-ce-glue.c needs an extra __maybe_unused.

--
Stefan
Ard Biesheuvel Sept. 13, 2018, 7:52 a.m. UTC | #4 
On 13 September 2018 at 08:24, Stefan Agner <stefan@agner.ch> wrote:
> On 10.09.2018 00:01, Ard Biesheuvel wrote:

>> On 10 September 2018 at 08:21, Stefan Agner <stefan@agner.ch> wrote:

>>> Hi Ard,

>>>

>>> On 21.05.2017 03:23, Ard Biesheuvel wrote:

>>>> Make the module autoloadable by tying it to the CPU feature bit that

>>>> describes whether the optional instructions it relies on are implemented

>>>> by the current CPU.

>>>>

>>>

>>> This leads to a compiler warning when compiling multi_v7_defconfig/ARM32

>>> using Clang 6.0.1:

>>>

>>> arch/arm/crypto/aes-ce-glue.c:450:1: warning: variable

>>> 'cpu_feature_match_AES' is not needed and will not

>>>       be emitted [-Wunneeded-internal-declaration]

>>> module_cpu_feature_match(AES, aes_init);

>>>

>>> ./include/linux/cpufeature.h:48:33: note: expanded from macro

>>> 'module_cpu_feature_match'

>>> static struct cpu_feature const cpu_feature_match_ ## x[] =     \

>>>

>>> <scratch space>:83:1: note: expanded from here

>>> cpu_feature_match_AES

>>> ^

>>> 1 warning generated.

>>>

>>> Do you happen to have an idea how to alleviate?

>>>

>>

>> I guess this only happens for modules that are selected as builtin,

>> and so MODULE_DEVICE_TABLE() resolves to nothing?

>> Does this only occur for CPU features?

>

> So in the above case CONFIG_ARM_CRYPTO=y, CONFIG_CRYPTO_AES_ARM_CE=m...

>

> Right now I only saw it with CPU features... I remember seen similar issues, which got resolved. Digging in the git history I found 1f318a8bafcf ("modules: mark __inittest/__exittest as __maybe_unused"),

>

> This seems to resolve it:

>

> --- a/include/linux/cpufeature.h

> +++ b/include/linux/cpufeature.h

> @@ -45,7 +45,7 @@

>   * 'asm/cpufeature.h' of your favorite architecture.

>   */

>  #define module_cpu_feature_match(x, __initfunc)                        \

> -static struct cpu_feature const cpu_feature_match_ ## x[] =    \

> +static struct cpu_feature const __maybe_unused cpu_feature_match_ ## x[] = \

>         { { .feature = cpu_feature(x) }, { } };                 \

>  MODULE_DEVICE_TABLE(cpu, cpu_feature_match_ ## x);             \

>                                                                 \

>

> Also arch/arm/crypto/crc32-ce-glue.c needs an extra __maybe_unused.

>


Yes, that looks like the right approach to me. The difference between
other uses of MODULE_DEVICE_TABLE() is that the second argument
usually gets referenced in some way in the driver struct. It the CPU
feature case, that does not happen and so the struct ends up being
unreferenced when building the code into the kernel.
diff mbox series

Patch

diff --git a/arch/arm/crypto/aes-ce-glue.c b/arch/arm/crypto/aes-ce-glue.c
index 883b84d828c5..0f966a8ca1ce 100644
--- a/arch/arm/crypto/aes-ce-glue.c
+++ b/arch/arm/crypto/aes-ce-glue.c
@@ -14,6 +14,7 @@ 
 #include <crypto/aes.h>
 #include <crypto/internal/simd.h>
 #include <crypto/internal/skcipher.h>
+#include <linux/cpufeature.h>
 #include <linux/module.h>
 #include <crypto/xts.h>
 
@@ -425,9 +426,6 @@  static int __init aes_init(void)
 	int err;
 	int i;
 
-	if (!(elf_hwcap2 & HWCAP2_AES))
-		return -ENODEV;
-
 	err = crypto_register_skciphers(aes_algs, ARRAY_SIZE(aes_algs));
 	if (err)
 		return err;
@@ -451,5 +449,5 @@  static int __init aes_init(void)
 	return err;
 }
 
-module_init(aes_init);
+module_cpu_feature_match(AES, aes_init);
 module_exit(aes_exit);