* [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-16 11:53 Tetsuo Handa 2013-07-16 11:55 ` Herbert Xu 0 siblings, 1 reply; 72+ messages in thread From: Tetsuo Handa @ 2013-07-16 11:53 UTC (permalink / raw) To: tim.c.chen; +Cc: herbert, linux-kernel I got below failure. [ 5.258911] scsi 1:0:0:0: CD-ROM NECVMWar VMware IDE CDR10 1.00 PQ: 0 ANSI: 5 [ 5.267651] modprobe (156) used greatest stack depth: 4064 bytes left [ 5.293607] Fusion MPT base driver 3.04.20 [ 5.294416] Copyright (c) 1999-2008 LSI Corporation [ 5.300109] Fusion MPT SPI Host driver 3.04.20 [ 5.300967] Switched to clocksource tsc [ 5.310921] mptbase: ioc0: Initiating bringup [ 5.329480] ioc0: LSI53C1030 B0: Capabilities={Initiator} [ 5.373136] scsi2 : ioc0: LSI53C1030 B0, FwRev=01032920h, Ports=1, MaxQ=128, IRQ=17 [ 5.406190] scsi 2:0:0:0: Direct-Access VMware, VMware Virtual S 1.0 PQ: 0 ANSI: 2 [ 5.408582] scsi target2:0:0: Beginning Domain Validation [ 5.415451] scsi target2:0:0: Domain Validation skipping write tests [ 5.416399] scsi target2:0:0: Ending Domain Validation [ 5.417445] scsi target2:0:0: FAST-40 WIDE SCSI 80.0 MB/s ST (25 ns, offset 127) [ 5.419855] scsi 2:0:1:0: Direct-Access VMware, VMware Virtual S 1.0 PQ: 0 ANSI: 2 [ 5.420404] scsi target2:0:1: Beginning Domain Validation [ 5.423171] scsi target2:0:1: Domain Validation skipping write tests [ 5.423363] scsi target2:0:1: Ending Domain Validation [ 5.424440] scsi target2:0:1: FAST-40 WIDE SCSI 80.0 MB/s ST (25 ns, offset 127) [ 5.439700] modprobe (211) used greatest stack depth: 3872 bytes left [ 5.561700] sr0: scsi3-mmc drive: 1x/1x writer dvd-ram cd/rw xa/form2 cdda tray [ 5.562596] cdrom: Uniform CD-ROM driver Revision: 3.20 [ 5.667330] scsi_id (264) used greatest stack depth: 3568 bytes left FATAL: Module scsi_wait_scan not found. FATAL: Module scsi_wait_scan not found. FATAL: Module scsi_wait_scan not found. (...snipped...) [ 60.308916] dracut Warning: Boot has failed. To debug this issue add "rdshell" to the kernel command line. [ 60.311431] dracut Warning: Signal caught! Kernel config is at http://I-love.SAKURA.ne.jp/tmp/config-3.11-rc1 Bisected to commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework", and confirmed that changing from CONFIG_CRYPTO_CRCT10DIF=m to CONFIG_CRYPTO_CRCT10DIF=y solves this problem. ---------- >From ad396f0c049fe6d4ab14793d10367e32227c5991 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Tue, 16 Jul 2013 18:33:40 +0900 Subject: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework" was added without updating module dependency, breaking at least one module which depends on CONFIG_CRYPTO_CRCT10DIF=y. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- crypto/Kconfig | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/crypto/Kconfig b/crypto/Kconfig index 69ce573..aa8edba 100644 --- a/crypto/Kconfig +++ b/crypto/Kconfig @@ -377,7 +377,7 @@ config CRYPTO_CRC32_PCLMUL and gain better performance as compared with the table implementation. config CRYPTO_CRCT10DIF - tristate "CRCT10DIF algorithm" + bool "CRCT10DIF algorithm" select CRYPTO_HASH help CRC T10 Data Integrity Field computation is being cast as -- 1.7.1 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-16 11:53 [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Tetsuo Handa @ 2013-07-16 11:55 ` Herbert Xu 2013-07-16 13:49 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency Tetsuo Handa 0 siblings, 1 reply; 72+ messages in thread From: Herbert Xu @ 2013-07-16 11:55 UTC (permalink / raw) To: Tetsuo Handa; +Cc: tim.c.chen, linux-kernel On Tue, Jul 16, 2013 at 08:53:02PM +0900, Tetsuo Handa wrote: > I got below failure. > > [ 5.258911] scsi 1:0:0:0: CD-ROM NECVMWar VMware IDE CDR10 1.00 PQ: 0 ANSI: 5 > [ 5.267651] modprobe (156) used greatest stack depth: 4064 bytes left > [ 5.293607] Fusion MPT base driver 3.04.20 > [ 5.294416] Copyright (c) 1999-2008 LSI Corporation > [ 5.300109] Fusion MPT SPI Host driver 3.04.20 > [ 5.300967] Switched to clocksource tsc > [ 5.310921] mptbase: ioc0: Initiating bringup > [ 5.329480] ioc0: LSI53C1030 B0: Capabilities={Initiator} > [ 5.373136] scsi2 : ioc0: LSI53C1030 B0, FwRev=01032920h, Ports=1, MaxQ=128, IRQ=17 > [ 5.406190] scsi 2:0:0:0: Direct-Access VMware, VMware Virtual S 1.0 PQ: 0 ANSI: 2 > [ 5.408582] scsi target2:0:0: Beginning Domain Validation > [ 5.415451] scsi target2:0:0: Domain Validation skipping write tests > [ 5.416399] scsi target2:0:0: Ending Domain Validation > [ 5.417445] scsi target2:0:0: FAST-40 WIDE SCSI 80.0 MB/s ST (25 ns, offset 127) > [ 5.419855] scsi 2:0:1:0: Direct-Access VMware, VMware Virtual S 1.0 PQ: 0 ANSI: 2 > [ 5.420404] scsi target2:0:1: Beginning Domain Validation > [ 5.423171] scsi target2:0:1: Domain Validation skipping write tests > [ 5.423363] scsi target2:0:1: Ending Domain Validation > [ 5.424440] scsi target2:0:1: FAST-40 WIDE SCSI 80.0 MB/s ST (25 ns, offset 127) > [ 5.439700] modprobe (211) used greatest stack depth: 3872 bytes left > [ 5.561700] sr0: scsi3-mmc drive: 1x/1x writer dvd-ram cd/rw xa/form2 cdda tray > [ 5.562596] cdrom: Uniform CD-ROM driver Revision: 3.20 > [ 5.667330] scsi_id (264) used greatest stack depth: 3568 bytes left > FATAL: Module scsi_wait_scan not found. > > FATAL: Module scsi_wait_scan not found. > > FATAL: Module scsi_wait_scan not found. > (...snipped...) > [ 60.308916] dracut Warning: Boot has failed. To debug this issue add "rdshell" to the kernel command line. > [ 60.311431] dracut Warning: Signal caught! > > Kernel config is at http://I-love.SAKURA.ne.jp/tmp/config-3.11-rc1 > > Bisected to commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all > to use crypto transform framework", and confirmed that changing from > CONFIG_CRYPTO_CRCT10DIF=m to CONFIG_CRYPTO_CRCT10DIF=y solves this problem. Looks like a bug in whatever is creating the initrd as it isn't including modules necessary for the boot. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency. 2013-07-16 11:55 ` Herbert Xu @ 2013-07-16 13:49 ` Tetsuo Handa 2013-07-16 16:23 ` Tim Chen 0 siblings, 1 reply; 72+ messages in thread From: Tetsuo Handa @ 2013-07-16 13:49 UTC (permalink / raw) To: herbert; +Cc: tim.c.chen, linux-kernel Herbert Xu wrote: > Looks like a bug in whatever is creating the initrd as it isn't > including modules necessary for the boot. It turned out that it is already wrong as of creating modules.dep. # grep crc /lib/modules/3.11.0-rc1/modules.dep kernel/crypto/crct10dif.ko: kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/lib/crc-t10dif.ko: modules.dep says (1) sd_mod.ko depends on crc-t10dif.ko (2) crc-t10dif.ko does not depend on crct10dif.ko but commit 2d31e518 made crc-t10dif.ko depend on crct10dif.ko , didn't it? crct10dif.ko need to be loaded before crc-t10dif.ko is loaded, but doing diff --git a/lib/Kconfig b/lib/Kconfig index 35da513..53ee0fd 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -68,6 +68,7 @@ config CRC_T10DIF tristate "CRC calculation for the T10 Data Integrity Field" select CRYPTO select CRYPTO_CRCT10DIF + depends on CRYPTO_CRCT10DIF help This option is only needed if a module that's not in the kernel tree needs to calculate CRC checks for use with the causes below warning. crypto/Kconfig:379: symbol CRYPTO_CRCT10DIF is selected by CRC_T10DIF warning: (BLK_DEV_SD && SCSI_LPFC && SCSI_DEBUG) selects CRC_T10DIF which has unmet direct dependencies (CRYPTO_CRCT10DIF) ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency. 2013-07-16 13:49 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency Tetsuo Handa @ 2013-07-16 16:23 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-16 16:23 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Tue, 2013-07-16 at 22:49 +0900, Tetsuo Handa wrote: > Herbert Xu wrote: > > Looks like a bug in whatever is creating the initrd as it isn't > > including modules necessary for the boot. > > It turned out that it is already wrong as of creating modules.dep. > > # grep crc /lib/modules/3.11.0-rc1/modules.dep > kernel/crypto/crct10dif.ko: > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > kernel/lib/crc-t10dif.ko: > > modules.dep says > > (1) sd_mod.ko depends on crc-t10dif.ko > (2) crc-t10dif.ko does not depend on crct10dif.ko Yes, the generator of modules.dep does not seem to pick up the right dependency. > > but commit 2d31e518 made crc-t10dif.ko depend on crct10dif.ko , didn't it? > > crct10dif.ko need to be loaded before crc-t10dif.ko is loaded, but doing > > diff --git a/lib/Kconfig b/lib/Kconfig > index 35da513..53ee0fd 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -68,6 +68,7 @@ config CRC_T10DIF > tristate "CRC calculation for the T10 Data Integrity Field" > select CRYPTO > select CRYPTO_CRCT10DIF > + depends on CRYPTO_CRCT10DIF Herbert, seems like modules.dep generator wants explicit - select CRYPTO_CRCT10DIF + depends on CRYPTO_CRCT10DIF But it seems to me like it should have known CRC_T10DIF needs CRYPTO_CRCT10DIF when we do select CRYPTO_CRCT10DIF Your thoughts? Thanks. Tim > help > This option is only needed if a module that's not in the > kernel tree needs to calculate CRC checks for use with the > > causes below warning. > > crypto/Kconfig:379: symbol CRYPTO_CRCT10DIF is selected by CRC_T10DIF > warning: (BLK_DEV_SD && SCSI_LPFC && SCSI_DEBUG) selects CRC_T10DIF which has unmet direct dependencies (CRYPTO_CRCT10DIF) ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency. @ 2013-07-16 16:23 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-16 16:23 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Tue, 2013-07-16 at 22:49 +0900, Tetsuo Handa wrote: > Herbert Xu wrote: > > Looks like a bug in whatever is creating the initrd as it isn't > > including modules necessary for the boot. > > It turned out that it is already wrong as of creating modules.dep. > > # grep crc /lib/modules/3.11.0-rc1/modules.dep > kernel/crypto/crct10dif.ko: > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > kernel/lib/crc-t10dif.ko: > > modules.dep says > > (1) sd_mod.ko depends on crc-t10dif.ko > (2) crc-t10dif.ko does not depend on crct10dif.ko Yes, the generator of modules.dep does not seem to pick up the right dependency. > > but commit 2d31e518 made crc-t10dif.ko depend on crct10dif.ko , didn't it? > > crct10dif.ko need to be loaded before crc-t10dif.ko is loaded, but doing > > diff --git a/lib/Kconfig b/lib/Kconfig > index 35da513..53ee0fd 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -68,6 +68,7 @@ config CRC_T10DIF > tristate "CRC calculation for the T10 Data Integrity Field" > select CRYPTO > select CRYPTO_CRCT10DIF > + depends on CRYPTO_CRCT10DIF Herbert, seems like modules.dep generator wants explicit - select CRYPTO_CRCT10DIF + depends on CRYPTO_CRCT10DIF But it seems to me like it should have known CRC_T10DIF needs CRYPTO_CRCT10DIF when we do select CRYPTO_CRCT10DIF Your thoughts? Thanks. Tim > help > This option is only needed if a module that's not in the > kernel tree needs to calculate CRC checks for use with the > > causes below warning. > > crypto/Kconfig:379: symbol CRYPTO_CRCT10DIF is selected by CRC_T10DIF > warning: (BLK_DEV_SD && SCSI_LPFC && SCSI_DEBUG) selects CRC_T10DIF which has unmet direct dependencies (CRYPTO_CRCT10DIF) ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due tomoduledependency. 2013-07-16 16:23 ` Tim Chen @ 2013-07-17 11:52 ` Tetsuo Handa -1 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-17 11:52 UTC (permalink / raw) To: tim.c.chen; +Cc: herbert, linux-kernel, linux-crypto Tim Chen wrote: > Herbert, seems like modules.dep generator wants explicit > > - select CRYPTO_CRCT10DIF > + depends on CRYPTO_CRCT10DIF > > But it seems to me like it should have known CRC_T10DIF needs > CRYPTO_CRCT10DIF when we do > select CRYPTO_CRCT10DIF > > Your thoughts? "select" cannot tell /sbin/depmod that there is dependency, for /sbin/depmod calculates dependency by enumerating symbols in each module rather than by parsing Kconfig files which depends on "kernel-source_files_installed = y". Therefore, I think possible solutions are either (a) built-in the dependent modules # grep crc /lib/modules/3.11.0-rc1+/modules.dep kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/lib/crc-t10dif.ko: or (b) embed explicit reference to the dependent module's symbols # grep crc /lib/modules/3.11.0-rc1+/modules.dep kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif.ko kernel/crypto/crct10dif.ko: kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko kernel/lib/crc-t10dif.ko: kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko . Two patches ((a) and (b) respectively) follow, but I think patch (b) will not work unless additional change static int __init crct10dif_intel_mod_init(void) { if (x86_match_cpu(crct10dif_cpu_id)) return crypto_register_shash(&alg); return 0; } static void __exit crct10dif_intel_mod_fini(void) { if (x86_match_cpu(crct10dif_cpu_id)) crypto_unregister_shash(&alg); } is made, for currently crct10dif-pclmul.ko cannot be loaded on !X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ) systems. ------------------------------------------------------------ >From d8d9b7c3e5be9c5a6198dac6fe7279ca904343a8 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Wed, 17 Jul 2013 19:45:19 +0900 Subject: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework" was added without telling that "crc-t10dif.ko depends on crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". Fix this by changing crct10dif.ko and crct10dif-pclmul.ko from "tristate" to "bool" so that suitable version is chosen. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- crypto/Kconfig | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/Kconfig b/crypto/Kconfig index 69ce573..dd3b79e 100644 --- a/crypto/Kconfig +++ b/crypto/Kconfig @@ -377,7 +377,7 @@ config CRYPTO_CRC32_PCLMUL and gain better performance as compared with the table implementation. config CRYPTO_CRCT10DIF - tristate "CRCT10DIF algorithm" + bool "CRCT10DIF algorithm" select CRYPTO_HASH help CRC T10 Data Integrity Field computation is being cast as @@ -385,7 +385,7 @@ config CRYPTO_CRCT10DIF transforms to be used if they are available. config CRYPTO_CRCT10DIF_PCLMUL - tristate "CRCT10DIF PCLMULQDQ hardware acceleration" + bool "CRCT10DIF PCLMULQDQ hardware acceleration" depends on X86 && 64BIT && CRC_T10DIF select CRYPTO_HASH help -- 1.7.1 ------------------------------------------------------------ >From 153e209fc9a7e1df42555829c396ee9ed53e83d0 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Wed, 17 Jul 2013 20:23:15 +0900 Subject: [PATCH] [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework" was added without telling that "crc-t10dif.ko depends on crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". Fix this by describing "crc-t10dif.ko depends on crct10dif.ko". Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- arch/x86/crypto/crct10dif-pclmul_glue.c | 6 ++++++ lib/crc-t10dif.c | 7 +++++++ 2 files changed, 13 insertions(+), 0 deletions(-) diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c index 7845d7f..2964608 100644 --- a/arch/x86/crypto/crct10dif-pclmul_glue.c +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c @@ -149,3 +149,9 @@ MODULE_LICENSE("GPL"); MODULE_ALIAS("crct10dif"); MODULE_ALIAS("crct10dif-pclmul"); + +/* Dummy for describing module dependency. */ +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) +const char crct10dif_pclmul; +EXPORT_SYMBOL(crct10dif_pclmul); +#endif diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index fe3428c..376f795 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -38,6 +38,13 @@ EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void) { + /* Dummy for describing module dependency. */ +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) + extern const char crct10dif_pclmul; + crc_t10dif_generic(crct10dif_pclmul, NULL, 0); +#elif defined(CONFIG_CRYPTO_CRCT10DIF_MODULE) + crc_t10dif_generic(0, NULL, 0); +#endif crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); return PTR_RET(crct10dif_tfm); } -- 1.7.1 ------------------------------------------------------------ ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due tomoduledependency. @ 2013-07-17 11:52 ` Tetsuo Handa 0 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-17 11:52 UTC (permalink / raw) To: tim.c.chen; +Cc: herbert, linux-kernel, linux-crypto Tim Chen wrote: > Herbert, seems like modules.dep generator wants explicit > > - select CRYPTO_CRCT10DIF > + depends on CRYPTO_CRCT10DIF > > But it seems to me like it should have known CRC_T10DIF needs > CRYPTO_CRCT10DIF when we do > select CRYPTO_CRCT10DIF > > Your thoughts? "select" cannot tell /sbin/depmod that there is dependency, for /sbin/depmod calculates dependency by enumerating symbols in each module rather than by parsing Kconfig files which depends on "kernel-source_files_installed = y". Therefore, I think possible solutions are either (a) built-in the dependent modules # grep crc /lib/modules/3.11.0-rc1+/modules.dep kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/lib/crc-t10dif.ko: or (b) embed explicit reference to the dependent module's symbols # grep crc /lib/modules/3.11.0-rc1+/modules.dep kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif.ko kernel/crypto/crct10dif.ko: kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko kernel/lib/crc-t10dif.ko: kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko . Two patches ((a) and (b) respectively) follow, but I think patch (b) will not work unless additional change static int __init crct10dif_intel_mod_init(void) { if (x86_match_cpu(crct10dif_cpu_id)) return crypto_register_shash(&alg); return 0; } static void __exit crct10dif_intel_mod_fini(void) { if (x86_match_cpu(crct10dif_cpu_id)) crypto_unregister_shash(&alg); } is made, for currently crct10dif-pclmul.ko cannot be loaded on !X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ) systems. ------------------------------------------------------------ >From d8d9b7c3e5be9c5a6198dac6fe7279ca904343a8 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Wed, 17 Jul 2013 19:45:19 +0900 Subject: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework" was added without telling that "crc-t10dif.ko depends on crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". Fix this by changing crct10dif.ko and crct10dif-pclmul.ko from "tristate" to "bool" so that suitable version is chosen. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- crypto/Kconfig | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/Kconfig b/crypto/Kconfig index 69ce573..dd3b79e 100644 --- a/crypto/Kconfig +++ b/crypto/Kconfig @@ -377,7 +377,7 @@ config CRYPTO_CRC32_PCLMUL and gain better performance as compared with the table implementation. config CRYPTO_CRCT10DIF - tristate "CRCT10DIF algorithm" + bool "CRCT10DIF algorithm" select CRYPTO_HASH help CRC T10 Data Integrity Field computation is being cast as @@ -385,7 +385,7 @@ config CRYPTO_CRCT10DIF transforms to be used if they are available. config CRYPTO_CRCT10DIF_PCLMUL - tristate "CRCT10DIF PCLMULQDQ hardware acceleration" + bool "CRCT10DIF PCLMULQDQ hardware acceleration" depends on X86 && 64BIT && CRC_T10DIF select CRYPTO_HASH help -- 1.7.1 ------------------------------------------------------------ >From 153e209fc9a7e1df42555829c396ee9ed53e83d0 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Wed, 17 Jul 2013 20:23:15 +0900 Subject: [PATCH] [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework" was added without telling that "crc-t10dif.ko depends on crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". Fix this by describing "crc-t10dif.ko depends on crct10dif.ko". Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- arch/x86/crypto/crct10dif-pclmul_glue.c | 6 ++++++ lib/crc-t10dif.c | 7 +++++++ 2 files changed, 13 insertions(+), 0 deletions(-) diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c index 7845d7f..2964608 100644 --- a/arch/x86/crypto/crct10dif-pclmul_glue.c +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c @@ -149,3 +149,9 @@ MODULE_LICENSE("GPL"); MODULE_ALIAS("crct10dif"); MODULE_ALIAS("crct10dif-pclmul"); + +/* Dummy for describing module dependency. */ +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) +const char crct10dif_pclmul; +EXPORT_SYMBOL(crct10dif_pclmul); +#endif diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index fe3428c..376f795 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -38,6 +38,13 @@ EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void) { + /* Dummy for describing module dependency. */ +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) + extern const char crct10dif_pclmul; + crc_t10dif_generic(crct10dif_pclmul, NULL, 0); +#elif defined(CONFIG_CRYPTO_CRCT10DIF_MODULE) + crc_t10dif_generic(0, NULL, 0); +#endif crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); return PTR_RET(crct10dif_tfm); } -- 1.7.1 ------------------------------------------------------------ ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due tomoduledependency. 2013-07-17 11:52 ` Tetsuo Handa @ 2013-07-17 16:46 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-17 16:46 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Wed, 2013-07-17 at 20:52 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > Herbert, seems like modules.dep generator wants explicit > > > > - select CRYPTO_CRCT10DIF > > + depends on CRYPTO_CRCT10DIF > > > > But it seems to me like it should have known CRC_T10DIF needs > > CRYPTO_CRCT10DIF when we do > > select CRYPTO_CRCT10DIF > > > > Your thoughts? > > "select" cannot tell /sbin/depmod that there is dependency, for /sbin/depmod > calculates dependency by enumerating symbols in each module rather than by > parsing Kconfig files which depends on "kernel-source_files_installed = y". > > Therefore, I think possible solutions are either > > (a) built-in the dependent modules > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > kernel/lib/crc-t10dif.ko: This approach will increase kernel size for those who are not using t10dif so some people may object. BTW, The PCLMULQDQ version does not need to be build in. > > or > > (b) embed explicit reference to the dependent module's symbols > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif.ko > kernel/crypto/crct10dif.ko: > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko > kernel/lib/crc-t10dif.ko: kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko > Your approach is quite complicated. I think something simpler like the following will work: Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com> --- diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index fe3428c..27d6be3 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -15,7 +15,8 @@ #include <linux/init.h> #include <crypto/hash.h> -static struct crypto_shash *crct10dif_tfm; +__u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, size_t len); +static struct crypto_shash *crct10dif_tfm = crc_t10dif_generic; __u16 crc_t10dif(const unsigned char *buffer, size_t len) { > . > > Two patches ((a) and (b) respectively) follow, but I think patch (b) will not > work unless additional change > > static int __init crct10dif_intel_mod_init(void) > { > if (x86_match_cpu(crct10dif_cpu_id)) > return crypto_register_shash(&alg); > return 0; > } > > static void __exit crct10dif_intel_mod_fini(void) > { > if (x86_match_cpu(crct10dif_cpu_id)) > crypto_unregister_shash(&alg); > } > > is made, for currently crct10dif-pclmul.ko cannot be loaded on > !X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ) systems. crct10dif-pclmul.ko should not be loaded if X86_FEATURE_PCLMULQDQ is not supported. The module needs the cpu to have support for PCLMULQDQ. > > ------------------------------------------------------------ > > >From d8d9b7c3e5be9c5a6198dac6fe7279ca904343a8 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Wed, 17 Jul 2013 19:45:19 +0900 > Subject: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. > > Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto > transform framework" was added without telling that "crc-t10dif.ko depends on > crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on > crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". > > Fix this by changing crct10dif.ko and crct10dif-pclmul.ko from "tristate" to > "bool" so that suitable version is chosen. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > crypto/Kconfig | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/crypto/Kconfig b/crypto/Kconfig > index 69ce573..dd3b79e 100644 > --- a/crypto/Kconfig > +++ b/crypto/Kconfig > @@ -377,7 +377,7 @@ config CRYPTO_CRC32_PCLMUL > and gain better performance as compared with the table implementation. > > config CRYPTO_CRCT10DIF > - tristate "CRCT10DIF algorithm" > + bool "CRCT10DIF algorithm" > select CRYPTO_HASH > help > CRC T10 Data Integrity Field computation is being cast as > @@ -385,7 +385,7 @@ config CRYPTO_CRCT10DIF > transforms to be used if they are available. > > config CRYPTO_CRCT10DIF_PCLMUL > - tristate "CRCT10DIF PCLMULQDQ hardware acceleration" > + bool "CRCT10DIF PCLMULQDQ hardware acceleration" This is not necessary. Keep it as tristate. > depends on X86 && 64BIT && CRC_T10DIF > select CRYPTO_HASH > help > -- > 1.7.1 > > ------------------------------------------------------------ > > >From 153e209fc9a7e1df42555829c396ee9ed53e83d0 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Wed, 17 Jul 2013 20:23:15 +0900 > Subject: [PATCH] [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. > > Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto > transform framework" was added without telling that "crc-t10dif.ko depends on > crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on > crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". > > Fix this by describing "crc-t10dif.ko depends on crct10dif.ko". > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > arch/x86/crypto/crct10dif-pclmul_glue.c | 6 ++++++ > lib/crc-t10dif.c | 7 +++++++ > 2 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c > index 7845d7f..2964608 100644 > --- a/arch/x86/crypto/crct10dif-pclmul_glue.c > +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c > @@ -149,3 +149,9 @@ MODULE_LICENSE("GPL"); > > MODULE_ALIAS("crct10dif"); > MODULE_ALIAS("crct10dif-pclmul"); > + > +/* Dummy for describing module dependency. */ > +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) > +const char crct10dif_pclmul; > +EXPORT_SYMBOL(crct10dif_pclmul); > +#endif > diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c > index fe3428c..376f795 100644 > --- a/lib/crc-t10dif.c > +++ b/lib/crc-t10dif.c > @@ -38,6 +38,13 @@ EXPORT_SYMBOL(crc_t10dif); > > static int __init crc_t10dif_mod_init(void) > { > + /* Dummy for describing module dependency. */ > +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) > + extern const char crct10dif_pclmul; > + crc_t10dif_generic(crct10dif_pclmul, NULL, 0); > +#elif defined(CONFIG_CRYPTO_CRCT10DIF_MODULE) > + crc_t10dif_generic(0, NULL, 0); > +#endif > crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); > return PTR_RET(crct10dif_tfm); > } ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due tomoduledependency. @ 2013-07-17 16:46 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-17 16:46 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Wed, 2013-07-17 at 20:52 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > Herbert, seems like modules.dep generator wants explicit > > > > - select CRYPTO_CRCT10DIF > > + depends on CRYPTO_CRCT10DIF > > > > But it seems to me like it should have known CRC_T10DIF needs > > CRYPTO_CRCT10DIF when we do > > select CRYPTO_CRCT10DIF > > > > Your thoughts? > > "select" cannot tell /sbin/depmod that there is dependency, for /sbin/depmod > calculates dependency by enumerating symbols in each module rather than by > parsing Kconfig files which depends on "kernel-source_files_installed = y". > > Therefore, I think possible solutions are either > > (a) built-in the dependent modules > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > kernel/lib/crc-t10dif.ko: This approach will increase kernel size for those who are not using t10dif so some people may object. BTW, The PCLMULQDQ version does not need to be build in. > > or > > (b) embed explicit reference to the dependent module's symbols > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif.ko > kernel/crypto/crct10dif.ko: > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko > kernel/lib/crc-t10dif.ko: kernel/arch/x86/crypto/crct10dif-pclmul.ko kernel/crypto/crct10dif.ko > Your approach is quite complicated. I think something simpler like the following will work: Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com> --- diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index fe3428c..27d6be3 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -15,7 +15,8 @@ #include <linux/init.h> #include <crypto/hash.h> -static struct crypto_shash *crct10dif_tfm; +__u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, size_t len); +static struct crypto_shash *crct10dif_tfm = crc_t10dif_generic; __u16 crc_t10dif(const unsigned char *buffer, size_t len) { > . > > Two patches ((a) and (b) respectively) follow, but I think patch (b) will not > work unless additional change > > static int __init crct10dif_intel_mod_init(void) > { > if (x86_match_cpu(crct10dif_cpu_id)) > return crypto_register_shash(&alg); > return 0; > } > > static void __exit crct10dif_intel_mod_fini(void) > { > if (x86_match_cpu(crct10dif_cpu_id)) > crypto_unregister_shash(&alg); > } > > is made, for currently crct10dif-pclmul.ko cannot be loaded on > !X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ) systems. crct10dif-pclmul.ko should not be loaded if X86_FEATURE_PCLMULQDQ is not supported. The module needs the cpu to have support for PCLMULQDQ. > > ------------------------------------------------------------ > > >From d8d9b7c3e5be9c5a6198dac6fe7279ca904343a8 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Wed, 17 Jul 2013 19:45:19 +0900 > Subject: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. > > Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto > transform framework" was added without telling that "crc-t10dif.ko depends on > crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on > crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". > > Fix this by changing crct10dif.ko and crct10dif-pclmul.ko from "tristate" to > "bool" so that suitable version is chosen. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > crypto/Kconfig | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/crypto/Kconfig b/crypto/Kconfig > index 69ce573..dd3b79e 100644 > --- a/crypto/Kconfig > +++ b/crypto/Kconfig > @@ -377,7 +377,7 @@ config CRYPTO_CRC32_PCLMUL > and gain better performance as compared with the table implementation. > > config CRYPTO_CRCT10DIF > - tristate "CRCT10DIF algorithm" > + bool "CRCT10DIF algorithm" > select CRYPTO_HASH > help > CRC T10 Data Integrity Field computation is being cast as > @@ -385,7 +385,7 @@ config CRYPTO_CRCT10DIF > transforms to be used if they are available. > > config CRYPTO_CRCT10DIF_PCLMUL > - tristate "CRCT10DIF PCLMULQDQ hardware acceleration" > + bool "CRCT10DIF PCLMULQDQ hardware acceleration" This is not necessary. Keep it as tristate. > depends on X86 && 64BIT && CRC_T10DIF > select CRYPTO_HASH > help > -- > 1.7.1 > > ------------------------------------------------------------ > > >From 153e209fc9a7e1df42555829c396ee9ed53e83d0 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Wed, 17 Jul 2013 20:23:15 +0900 > Subject: [PATCH] [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. > > Commit 2d31e518 "crypto: crct10dif - Wrap crc_t10dif function all to use crypto > transform framework" was added without telling that "crc-t10dif.ko depends on > crct10dif.ko". This resulted in boot failure because "sd_mod.ko depends on > crc-t10dif.ko" but "crct10dif.ko is not loaded automatically". > > Fix this by describing "crc-t10dif.ko depends on crct10dif.ko". > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > arch/x86/crypto/crct10dif-pclmul_glue.c | 6 ++++++ > lib/crc-t10dif.c | 7 +++++++ > 2 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c > index 7845d7f..2964608 100644 > --- a/arch/x86/crypto/crct10dif-pclmul_glue.c > +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c > @@ -149,3 +149,9 @@ MODULE_LICENSE("GPL"); > > MODULE_ALIAS("crct10dif"); > MODULE_ALIAS("crct10dif-pclmul"); > + > +/* Dummy for describing module dependency. */ > +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) > +const char crct10dif_pclmul; > +EXPORT_SYMBOL(crct10dif_pclmul); > +#endif > diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c > index fe3428c..376f795 100644 > --- a/lib/crc-t10dif.c > +++ b/lib/crc-t10dif.c > @@ -38,6 +38,13 @@ EXPORT_SYMBOL(crc_t10dif); > > static int __init crc_t10dif_mod_init(void) > { > + /* Dummy for describing module dependency. */ > +#if defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) > + extern const char crct10dif_pclmul; > + crc_t10dif_generic(crct10dif_pclmul, NULL, 0); > +#elif defined(CONFIG_CRYPTO_CRCT10DIF_MODULE) > + crc_t10dif_generic(0, NULL, 0); > +#endif > crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); > return PTR_RET(crct10dif_tfm); > } ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure duetomoduledependency. 2013-07-17 16:46 ` Tim Chen @ 2013-07-17 20:50 ` Tetsuo Handa -1 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-17 20:50 UTC (permalink / raw) To: tim.c.chen; +Cc: herbert, linux-kernel, linux-crypto Tim Chen wrote: > > Therefore, I think possible solutions are either > > > > (a) built-in the dependent modules > > > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > > kernel/lib/crc-t10dif.ko: > > This approach will increase kernel size for those who are not using > t10dif so some people may object. > BTW, The PCLMULQDQ version does not need to be build in. sd_mod.ko must choose one from versions available as of loading sd_mod.ko. Although it is not needed to built-in the PCLMULQDQ version for fixing the boot failure, it is needed to built-in the PCLMULQDQ version for getting performance improvement when PCLMULQDQ is supported. > Your approach is quite complicated. I think something simpler like the > following will work: We cannot benefit from PCLMULQDQ. Is it acceptable for you? ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure duetomoduledependency. @ 2013-07-17 20:50 ` Tetsuo Handa 0 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-17 20:50 UTC (permalink / raw) To: tim.c.chen; +Cc: herbert, linux-kernel, linux-crypto Tim Chen wrote: > > Therefore, I think possible solutions are either > > > > (a) built-in the dependent modules > > > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > > kernel/lib/crc-t10dif.ko: > > This approach will increase kernel size for those who are not using > t10dif so some people may object. > BTW, The PCLMULQDQ version does not need to be build in. sd_mod.ko must choose one from versions available as of loading sd_mod.ko. Although it is not needed to built-in the PCLMULQDQ version for fixing the boot failure, it is needed to built-in the PCLMULQDQ version for getting performance improvement when PCLMULQDQ is supported. > Your approach is quite complicated. I think something simpler like the > following will work: We cannot benefit from PCLMULQDQ. Is it acceptable for you? ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure duetomoduledependency. 2013-07-17 20:50 ` Tetsuo Handa @ 2013-07-17 21:53 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-17 21:53 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Thu, 2013-07-18 at 05:50 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > > Therefore, I think possible solutions are either > > > > > > (a) built-in the dependent modules > > > > > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > > > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > > > kernel/lib/crc-t10dif.ko: > > > > This approach will increase kernel size for those who are not using > > t10dif so some people may object. > > BTW, The PCLMULQDQ version does not need to be build in. > > sd_mod.ko must choose one from versions available as of loading sd_mod.ko. > Although it is not needed to built-in the PCLMULQDQ version for fixing the boot > failure, it is needed to built-in the PCLMULQDQ version for getting performance > improvement when PCLMULQDQ is supported. > > > Your approach is quite complicated. I think something simpler like the > > following will work: > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? The following code in crct10dif-pclmul_glue.c static const struct x86_cpu_id crct10dif_cpu_id[] = { X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), {} }; MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); will put the module in the device table and get the module loaded, as long as the cpu support PCLMULQDQ. So we should be able to benefit. Tim ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure duetomoduledependency. @ 2013-07-17 21:53 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-17 21:53 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Thu, 2013-07-18 at 05:50 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > > Therefore, I think possible solutions are either > > > > > > (a) built-in the dependent modules > > > > > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > > > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > > > kernel/lib/crc-t10dif.ko: > > > > This approach will increase kernel size for those who are not using > > t10dif so some people may object. > > BTW, The PCLMULQDQ version does not need to be build in. > > sd_mod.ko must choose one from versions available as of loading sd_mod.ko. > Although it is not needed to built-in the PCLMULQDQ version for fixing the boot > failure, it is needed to built-in the PCLMULQDQ version for getting performance > improvement when PCLMULQDQ is supported. > > > Your approach is quite complicated. I think something simpler like the > > following will work: > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? The following code in crct10dif-pclmul_glue.c static const struct x86_cpu_id crct10dif_cpu_id[] = { X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), {} }; MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); will put the module in the device table and get the module loaded, as long as the cpu support PCLMULQDQ. So we should be able to benefit. Tim ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure duetomoduledependency. 2013-07-17 20:50 ` Tetsuo Handa @ 2013-07-17 22:08 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-17 22:08 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Thu, 2013-07-18 at 05:50 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > > Therefore, I think possible solutions are either > > > > > > (a) built-in the dependent modules > > > > > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > > > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > > > kernel/lib/crc-t10dif.ko: > > > > This approach will increase kernel size for those who are not using > > t10dif so some people may object. > > BTW, The PCLMULQDQ version does not need to be build in. > > sd_mod.ko must choose one from versions available as of loading sd_mod.ko. > Although it is not needed to built-in the PCLMULQDQ version for fixing the boot > failure, it is needed to built-in the PCLMULQDQ version for getting performance > improvement when PCLMULQDQ is supported. The code in crct10dif transform allocation: crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0) will allocate a t10dif crypto transform with highest priority. So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, the pclmulqdq t10dif will have a higher priority and get allocated and used. Thanks. Tim > > > Your approach is quite complicated. I think something simpler like the > > following will work: > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure duetomoduledependency. @ 2013-07-17 22:08 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-17 22:08 UTC (permalink / raw) To: Tetsuo Handa; +Cc: herbert, linux-kernel, linux-crypto On Thu, 2013-07-18 at 05:50 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > > Therefore, I think possible solutions are either > > > > > > (a) built-in the dependent modules > > > > > > # grep crc /lib/modules/3.11.0-rc1+/modules.dep > > > kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko > > > kernel/lib/crc-t10dif.ko: > > > > This approach will increase kernel size for those who are not using > > t10dif so some people may object. > > BTW, The PCLMULQDQ version does not need to be build in. > > sd_mod.ko must choose one from versions available as of loading sd_mod.ko. > Although it is not needed to built-in the PCLMULQDQ version for fixing the boot > failure, it is needed to built-in the PCLMULQDQ version for getting performance > improvement when PCLMULQDQ is supported. The code in crct10dif transform allocation: crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0) will allocate a t10dif crypto transform with highest priority. So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, the pclmulqdq t10dif will have a higher priority and get allocated and used. Thanks. Tim > > > Your approach is quite complicated. I think something simpler like the > > following will work: > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-17 22:08 ` Tim Chen @ 2013-07-18 3:47 ` Tetsuo Handa -1 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-18 3:47 UTC (permalink / raw) To: Tim Chen; +Cc: herbert, linux-kernel, linux-crypto Tim Chen wrote: > > > Your approach is quite complicated. I think something simpler like the > > > following will work: > > > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > > The following code in crct10dif-pclmul_glue.c > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > {} > }; > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > will put the module in the device table and get the module > loaded, as long as the cpu support PCLMULQDQ. So we should be able > to benefit. Excuse me, how can crct10dif-pclmul.ko get loaded automatically? Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c index 7845d7f..a8a95aa 100644 --- a/arch/x86/crypto/crct10dif-pclmul_glue.c +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c @@ -129,9 +129,10 @@ MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); static int __init crct10dif_intel_mod_init(void) { + printk(KERN_WARNING "********** Checking for X86_FEATURE_PCLMULQDQ\n"); if (!x86_match_cpu(crct10dif_cpu_id)) return -ENODEV; - + printk(KERN_WARNING "********** Registering crct10dif-pclmul\n"); return crypto_register_shash(&alg); } As far as I tested, crct10dif-pclmul.ko will not be loaded unless manually adding "modprobe crct10dif-pclmul" to initramfs's /init or choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. > So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, > the pclmulqdq t10dif will have a higher priority and get allocated > and used. What I'm talking are (1) Since mkinitramfs is unable to know that crct10dif-pclmul.ko has higher priority than crct10dif.ko , mkinitramfs will not include "modprobe crct10dif-pclmul" line in the generated initramfs. (2) In order to get benefit from PCLMULQDQ, users have to manually make sure that "modprobe crct10dif-pclmul" is called before crc-t10dif.ko (which is loaded before sd_mod.ko is loaded) is loaded by their initramfs's /init script. (3) The cause of (1) is that crct10dif-pclmul.ko will not be loaded automatically unless choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. (4) The cause of (3) is that modules.dep does not describe that users will benefit by loading crct10dif-pclmul.ko before loading crc-t10dif.ko . (5) Currently crct10dif-pclmul.ko cannot be loaded if PCLMULQDQ is not supported. This leads to boot failure (since sd_mod.ko cannot be loaded) if modules.dep says that "crct10dif-pclmul.ko is required by crc-t10dif.ko". (6) To solve (4) and (5), modules.dep should say "crct10dif-pclmul.ko is preferred for crc-t10dif.ko but is not required by crc-t10dif.ko". But there is no such mechanism. Thus, currently available choice is "allow loading crct10dif-pclmul.ko even if PCLMULQDQ is not supported" or "ignore errors by built-in the crct10dif-pclmul.ko module". My patch (b) seems to be complicated but is required in order to solve (4) without asking users to manually add "modprobe crct10dif-pclmul" into their initramfs. If we choose patch (b) rather than patch (a), we need to solve (5). ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-18 3:47 ` Tetsuo Handa 0 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-18 3:47 UTC (permalink / raw) To: Tim Chen; +Cc: herbert, linux-kernel, linux-crypto Tim Chen wrote: > > > Your approach is quite complicated. I think something simpler like the > > > following will work: > > > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > > The following code in crct10dif-pclmul_glue.c > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > {} > }; > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > will put the module in the device table and get the module > loaded, as long as the cpu support PCLMULQDQ. So we should be able > to benefit. Excuse me, how can crct10dif-pclmul.ko get loaded automatically? Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c index 7845d7f..a8a95aa 100644 --- a/arch/x86/crypto/crct10dif-pclmul_glue.c +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c @@ -129,9 +129,10 @@ MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); static int __init crct10dif_intel_mod_init(void) { + printk(KERN_WARNING "********** Checking for X86_FEATURE_PCLMULQDQ\n"); if (!x86_match_cpu(crct10dif_cpu_id)) return -ENODEV; - + printk(KERN_WARNING "********** Registering crct10dif-pclmul\n"); return crypto_register_shash(&alg); } As far as I tested, crct10dif-pclmul.ko will not be loaded unless manually adding "modprobe crct10dif-pclmul" to initramfs's /init or choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. > So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, > the pclmulqdq t10dif will have a higher priority and get allocated > and used. What I'm talking are (1) Since mkinitramfs is unable to know that crct10dif-pclmul.ko has higher priority than crct10dif.ko , mkinitramfs will not include "modprobe crct10dif-pclmul" line in the generated initramfs. (2) In order to get benefit from PCLMULQDQ, users have to manually make sure that "modprobe crct10dif-pclmul" is called before crc-t10dif.ko (which is loaded before sd_mod.ko is loaded) is loaded by their initramfs's /init script. (3) The cause of (1) is that crct10dif-pclmul.ko will not be loaded automatically unless choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. (4) The cause of (3) is that modules.dep does not describe that users will benefit by loading crct10dif-pclmul.ko before loading crc-t10dif.ko . (5) Currently crct10dif-pclmul.ko cannot be loaded if PCLMULQDQ is not supported. This leads to boot failure (since sd_mod.ko cannot be loaded) if modules.dep says that "crct10dif-pclmul.ko is required by crc-t10dif.ko". (6) To solve (4) and (5), modules.dep should say "crct10dif-pclmul.ko is preferred for crc-t10dif.ko but is not required by crc-t10dif.ko". But there is no such mechanism. Thus, currently available choice is "allow loading crct10dif-pclmul.ko even if PCLMULQDQ is not supported" or "ignore errors by built-in the crct10dif-pclmul.ko module". My patch (b) seems to be complicated but is required in order to solve (4) without asking users to manually add "modprobe crct10dif-pclmul" into their initramfs. If we choose patch (b) rather than patch (a), we need to solve (5). ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-18 3:47 ` Tetsuo Handa @ 2013-07-18 21:00 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-18 21:00 UTC (permalink / raw) To: Tetsuo Handa, Rafael J. Wysocki; +Cc: herbert, linux-kernel, linux-crypto, ak On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > > > Your approach is quite complicated. I think something simpler like the > > > > following will work: > > > > > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > > > > > The following code in crct10dif-pclmul_glue.c > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > {} > > }; > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > will put the module in the device table and get the module > > loaded, as long as the cpu support PCLMULQDQ. So we should be able > > to benefit. > > Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? The code: static const struct x86_cpu_id crct10dif_cpu_id[] = { X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), {} }; MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); causes the following line to be added to modules.alias file: alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul This should cause udev to load the crct10dif_pclml module when cpu support the PCLMULQDQ (feature code 0081). I did my testing during development on 3.10 and the module was indeed loaded. However, I found that the following commit under 3.11-rc1 broke the mechanism after some bisection. commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Date: Fri May 3 00:26:22 2013 +0200 ACPI / processor: Use common hotplug infrastructure Split the ACPI processor driver into two parts, one that is non-modular, resides in the ACPI core and handles the enumeration and hotplug of processors and one that implements the rest of the existing processor driver functionality. Rafael, can you check and see if this can be fixed so those optimized crypto modules for Intel cpu that support them can be loaded? Herbert, we had a discussion earlier on a separate issue regarding the module load order. We wanted to load all supported crypto t10dif modules before the crc-t10dif module. You mentioned that the MODULE_ALIAS also need some fixing and wonder if those changes have been merged into 3.11-rc1? See https://lkml.org/lkml/2013/5/21/216 . Theoretically, that fix should also get all the crypto modules support t10dif be loaded. Thanks. Tim > > diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c > index 7845d7f..a8a95aa 100644 > --- a/arch/x86/crypto/crct10dif-pclmul_glue.c > +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c > @@ -129,9 +129,10 @@ MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > static int __init crct10dif_intel_mod_init(void) > { > + printk(KERN_WARNING "********** Checking for X86_FEATURE_PCLMULQDQ\n"); > if (!x86_match_cpu(crct10dif_cpu_id)) > return -ENODEV; > - > + printk(KERN_WARNING "********** Registering crct10dif-pclmul\n"); > return crypto_register_shash(&alg); > } > > As far as I tested, crct10dif-pclmul.ko will not be loaded unless manually > adding "modprobe crct10dif-pclmul" to initramfs's /init or choosing > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. > > > So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, > > the pclmulqdq t10dif will have a higher priority and get allocated > > and used. > > What I'm talking are > > (1) Since mkinitramfs is unable to know that crct10dif-pclmul.ko has higher > priority than crct10dif.ko , mkinitramfs will not include > "modprobe crct10dif-pclmul" line in the generated initramfs. > > (2) In order to get benefit from PCLMULQDQ, users have to manually make sure > that "modprobe crct10dif-pclmul" is called before crc-t10dif.ko (which is > loaded before sd_mod.ko is loaded) is loaded by their initramfs's /init > script. > > (3) The cause of (1) is that crct10dif-pclmul.ko will not be loaded > automatically unless choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. > > (4) The cause of (3) is that modules.dep does not describe that users will > benefit by loading crct10dif-pclmul.ko before loading crc-t10dif.ko . > > (5) Currently crct10dif-pclmul.ko cannot be loaded if PCLMULQDQ is not > supported. This leads to boot failure (since sd_mod.ko cannot be loaded) > if modules.dep says that "crct10dif-pclmul.ko is required by > crc-t10dif.ko". > > (6) To solve (4) and (5), modules.dep should say "crct10dif-pclmul.ko is > preferred for crc-t10dif.ko but is not required by crc-t10dif.ko". > But there is no such mechanism. Thus, currently available choice is > "allow loading crct10dif-pclmul.ko even if PCLMULQDQ is not supported" > or "ignore errors by built-in the crct10dif-pclmul.ko module". > > My patch (b) seems to be complicated but is required in order to solve (4) > without asking users to manually add "modprobe crct10dif-pclmul" into their > initramfs. If we choose patch (b) rather than patch (a), we need to solve (5). > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-18 21:00 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-18 21:00 UTC (permalink / raw) To: Tetsuo Handa, Rafael J. Wysocki; +Cc: herbert, linux-kernel, linux-crypto, ak On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > Tim Chen wrote: > > > > Your approach is quite complicated. I think something simpler like the > > > > following will work: > > > > > > We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > > > > > The following code in crct10dif-pclmul_glue.c > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > {} > > }; > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > will put the module in the device table and get the module > > loaded, as long as the cpu support PCLMULQDQ. So we should be able > > to benefit. > > Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? The code: static const struct x86_cpu_id crct10dif_cpu_id[] = { X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), {} }; MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); causes the following line to be added to modules.alias file: alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul This should cause udev to load the crct10dif_pclml module when cpu support the PCLMULQDQ (feature code 0081). I did my testing during development on 3.10 and the module was indeed loaded. However, I found that the following commit under 3.11-rc1 broke the mechanism after some bisection. commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Date: Fri May 3 00:26:22 2013 +0200 ACPI / processor: Use common hotplug infrastructure Split the ACPI processor driver into two parts, one that is non-modular, resides in the ACPI core and handles the enumeration and hotplug of processors and one that implements the rest of the existing processor driver functionality. Rafael, can you check and see if this can be fixed so those optimized crypto modules for Intel cpu that support them can be loaded? Herbert, we had a discussion earlier on a separate issue regarding the module load order. We wanted to load all supported crypto t10dif modules before the crc-t10dif module. You mentioned that the MODULE_ALIAS also need some fixing and wonder if those changes have been merged into 3.11-rc1? See https://lkml.org/lkml/2013/5/21/216 . Theoretically, that fix should also get all the crypto modules support t10dif be loaded. Thanks. Tim > > diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c > index 7845d7f..a8a95aa 100644 > --- a/arch/x86/crypto/crct10dif-pclmul_glue.c > +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c > @@ -129,9 +129,10 @@ MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > static int __init crct10dif_intel_mod_init(void) > { > + printk(KERN_WARNING "********** Checking for X86_FEATURE_PCLMULQDQ\n"); > if (!x86_match_cpu(crct10dif_cpu_id)) > return -ENODEV; > - > + printk(KERN_WARNING "********** Registering crct10dif-pclmul\n"); > return crypto_register_shash(&alg); > } > > As far as I tested, crct10dif-pclmul.ko will not be loaded unless manually > adding "modprobe crct10dif-pclmul" to initramfs's /init or choosing > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. > > > So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, > > the pclmulqdq t10dif will have a higher priority and get allocated > > and used. > > What I'm talking are > > (1) Since mkinitramfs is unable to know that crct10dif-pclmul.ko has higher > priority than crct10dif.ko , mkinitramfs will not include > "modprobe crct10dif-pclmul" line in the generated initramfs. > > (2) In order to get benefit from PCLMULQDQ, users have to manually make sure > that "modprobe crct10dif-pclmul" is called before crc-t10dif.ko (which is > loaded before sd_mod.ko is loaded) is loaded by their initramfs's /init > script. > > (3) The cause of (1) is that crct10dif-pclmul.ko will not be loaded > automatically unless choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. > > (4) The cause of (3) is that modules.dep does not describe that users will > benefit by loading crct10dif-pclmul.ko before loading crc-t10dif.ko . > > (5) Currently crct10dif-pclmul.ko cannot be loaded if PCLMULQDQ is not > supported. This leads to boot failure (since sd_mod.ko cannot be loaded) > if modules.dep says that "crct10dif-pclmul.ko is required by > crc-t10dif.ko". > > (6) To solve (4) and (5), modules.dep should say "crct10dif-pclmul.ko is > preferred for crc-t10dif.ko but is not required by crc-t10dif.ko". > But there is no such mechanism. Thus, currently available choice is > "allow loading crct10dif-pclmul.ko even if PCLMULQDQ is not supported" > or "ignore errors by built-in the crct10dif-pclmul.ko module". > > My patch (b) seems to be complicated but is required in order to solve (4) > without asking users to manually add "modprobe crct10dif-pclmul" into their > initramfs. If we choose patch (b) rather than patch (a), we need to solve (5). > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-18 21:00 ` Tim Chen @ 2013-07-18 22:17 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-18 22:17 UTC (permalink / raw) To: Tim Chen; +Cc: Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak, rjw On 7/18/2013 11:00 PM, Tim Chen wrote: > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: >> Tim Chen wrote: >>>>> Your approach is quite complicated. I think something simpler like the >>>>> following will work: >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? >>> >>> The following code in crct10dif-pclmul_glue.c >>> >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), >>> {} >>> }; >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); >>> >>> will put the module in the device table and get the module >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able >>> to benefit. >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > The code: > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > {} > }; > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > causes the following line to be added to modules.alias file: > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > This should cause udev to load the crct10dif_pclml module when cpu > support the PCLMULQDQ (feature code 0081). I did my testing during > development on 3.10 and the module was indeed loaded. > > However, I found that the following commit under 3.11-rc1 broke > the mechanism after some bisection. > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Date: Fri May 3 00:26:22 2013 +0200 > > ACPI / processor: Use common hotplug infrastructure > > Split the ACPI processor driver into two parts, one that is > non-modular, resides in the ACPI core and handles the enumeration > and hotplug of processors and one that implements the rest of the > existing processor driver functionality. > > Rafael, can you check and see if this can be fixed so those optimized > crypto modules for Intel cpu that support them can be loaded? I think this is an ordering issue between udev startup and the time when devices are registered. I wonder what happens if you put those modules into the initramfs image? Rafael > Herbert, we had a discussion earlier on a separate issue regarding the > module load order. We wanted to load all supported crypto t10dif modules > before the crc-t10dif module. You mentioned that the MODULE_ALIAS also > need some fixing and wonder if those changes have been merged into > 3.11-rc1? See https://lkml.org/lkml/2013/5/21/216 . Theoretically, that > fix should also get all the crypto modules support t10dif be loaded. > > Thanks. > > Tim > >> diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c >> index 7845d7f..a8a95aa 100644 >> --- a/arch/x86/crypto/crct10dif-pclmul_glue.c >> +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c >> @@ -129,9 +129,10 @@ MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); >> >> static int __init crct10dif_intel_mod_init(void) >> { >> + printk(KERN_WARNING "********** Checking for X86_FEATURE_PCLMULQDQ\n"); >> if (!x86_match_cpu(crct10dif_cpu_id)) >> return -ENODEV; >> - >> + printk(KERN_WARNING "********** Registering crct10dif-pclmul\n"); >> return crypto_register_shash(&alg); >> } >> >> As far as I tested, crct10dif-pclmul.ko will not be loaded unless manually >> adding "modprobe crct10dif-pclmul" to initramfs's /init or choosing >> CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. >> >>> So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, >>> the pclmulqdq t10dif will have a higher priority and get allocated >>> and used. >> What I'm talking are >> >> (1) Since mkinitramfs is unable to know that crct10dif-pclmul.ko has higher >> priority than crct10dif.ko , mkinitramfs will not include >> "modprobe crct10dif-pclmul" line in the generated initramfs. >> >> (2) In order to get benefit from PCLMULQDQ, users have to manually make sure >> that "modprobe crct10dif-pclmul" is called before crc-t10dif.ko (which is >> loaded before sd_mod.ko is loaded) is loaded by their initramfs's /init >> script. >> >> (3) The cause of (1) is that crct10dif-pclmul.ko will not be loaded >> automatically unless choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. >> >> (4) The cause of (3) is that modules.dep does not describe that users will >> benefit by loading crct10dif-pclmul.ko before loading crc-t10dif.ko . >> >> (5) Currently crct10dif-pclmul.ko cannot be loaded if PCLMULQDQ is not >> supported. This leads to boot failure (since sd_mod.ko cannot be loaded) >> if modules.dep says that "crct10dif-pclmul.ko is required by >> crc-t10dif.ko". >> >> (6) To solve (4) and (5), modules.dep should say "crct10dif-pclmul.ko is >> preferred for crc-t10dif.ko but is not required by crc-t10dif.ko". >> But there is no such mechanism. Thus, currently available choice is >> "allow loading crct10dif-pclmul.ko even if PCLMULQDQ is not supported" >> or "ignore errors by built-in the crct10dif-pclmul.ko module". >> >> My patch (b) seems to be complicated but is required in order to solve (4) >> without asking users to manually add "modprobe crct10dif-pclmul" into their >> initramfs. If we choose patch (b) rather than patch (a), we need to solve (5). >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ > --------------------------------------------------------------------- Intel Technology Poland sp. z o.o. z siedziba w Gdansku ul. Slowackiego 173 80-298 Gdansk Sad Rejonowy Gdansk Polnoc w Gdansku, VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, numer KRS 101882 NIP 957-07-52-316 Kapital zakladowy 200.000 zl This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-18 22:17 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-18 22:17 UTC (permalink / raw) To: Tim Chen; +Cc: Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak, rjw [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8"; format="flowed", Size: 6445 bytes --] On 7/18/2013 11:00 PM, Tim Chen wrote: > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: >> Tim Chen wrote: >>>>> Your approach is quite complicated. I think something simpler like the >>>>> following will work: >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? >>> >>> The following code in crct10dif-pclmul_glue.c >>> >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), >>> {} >>> }; >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); >>> >>> will put the module in the device table and get the module >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able >>> to benefit. >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > The code: > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > {} > }; > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > causes the following line to be added to modules.alias file: > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > This should cause udev to load the crct10dif_pclml module when cpu > support the PCLMULQDQ (feature code 0081). I did my testing during > development on 3.10 and the module was indeed loaded. > > However, I found that the following commit under 3.11-rc1 broke > the mechanism after some bisection. > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Date: Fri May 3 00:26:22 2013 +0200 > > ACPI / processor: Use common hotplug infrastructure > > Split the ACPI processor driver into two parts, one that is > non-modular, resides in the ACPI core and handles the enumeration > and hotplug of processors and one that implements the rest of the > existing processor driver functionality. > > Rafael, can you check and see if this can be fixed so those optimized > crypto modules for Intel cpu that support them can be loaded? I think this is an ordering issue between udev startup and the time when devices are registered. I wonder what happens if you put those modules into the initramfs image? Rafael > Herbert, we had a discussion earlier on a separate issue regarding the > module load order. We wanted to load all supported crypto t10dif modules > before the crc-t10dif module. You mentioned that the MODULE_ALIAS also > need some fixing and wonder if those changes have been merged into > 3.11-rc1? See https://lkml.org/lkml/2013/5/21/216 . Theoretically, that > fix should also get all the crypto modules support t10dif be loaded. > > Thanks. > > Tim > >> diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c >> index 7845d7f..a8a95aa 100644 >> --- a/arch/x86/crypto/crct10dif-pclmul_glue.c >> +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c >> @@ -129,9 +129,10 @@ MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); >> >> static int __init crct10dif_intel_mod_init(void) >> { >> + printk(KERN_WARNING "********** Checking for X86_FEATURE_PCLMULQDQ\n"); >> if (!x86_match_cpu(crct10dif_cpu_id)) >> return -ENODEV; >> - >> + printk(KERN_WARNING "********** Registering crct10dif-pclmul\n"); >> return crypto_register_shash(&alg); >> } >> >> As far as I tested, crct10dif-pclmul.ko will not be loaded unless manually >> adding "modprobe crct10dif-pclmul" to initramfs's /init or choosing >> CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. >> >>> So as long as the crct10dif.ko and crct10dif-pclmul.ko are loaded, >>> the pclmulqdq t10dif will have a higher priority and get allocated >>> and used. >> What I'm talking are >> >> (1) Since mkinitramfs is unable to know that crct10dif-pclmul.ko has higher >> priority than crct10dif.ko , mkinitramfs will not include >> "modprobe crct10dif-pclmul" line in the generated initramfs. >> >> (2) In order to get benefit from PCLMULQDQ, users have to manually make sure >> that "modprobe crct10dif-pclmul" is called before crc-t10dif.ko (which is >> loaded before sd_mod.ko is loaded) is loaded by their initramfs's /init >> script. >> >> (3) The cause of (1) is that crct10dif-pclmul.ko will not be loaded >> automatically unless choosing CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y. >> >> (4) The cause of (3) is that modules.dep does not describe that users will >> benefit by loading crct10dif-pclmul.ko before loading crc-t10dif.ko . >> >> (5) Currently crct10dif-pclmul.ko cannot be loaded if PCLMULQDQ is not >> supported. This leads to boot failure (since sd_mod.ko cannot be loaded) >> if modules.dep says that "crct10dif-pclmul.ko is required by >> crc-t10dif.ko". >> >> (6) To solve (4) and (5), modules.dep should say "crct10dif-pclmul.ko is >> preferred for crc-t10dif.ko but is not required by crc-t10dif.ko". >> But there is no such mechanism. Thus, currently available choice is >> "allow loading crct10dif-pclmul.ko even if PCLMULQDQ is not supported" >> or "ignore errors by built-in the crct10dif-pclmul.ko module". >> >> My patch (b) seems to be complicated but is required in order to solve (4) >> without asking users to manually add "modprobe crct10dif-pclmul" into their >> initramfs. If we choose patch (b) rather than patch (a), we need to solve (5). >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ > --------------------------------------------------------------------- Intel Technology Poland sp. z o.o. z siedziba w Gdansku ul. Slowackiego 173 80-298 Gdansk Sad Rejonowy Gdansk Polnoc w Gdansku, VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, numer KRS 101882 NIP 957-07-52-316 Kapital zakladowy 200.000 zl This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-18 22:17 ` Rafael J. Wysocki @ 2013-07-18 23:08 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-18 23:08 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak, rjw On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote: > On 7/18/2013 11:00 PM, Tim Chen wrote: > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > >> Tim Chen wrote: > >>>>> Your approach is quite complicated. I think something simpler like the > >>>>> following will work: > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? > >>> > >>> The following code in crct10dif-pclmul_glue.c > >>> > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > >>> {} > >>> }; > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > >>> > >>> will put the module in the device table and get the module > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able > >>> to benefit. > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > > The code: > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > {} > > }; > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > causes the following line to be added to modules.alias file: > > > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > > > This should cause udev to load the crct10dif_pclml module when cpu > > support the PCLMULQDQ (feature code 0081). I did my testing during > > development on 3.10 and the module was indeed loaded. > > > > However, I found that the following commit under 3.11-rc1 broke > > the mechanism after some bisection. > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Date: Fri May 3 00:26:22 2013 +0200 > > > > ACPI / processor: Use common hotplug infrastructure > > > > Split the ACPI processor driver into two parts, one that is > > non-modular, resides in the ACPI core and handles the enumeration > > and hotplug of processors and one that implements the rest of the > > existing processor driver functionality. > > > > Rafael, can you check and see if this can be fixed so those optimized > > crypto modules for Intel cpu that support them can be loaded? > > I think this is an ordering issue between udev startup and the time when > devices are registered. Something that can be fixed? > > I wonder what happens if you put those modules into the initramfs image? Under initramfs image's /lib/modules/3.11.0-rc1/kernel/arch/x86/crypto directory? Any files in /lib/modules/3.11.0-rc1/modules.* need to be modified? Tim ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-18 23:08 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-18 23:08 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak, rjw On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote: > On 7/18/2013 11:00 PM, Tim Chen wrote: > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > >> Tim Chen wrote: > >>>>> Your approach is quite complicated. I think something simpler like the > >>>>> following will work: > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? > >>> > >>> The following code in crct10dif-pclmul_glue.c > >>> > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > >>> {} > >>> }; > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > >>> > >>> will put the module in the device table and get the module > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able > >>> to benefit. > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > > The code: > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > {} > > }; > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > causes the following line to be added to modules.alias file: > > > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > > > This should cause udev to load the crct10dif_pclml module when cpu > > support the PCLMULQDQ (feature code 0081). I did my testing during > > development on 3.10 and the module was indeed loaded. > > > > However, I found that the following commit under 3.11-rc1 broke > > the mechanism after some bisection. > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Date: Fri May 3 00:26:22 2013 +0200 > > > > ACPI / processor: Use common hotplug infrastructure > > > > Split the ACPI processor driver into two parts, one that is > > non-modular, resides in the ACPI core and handles the enumeration > > and hotplug of processors and one that implements the rest of the > > existing processor driver functionality. > > > > Rafael, can you check and see if this can be fixed so those optimized > > crypto modules for Intel cpu that support them can be loaded? > > I think this is an ordering issue between udev startup and the time when > devices are registered. Something that can be fixed? > > I wonder what happens if you put those modules into the initramfs image? Under initramfs image's /lib/modules/3.11.0-rc1/kernel/arch/x86/crypto directory? Any files in /lib/modules/3.11.0-rc1/modules.* need to be modified? Tim ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-18 23:08 ` Tim Chen @ 2013-07-19 13:03 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-19 13:03 UTC (permalink / raw) To: Tim Chen Cc: Rafael J. Wysocki, Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak On Thursday, July 18, 2013 04:08:14 PM Tim Chen wrote: > On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote: > > On 7/18/2013 11:00 PM, Tim Chen wrote: > > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > > >> Tim Chen wrote: > > >>>>> Your approach is quite complicated. I think something simpler like the > > >>>>> following will work: > > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > >>> > > >>> The following code in crct10dif-pclmul_glue.c > > >>> > > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { > > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > >>> {} > > >>> }; > > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > >>> > > >>> will put the module in the device table and get the module > > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able > > >>> to benefit. > > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > > > The code: > > > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > > {} > > > }; > > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > > > causes the following line to be added to modules.alias file: > > > > > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > development on 3.10 and the module was indeed loaded. > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > the mechanism after some bisection. > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > Split the ACPI processor driver into two parts, one that is > > > non-modular, resides in the ACPI core and handles the enumeration > > > and hotplug of processors and one that implements the rest of the > > > existing processor driver functionality. > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > crypto modules for Intel cpu that support them can be loaded? > > > > I think this is an ordering issue between udev startup and the time when > > devices are registered. > > Something that can be fixed? I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real breakage in the kernel yet. I need to figure out that part. > > I wonder what happens if you put those modules into the initramfs image? > > Under initramfs image's /lib/modules/3.11.0-rc1/kernel/arch/x86/crypto > directory? Any files in /lib/modules/3.11.0-rc1/modules.* need to be > modified? No, you shouldn't need to modify any files, just please try to make your mkinitrd (or whatever else you use) put the modules you'd like to be autoloaded into the image. That's only something I'd like to know at this point, though, not a "workaround" or something like that. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-19 13:03 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-19 13:03 UTC (permalink / raw) To: Tim Chen Cc: Rafael J. Wysocki, Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak On Thursday, July 18, 2013 04:08:14 PM Tim Chen wrote: > On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote: > > On 7/18/2013 11:00 PM, Tim Chen wrote: > > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > > >> Tim Chen wrote: > > >>>>> Your approach is quite complicated. I think something simpler like the > > >>>>> following will work: > > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > >>> > > >>> The following code in crct10dif-pclmul_glue.c > > >>> > > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { > > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > >>> {} > > >>> }; > > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > >>> > > >>> will put the module in the device table and get the module > > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able > > >>> to benefit. > > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > > > The code: > > > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > > {} > > > }; > > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > > > causes the following line to be added to modules.alias file: > > > > > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > development on 3.10 and the module was indeed loaded. > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > the mechanism after some bisection. > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > Split the ACPI processor driver into two parts, one that is > > > non-modular, resides in the ACPI core and handles the enumeration > > > and hotplug of processors and one that implements the rest of the > > > existing processor driver functionality. > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > crypto modules for Intel cpu that support them can be loaded? > > > > I think this is an ordering issue between udev startup and the time when > > devices are registered. > > Something that can be fixed? I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real breakage in the kernel yet. I need to figure out that part. > > I wonder what happens if you put those modules into the initramfs image? > > Under initramfs image's /lib/modules/3.11.0-rc1/kernel/arch/x86/crypto > directory? Any files in /lib/modules/3.11.0-rc1/modules.* need to be > modified? No, you shouldn't need to modify any files, just please try to make your mkinitrd (or whatever else you use) put the modules you'd like to be autoloaded into the image. That's only something I'd like to know at this point, though, not a "workaround" or something like that. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 13:03 ` Rafael J. Wysocki @ 2013-07-19 14:49 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-19 14:49 UTC (permalink / raw) To: Tim Chen Cc: Rafael J. Wysocki, Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak, H. Peter Anvin, Greg Kroah-Hartman On Friday, July 19, 2013 03:03:36 PM Rafael J. Wysocki wrote: > On Thursday, July 18, 2013 04:08:14 PM Tim Chen wrote: > > On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote: > > > On 7/18/2013 11:00 PM, Tim Chen wrote: > > > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > > > >> Tim Chen wrote: > > > >>>>> Your approach is quite complicated. I think something simpler like the > > > >>>>> following will work: > > > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > > >>> > > > >>> The following code in crct10dif-pclmul_glue.c > > > >>> > > > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { > > > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > > >>> {} > > > >>> }; > > > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > >>> > > > >>> will put the module in the device table and get the module > > > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able > > > >>> to benefit. > > > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > > > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > > > > The code: > > > > > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > > > {} > > > > }; > > > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > > > > > causes the following line to be added to modules.alias file: > > > > > > > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > > > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > > development on 3.10 and the module was indeed loaded. > > > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > > the mechanism after some bisection. > > > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > > > Split the ACPI processor driver into two parts, one that is > > > > non-modular, resides in the ACPI core and handles the enumeration > > > > and hotplug of processors and one that implements the rest of the > > > > existing processor driver functionality. > > > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > > crypto modules for Intel cpu that support them can be loaded? > > > > > > I think this is an ordering issue between udev startup and the time when > > > devices are registered. > > > > Something that can be fixed? > > I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real > breakage in the kernel yet. I need to figure out that part. OK I wonder if the appended patch makes the issue go away for you? Rafael --- drivers/acpi/acpi_platform.c | 24 +++++++++++++----------- drivers/acpi/acpi_processor.c | 14 ++++---------- drivers/acpi/processor_driver.c | 26 ++++++++++++++------------ 3 files changed, 31 insertions(+), 33 deletions(-) Index: linux-pm/drivers/acpi/acpi_processor.c =================================================================== --- linux-pm.orig/drivers/acpi/acpi_processor.c +++ linux-pm/drivers/acpi/acpi_processor.c @@ -397,20 +397,14 @@ static int __cpuinit acpi_processor_add( result = -ENODEV; goto err; } - - result = acpi_bind_one(dev, pr->handle); - if (result) - goto err; - pr->dev = dev; dev->offline = pr->flags.need_hotplug_init; - /* Trigger the processor driver's .probe() if present. */ - if (device_attach(dev) >= 0) - return 1; + result = acpi_create_platform_device(device, id); + if (result > 0) + return result; - dev_err(dev, "Processor driver could not be attached\n"); - acpi_unbind_one(dev); + dev_err(dev, "Unable to create processor platform device\n"); err: free_cpumask_var(pr->throttling.shared_cpu_map); Index: linux-pm/drivers/acpi/acpi_platform.c =================================================================== --- linux-pm.orig/drivers/acpi/acpi_platform.c +++ linux-pm/drivers/acpi/acpi_platform.c @@ -52,7 +52,7 @@ int acpi_create_platform_device(struct a struct platform_device_info pdevinfo; struct resource_list_entry *rentry; struct list_head resource_list; - struct resource *resources; + struct resource *resources = NULL; int count; /* If the ACPI node already has a physical device attached, skip it. */ @@ -61,20 +61,22 @@ int acpi_create_platform_device(struct a INIT_LIST_HEAD(&resource_list); count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); - if (count <= 0) + if (count < 0) { return 0; + } else if (count > 0) { + resources = kmalloc(count * sizeof(struct resource), + GFP_KERNEL); + if (!resources) { + dev_err(&adev->dev, "No memory for resources\n"); + acpi_dev_free_resource_list(&resource_list); + return -ENOMEM; + } + count = 0; + list_for_each_entry(rentry, &resource_list, node) + resources[count++] = rentry->res; - resources = kmalloc(count * sizeof(struct resource), GFP_KERNEL); - if (!resources) { - dev_err(&adev->dev, "No memory for resources\n"); acpi_dev_free_resource_list(&resource_list); - return -ENOMEM; } - count = 0; - list_for_each_entry(rentry, &resource_list, node) - resources[count++] = rentry->res; - - acpi_dev_free_resource_list(&resource_list); memset(&pdevinfo, 0, sizeof(pdevinfo)); /* Index: linux-pm/drivers/acpi/processor_driver.c =================================================================== --- linux-pm.orig/drivers/acpi/processor_driver.c +++ linux-pm/drivers/acpi/processor_driver.c @@ -36,6 +36,7 @@ #include <linux/cpuidle.h> #include <linux/slab.h> #include <linux/acpi.h> +#include <linux/platform_device.h> #include <acpi/processor.h> @@ -54,8 +55,8 @@ MODULE_AUTHOR("Paul Diefenbaugh"); MODULE_DESCRIPTION("ACPI Processor Driver"); MODULE_LICENSE("GPL"); -static int acpi_processor_start(struct device *dev); -static int acpi_processor_stop(struct device *dev); +static int acpi_processor_start(struct platform_device *pdev); +static int acpi_processor_stop(struct platform_device *pdev); static const struct acpi_device_id processor_device_ids[] = { {ACPI_PROCESSOR_OBJECT_HID, 0}, @@ -64,10 +65,11 @@ static const struct acpi_device_id proce }; MODULE_DEVICE_TABLE(acpi, processor_device_ids); -static struct device_driver acpi_processor_driver = { - .name = "processor", - .bus = &cpu_subsys, - .acpi_match_table = processor_device_ids, +static struct platform_driver acpi_processor_driver = { + .driver = { + .name = "processor", + .acpi_match_table = processor_device_ids, + }, .probe = acpi_processor_start, .remove = acpi_processor_stop, }; @@ -222,22 +224,22 @@ static __cpuinit int __acpi_processor_st return result; } -static int __cpuinit acpi_processor_start(struct device *dev) +static int __cpuinit acpi_processor_start(struct platform_device *pdev) { struct acpi_device *device; - if (acpi_bus_get_device(ACPI_HANDLE(dev), &device)) + if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &device)) return -ENODEV; return __acpi_processor_start(device); } -static int acpi_processor_stop(struct device *dev) +static int acpi_processor_stop(struct platform_device *pdev) { struct acpi_device *device; struct acpi_processor *pr; - if (acpi_bus_get_device(ACPI_HANDLE(dev), &device)) + if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &device)) return 0; acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, @@ -271,7 +273,7 @@ static int __init acpi_processor_driver_ if (acpi_disabled) return 0; - result = driver_register(&acpi_processor_driver); + result = platform_driver_register(&acpi_processor_driver); if (result < 0) return result; @@ -292,7 +294,7 @@ static void __exit acpi_processor_driver acpi_thermal_cpufreq_exit(); unregister_hotcpu_notifier(&acpi_cpu_notifier); acpi_processor_syscore_exit(); - driver_unregister(&acpi_processor_driver); + platform_driver_unregister(&acpi_processor_driver); } module_init(acpi_processor_driver_init); ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-19 14:49 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-19 14:49 UTC (permalink / raw) To: Tim Chen Cc: Rafael J. Wysocki, Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak, H. Peter Anvin, Greg Kroah-Hartman On Friday, July 19, 2013 03:03:36 PM Rafael J. Wysocki wrote: > On Thursday, July 18, 2013 04:08:14 PM Tim Chen wrote: > > On Fri, 2013-07-19 at 00:17 +0200, Rafael J. Wysocki wrote: > > > On 7/18/2013 11:00 PM, Tim Chen wrote: > > > > On Thu, 2013-07-18 at 12:47 +0900, Tetsuo Handa wrote: > > > >> Tim Chen wrote: > > > >>>>> Your approach is quite complicated. I think something simpler like the > > > >>>>> following will work: > > > >>>> We cannot benefit from PCLMULQDQ. Is it acceptable for you? > > > >>> > > > >>> The following code in crct10dif-pclmul_glue.c > > > >>> > > > >>> static const struct x86_cpu_id crct10dif_cpu_id[] = { > > > >>> X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > > >>> {} > > > >>> }; > > > >>> MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > >>> > > > >>> will put the module in the device table and get the module > > > >>> loaded, as long as the cpu support PCLMULQDQ. So we should be able > > > >>> to benefit. > > > >> Excuse me, how can crct10dif-pclmul.ko get loaded automatically? > > > >> Did you test CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m with below debug message? > > > > The code: > > > > > > > > static const struct x86_cpu_id crct10dif_cpu_id[] = { > > > > X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), > > > > {} > > > > }; > > > > MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); > > > > > > > > causes the following line to be added to modules.alias file: > > > > > > > > alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > > > > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > > development on 3.10 and the module was indeed loaded. > > > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > > the mechanism after some bisection. > > > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > > > Split the ACPI processor driver into two parts, one that is > > > > non-modular, resides in the ACPI core and handles the enumeration > > > > and hotplug of processors and one that implements the rest of the > > > > existing processor driver functionality. > > > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > > crypto modules for Intel cpu that support them can be loaded? > > > > > > I think this is an ordering issue between udev startup and the time when > > > devices are registered. > > > > Something that can be fixed? > > I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real > breakage in the kernel yet. I need to figure out that part. OK I wonder if the appended patch makes the issue go away for you? Rafael --- drivers/acpi/acpi_platform.c | 24 +++++++++++++----------- drivers/acpi/acpi_processor.c | 14 ++++---------- drivers/acpi/processor_driver.c | 26 ++++++++++++++------------ 3 files changed, 31 insertions(+), 33 deletions(-) Index: linux-pm/drivers/acpi/acpi_processor.c =================================================================== --- linux-pm.orig/drivers/acpi/acpi_processor.c +++ linux-pm/drivers/acpi/acpi_processor.c @@ -397,20 +397,14 @@ static int __cpuinit acpi_processor_add( result = -ENODEV; goto err; } - - result = acpi_bind_one(dev, pr->handle); - if (result) - goto err; - pr->dev = dev; dev->offline = pr->flags.need_hotplug_init; - /* Trigger the processor driver's .probe() if present. */ - if (device_attach(dev) >= 0) - return 1; + result = acpi_create_platform_device(device, id); + if (result > 0) + return result; - dev_err(dev, "Processor driver could not be attached\n"); - acpi_unbind_one(dev); + dev_err(dev, "Unable to create processor platform device\n"); err: free_cpumask_var(pr->throttling.shared_cpu_map); Index: linux-pm/drivers/acpi/acpi_platform.c =================================================================== --- linux-pm.orig/drivers/acpi/acpi_platform.c +++ linux-pm/drivers/acpi/acpi_platform.c @@ -52,7 +52,7 @@ int acpi_create_platform_device(struct a struct platform_device_info pdevinfo; struct resource_list_entry *rentry; struct list_head resource_list; - struct resource *resources; + struct resource *resources = NULL; int count; /* If the ACPI node already has a physical device attached, skip it. */ @@ -61,20 +61,22 @@ int acpi_create_platform_device(struct a INIT_LIST_HEAD(&resource_list); count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); - if (count <= 0) + if (count < 0) { return 0; + } else if (count > 0) { + resources = kmalloc(count * sizeof(struct resource), + GFP_KERNEL); + if (!resources) { + dev_err(&adev->dev, "No memory for resources\n"); + acpi_dev_free_resource_list(&resource_list); + return -ENOMEM; + } + count = 0; + list_for_each_entry(rentry, &resource_list, node) + resources[count++] = rentry->res; - resources = kmalloc(count * sizeof(struct resource), GFP_KERNEL); - if (!resources) { - dev_err(&adev->dev, "No memory for resources\n"); acpi_dev_free_resource_list(&resource_list); - return -ENOMEM; } - count = 0; - list_for_each_entry(rentry, &resource_list, node) - resources[count++] = rentry->res; - - acpi_dev_free_resource_list(&resource_list); memset(&pdevinfo, 0, sizeof(pdevinfo)); /* Index: linux-pm/drivers/acpi/processor_driver.c =================================================================== --- linux-pm.orig/drivers/acpi/processor_driver.c +++ linux-pm/drivers/acpi/processor_driver.c @@ -36,6 +36,7 @@ #include <linux/cpuidle.h> #include <linux/slab.h> #include <linux/acpi.h> +#include <linux/platform_device.h> #include <acpi/processor.h> @@ -54,8 +55,8 @@ MODULE_AUTHOR("Paul Diefenbaugh"); MODULE_DESCRIPTION("ACPI Processor Driver"); MODULE_LICENSE("GPL"); -static int acpi_processor_start(struct device *dev); -static int acpi_processor_stop(struct device *dev); +static int acpi_processor_start(struct platform_device *pdev); +static int acpi_processor_stop(struct platform_device *pdev); static const struct acpi_device_id processor_device_ids[] = { {ACPI_PROCESSOR_OBJECT_HID, 0}, @@ -64,10 +65,11 @@ static const struct acpi_device_id proce }; MODULE_DEVICE_TABLE(acpi, processor_device_ids); -static struct device_driver acpi_processor_driver = { - .name = "processor", - .bus = &cpu_subsys, - .acpi_match_table = processor_device_ids, +static struct platform_driver acpi_processor_driver = { + .driver = { + .name = "processor", + .acpi_match_table = processor_device_ids, + }, .probe = acpi_processor_start, .remove = acpi_processor_stop, }; @@ -222,22 +224,22 @@ static __cpuinit int __acpi_processor_st return result; } -static int __cpuinit acpi_processor_start(struct device *dev) +static int __cpuinit acpi_processor_start(struct platform_device *pdev) { struct acpi_device *device; - if (acpi_bus_get_device(ACPI_HANDLE(dev), &device)) + if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &device)) return -ENODEV; return __acpi_processor_start(device); } -static int acpi_processor_stop(struct device *dev) +static int acpi_processor_stop(struct platform_device *pdev) { struct acpi_device *device; struct acpi_processor *pr; - if (acpi_bus_get_device(ACPI_HANDLE(dev), &device)) + if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &device)) return 0; acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, @@ -271,7 +273,7 @@ static int __init acpi_processor_driver_ if (acpi_disabled) return 0; - result = driver_register(&acpi_processor_driver); + result = platform_driver_register(&acpi_processor_driver); if (result < 0) return result; @@ -292,7 +294,7 @@ static void __exit acpi_processor_driver acpi_thermal_cpufreq_exit(); unregister_hotcpu_notifier(&acpi_cpu_notifier); acpi_processor_syscore_exit(); - driver_unregister(&acpi_processor_driver); + platform_driver_unregister(&acpi_processor_driver); } module_init(acpi_processor_driver_init); ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 14:49 ` Rafael J. Wysocki @ 2013-07-19 18:08 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-19 18:08 UTC (permalink / raw) To: Rafael J. Wysocki, Herbert Xu Cc: Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, Greg Kroah-Hartman On Fri, 2013-07-19 at 16:49 +0200, Rafael J. Wysocki wrote: > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > > > development on 3.10 and the module was indeed loaded. > > > > > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > > > the mechanism after some bisection. > > > > > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > > > > > Split the ACPI processor driver into two parts, one that is > > > > > non-modular, resides in the ACPI core and handles the enumeration > > > > > and hotplug of processors and one that implements the rest of the > > > > > existing processor driver functionality. > > > > > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > > > crypto modules for Intel cpu that support them can be loaded? > > > > > > > > I think this is an ordering issue between udev startup and the time when > > > > devices are registered. > > > > > > Something that can be fixed? > > > > I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real > > breakage in the kernel yet. I need to figure out that part. > > OK > > I wonder if the appended patch makes the issue go away for you? > Rafael, the patch did fix the cpu feature based module loading issue. Thanks for your quick response. Herbert, the patch below should fix the boot failure issue. I also added the change to rename crct10dif.c to crct10dif_generic.c and added MODULE_ALIAS(crct10dif) to crct10dif_generic.c. This was according to our discussion: https://lkml.org/lkml/2013/5/21/216 However, when I have the CONFIG_CRYPTO_CRCT10DIF=y CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m CONFIG_CRC_T10DIF=y combination, the crc-t10dif library function was still initialized before the crct10dif-pclmul.ko was loaded, leading to the generic version being used. This is also one of Tetsuo's concerns. In theory, loading the generic version should also load the pclmul version as they have the same MODULE_ALIAS, before crc-t10dif library try to allocate the crypto transform. Something else that I'm missing and need to be changed? I will be away for a week without internet access so my response will be slow. Thanks. Tim Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com> --- Fix boot failure as crc-t10dif library may not cause crct10dif.ko containing the generic algorithm to be loaded. Rename crct10dif.c to crct10dif_generic.c and add module alias. This should load all the modules supporting crct10dif algorithm at the same time. --- crypto/Makefile | 2 +- crypto/{crct10dif.c => crct10dif_generic.c} | 1 + lib/crc-t10dif.c | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) rename crypto/{crct10dif.c => crct10dif_generic.c} (99%) diff --git a/crypto/Makefile b/crypto/Makefile index 2d5ed08..3fd76fa 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -83,7 +83,7 @@ obj-$(CONFIG_CRYPTO_ZLIB) += zlib.o obj-$(CONFIG_CRYPTO_MICHAEL_MIC) += michael_mic.o obj-$(CONFIG_CRYPTO_CRC32C) += crc32c.o obj-$(CONFIG_CRYPTO_CRC32) += crc32.o -obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif.o +obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif_generic.o obj-$(CONFIG_CRYPTO_AUTHENC) += authenc.o authencesn.o obj-$(CONFIG_CRYPTO_LZO) += lzo.o obj-$(CONFIG_CRYPTO_LZ4) += lz4.o diff --git a/crypto/crct10dif.c b/crypto/crct10dif_generic.c similarity index 99% rename from crypto/crct10dif.c rename to crypto/crct10dif_generic.c index 92aca96..45a9acd 100644 --- a/crypto/crct10dif.c +++ b/crypto/crct10dif_generic.c @@ -176,3 +176,4 @@ module_exit(crct10dif_mod_fini); MODULE_AUTHOR("Tim Chen <tim.c.chen@linux.intel.com>"); MODULE_DESCRIPTION("T10 DIF CRC calculation."); MODULE_LICENSE("GPL"); +MODULE_ALIAS(crct10dif); diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index d102d83..37b3eb4 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -15,7 +15,8 @@ #include <linux/init.h> #include <crypto/hash.h> -static struct crypto_shash *crct10dif_tfm; +__u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, size_t len); +static struct crypto_shash *crct10dif_tfm = crc_t10dif_generic; __u16 crc_t10dif(const unsigned char *buffer, size_t len) { -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-19 18:08 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-19 18:08 UTC (permalink / raw) To: Rafael J. Wysocki, Herbert Xu Cc: Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, Greg Kroah-Hartman On Fri, 2013-07-19 at 16:49 +0200, Rafael J. Wysocki wrote: > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > > > development on 3.10 and the module was indeed loaded. > > > > > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > > > the mechanism after some bisection. > > > > > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > > > > > Split the ACPI processor driver into two parts, one that is > > > > > non-modular, resides in the ACPI core and handles the enumeration > > > > > and hotplug of processors and one that implements the rest of the > > > > > existing processor driver functionality. > > > > > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > > > crypto modules for Intel cpu that support them can be loaded? > > > > > > > > I think this is an ordering issue between udev startup and the time when > > > > devices are registered. > > > > > > Something that can be fixed? > > > > I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real > > breakage in the kernel yet. I need to figure out that part. > > OK > > I wonder if the appended patch makes the issue go away for you? > Rafael, the patch did fix the cpu feature based module loading issue. Thanks for your quick response. Herbert, the patch below should fix the boot failure issue. I also added the change to rename crct10dif.c to crct10dif_generic.c and added MODULE_ALIAS(crct10dif) to crct10dif_generic.c. This was according to our discussion: https://lkml.org/lkml/2013/5/21/216 However, when I have the CONFIG_CRYPTO_CRCT10DIF=y CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m CONFIG_CRC_T10DIF=y combination, the crc-t10dif library function was still initialized before the crct10dif-pclmul.ko was loaded, leading to the generic version being used. This is also one of Tetsuo's concerns. In theory, loading the generic version should also load the pclmul version as they have the same MODULE_ALIAS, before crc-t10dif library try to allocate the crypto transform. Something else that I'm missing and need to be changed? I will be away for a week without internet access so my response will be slow. Thanks. Tim Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com> --- Fix boot failure as crc-t10dif library may not cause crct10dif.ko containing the generic algorithm to be loaded. Rename crct10dif.c to crct10dif_generic.c and add module alias. This should load all the modules supporting crct10dif algorithm at the same time. --- crypto/Makefile | 2 +- crypto/{crct10dif.c => crct10dif_generic.c} | 1 + lib/crc-t10dif.c | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) rename crypto/{crct10dif.c => crct10dif_generic.c} (99%) diff --git a/crypto/Makefile b/crypto/Makefile index 2d5ed08..3fd76fa 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -83,7 +83,7 @@ obj-$(CONFIG_CRYPTO_ZLIB) += zlib.o obj-$(CONFIG_CRYPTO_MICHAEL_MIC) += michael_mic.o obj-$(CONFIG_CRYPTO_CRC32C) += crc32c.o obj-$(CONFIG_CRYPTO_CRC32) += crc32.o -obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif.o +obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif_generic.o obj-$(CONFIG_CRYPTO_AUTHENC) += authenc.o authencesn.o obj-$(CONFIG_CRYPTO_LZO) += lzo.o obj-$(CONFIG_CRYPTO_LZ4) += lz4.o diff --git a/crypto/crct10dif.c b/crypto/crct10dif_generic.c similarity index 99% rename from crypto/crct10dif.c rename to crypto/crct10dif_generic.c index 92aca96..45a9acd 100644 --- a/crypto/crct10dif.c +++ b/crypto/crct10dif_generic.c @@ -176,3 +176,4 @@ module_exit(crct10dif_mod_fini); MODULE_AUTHOR("Tim Chen <tim.c.chen@linux.intel.com>"); MODULE_DESCRIPTION("T10 DIF CRC calculation."); MODULE_LICENSE("GPL"); +MODULE_ALIAS(crct10dif); diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index d102d83..37b3eb4 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -15,7 +15,8 @@ #include <linux/init.h> #include <crypto/hash.h> -static struct crypto_shash *crct10dif_tfm; +__u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, size_t len); +static struct crypto_shash *crct10dif_tfm = crc_t10dif_generic; __u16 crc_t10dif(const unsigned char *buffer, size_t len) { -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 18:08 ` Tim Chen @ 2013-07-19 21:38 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-19 21:38 UTC (permalink / raw) To: Tim Chen, Greg Kroah-Hartman Cc: Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Friday, July 19, 2013 11:08:49 AM Tim Chen wrote: > On Fri, 2013-07-19 at 16:49 +0200, Rafael J. Wysocki wrote: > > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > > > > development on 3.10 and the module was indeed loaded. > > > > > > > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > > > > the mechanism after some bisection. > > > > > > > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > > > > > > > Split the ACPI processor driver into two parts, one that is > > > > > > non-modular, resides in the ACPI core and handles the enumeration > > > > > > and hotplug of processors and one that implements the rest of the > > > > > > existing processor driver functionality. > > > > > > > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > > > > crypto modules for Intel cpu that support them can be loaded? > > > > > > > > > > I think this is an ordering issue between udev startup and the time when > > > > > devices are registered. > > > > > > > > Something that can be fixed? > > > > > > I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real > > > breakage in the kernel yet. I need to figure out that part. > > > > OK > > > > I wonder if the appended patch makes the issue go away for you? > > > > Rafael, the patch did fix the cpu feature based module loading issue. > Thanks for your quick response. Alas, this is not the one I'd like to apply. With that patch applied, new device objects are created to avoid binding the processor driver directly to the cpu system device objects, because that apparently confuses udev and it starts to ignore the cpu modalias once the driver has been bound to any of those objects. I've verified in the meantime that this indeed is the case. A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ Greg, I asked you some time ago whether or not it was possible for udev to stop autoloading modules that matched the cpu modalias after a driver had been bound to the cpu system device objects and you said "no". However, this time I can say with certainty that that really is the case. So, the question now is whether or not we can do anything in the kernel to avoid that confusion in udev instead of applying the patch linked above (which is beyond ugly in my not so humble opinion)? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-19 21:38 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-19 21:38 UTC (permalink / raw) To: Tim Chen, Greg Kroah-Hartman Cc: Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Friday, July 19, 2013 11:08:49 AM Tim Chen wrote: > On Fri, 2013-07-19 at 16:49 +0200, Rafael J. Wysocki wrote: > > > > > > This should cause udev to load the crct10dif_pclml module when cpu > > > > > > support the PCLMULQDQ (feature code 0081). I did my testing during > > > > > > development on 3.10 and the module was indeed loaded. > > > > > > > > > > > > However, I found that the following commit under 3.11-rc1 broke > > > > > > the mechanism after some bisection. > > > > > > > > > > > > commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > > > > > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Date: Fri May 3 00:26:22 2013 +0200 > > > > > > > > > > > > ACPI / processor: Use common hotplug infrastructure > > > > > > > > > > > > Split the ACPI processor driver into two parts, one that is > > > > > > non-modular, resides in the ACPI core and handles the enumeration > > > > > > and hotplug of processors and one that implements the rest of the > > > > > > existing processor driver functionality. > > > > > > > > > > > > Rafael, can you check and see if this can be fixed so those optimized > > > > > > crypto modules for Intel cpu that support them can be loaded? > > > > > > > > > > I think this is an ordering issue between udev startup and the time when > > > > > devices are registered. > > > > > > > > Something that can be fixed? > > > > > > I'm sure it can be fixes, but I'm not sure whether it's a udev bug or real > > > breakage in the kernel yet. I need to figure out that part. > > > > OK > > > > I wonder if the appended patch makes the issue go away for you? > > > > Rafael, the patch did fix the cpu feature based module loading issue. > Thanks for your quick response. Alas, this is not the one I'd like to apply. With that patch applied, new device objects are created to avoid binding the processor driver directly to the cpu system device objects, because that apparently confuses udev and it starts to ignore the cpu modalias once the driver has been bound to any of those objects. I've verified in the meantime that this indeed is the case. A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ Greg, I asked you some time ago whether or not it was possible for udev to stop autoloading modules that matched the cpu modalias after a driver had been bound to the cpu system device objects and you said "no". However, this time I can say with certainty that that really is the case. So, the question now is whether or not we can do anything in the kernel to avoid that confusion in udev instead of applying the patch linked above (which is beyond ugly in my not so humble opinion)? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 21:38 ` Rafael J. Wysocki @ 2013-07-19 23:16 ` Greg Kroah-Hartman -1 siblings, 0 replies; 72+ messages in thread From: Greg Kroah-Hartman @ 2013-07-19 23:16 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > Alas, this is not the one I'd like to apply. > > With that patch applied, new device objects are created to avoid binding the > processor driver directly to the cpu system device objects, because that > apparently confuses udev and it starts to ignore the cpu modalias once the > driver has been bound to any of those objects. > > I've verified in the meantime that this indeed is the case. > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > Greg, I asked you some time ago whether or not it was possible for udev to stop > autoloading modules that matched the cpu modalias after a driver had been bound > to the cpu system device objects and you said "no". However, this time I can > say with certainty that that really is the case. So, the question now is > whether or not we can do anything in the kernel to avoid that confusion in udev > instead of applying the patch linked above (which is beyond ugly in my not so > humble opinion)? udev isn't doing any module loading, 'modprobe' is just being called for any new module alias that shows up in the system, and all of the drivers that match it then get loaded. How is it a problem if a module is attempted to be loaded that is already loaded? How is it a problem if a different module is loaded for a device already bound to a driver? Both of those should be total "no-ops" for the kernel. But, I don't know anything about the cpu code, how is loading a module causing problems? That sounds like it needs to be fixes, as any root user can load modules whenever they want, you can't protect the kernel from doing that. thanks, greg k-h ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-19 23:16 ` Greg Kroah-Hartman 0 siblings, 0 replies; 72+ messages in thread From: Greg Kroah-Hartman @ 2013-07-19 23:16 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > Alas, this is not the one I'd like to apply. > > With that patch applied, new device objects are created to avoid binding the > processor driver directly to the cpu system device objects, because that > apparently confuses udev and it starts to ignore the cpu modalias once the > driver has been bound to any of those objects. > > I've verified in the meantime that this indeed is the case. > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > Greg, I asked you some time ago whether or not it was possible for udev to stop > autoloading modules that matched the cpu modalias after a driver had been bound > to the cpu system device objects and you said "no". However, this time I can > say with certainty that that really is the case. So, the question now is > whether or not we can do anything in the kernel to avoid that confusion in udev > instead of applying the patch linked above (which is beyond ugly in my not so > humble opinion)? udev isn't doing any module loading, 'modprobe' is just being called for any new module alias that shows up in the system, and all of the drivers that match it then get loaded. How is it a problem if a module is attempted to be loaded that is already loaded? How is it a problem if a different module is loaded for a device already bound to a driver? Both of those should be total "no-ops" for the kernel. But, I don't know anything about the cpu code, how is loading a module causing problems? That sounds like it needs to be fixes, as any root user can load modules whenever they want, you can't protect the kernel from doing that. thanks, greg k-h ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 23:16 ` Greg Kroah-Hartman (?) @ 2013-07-19 23:21 ` H. Peter Anvin 2013-07-19 23:24 ` Herbert Xu 2013-07-19 23:26 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Greg Kroah-Hartman -1 siblings, 2 replies; 72+ messages in thread From: H. Peter Anvin @ 2013-07-19 23:21 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rafael J. Wysocki, Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On 07/19/2013 04:16 PM, Greg Kroah-Hartman wrote: > > udev isn't doing any module loading, 'modprobe' is just being called for > any new module alias that shows up in the system, and all of the drivers > that match it then get loaded. > > How is it a problem if a module is attempted to be loaded that is > already loaded? How is it a problem if a different module is loaded for > a device already bound to a driver? Both of those should be total > "no-ops" for the kernel. > > But, I don't know anything about the cpu code, how is loading a module > causing problems? That sounds like it needs to be fixes, as any root > user can load modules whenever they want, you can't protect the kernel > from doing that. > The issue here seems to be the dynamic binding nature of the crypto subsystem. When something needs crypto, it will request the appropriate crypto module (e.g. crct10dif), which may race with detecting a specific hardware accelerator based on CPUID or device information (e.g. crct10dif_pclmul). RAID has effectively the same issue, and we just "solved" it by compiling in all the accelerators into the top-level module. -hpa ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 23:21 ` H. Peter Anvin @ 2013-07-19 23:24 ` Herbert Xu 2013-07-19 23:26 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Greg Kroah-Hartman 1 sibling, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-07-19 23:24 UTC (permalink / raw) To: H. Peter Anvin Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Tim Chen, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Fri, Jul 19, 2013 at 04:21:09PM -0700, H. Peter Anvin wrote: > > The issue here seems to be the dynamic binding nature of the crypto > subsystem. When something needs crypto, it will request the appropriate > crypto module (e.g. crct10dif), which may race with detecting a specific > hardware accelerator based on CPUID or device information (e.g. > crct10dif_pclmul). > > RAID has effectively the same issue, and we just "solved" it by > compiling in all the accelerators into the top-level module. I think for crypto the simplest solution is to not do CPUID-based loading. Then crypto users will simply load the module alias which causes modprobe to load all modules providing that alias. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-19 23:24 ` Herbert Xu 0 siblings, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-07-19 23:24 UTC (permalink / raw) To: H. Peter Anvin Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Tim Chen, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Fri, Jul 19, 2013 at 04:21:09PM -0700, H. Peter Anvin wrote: > > The issue here seems to be the dynamic binding nature of the crypto > subsystem. When something needs crypto, it will request the appropriate > crypto module (e.g. crct10dif), which may race with detecting a specific > hardware accelerator based on CPUID or device information (e.g. > crct10dif_pclmul). > > RAID has effectively the same issue, and we just "solved" it by > compiling in all the accelerators into the top-level module. I think for crypto the simplest solution is to not do CPUID-based loading. Then crypto users will simply load the module alias which causes modprobe to load all modules providing that alias. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 23:24 ` Herbert Xu @ 2013-07-19 23:37 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-19 23:37 UTC (permalink / raw) To: Herbert Xu Cc: H. Peter Anvin, Greg Kroah-Hartman, Rafael J. Wysocki, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Sat, 2013-07-20 at 09:24 +1000, Herbert Xu wrote: > On Fri, Jul 19, 2013 at 04:21:09PM -0700, H. Peter Anvin wrote: > > > > The issue here seems to be the dynamic binding nature of the crypto > > subsystem. When something needs crypto, it will request the appropriate > > crypto module (e.g. crct10dif), which may race with detecting a specific > > hardware accelerator based on CPUID or device information (e.g. > > crct10dif_pclmul). > > > > RAID has effectively the same issue, and we just "solved" it by > > compiling in all the accelerators into the top-level module. > > I think for crypto the simplest solution is to not do CPUID-based > loading. Then crypto users will simply load the module alias which > causes modprobe to load all modules providing that alias. > > Cheers, Herbert, I've tried the module alias approach (see my earlier mail with patch) but it didn't seem to load things properly. Can you check to see if there's anything I did incorrectly. Tim ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-19 23:37 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-19 23:37 UTC (permalink / raw) To: Herbert Xu Cc: H. Peter Anvin, Greg Kroah-Hartman, Rafael J. Wysocki, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Sat, 2013-07-20 at 09:24 +1000, Herbert Xu wrote: > On Fri, Jul 19, 2013 at 04:21:09PM -0700, H. Peter Anvin wrote: > > > > The issue here seems to be the dynamic binding nature of the crypto > > subsystem. When something needs crypto, it will request the appropriate > > crypto module (e.g. crct10dif), which may race with detecting a specific > > hardware accelerator based on CPUID or device information (e.g. > > crct10dif_pclmul). > > > > RAID has effectively the same issue, and we just "solved" it by > > compiling in all the accelerators into the top-level module. > > I think for crypto the simplest solution is to not do CPUID-based > loading. Then crypto users will simply load the module alias which > causes modprobe to load all modules providing that alias. > > Cheers, Herbert, I've tried the module alias approach (see my earlier mail with patch) but it didn't seem to load things properly. Can you check to see if there's anything I did incorrectly. Tim ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 23:37 ` Tim Chen @ 2013-07-20 1:31 ` Tim Chen -1 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-20 1:31 UTC (permalink / raw) To: Herbert Xu Cc: H. Peter Anvin, Greg Kroah-Hartman, Rafael J. Wysocki, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Fri, 2013-07-19 at 16:37 -0700, Tim Chen wrote: > On Sat, 2013-07-20 at 09:24 +1000, Herbert Xu wrote: > > On Fri, Jul 19, 2013 at 04:21:09PM -0700, H. Peter Anvin wrote: > > > > > > The issue here seems to be the dynamic binding nature of the crypto > > > subsystem. When something needs crypto, it will request the appropriate > > > crypto module (e.g. crct10dif), which may race with detecting a specific > > > hardware accelerator based on CPUID or device information (e.g. > > > crct10dif_pclmul). > > > > > > RAID has effectively the same issue, and we just "solved" it by > > > compiling in all the accelerators into the top-level module. > > > > I think for crypto the simplest solution is to not do CPUID-based > > loading. Then crypto users will simply load the module alias which > > causes modprobe to load all modules providing that alias. > > > > Cheers, > > Herbert, > > I've tried the module alias approach (see my earlier mail with patch) > but it didn't seem to load things properly. Can you check to see if > there's anything I did incorrectly. > > Tim I fixed a missing request_module statement in crct10dif library. So now things work if I have the following config: CONFIG_CRYPTO_CRCT10DIF=m CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m CONFIG_CRC_T10DIF=m However, when I have the library and generic algorithm compiled in, I do not see the PCLMULQDQ version loaded. CONFIG_CRYPTO_CRCT10DIF=y CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m CONFIG_CRC_T10DIF=y Perhaps I am initiating the crct10dif library at a really early stage when things are compiled in, where the module is not in initramfs? In that case, perhaps we should only allow PCLMUL version to be compiled in and not exist as a module? Here's the patch I tried: Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com> --- arch/x86/crypto/crct10dif-pclmul_glue.c | 8 +------- crypto/Makefile | 2 +- crypto/{crct10dif.c => crct10dif_generic.c} | 1 + lib/crc-t10dif.c | 1 + 4 files changed, 4 insertions(+), 8 deletions(-) rename crypto/{crct10dif.c => crct10dif_generic.c} (99%) diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c index 7845d7f..7ad5f09 100644 --- a/arch/x86/crypto/crct10dif-pclmul_glue.c +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c @@ -121,15 +121,9 @@ static struct shash_alg alg = { } }; -static const struct x86_cpu_id crct10dif_cpu_id[] = { - X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), - {} -}; -MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); - static int __init crct10dif_intel_mod_init(void) { - if (!x86_match_cpu(crct10dif_cpu_id)) + if (!cpu_has_pclmulqdq) return -ENODEV; return crypto_register_shash(&alg); diff --git a/crypto/Makefile b/crypto/Makefile index 2d5ed08..3fd76fa 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -83,7 +83,7 @@ obj-$(CONFIG_CRYPTO_ZLIB) += zlib.o obj-$(CONFIG_CRYPTO_MICHAEL_MIC) += michael_mic.o obj-$(CONFIG_CRYPTO_CRC32C) += crc32c.o obj-$(CONFIG_CRYPTO_CRC32) += crc32.o -obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif.o +obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif_generic.o obj-$(CONFIG_CRYPTO_AUTHENC) += authenc.o authencesn.o obj-$(CONFIG_CRYPTO_LZO) += lzo.o obj-$(CONFIG_CRYPTO_LZ4) += lz4.o diff --git a/crypto/crct10dif.c b/crypto/crct10dif_generic.c similarity index 99% rename from crypto/crct10dif.c rename to crypto/crct10dif_generic.c index 92aca96..c960a95 100644 --- a/crypto/crct10dif.c +++ b/crypto/crct10dif_generic.c @@ -176,3 +176,4 @@ module_exit(crct10dif_mod_fini); MODULE_AUTHOR("Tim Chen <tim.c.chen@linux.intel.com>"); MODULE_DESCRIPTION("T10 DIF CRC calculation."); MODULE_LICENSE("GPL"); +MODULE_ALIAS("crct10dif"); diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index fe3428c..d8cd353 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -38,6 +38,7 @@ EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void) { + request_module("crct10dif"); crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); return PTR_RET(crct10dif_tfm); } -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-20 1:31 ` Tim Chen 0 siblings, 0 replies; 72+ messages in thread From: Tim Chen @ 2013-07-20 1:31 UTC (permalink / raw) To: Herbert Xu Cc: H. Peter Anvin, Greg Kroah-Hartman, Rafael J. Wysocki, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Fri, 2013-07-19 at 16:37 -0700, Tim Chen wrote: > On Sat, 2013-07-20 at 09:24 +1000, Herbert Xu wrote: > > On Fri, Jul 19, 2013 at 04:21:09PM -0700, H. Peter Anvin wrote: > > > > > > The issue here seems to be the dynamic binding nature of the crypto > > > subsystem. When something needs crypto, it will request the appropriate > > > crypto module (e.g. crct10dif), which may race with detecting a specific > > > hardware accelerator based on CPUID or device information (e.g. > > > crct10dif_pclmul). > > > > > > RAID has effectively the same issue, and we just "solved" it by > > > compiling in all the accelerators into the top-level module. > > > > I think for crypto the simplest solution is to not do CPUID-based > > loading. Then crypto users will simply load the module alias which > > causes modprobe to load all modules providing that alias. > > > > Cheers, > > Herbert, > > I've tried the module alias approach (see my earlier mail with patch) > but it didn't seem to load things properly. Can you check to see if > there's anything I did incorrectly. > > Tim I fixed a missing request_module statement in crct10dif library. So now things work if I have the following config: CONFIG_CRYPTO_CRCT10DIF=m CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m CONFIG_CRC_T10DIF=m However, when I have the library and generic algorithm compiled in, I do not see the PCLMULQDQ version loaded. CONFIG_CRYPTO_CRCT10DIF=y CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m CONFIG_CRC_T10DIF=y Perhaps I am initiating the crct10dif library at a really early stage when things are compiled in, where the module is not in initramfs? In that case, perhaps we should only allow PCLMUL version to be compiled in and not exist as a module? Here's the patch I tried: Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com> --- arch/x86/crypto/crct10dif-pclmul_glue.c | 8 +------- crypto/Makefile | 2 +- crypto/{crct10dif.c => crct10dif_generic.c} | 1 + lib/crc-t10dif.c | 1 + 4 files changed, 4 insertions(+), 8 deletions(-) rename crypto/{crct10dif.c => crct10dif_generic.c} (99%) diff --git a/arch/x86/crypto/crct10dif-pclmul_glue.c b/arch/x86/crypto/crct10dif-pclmul_glue.c index 7845d7f..7ad5f09 100644 --- a/arch/x86/crypto/crct10dif-pclmul_glue.c +++ b/arch/x86/crypto/crct10dif-pclmul_glue.c @@ -121,15 +121,9 @@ static struct shash_alg alg = { } }; -static const struct x86_cpu_id crct10dif_cpu_id[] = { - X86_FEATURE_MATCH(X86_FEATURE_PCLMULQDQ), - {} -}; -MODULE_DEVICE_TABLE(x86cpu, crct10dif_cpu_id); - static int __init crct10dif_intel_mod_init(void) { - if (!x86_match_cpu(crct10dif_cpu_id)) + if (!cpu_has_pclmulqdq) return -ENODEV; return crypto_register_shash(&alg); diff --git a/crypto/Makefile b/crypto/Makefile index 2d5ed08..3fd76fa 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -83,7 +83,7 @@ obj-$(CONFIG_CRYPTO_ZLIB) += zlib.o obj-$(CONFIG_CRYPTO_MICHAEL_MIC) += michael_mic.o obj-$(CONFIG_CRYPTO_CRC32C) += crc32c.o obj-$(CONFIG_CRYPTO_CRC32) += crc32.o -obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif.o +obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif_generic.o obj-$(CONFIG_CRYPTO_AUTHENC) += authenc.o authencesn.o obj-$(CONFIG_CRYPTO_LZO) += lzo.o obj-$(CONFIG_CRYPTO_LZ4) += lz4.o diff --git a/crypto/crct10dif.c b/crypto/crct10dif_generic.c similarity index 99% rename from crypto/crct10dif.c rename to crypto/crct10dif_generic.c index 92aca96..c960a95 100644 --- a/crypto/crct10dif.c +++ b/crypto/crct10dif_generic.c @@ -176,3 +176,4 @@ module_exit(crct10dif_mod_fini); MODULE_AUTHOR("Tim Chen <tim.c.chen@linux.intel.com>"); MODULE_DESCRIPTION("T10 DIF CRC calculation."); MODULE_LICENSE("GPL"); +MODULE_ALIAS("crct10dif"); diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index fe3428c..d8cd353 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -38,6 +38,7 @@ EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void) { + request_module("crct10dif"); crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); return PTR_RET(crct10dif_tfm); } -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency. 2013-07-20 1:31 ` Tim Chen @ 2013-07-20 2:19 ` Tetsuo Handa -1 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-20 2:19 UTC (permalink / raw) To: tim.c.chen, herbert Cc: hpa, gregkh, rjw, rafael.j.wysocki, linux-kernel, linux-crypto, ak, linux-acpi Tim Chen wrote: > On Fri, 2013-07-19 at 16:37 -0700, Tim Chen wrote: > > Herbert, > > > > I've tried the module alias approach (see my earlier mail with patch) > > but it didn't seem to load things properly. Can you check to see if > > there's anything I did incorrectly. > > > > Tim > > I fixed a missing request_module statement in crct10dif library. > So now things work if I have the following config: > > CONFIG_CRYPTO_CRCT10DIF=m > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > CONFIG_CRC_T10DIF=m > > However, when I have the library and generic algorithm compiled in, > I do not see the PCLMULQDQ version loaded. > > CONFIG_CRYPTO_CRCT10DIF=y > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > CONFIG_CRC_T10DIF=y > > Perhaps I am initiating the crct10dif library at a really early > stage when things are compiled in, where the module is not in > initramfs? In that case, perhaps we should only allow > PCLMUL version to be compiled in > and not exist as a module? I think that use of request_module("crct10dif") does not help loading crct10dif-pclmul.ko when CONFIG_CRC_T10DIF=y CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m , for there is no / directory (note that the initramfs is not yet mounted as / for loading modules which are not in vmlinux) when any module_init() functions which are in vmlinux are called. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency. @ 2013-07-20 2:19 ` Tetsuo Handa 0 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-20 2:19 UTC (permalink / raw) To: tim.c.chen, herbert Cc: hpa, gregkh, rjw, rafael.j.wysocki, linux-kernel, linux-crypto, ak, linux-acpi Tim Chen wrote: > On Fri, 2013-07-19 at 16:37 -0700, Tim Chen wrote: > > Herbert, > > > > I've tried the module alias approach (see my earlier mail with patch) > > but it didn't seem to load things properly. Can you check to see if > > there's anything I did incorrectly. > > > > Tim > > I fixed a missing request_module statement in crct10dif library. > So now things work if I have the following config: > > CONFIG_CRYPTO_CRCT10DIF=m > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > CONFIG_CRC_T10DIF=m > > However, when I have the library and generic algorithm compiled in, > I do not see the PCLMULQDQ version loaded. > > CONFIG_CRYPTO_CRCT10DIF=y > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > CONFIG_CRC_T10DIF=y > > Perhaps I am initiating the crct10dif library at a really early > stage when things are compiled in, where the module is not in > initramfs? In that case, perhaps we should only allow > PCLMUL version to be compiled in > and not exist as a module? I think that use of request_module("crct10dif") does not help loading crct10dif-pclmul.ko when CONFIG_CRC_T10DIF=y CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m , for there is no / directory (note that the initramfs is not yet mounted as / for loading modules which are not in vmlinux) when any module_init() functions which are in vmlinux are called. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-20 1:31 ` Tim Chen @ 2013-07-20 5:30 ` Herbert Xu -1 siblings, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-07-20 5:30 UTC (permalink / raw) To: Tim Chen Cc: H. Peter Anvin, Greg Kroah-Hartman, Rafael J. Wysocki, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Fri, Jul 19, 2013 at 06:31:04PM -0700, Tim Chen wrote: > > However, when I have the library and generic algorithm compiled in, > I do not see the PCLMULQDQ version loaded. > > CONFIG_CRYPTO_CRCT10DIF=y > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > CONFIG_CRC_T10DIF=y That is completely expected. I don't really think we need to do anything about this case. After all, if the admin wants to use the optimised version for CRC_T10DIF then they could simply compile that in as well. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-20 5:30 ` Herbert Xu 0 siblings, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-07-20 5:30 UTC (permalink / raw) To: Tim Chen Cc: H. Peter Anvin, Greg Kroah-Hartman, Rafael J. Wysocki, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Fri, Jul 19, 2013 at 06:31:04PM -0700, Tim Chen wrote: > > However, when I have the library and generic algorithm compiled in, > I do not see the PCLMULQDQ version loaded. > > CONFIG_CRYPTO_CRCT10DIF=y > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > CONFIG_CRC_T10DIF=y That is completely expected. I don't really think we need to do anything about this case. After all, if the admin wants to use the optimised version for CRC_T10DIF then they could simply compile that in as well. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency. 2013-07-20 5:30 ` Herbert Xu @ 2013-07-20 5:56 ` Tetsuo Handa -1 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-20 5:56 UTC (permalink / raw) To: herbert, tim.c.chen Cc: hpa, gregkh, rjw, rafael.j.wysocki, linux-kernel, linux-crypto, ak, linux-acpi Herbert Xu wrote: > On Fri, Jul 19, 2013 at 06:31:04PM -0700, Tim Chen wrote: > > > > However, when I have the library and generic algorithm compiled in, > > I do not see the PCLMULQDQ version loaded. > > > > CONFIG_CRYPTO_CRCT10DIF=y > > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > > CONFIG_CRC_T10DIF=y > > That is completely expected. I don't really think we need to > do anything about this case. After all, if the admin wants to > use the optimised version for CRC_T10DIF then they could simply > compile that in as well. > Wow! ;-) But I'd expect something like static int __init crc_t10dif_mod_init(void) { +#if !defined(CONFIG_CRC_T10DIF_MODULE) && defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) + printk(KERN_WARNING "Consider CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y for better performance\n"); +#endif crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); return PTR_RET(crct10dif_tfm); } because the admin might not be aware of this implication. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency. @ 2013-07-20 5:56 ` Tetsuo Handa 0 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-07-20 5:56 UTC (permalink / raw) To: herbert, tim.c.chen Cc: hpa, gregkh, rjw, rafael.j.wysocki, linux-kernel, linux-crypto, ak, linux-acpi Herbert Xu wrote: > On Fri, Jul 19, 2013 at 06:31:04PM -0700, Tim Chen wrote: > > > > However, when I have the library and generic algorithm compiled in, > > I do not see the PCLMULQDQ version loaded. > > > > CONFIG_CRYPTO_CRCT10DIF=y > > CONFIG_CRYPTO_CRCT10DIF_PCLMUL=m > > CONFIG_CRC_T10DIF=y > > That is completely expected. I don't really think we need to > do anything about this case. After all, if the admin wants to > use the optimised version for CRC_T10DIF then they could simply > compile that in as well. > Wow! ;-) But I'd expect something like static int __init crc_t10dif_mod_init(void) { +#if !defined(CONFIG_CRC_T10DIF_MODULE) && defined(CONFIG_CRYPTO_CRCT10DIF_PCLMUL_MODULE) + printk(KERN_WARNING "Consider CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y for better performance\n"); +#endif crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); return PTR_RET(crct10dif_tfm); } because the admin might not be aware of this implication. ^ permalink raw reply [flat|nested] 72+ messages in thread
* [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-07-20 5:56 ` Tetsuo Handa (?) @ 2013-09-11 11:43 ` Tetsuo Handa 2013-09-12 4:26 ` Herbert Xu -1 siblings, 1 reply; 72+ messages in thread From: Tetsuo Handa @ 2013-09-11 11:43 UTC (permalink / raw) To: herbert, andr345; +Cc: rusty, linux-kernel Hello. I'm again having the boot failure problem due to commit 68411521 'Reinstate "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework"' in linux.git . ---------- debug start ---------- --- a/kernel/kmod.c +++ b/kernel/kmod.c @@ -188,7 +188,9 @@ int __request_module(bool wait, const char *fmt, ...) trace_module_request(module_name, wait, _RET_IP_); + printk(KERN_WARNING "request_module(%s) start\n", module_name); ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC); + printk(KERN_WARNING "request_module(%s) end\n", module_name); atomic_dec(&kmod_concurrent); return ret; ---------- debug end ---------- ---------- dmesg start ---------- [ 5.130608] Fusion MPT base driver 3.04.20 [ 5.130625] Copyright (c) 1999-2008 LSI Corporation [ 5.136709] Fusion MPT SPI Host driver 3.04.20 [ 5.151422] mptbase: ioc0: Initiating bringup [ 5.169695] ioc0: LSI53C1030 B0: Capabilities={Initiator} [ 5.213380] scsi2 : ioc0: LSI53C1030 B0, FwRev=01032920h, Ports=1, MaxQ=128, IRQ=17 [ 5.223811] Switched to clocksource tsc [ 5.247993] scsi 2:0:0:0: Direct-Access VMware, VMware Virtual S 1.0 PQ: 0 ANSI: 2 [ 5.249783] scsi target2:0:0: Beginning Domain Validation [ 5.258664] scsi target2:0:0: Domain Validation skipping write tests [ 5.259592] scsi target2:0:0: Ending Domain Validation [ 5.260933] scsi target2:0:0: FAST-40 WIDE SCSI 80.0 MB/s ST (25 ns, offset 127) [ 5.263868] scsi 2:0:1:0: Direct-Access VMware, VMware Virtual S 1.0 PQ: 0 ANSI: 2 [ 5.264627] scsi target2:0:1: Beginning Domain Validation [ 5.270310] scsi target2:0:1: Domain Validation skipping write tests [ 5.270563] scsi target2:0:1: Ending Domain Validation [ 5.271742] scsi target2:0:1: FAST-40 WIDE SCSI 80.0 MB/s ST (25 ns, offset 127) [ 5.423813] sr0: scsi3-mmc drive: 1x/1x writer dvd-ram cd/rw xa/form2 cdda tray [ 5.424805] cdrom: Uniform CD-ROM driver Revision: 3.20 [ 5.436731] request_module(crct10dif) start [ 5.441143] request_module(crct10dif) end [ 5.441873] request_module(crct10dif-all) start [ 5.446616] request_module(crct10dif-all) end [ 5.462070] request_module(crct10dif) start [ 5.466579] request_module(crct10dif) end [ 5.466648] request_module(crct10dif-all) start [ 5.470592] request_module(crct10dif-all) end [ 5.544469] scsi_id (268) used greatest stack depth: 3552 bytes left FATAL: Module scsi_wait_scan not found. (...snipped...) FATAL: Module scsi_wait_scan not found. [ 59.306043] dracut Warning: Boot has failed. To debug this issue add "rdshell" to the kernel command line. [ 59.308188] dracut Warning: Signal caught! ---------- dmesg end ---------- In the initramfs, crc-t10dif.ko is included but crct10dif.ko and crct10dif-pclmul.ko are not included. This is because modules.dep does not describe that crc-t10dif.ko depends on crct10dif.ko and optionally depends on crct10dif-pclmul.ko . ---------- dependency start ---------- $ grep t10dif modules.dep kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif.ko kernel/crypto/crct10dif.ko: kernel/drivers/scsi/lpfc/lpfc.ko: kernel/drivers/scsi/scsi_transport_fc.ko kernel/drivers/scsi/scsi_tgt.ko kernel/lib/crc-t10dif.ko kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/drivers/scsi/scsi_debug.ko: kernel/lib/crc-t10dif.ko kernel/lib/crc-t10dif.ko: ---------- dependency end ---------- I'm using module-init-tools-3.9-21.el6_4 / binutils-2.20.51.0.2-5.36.el6 / dracut-004-303.el6 / gcc-4.4.7-3.el6 and there is no modules.softdep file. Did commit 7cb14ba7 "modules: add support for soft module dependencies" silently introduced dependency on module-init-tools which can generate modules.softdep file ( module-init-tools >= 3.11 ) ? Kernel config is at http://I-love.SAKURA.ne.jp/tmp/config-3.12-rc1-modules . Regards. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-11 11:43 ` [3.12-rc1] Dependency on module-init-tools >= 3.11 ? Tetsuo Handa @ 2013-09-12 4:26 ` Herbert Xu 2013-09-12 5:03 ` Tetsuo Handa 2013-09-12 11:12 ` Arthur Marsh 0 siblings, 2 replies; 72+ messages in thread From: Herbert Xu @ 2013-09-12 4:26 UTC (permalink / raw) To: Tetsuo Handa; +Cc: andr345, rusty, linux-kernel, Arthur Marsh, Tim Chen On Wed, Sep 11, 2013 at 08:43:19PM +0900, Tetsuo Handa wrote: > Hello. > > I'm again having the boot failure problem due to commit 68411521 'Reinstate > "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform > framework"' in linux.git . OK, can you please try this patch on top of the current tree? This way at least you'll have a working system until your initramfs tool is fixed to do the right thing. diff --git a/crypto/Makefile b/crypto/Makefile index 2d5ed08..80019ba 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -83,7 +83,7 @@ obj-$(CONFIG_CRYPTO_ZLIB) += zlib.o obj-$(CONFIG_CRYPTO_MICHAEL_MIC) += michael_mic.o obj-$(CONFIG_CRYPTO_CRC32C) += crc32c.o obj-$(CONFIG_CRYPTO_CRC32) += crc32.o -obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif.o +obj-$(CONFIG_CRYPTO_CRCT10DIF) += crct10dif_common.o crct10dif_generic.o obj-$(CONFIG_CRYPTO_AUTHENC) += authenc.o authencesn.o obj-$(CONFIG_CRYPTO_LZO) += lzo.o obj-$(CONFIG_CRYPTO_LZ4) += lz4.o diff --git a/crypto/crct10dif.c b/crypto/crct10dif.c deleted file mode 100644 index 92aca96..0000000 --- a/crypto/crct10dif.c +++ /dev/null @@ -1,178 +0,0 @@ -/* - * Cryptographic API. - * - * T10 Data Integrity Field CRC16 Crypto Transform - * - * Copyright (c) 2007 Oracle Corporation. All rights reserved. - * Written by Martin K. Petersen <martin.petersen@oracle.com> - * Copyright (C) 2013 Intel Corporation - * Author: Tim Chen <tim.c.chen@linux.intel.com> - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License as published by the Free - * Software Foundation; either version 2 of the License, or (at your option) - * any later version. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, - * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF - * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND - * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS - * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN - * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN - * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE - * SOFTWARE. - * - */ - -#include <linux/types.h> -#include <linux/module.h> -#include <linux/crc-t10dif.h> -#include <crypto/internal/hash.h> -#include <linux/init.h> -#include <linux/string.h> -#include <linux/kernel.h> - -struct chksum_desc_ctx { - __u16 crc; -}; - -/* Table generated using the following polynomium: - * x^16 + x^15 + x^11 + x^9 + x^8 + x^7 + x^5 + x^4 + x^2 + x + 1 - * gt: 0x8bb7 - */ -static const __u16 t10_dif_crc_table[256] = { - 0x0000, 0x8BB7, 0x9CD9, 0x176E, 0xB205, 0x39B2, 0x2EDC, 0xA56B, - 0xEFBD, 0x640A, 0x7364, 0xF8D3, 0x5DB8, 0xD60F, 0xC161, 0x4AD6, - 0x54CD, 0xDF7A, 0xC814, 0x43A3, 0xE6C8, 0x6D7F, 0x7A11, 0xF1A6, - 0xBB70, 0x30C7, 0x27A9, 0xAC1E, 0x0975, 0x82C2, 0x95AC, 0x1E1B, - 0xA99A, 0x222D, 0x3543, 0xBEF4, 0x1B9F, 0x9028, 0x8746, 0x0CF1, - 0x4627, 0xCD90, 0xDAFE, 0x5149, 0xF422, 0x7F95, 0x68FB, 0xE34C, - 0xFD57, 0x76E0, 0x618E, 0xEA39, 0x4F52, 0xC4E5, 0xD38B, 0x583C, - 0x12EA, 0x995D, 0x8E33, 0x0584, 0xA0EF, 0x2B58, 0x3C36, 0xB781, - 0xD883, 0x5334, 0x445A, 0xCFED, 0x6A86, 0xE131, 0xF65F, 0x7DE8, - 0x373E, 0xBC89, 0xABE7, 0x2050, 0x853B, 0x0E8C, 0x19E2, 0x9255, - 0x8C4E, 0x07F9, 0x1097, 0x9B20, 0x3E4B, 0xB5FC, 0xA292, 0x2925, - 0x63F3, 0xE844, 0xFF2A, 0x749D, 0xD1F6, 0x5A41, 0x4D2F, 0xC698, - 0x7119, 0xFAAE, 0xEDC0, 0x6677, 0xC31C, 0x48AB, 0x5FC5, 0xD472, - 0x9EA4, 0x1513, 0x027D, 0x89CA, 0x2CA1, 0xA716, 0xB078, 0x3BCF, - 0x25D4, 0xAE63, 0xB90D, 0x32BA, 0x97D1, 0x1C66, 0x0B08, 0x80BF, - 0xCA69, 0x41DE, 0x56B0, 0xDD07, 0x786C, 0xF3DB, 0xE4B5, 0x6F02, - 0x3AB1, 0xB106, 0xA668, 0x2DDF, 0x88B4, 0x0303, 0x146D, 0x9FDA, - 0xD50C, 0x5EBB, 0x49D5, 0xC262, 0x6709, 0xECBE, 0xFBD0, 0x7067, - 0x6E7C, 0xE5CB, 0xF2A5, 0x7912, 0xDC79, 0x57CE, 0x40A0, 0xCB17, - 0x81C1, 0x0A76, 0x1D18, 0x96AF, 0x33C4, 0xB873, 0xAF1D, 0x24AA, - 0x932B, 0x189C, 0x0FF2, 0x8445, 0x212E, 0xAA99, 0xBDF7, 0x3640, - 0x7C96, 0xF721, 0xE04F, 0x6BF8, 0xCE93, 0x4524, 0x524A, 0xD9FD, - 0xC7E6, 0x4C51, 0x5B3F, 0xD088, 0x75E3, 0xFE54, 0xE93A, 0x628D, - 0x285B, 0xA3EC, 0xB482, 0x3F35, 0x9A5E, 0x11E9, 0x0687, 0x8D30, - 0xE232, 0x6985, 0x7EEB, 0xF55C, 0x5037, 0xDB80, 0xCCEE, 0x4759, - 0x0D8F, 0x8638, 0x9156, 0x1AE1, 0xBF8A, 0x343D, 0x2353, 0xA8E4, - 0xB6FF, 0x3D48, 0x2A26, 0xA191, 0x04FA, 0x8F4D, 0x9823, 0x1394, - 0x5942, 0xD2F5, 0xC59B, 0x4E2C, 0xEB47, 0x60F0, 0x779E, 0xFC29, - 0x4BA8, 0xC01F, 0xD771, 0x5CC6, 0xF9AD, 0x721A, 0x6574, 0xEEC3, - 0xA415, 0x2FA2, 0x38CC, 0xB37B, 0x1610, 0x9DA7, 0x8AC9, 0x017E, - 0x1F65, 0x94D2, 0x83BC, 0x080B, 0xAD60, 0x26D7, 0x31B9, 0xBA0E, - 0xF0D8, 0x7B6F, 0x6C01, 0xE7B6, 0x42DD, 0xC96A, 0xDE04, 0x55B3 -}; - -__u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, size_t len) -{ - unsigned int i; - - for (i = 0 ; i < len ; i++) - crc = (crc << 8) ^ t10_dif_crc_table[((crc >> 8) ^ buffer[i]) & 0xff]; - - return crc; -} -EXPORT_SYMBOL(crc_t10dif_generic); - -/* - * Steps through buffer one byte at at time, calculates reflected - * crc using table. - */ - -static int chksum_init(struct shash_desc *desc) -{ - struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); - - ctx->crc = 0; - - return 0; -} - -static int chksum_update(struct shash_desc *desc, const u8 *data, - unsigned int length) -{ - struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); - - ctx->crc = crc_t10dif_generic(ctx->crc, data, length); - return 0; -} - -static int chksum_final(struct shash_desc *desc, u8 *out) -{ - struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); - - *(__u16 *)out = ctx->crc; - return 0; -} - -static int __chksum_finup(__u16 *crcp, const u8 *data, unsigned int len, - u8 *out) -{ - *(__u16 *)out = crc_t10dif_generic(*crcp, data, len); - return 0; -} - -static int chksum_finup(struct shash_desc *desc, const u8 *data, - unsigned int len, u8 *out) -{ - struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); - - return __chksum_finup(&ctx->crc, data, len, out); -} - -static int chksum_digest(struct shash_desc *desc, const u8 *data, - unsigned int length, u8 *out) -{ - struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); - - return __chksum_finup(&ctx->crc, data, length, out); -} - -static struct shash_alg alg = { - .digestsize = CRC_T10DIF_DIGEST_SIZE, - .init = chksum_init, - .update = chksum_update, - .final = chksum_final, - .finup = chksum_finup, - .digest = chksum_digest, - .descsize = sizeof(struct chksum_desc_ctx), - .base = { - .cra_name = "crct10dif", - .cra_driver_name = "crct10dif-generic", - .cra_priority = 100, - .cra_blocksize = CRC_T10DIF_BLOCK_SIZE, - .cra_module = THIS_MODULE, - } -}; - -static int __init crct10dif_mod_init(void) -{ - int ret; - - ret = crypto_register_shash(&alg); - return ret; -} - -static void __exit crct10dif_mod_fini(void) -{ - crypto_unregister_shash(&alg); -} - -module_init(crct10dif_mod_init); -module_exit(crct10dif_mod_fini); - -MODULE_AUTHOR("Tim Chen <tim.c.chen@linux.intel.com>"); -MODULE_DESCRIPTION("T10 DIF CRC calculation."); -MODULE_LICENSE("GPL"); diff --git a/crypto/crct10dif_common.c b/crypto/crct10dif_common.c new file mode 100644 index 0000000..b2fab36 --- /dev/null +++ b/crypto/crct10dif_common.c @@ -0,0 +1,82 @@ +/* + * Cryptographic API. + * + * T10 Data Integrity Field CRC16 Crypto Transform + * + * Copyright (c) 2007 Oracle Corporation. All rights reserved. + * Written by Martin K. Petersen <martin.petersen@oracle.com> + * Copyright (C) 2013 Intel Corporation + * Author: Tim Chen <tim.c.chen@linux.intel.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + */ + +#include <linux/crc-t10dif.h> +#include <linux/module.h> +#include <linux/kernel.h> + +/* Table generated using the following polynomium: + * x^16 + x^15 + x^11 + x^9 + x^8 + x^7 + x^5 + x^4 + x^2 + x + 1 + * gt: 0x8bb7 + */ +static const __u16 t10_dif_crc_table[256] = { + 0x0000, 0x8BB7, 0x9CD9, 0x176E, 0xB205, 0x39B2, 0x2EDC, 0xA56B, + 0xEFBD, 0x640A, 0x7364, 0xF8D3, 0x5DB8, 0xD60F, 0xC161, 0x4AD6, + 0x54CD, 0xDF7A, 0xC814, 0x43A3, 0xE6C8, 0x6D7F, 0x7A11, 0xF1A6, + 0xBB70, 0x30C7, 0x27A9, 0xAC1E, 0x0975, 0x82C2, 0x95AC, 0x1E1B, + 0xA99A, 0x222D, 0x3543, 0xBEF4, 0x1B9F, 0x9028, 0x8746, 0x0CF1, + 0x4627, 0xCD90, 0xDAFE, 0x5149, 0xF422, 0x7F95, 0x68FB, 0xE34C, + 0xFD57, 0x76E0, 0x618E, 0xEA39, 0x4F52, 0xC4E5, 0xD38B, 0x583C, + 0x12EA, 0x995D, 0x8E33, 0x0584, 0xA0EF, 0x2B58, 0x3C36, 0xB781, + 0xD883, 0x5334, 0x445A, 0xCFED, 0x6A86, 0xE131, 0xF65F, 0x7DE8, + 0x373E, 0xBC89, 0xABE7, 0x2050, 0x853B, 0x0E8C, 0x19E2, 0x9255, + 0x8C4E, 0x07F9, 0x1097, 0x9B20, 0x3E4B, 0xB5FC, 0xA292, 0x2925, + 0x63F3, 0xE844, 0xFF2A, 0x749D, 0xD1F6, 0x5A41, 0x4D2F, 0xC698, + 0x7119, 0xFAAE, 0xEDC0, 0x6677, 0xC31C, 0x48AB, 0x5FC5, 0xD472, + 0x9EA4, 0x1513, 0x027D, 0x89CA, 0x2CA1, 0xA716, 0xB078, 0x3BCF, + 0x25D4, 0xAE63, 0xB90D, 0x32BA, 0x97D1, 0x1C66, 0x0B08, 0x80BF, + 0xCA69, 0x41DE, 0x56B0, 0xDD07, 0x786C, 0xF3DB, 0xE4B5, 0x6F02, + 0x3AB1, 0xB106, 0xA668, 0x2DDF, 0x88B4, 0x0303, 0x146D, 0x9FDA, + 0xD50C, 0x5EBB, 0x49D5, 0xC262, 0x6709, 0xECBE, 0xFBD0, 0x7067, + 0x6E7C, 0xE5CB, 0xF2A5, 0x7912, 0xDC79, 0x57CE, 0x40A0, 0xCB17, + 0x81C1, 0x0A76, 0x1D18, 0x96AF, 0x33C4, 0xB873, 0xAF1D, 0x24AA, + 0x932B, 0x189C, 0x0FF2, 0x8445, 0x212E, 0xAA99, 0xBDF7, 0x3640, + 0x7C96, 0xF721, 0xE04F, 0x6BF8, 0xCE93, 0x4524, 0x524A, 0xD9FD, + 0xC7E6, 0x4C51, 0x5B3F, 0xD088, 0x75E3, 0xFE54, 0xE93A, 0x628D, + 0x285B, 0xA3EC, 0xB482, 0x3F35, 0x9A5E, 0x11E9, 0x0687, 0x8D30, + 0xE232, 0x6985, 0x7EEB, 0xF55C, 0x5037, 0xDB80, 0xCCEE, 0x4759, + 0x0D8F, 0x8638, 0x9156, 0x1AE1, 0xBF8A, 0x343D, 0x2353, 0xA8E4, + 0xB6FF, 0x3D48, 0x2A26, 0xA191, 0x04FA, 0x8F4D, 0x9823, 0x1394, + 0x5942, 0xD2F5, 0xC59B, 0x4E2C, 0xEB47, 0x60F0, 0x779E, 0xFC29, + 0x4BA8, 0xC01F, 0xD771, 0x5CC6, 0xF9AD, 0x721A, 0x6574, 0xEEC3, + 0xA415, 0x2FA2, 0x38CC, 0xB37B, 0x1610, 0x9DA7, 0x8AC9, 0x017E, + 0x1F65, 0x94D2, 0x83BC, 0x080B, 0xAD60, 0x26D7, 0x31B9, 0xBA0E, + 0xF0D8, 0x7B6F, 0x6C01, 0xE7B6, 0x42DD, 0xC96A, 0xDE04, 0x55B3 +}; + +__u16 crc_t10dif_generic(__u16 crc, const unsigned char *buffer, size_t len) +{ + unsigned int i; + + for (i = 0 ; i < len ; i++) + crc = (crc << 8) ^ t10_dif_crc_table[((crc >> 8) ^ buffer[i]) & 0xff]; + + return crc; +} +EXPORT_SYMBOL(crc_t10dif_generic); + +MODULE_DESCRIPTION("T10 DIF CRC calculation common code"); +MODULE_LICENSE("GPL"); diff --git a/crypto/crct10dif_generic.c b/crypto/crct10dif_generic.c new file mode 100644 index 0000000..877e711 --- /dev/null +++ b/crypto/crct10dif_generic.c @@ -0,0 +1,127 @@ +/* + * Cryptographic API. + * + * T10 Data Integrity Field CRC16 Crypto Transform + * + * Copyright (c) 2007 Oracle Corporation. All rights reserved. + * Written by Martin K. Petersen <martin.petersen@oracle.com> + * Copyright (C) 2013 Intel Corporation + * Author: Tim Chen <tim.c.chen@linux.intel.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + */ + +#include <linux/module.h> +#include <linux/crc-t10dif.h> +#include <crypto/internal/hash.h> +#include <linux/init.h> +#include <linux/kernel.h> + +struct chksum_desc_ctx { + __u16 crc; +}; + +/* + * Steps through buffer one byte at at time, calculates reflected + * crc using table. + */ + +static int chksum_init(struct shash_desc *desc) +{ + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); + + ctx->crc = 0; + + return 0; +} + +static int chksum_update(struct shash_desc *desc, const u8 *data, + unsigned int length) +{ + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); + + ctx->crc = crc_t10dif_generic(ctx->crc, data, length); + return 0; +} + +static int chksum_final(struct shash_desc *desc, u8 *out) +{ + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); + + *(__u16 *)out = ctx->crc; + return 0; +} + +static int __chksum_finup(__u16 *crcp, const u8 *data, unsigned int len, + u8 *out) +{ + *(__u16 *)out = crc_t10dif_generic(*crcp, data, len); + return 0; +} + +static int chksum_finup(struct shash_desc *desc, const u8 *data, + unsigned int len, u8 *out) +{ + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); + + return __chksum_finup(&ctx->crc, data, len, out); +} + +static int chksum_digest(struct shash_desc *desc, const u8 *data, + unsigned int length, u8 *out) +{ + struct chksum_desc_ctx *ctx = shash_desc_ctx(desc); + + return __chksum_finup(&ctx->crc, data, length, out); +} + +static struct shash_alg alg = { + .digestsize = CRC_T10DIF_DIGEST_SIZE, + .init = chksum_init, + .update = chksum_update, + .final = chksum_final, + .finup = chksum_finup, + .digest = chksum_digest, + .descsize = sizeof(struct chksum_desc_ctx), + .base = { + .cra_name = "crct10dif", + .cra_driver_name = "crct10dif-generic", + .cra_priority = 100, + .cra_blocksize = CRC_T10DIF_BLOCK_SIZE, + .cra_module = THIS_MODULE, + } +}; + +static int __init crct10dif_mod_init(void) +{ + int ret; + + ret = crypto_register_shash(&alg); + return ret; +} + +static void __exit crct10dif_mod_fini(void) +{ + crypto_unregister_shash(&alg); +} + +module_init(crct10dif_mod_init); +module_exit(crct10dif_mod_fini); + +MODULE_AUTHOR("Tim Chen <tim.c.chen@linux.intel.com>"); +MODULE_DESCRIPTION("T10 DIF CRC calculation."); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("crct10dif"); diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index 43bc5b0..dfe6ec1 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -14,8 +14,10 @@ #include <linux/err.h> #include <linux/init.h> #include <crypto/hash.h> +#include <linux/static_key.h> static struct crypto_shash *crct10dif_tfm; +static struct static_key crct10dif_fallback __read_mostly; __u16 crc_t10dif(const unsigned char *buffer, size_t len) { @@ -25,6 +27,9 @@ __u16 crc_t10dif(const unsigned char *buffer, size_t len) } desc; int err; + if (static_key_false(&crct10dif_fallback)) + return crc_t10dif_generic(0, buffer, len); + desc.shash.tfm = crct10dif_tfm; desc.shash.flags = 0; *(__u16 *)desc.ctx = 0; @@ -39,7 +44,11 @@ EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void) { crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); - return PTR_RET(crct10dif_tfm); + if (IS_ERR(crct10dif_tfm)) { + static_key_slow_inc(&crct10dif_fallback); + crct10dif_tfm = NULL; + } + return 0; } static void __exit crc_t10dif_mod_fini(void) Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-12 4:26 ` Herbert Xu @ 2013-09-12 5:03 ` Tetsuo Handa 2013-09-12 5:28 ` Herbert Xu 2013-09-12 11:12 ` Arthur Marsh 1 sibling, 1 reply; 72+ messages in thread From: Tetsuo Handa @ 2013-09-12 5:03 UTC (permalink / raw) To: herbert; +Cc: andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen Herbert Xu wrote: > This way at least you'll have a working system until your initramfs > tool is fixed to do the right thing. Thank you. But it is module-init-tools-3.9-21.el6_4 in RHEL 6.4. We can't wait until Red Hat backports module-init-tools >= 3.11 to RHEL 6.x. Since most people are already using module-init-tools >= 3.11 and there is workaround for my case (i.e. choose built-in), just updating module-init-tools 0.9.10 # depmod -V line at "Current Minimal Requirements" in Documentation/Changes will be OK. Regards. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-12 5:03 ` Tetsuo Handa @ 2013-09-12 5:28 ` Herbert Xu 0 siblings, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-09-12 5:28 UTC (permalink / raw) To: Tetsuo Handa Cc: andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, Linux Crypto Mailing List On Thu, Sep 12, 2013 at 02:03:41PM +0900, Tetsuo Handa wrote: > Herbert Xu wrote: > > This way at least you'll have a working system until your initramfs > > tool is fixed to do the right thing. > > Thank you. But it is module-init-tools-3.9-21.el6_4 in RHEL 6.4. > We can't wait until Red Hat backports module-init-tools >= 3.11 to RHEL 6.x. > > Since most people are already using module-init-tools >= 3.11 and > there is workaround for my case (i.e. choose built-in), just updating > > module-init-tools 0.9.10 # depmod -V > > line at "Current Minimal Requirements" in Documentation/Changes will be OK. The trouble is not all distros will include the softdep modules in the initramfs. So for now I think we will have to live with a fallback. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? @ 2013-09-12 5:28 ` Herbert Xu 0 siblings, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-09-12 5:28 UTC (permalink / raw) To: Tetsuo Handa Cc: andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, Linux Crypto Mailing List On Thu, Sep 12, 2013 at 02:03:41PM +0900, Tetsuo Handa wrote: > Herbert Xu wrote: > > This way at least you'll have a working system until your initramfs > > tool is fixed to do the right thing. > > Thank you. But it is module-init-tools-3.9-21.el6_4 in RHEL 6.4. > We can't wait until Red Hat backports module-init-tools >= 3.11 to RHEL 6.x. > > Since most people are already using module-init-tools >= 3.11 and > there is workaround for my case (i.e. choose built-in), just updating > > module-init-tools 0.9.10 # depmod -V > > line at "Current Minimal Requirements" in Documentation/Changes will be OK. The trouble is not all distros will include the softdep modules in the initramfs. So for now I think we will have to live with a fallback. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-12 5:28 ` Herbert Xu @ 2013-09-12 10:20 ` Tetsuo Handa -1 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-09-12 10:20 UTC (permalink / raw) To: herbert Cc: andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto Herbert Xu wrote: > The trouble is not all distros will include the softdep modules in > the initramfs. So for now I think we will have to live with a fallback. I see. Herbert Xu wrote: > OK, can you please try this patch on top of the current tree? > > This way at least you'll have a working system until your initramfs > tool is fixed to do the right thing. I tested the patch and confirmed that the boot failure was solved. But arch/x86/crypto/crct10dif-pclmul.ko is not included into initramfs and therefore we cannot benefit from PCLMULQDQ version. ---------- before applying patch ---------- kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif.ko kernel/crypto/crct10dif.ko: kernel/drivers/scsi/lpfc/lpfc.ko: kernel/drivers/scsi/scsi_transport_fc.ko kernel/drivers/scsi/scsi_tgt.ko kernel/lib/crc-t10dif.ko kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/drivers/scsi/scsi_debug.ko: kernel/lib/crc-t10dif.ko kernel/lib/crc-t10dif.ko: ---------- before applying patch ---------- ---------- after applying patch ---------- kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif_common.ko kernel/crypto/crct10dif_common.ko: kernel/crypto/crct10dif_generic.ko: kernel/crypto/crct10dif_common.ko kernel/drivers/scsi/lpfc/lpfc.ko: kernel/drivers/scsi/scsi_transport_fc.ko kernel/drivers/scsi/scsi_tgt.ko kernel/lib/crc-t10dif.ko kernel/crypto/crct10dif_common.ko kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/crypto/crct10dif_common.ko kernel/drivers/scsi/scsi_debug.ko: kernel/lib/crc-t10dif.ko kernel/crypto/crct10dif_common.ko kernel/lib/crc-t10dif.ko: kernel/crypto/crct10dif_common.ko ---------- after applying patch ---------- ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? @ 2013-09-12 10:20 ` Tetsuo Handa 0 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-09-12 10:20 UTC (permalink / raw) To: herbert Cc: andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto Herbert Xu wrote: > The trouble is not all distros will include the softdep modules in > the initramfs. So for now I think we will have to live with a fallback. I see. Herbert Xu wrote: > OK, can you please try this patch on top of the current tree? > > This way at least you'll have a working system until your initramfs > tool is fixed to do the right thing. I tested the patch and confirmed that the boot failure was solved. But arch/x86/crypto/crct10dif-pclmul.ko is not included into initramfs and therefore we cannot benefit from PCLMULQDQ version. ---------- before applying patch ---------- kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif.ko kernel/crypto/crct10dif.ko: kernel/drivers/scsi/lpfc/lpfc.ko: kernel/drivers/scsi/scsi_transport_fc.ko kernel/drivers/scsi/scsi_tgt.ko kernel/lib/crc-t10dif.ko kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/drivers/scsi/scsi_debug.ko: kernel/lib/crc-t10dif.ko kernel/lib/crc-t10dif.ko: ---------- before applying patch ---------- ---------- after applying patch ---------- kernel/arch/x86/crypto/crct10dif-pclmul.ko: kernel/crypto/crct10dif_common.ko kernel/crypto/crct10dif_common.ko: kernel/crypto/crct10dif_generic.ko: kernel/crypto/crct10dif_common.ko kernel/drivers/scsi/lpfc/lpfc.ko: kernel/drivers/scsi/scsi_transport_fc.ko kernel/drivers/scsi/scsi_tgt.ko kernel/lib/crc-t10dif.ko kernel/crypto/crct10dif_common.ko kernel/drivers/scsi/sd_mod.ko: kernel/lib/crc-t10dif.ko kernel/crypto/crct10dif_common.ko kernel/drivers/scsi/scsi_debug.ko: kernel/lib/crc-t10dif.ko kernel/crypto/crct10dif_common.ko kernel/lib/crc-t10dif.ko: kernel/crypto/crct10dif_common.ko ---------- after applying patch ---------- ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-12 10:20 ` Tetsuo Handa @ 2013-09-12 10:29 ` Herbert Xu -1 siblings, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-09-12 10:29 UTC (permalink / raw) To: Tetsuo Handa Cc: andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto On Thu, Sep 12, 2013 at 07:20:23PM +0900, Tetsuo Handa wrote: > Herbert Xu wrote: > > The trouble is not all distros will include the softdep modules in > > the initramfs. So for now I think we will have to live with a fallback. > > I see. > > Herbert Xu wrote: > > OK, can you please try this patch on top of the current tree? > > > > This way at least you'll have a working system until your initramfs > > tool is fixed to do the right thing. > > I tested the patch and confirmed that the boot failure was solved. > > But arch/x86/crypto/crct10dif-pclmul.ko is not included into initramfs and > therefore we cannot benefit from PCLMULQDQ version. That is expected and is also the status quo. So once the initrd generation tool is fixed to include softdeps it will work properly. Thanks! -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? @ 2013-09-12 10:29 ` Herbert Xu 0 siblings, 0 replies; 72+ messages in thread From: Herbert Xu @ 2013-09-12 10:29 UTC (permalink / raw) To: Tetsuo Handa Cc: andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto On Thu, Sep 12, 2013 at 07:20:23PM +0900, Tetsuo Handa wrote: > Herbert Xu wrote: > > The trouble is not all distros will include the softdep modules in > > the initramfs. So for now I think we will have to live with a fallback. > > I see. > > Herbert Xu wrote: > > OK, can you please try this patch on top of the current tree? > > > > This way at least you'll have a working system until your initramfs > > tool is fixed to do the right thing. > > I tested the patch and confirmed that the boot failure was solved. > > But arch/x86/crypto/crct10dif-pclmul.ko is not included into initramfs and > therefore we cannot benefit from PCLMULQDQ version. That is expected and is also the status quo. So once the initrd generation tool is fixed to include softdeps it will work properly. Thanks! -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-12 10:29 ` Herbert Xu @ 2013-09-12 14:26 ` Waiman Long -1 siblings, 0 replies; 72+ messages in thread From: Waiman Long @ 2013-09-12 14:26 UTC (permalink / raw) To: Herbert Xu Cc: Tetsuo Handa, andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto On 09/12/2013 06:29 AM, Herbert Xu wrote: > On Thu, Sep 12, 2013 at 07:20:23PM +0900, Tetsuo Handa wrote: >> Herbert Xu wrote: >>> The trouble is not all distros will include the softdep modules in >>> the initramfs. So for now I think we will have to live with a fallback. >> I see. >> >> Herbert Xu wrote: >>> OK, can you please try this patch on top of the current tree? >>> >>> This way at least you'll have a working system until your initramfs >>> tool is fixed to do the right thing. >> I tested the patch and confirmed that the boot failure was solved. >> >> But arch/x86/crypto/crct10dif-pclmul.ko is not included into initramfs and >> therefore we cannot benefit from PCLMULQDQ version. > That is expected and is also the status quo. So once the initrd > generation tool is fixed to include softdeps it will work properly. > > Thanks! I would like to report that I also have the same boot problem on a RHEL6.4 box with the crypto patch. My workaround is to force kernel build to have the crc_t10dif code built-in by changing the config file: 4889c4889 < CONFIG_CRYPTO_CRCT10DIF=m --- > CONFIG_CRYPTO_CRCT10DIF=y 5002c5002 < CONFIG_CRC_T10DIF=m --- > CONFIG_CRC_T10DIF=y This solved the boot problem without any additional patch. Do you think you should consider changing the configuration default to "y" instead of "m" or doesn't allow the "m" option at all? Thanks! ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? @ 2013-09-12 14:26 ` Waiman Long 0 siblings, 0 replies; 72+ messages in thread From: Waiman Long @ 2013-09-12 14:26 UTC (permalink / raw) To: Herbert Xu Cc: Tetsuo Handa, andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto On 09/12/2013 06:29 AM, Herbert Xu wrote: > On Thu, Sep 12, 2013 at 07:20:23PM +0900, Tetsuo Handa wrote: >> Herbert Xu wrote: >>> The trouble is not all distros will include the softdep modules in >>> the initramfs. So for now I think we will have to live with a fallback. >> I see. >> >> Herbert Xu wrote: >>> OK, can you please try this patch on top of the current tree? >>> >>> This way at least you'll have a working system until your initramfs >>> tool is fixed to do the right thing. >> I tested the patch and confirmed that the boot failure was solved. >> >> But arch/x86/crypto/crct10dif-pclmul.ko is not included into initramfs and >> therefore we cannot benefit from PCLMULQDQ version. > That is expected and is also the status quo. So once the initrd > generation tool is fixed to include softdeps it will work properly. > > Thanks! I would like to report that I also have the same boot problem on a RHEL6.4 box with the crypto patch. My workaround is to force kernel build to have the crc_t10dif code built-in by changing the config file: 4889c4889 < CONFIG_CRYPTO_CRCT10DIF=m --- > CONFIG_CRYPTO_CRCT10DIF=y 5002c5002 < CONFIG_CRC_T10DIF=m --- > CONFIG_CRC_T10DIF=y This solved the boot problem without any additional patch. Do you think you should consider changing the configuration default to "y" instead of "m" or doesn't allow the "m" option at all? Thanks! ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-12 14:26 ` Waiman Long @ 2013-09-13 13:27 ` Tetsuo Handa -1 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-09-13 13:27 UTC (permalink / raw) To: waiman.long Cc: herbert, andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto Waiman Long wrote: > I would like to report that I also have the same boot problem on a > RHEL6.4 box with the crypto patch. My workaround is to force kernel > build to have the crc_t10dif code built-in by changing the config file: > > 4889c4889 > < CONFIG_CRYPTO_CRCT10DIF=m > --- > > CONFIG_CRYPTO_CRCT10DIF=y > 5002c5002 > < CONFIG_CRC_T10DIF=m > --- > > CONFIG_CRC_T10DIF=y > > This solved the boot problem without any additional patch. Do you think > you should consider changing the configuration default to "y" instead of > "m" or doesn't allow the "m" option at all? That was proposed but not accepted. https://lkml.org/lkml/2013/7/17/543 You should choose CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y in your kernel config if your CPU supports PCLMULQDQ. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? @ 2013-09-13 13:27 ` Tetsuo Handa 0 siblings, 0 replies; 72+ messages in thread From: Tetsuo Handa @ 2013-09-13 13:27 UTC (permalink / raw) To: waiman.long Cc: herbert, andr345, rusty, linux-kernel, arthur.marsh, tim.c.chen, linux-crypto Waiman Long wrote: > I would like to report that I also have the same boot problem on a > RHEL6.4 box with the crypto patch. My workaround is to force kernel > build to have the crc_t10dif code built-in by changing the config file: > > 4889c4889 > < CONFIG_CRYPTO_CRCT10DIF=m > --- > > CONFIG_CRYPTO_CRCT10DIF=y > 5002c5002 > < CONFIG_CRC_T10DIF=m > --- > > CONFIG_CRC_T10DIF=y > > This solved the boot problem without any additional patch. Do you think > you should consider changing the configuration default to "y" instead of > "m" or doesn't allow the "m" option at all? That was proposed but not accepted. https://lkml.org/lkml/2013/7/17/543 You should choose CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y in your kernel config if your CPU supports PCLMULQDQ. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [3.12-rc1] Dependency on module-init-tools >= 3.11 ? 2013-09-12 4:26 ` Herbert Xu 2013-09-12 5:03 ` Tetsuo Handa @ 2013-09-12 11:12 ` Arthur Marsh 1 sibling, 0 replies; 72+ messages in thread From: Arthur Marsh @ 2013-09-12 11:12 UTC (permalink / raw) To: Herbert Xu; +Cc: Tetsuo Handa, andr345, rusty, linux-kernel, Tim Chen Herbert Xu wrote, on 12/09/13 13:56: > On Wed, Sep 11, 2013 at 08:43:19PM +0900, Tetsuo Handa wrote: >> Hello. >> >> I'm again having the boot failure problem due to commit 68411521 'Reinstate >> "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform >> framework"' in linux.git . > > OK, can you please try this patch on top of the current tree? > > This way at least you'll have a working system until your initramfs > tool is fixed to do the right thing. > > diff --git a/crypto/Makefile b/crypto/Makefile [deleted] > Thanks, > Thanks for the help, I can confirm that with Herbet's patch applied I am now booting fine on Debian using CONFIG_SCSI=Y CONFIG_ATA=M CONFIG_BLK_DEV_SD=M on both an x86-64 (AMD Athlon64) machine with SATA boot disk and an i386 (Pentium 4) machine with PATA boot disk. Regards, Arthur. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 23:21 ` H. Peter Anvin 2013-07-19 23:24 ` Herbert Xu @ 2013-07-19 23:26 ` Greg Kroah-Hartman 2013-07-19 23:28 ` H. Peter Anvin 1 sibling, 1 reply; 72+ messages in thread From: Greg Kroah-Hartman @ 2013-07-19 23:26 UTC (permalink / raw) To: H. Peter Anvin Cc: Rafael J. Wysocki, Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On Fri, Jul 19, 2013 at 04:21:09PM -0700, H. Peter Anvin wrote: > On 07/19/2013 04:16 PM, Greg Kroah-Hartman wrote: > > > > udev isn't doing any module loading, 'modprobe' is just being called for > > any new module alias that shows up in the system, and all of the drivers > > that match it then get loaded. > > > > How is it a problem if a module is attempted to be loaded that is > > already loaded? How is it a problem if a different module is loaded for > > a device already bound to a driver? Both of those should be total > > "no-ops" for the kernel. > > > > But, I don't know anything about the cpu code, how is loading a module > > causing problems? That sounds like it needs to be fixes, as any root > > user can load modules whenever they want, you can't protect the kernel > > from doing that. > > > > The issue here seems to be the dynamic binding nature of the crypto > subsystem. When something needs crypto, it will request the appropriate > crypto module (e.g. crct10dif), which may race with detecting a specific > hardware accelerator based on CPUID or device information (e.g. > crct10dif_pclmul). > > RAID has effectively the same issue, and we just "solved" it by > compiling in all the accelerators into the top-level module. Then there's nothing to be done in udev or kmod, right? greg k-h ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 23:26 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Greg Kroah-Hartman @ 2013-07-19 23:28 ` H. Peter Anvin 0 siblings, 0 replies; 72+ messages in thread From: H. Peter Anvin @ 2013-07-19 23:28 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rafael J. Wysocki, Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, ACPI Devel Maling List On 07/19/2013 04:26 PM, Greg Kroah-Hartman wrote: >> >> RAID has effectively the same issue, and we just "solved" it by >> compiling in all the accelerators into the top-level module. > > Then there's nothing to be done in udev or kmod, right? > I don't know. -hpa ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-19 23:16 ` Greg Kroah-Hartman @ 2013-07-20 0:00 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 0:00 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > Alas, this is not the one I'd like to apply. > > > > With that patch applied, new device objects are created to avoid binding the > > processor driver directly to the cpu system device objects, because that > > apparently confuses udev and it starts to ignore the cpu modalias once the > > driver has been bound to any of those objects. > > > > I've verified in the meantime that this indeed is the case. > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > autoloading modules that matched the cpu modalias after a driver had been bound > > to the cpu system device objects and you said "no". However, this time I can > > say with certainty that that really is the case. So, the question now is > > whether or not we can do anything in the kernel to avoid that confusion in udev > > instead of applying the patch linked above (which is beyond ugly in my not so > > humble opinion)? > > udev isn't doing any module loading, 'modprobe' is just being called for > any new module alias that shows up in the system, and all of the drivers > that match it then get loaded. The problem is that that doesn't happen when a driver is bound to the cpu system device objects. modprobe is just not called for modules that match the cpu modalias in that case. If I call modprobe manually for any of the modules in question, it loads and works no problem. > How is it a problem if a module is attempted to be loaded that is > already loaded? How is it a problem if a different module is loaded for > a device already bound to a driver? Both of those should be total > "no-ops" for the kernel. Precisely, but that's not what happens in practice, hence my question. The situation is this: - With 3.11-rc1 modules that match the CPU modalias are not loaded automatically (that is, modprobe is not called for them by udev) after the processor module is loaded. - With 3.10 they are loaded automatically at any time. - With the patch at https://patchwork.kernel.org/patch/2830561/ on top of 3.11-rc1 the situation is the same as for 3.10. Therefore we have a kernel regression from 3.10 (with respect to the existing user space which is udev) in 3.11-rc1 3.10 and the patch at https://patchwork.kernel.org/patch/2830561/ makes that regression go away. However, that patch is totally voodoo programming, so I wonder if there is any saner alternative or we really need to do voodoo programming in the kernel to work around udev's confusion. > But, I don't know anything about the cpu code, how is loading a module > causing problems? That sounds like it needs to be fixes, as any root > user can load modules whenever they want, you can't protect the kernel > from doing that. Yes, that needs to be fixed, but I don't know *why* it is a problem in the first place. I'd like to understand what's going on, because I don't right now. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-20 0:00 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 0:00 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > Alas, this is not the one I'd like to apply. > > > > With that patch applied, new device objects are created to avoid binding the > > processor driver directly to the cpu system device objects, because that > > apparently confuses udev and it starts to ignore the cpu modalias once the > > driver has been bound to any of those objects. > > > > I've verified in the meantime that this indeed is the case. > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > autoloading modules that matched the cpu modalias after a driver had been bound > > to the cpu system device objects and you said "no". However, this time I can > > say with certainty that that really is the case. So, the question now is > > whether or not we can do anything in the kernel to avoid that confusion in udev > > instead of applying the patch linked above (which is beyond ugly in my not so > > humble opinion)? > > udev isn't doing any module loading, 'modprobe' is just being called for > any new module alias that shows up in the system, and all of the drivers > that match it then get loaded. The problem is that that doesn't happen when a driver is bound to the cpu system device objects. modprobe is just not called for modules that match the cpu modalias in that case. If I call modprobe manually for any of the modules in question, it loads and works no problem. > How is it a problem if a module is attempted to be loaded that is > already loaded? How is it a problem if a different module is loaded for > a device already bound to a driver? Both of those should be total > "no-ops" for the kernel. Precisely, but that's not what happens in practice, hence my question. The situation is this: - With 3.11-rc1 modules that match the CPU modalias are not loaded automatically (that is, modprobe is not called for them by udev) after the processor module is loaded. - With 3.10 they are loaded automatically at any time. - With the patch at https://patchwork.kernel.org/patch/2830561/ on top of 3.11-rc1 the situation is the same as for 3.10. Therefore we have a kernel regression from 3.10 (with respect to the existing user space which is udev) in 3.11-rc1 3.10 and the patch at https://patchwork.kernel.org/patch/2830561/ makes that regression go away. However, that patch is totally voodoo programming, so I wonder if there is any saner alternative or we really need to do voodoo programming in the kernel to work around udev's confusion. > But, I don't know anything about the cpu code, how is loading a module > causing problems? That sounds like it needs to be fixes, as any root > user can load modules whenever they want, you can't protect the kernel > from doing that. Yes, that needs to be fixed, but I don't know *why* it is a problem in the first place. I'd like to understand what's going on, because I don't right now. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-20 0:00 ` Rafael J. Wysocki @ 2013-07-20 3:06 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 3:06 UTC (permalink / raw) To: Greg Kroah-Hartman, Tim Chen Cc: Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote: > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > > Alas, this is not the one I'd like to apply. > > > > > > With that patch applied, new device objects are created to avoid binding the > > > processor driver directly to the cpu system device objects, because that > > > apparently confuses udev and it starts to ignore the cpu modalias once the > > > driver has been bound to any of those objects. > > > > > > I've verified in the meantime that this indeed is the case. > > > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > > autoloading modules that matched the cpu modalias after a driver had been bound > > > to the cpu system device objects and you said "no". However, this time I can > > > say with certainty that that really is the case. So, the question now is > > > whether or not we can do anything in the kernel to avoid that confusion in udev > > > instead of applying the patch linked above (which is beyond ugly in my not so > > > humble opinion)? > > > > udev isn't doing any module loading, 'modprobe' is just being called for > > any new module alias that shows up in the system, and all of the drivers > > that match it then get loaded. > > The problem is that that doesn't happen when a driver is bound to the > cpu system device objects. modprobe is just not called for modules that > match the cpu modalias in that case. > > If I call modprobe manually for any of the modules in question, it loads > and works no problem. OK, I know what's up. udev has no rule that would allow it to load stuff on the basis of MODALIAS if DRIVER is set in the event properties. So, the following rule needs to be added to udev rules for things to work as before: DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/ with the following contents: ACTION=="remove", GOTO="drivers_end" DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" LABEL="drivers_end" but I'm not sure how to propagate this to the udev maintainers. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-20 3:06 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 3:06 UTC (permalink / raw) To: Greg Kroah-Hartman, Tim Chen Cc: Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote: > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > > Alas, this is not the one I'd like to apply. > > > > > > With that patch applied, new device objects are created to avoid binding the > > > processor driver directly to the cpu system device objects, because that > > > apparently confuses udev and it starts to ignore the cpu modalias once the > > > driver has been bound to any of those objects. > > > > > > I've verified in the meantime that this indeed is the case. > > > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > > autoloading modules that matched the cpu modalias after a driver had been bound > > > to the cpu system device objects and you said "no". However, this time I can > > > say with certainty that that really is the case. So, the question now is > > > whether or not we can do anything in the kernel to avoid that confusion in udev > > > instead of applying the patch linked above (which is beyond ugly in my not so > > > humble opinion)? > > > > udev isn't doing any module loading, 'modprobe' is just being called for > > any new module alias that shows up in the system, and all of the drivers > > that match it then get loaded. > > The problem is that that doesn't happen when a driver is bound to the > cpu system device objects. modprobe is just not called for modules that > match the cpu modalias in that case. > > If I call modprobe manually for any of the modules in question, it loads > and works no problem. OK, I know what's up. udev has no rule that would allow it to load stuff on the basis of MODALIAS if DRIVER is set in the event properties. So, the following rule needs to be added to udev rules for things to work as before: DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/ with the following contents: ACTION=="remove", GOTO="drivers_end" DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" LABEL="drivers_end" but I'm not sure how to propagate this to the udev maintainers. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-20 3:06 ` Rafael J. Wysocki @ 2013-07-20 9:51 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 9:51 UTC (permalink / raw) To: Greg Kroah-Hartman, Tim Chen Cc: Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Saturday, July 20, 2013 05:06:29 AM Rafael J. Wysocki wrote: > On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote: > > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > > > Alas, this is not the one I'd like to apply. > > > > > > > > With that patch applied, new device objects are created to avoid binding the > > > > processor driver directly to the cpu system device objects, because that > > > > apparently confuses udev and it starts to ignore the cpu modalias once the > > > > driver has been bound to any of those objects. > > > > > > > > I've verified in the meantime that this indeed is the case. > > > > > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > > > autoloading modules that matched the cpu modalias after a driver had been bound > > > > to the cpu system device objects and you said "no". However, this time I can > > > > say with certainty that that really is the case. So, the question now is > > > > whether or not we can do anything in the kernel to avoid that confusion in udev > > > > instead of applying the patch linked above (which is beyond ugly in my not so > > > > humble opinion)? > > > > > > udev isn't doing any module loading, 'modprobe' is just being called for > > > any new module alias that shows up in the system, and all of the drivers > > > that match it then get loaded. > > > > The problem is that that doesn't happen when a driver is bound to the > > cpu system device objects. modprobe is just not called for modules that > > match the cpu modalias in that case. > > > > If I call modprobe manually for any of the modules in question, it loads > > and works no problem. > > OK, I know what's up. udev has no rule that would allow it to load stuff on > the basis of MODALIAS if DRIVER is set in the event properties. > > So, the following rule needs to be added to udev rules for things to work > as before: > > DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" > > To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/ Well, that wasn't a good idea, because the original 80-drivers.rules didn't work then, but I renamed the new file (in /etc/udev/rules.d/) to 80-cpu.rules and put this line (alone) into it: ACTION="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" After that change everything works happily again. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-20 9:51 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 9:51 UTC (permalink / raw) To: Greg Kroah-Hartman, Tim Chen Cc: Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Saturday, July 20, 2013 05:06:29 AM Rafael J. Wysocki wrote: > On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote: > > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > > > Alas, this is not the one I'd like to apply. > > > > > > > > With that patch applied, new device objects are created to avoid binding the > > > > processor driver directly to the cpu system device objects, because that > > > > apparently confuses udev and it starts to ignore the cpu modalias once the > > > > driver has been bound to any of those objects. > > > > > > > > I've verified in the meantime that this indeed is the case. > > > > > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > > > autoloading modules that matched the cpu modalias after a driver had been bound > > > > to the cpu system device objects and you said "no". However, this time I can > > > > say with certainty that that really is the case. So, the question now is > > > > whether or not we can do anything in the kernel to avoid that confusion in udev > > > > instead of applying the patch linked above (which is beyond ugly in my not so > > > > humble opinion)? > > > > > > udev isn't doing any module loading, 'modprobe' is just being called for > > > any new module alias that shows up in the system, and all of the drivers > > > that match it then get loaded. > > > > The problem is that that doesn't happen when a driver is bound to the > > cpu system device objects. modprobe is just not called for modules that > > match the cpu modalias in that case. > > > > If I call modprobe manually for any of the modules in question, it loads > > and works no problem. > > OK, I know what's up. udev has no rule that would allow it to load stuff on > the basis of MODALIAS if DRIVER is set in the event properties. > > So, the following rule needs to be added to udev rules for things to work > as before: > > DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" > > To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/ Well, that wasn't a good idea, because the original 80-drivers.rules didn't work then, but I renamed the new file (in /etc/udev/rules.d/) to 80-cpu.rules and put this line (alone) into it: ACTION="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" After that change everything works happily again. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-20 9:51 ` Rafael J. Wysocki @ 2013-07-20 11:01 ` Rafael J. Wysocki -1 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 11:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Saturday, July 20, 2013 11:51:55 AM Rafael J. Wysocki wrote: > On Saturday, July 20, 2013 05:06:29 AM Rafael J. Wysocki wrote: > > On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote: > > > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > > > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > > > > Alas, this is not the one I'd like to apply. > > > > > > > > > > With that patch applied, new device objects are created to avoid binding the > > > > > processor driver directly to the cpu system device objects, because that > > > > > apparently confuses udev and it starts to ignore the cpu modalias once the > > > > > driver has been bound to any of those objects. > > > > > > > > > > I've verified in the meantime that this indeed is the case. > > > > > > > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > > > > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > > > > autoloading modules that matched the cpu modalias after a driver had been bound > > > > > to the cpu system device objects and you said "no". However, this time I can > > > > > say with certainty that that really is the case. So, the question now is > > > > > whether or not we can do anything in the kernel to avoid that confusion in udev > > > > > instead of applying the patch linked above (which is beyond ugly in my not so > > > > > humble opinion)? > > > > > > > > udev isn't doing any module loading, 'modprobe' is just being called for > > > > any new module alias that shows up in the system, and all of the drivers > > > > that match it then get loaded. > > > > > > The problem is that that doesn't happen when a driver is bound to the > > > cpu system device objects. modprobe is just not called for modules that > > > match the cpu modalias in that case. > > > > > > If I call modprobe manually for any of the modules in question, it loads > > > and works no problem. > > > > OK, I know what's up. udev has no rule that would allow it to load stuff on > > the basis of MODALIAS if DRIVER is set in the event properties. > > > > So, the following rule needs to be added to udev rules for things to work > > as before: > > > > DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" > > > > To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/ > > Well, that wasn't a good idea, because the original 80-drivers.rules didn't > work then, but I renamed the new file (in /etc/udev/rules.d/) to 80-cpu.rules > and put this line (alone) into it: > > ACTION="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" I made a mistake in the above, which should be: ACTION=="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" sorry about that. > After that change everything works happily again. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. @ 2013-07-20 11:01 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-20 11:01 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Tim Chen, Herbert Xu, Rafael J. Wysocki, Tetsuo Handa, linux-kernel, linux-crypto, ak, H. Peter Anvin, ACPI Devel Maling List On Saturday, July 20, 2013 11:51:55 AM Rafael J. Wysocki wrote: > On Saturday, July 20, 2013 05:06:29 AM Rafael J. Wysocki wrote: > > On Saturday, July 20, 2013 02:00:44 AM Rafael J. Wysocki wrote: > > > On Friday, July 19, 2013 04:16:30 PM Greg Kroah-Hartman wrote: > > > > On Fri, Jul 19, 2013 at 11:38:04PM +0200, Rafael J. Wysocki wrote: > > > > > Alas, this is not the one I'd like to apply. > > > > > > > > > > With that patch applied, new device objects are created to avoid binding the > > > > > processor driver directly to the cpu system device objects, because that > > > > > apparently confuses udev and it starts to ignore the cpu modalias once the > > > > > driver has been bound to any of those objects. > > > > > > > > > > I've verified in the meantime that this indeed is the case. > > > > > > > > > > A link to the patch in question: https://patchwork.kernel.org/patch/2830561/ > > > > > > > > > > Greg, I asked you some time ago whether or not it was possible for udev to stop > > > > > autoloading modules that matched the cpu modalias after a driver had been bound > > > > > to the cpu system device objects and you said "no". However, this time I can > > > > > say with certainty that that really is the case. So, the question now is > > > > > whether or not we can do anything in the kernel to avoid that confusion in udev > > > > > instead of applying the patch linked above (which is beyond ugly in my not so > > > > > humble opinion)? > > > > > > > > udev isn't doing any module loading, 'modprobe' is just being called for > > > > any new module alias that shows up in the system, and all of the drivers > > > > that match it then get loaded. > > > > > > The problem is that that doesn't happen when a driver is bound to the > > > cpu system device objects. modprobe is just not called for modules that > > > match the cpu modalias in that case. > > > > > > If I call modprobe manually for any of the modules in question, it loads > > > and works no problem. > > > > OK, I know what's up. udev has no rule that would allow it to load stuff on > > the basis of MODALIAS if DRIVER is set in the event properties. > > > > So, the following rule needs to be added to udev rules for things to work > > as before: > > > > DRIVER=="processor", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" > > > > To be precise, I added a file called 80-drivers.rules to /etc/udev/rules.d/ > > Well, that wasn't a good idea, because the original 80-drivers.rules didn't > work then, but I renamed the new file (in /etc/udev/rules.d/) to 80-cpu.rules > and put this line (alone) into it: > > ACTION="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" I made a mistake in the above, which should be: ACTION=="add", SUBSYSTEM=="cpu", ENV{MODALIAS}=="?*", IMPORT{builtin}="kmod load $env{MODALIAS}" sorry about that. > After that change everything works happily again. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-18 22:17 ` Rafael J. Wysocki (?) (?) @ 2013-07-18 23:44 ` H. Peter Anvin 2013-07-19 12:57 ` Rafael J. Wysocki -1 siblings, 1 reply; 72+ messages in thread From: H. Peter Anvin @ 2013-07-18 23:44 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Tim Chen, Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak, rjw On 07/18/2013 03:17 PM, Rafael J. Wysocki wrote: >> >> alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul >> >> This should cause udev to load the crct10dif_pclml module when cpu >> support the PCLMULQDQ (feature code 0081). I did my testing during >> development on 3.10 and the module was indeed loaded. >> >> However, I found that the following commit under 3.11-rc1 broke >> the mechanism after some bisection. >> >> commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc >> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Date: Fri May 3 00:26:22 2013 +0200 >> >> ACPI / processor: Use common hotplug infrastructure >> Split the ACPI processor driver into two parts, one that is >> non-modular, resides in the ACPI core and handles the enumeration >> and hotplug of processors and one that implements the rest of the >> existing processor driver functionality. >> Rafael, can you check and see if this can be fixed so those >> optimized >> crypto modules for Intel cpu that support them can be loaded? > > I think this is an ordering issue between udev startup and the time when > devices are registered. > > I wonder what happens if you put those modules into the initramfs image? > OK, this bothers me on some pretty deep level... a set of changes exclusively in drivers/acpi breaking functionality which had nothing to do with ACPI, specifically CPU-feature-based module loading. Please let me know what the investigation comes up with, or if I need to get more directly involved. -hpa ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency. 2013-07-18 23:44 ` H. Peter Anvin @ 2013-07-19 12:57 ` Rafael J. Wysocki 0 siblings, 0 replies; 72+ messages in thread From: Rafael J. Wysocki @ 2013-07-19 12:57 UTC (permalink / raw) To: H. Peter Anvin Cc: Rafael J. Wysocki, Tim Chen, Tetsuo Handa, herbert, linux-kernel, linux-crypto, ak On Thursday, July 18, 2013 04:44:20 PM H. Peter Anvin wrote: > On 07/18/2013 03:17 PM, Rafael J. Wysocki wrote: > >> > >> alias x86cpu:vendor:*:family:*:model:*:feature:*0081* crct10dif_pclmul > >> > >> This should cause udev to load the crct10dif_pclml module when cpu > >> support the PCLMULQDQ (feature code 0081). I did my testing during > >> development on 3.10 and the module was indeed loaded. > >> > >> However, I found that the following commit under 3.11-rc1 broke > >> the mechanism after some bisection. > >> > >> commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc > >> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> Date: Fri May 3 00:26:22 2013 +0200 > >> > >> ACPI / processor: Use common hotplug infrastructure > >> Split the ACPI processor driver into two parts, one that is > >> non-modular, resides in the ACPI core and handles the enumeration > >> and hotplug of processors and one that implements the rest of the > >> existing processor driver functionality. > >> Rafael, can you check and see if this can be fixed so those > >> optimized > >> crypto modules for Intel cpu that support them can be loaded? > > > > I think this is an ordering issue between udev startup and the time when > > devices are registered. > > > > I wonder what happens if you put those modules into the initramfs image? > > > > OK, this bothers me on some pretty deep level... a set of changes > exclusively in drivers/acpi breaking functionality which had nothing to > do with ACPI, specifically CPU-feature-based module loading. Well, they are not exclusively in drivers/acpi, they are in drivers/base/cpu.c too and that most likely is the responsible part. > Please let me know what the investigation comes up with, or if I need to > get more directly involved. I will. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 72+ messages in thread
end of thread, other threads:[~2013-09-13 13:28 UTC | newest] Thread overview: 72+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-16 11:53 [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Tetsuo Handa 2013-07-16 11:55 ` Herbert Xu 2013-07-16 13:49 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency Tetsuo Handa 2013-07-16 16:23 ` Tim Chen 2013-07-16 16:23 ` Tim Chen 2013-07-17 11:52 ` [PATCH 3.11-rc1] crypto: Fix boot failure due tomoduledependency Tetsuo Handa 2013-07-17 11:52 ` Tetsuo Handa 2013-07-17 16:46 ` Tim Chen 2013-07-17 16:46 ` Tim Chen 2013-07-17 20:50 ` [PATCH 3.11-rc1] crypto: Fix boot failure duetomoduledependency Tetsuo Handa 2013-07-17 20:50 ` Tetsuo Handa 2013-07-17 21:53 ` Tim Chen 2013-07-17 21:53 ` Tim Chen 2013-07-17 22:08 ` Tim Chen 2013-07-17 22:08 ` Tim Chen 2013-07-18 3:47 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Tetsuo Handa 2013-07-18 3:47 ` Tetsuo Handa 2013-07-18 21:00 ` Tim Chen 2013-07-18 21:00 ` Tim Chen 2013-07-18 22:17 ` Rafael J. Wysocki 2013-07-18 22:17 ` Rafael J. Wysocki 2013-07-18 23:08 ` Tim Chen 2013-07-18 23:08 ` Tim Chen 2013-07-19 13:03 ` Rafael J. Wysocki 2013-07-19 13:03 ` Rafael J. Wysocki 2013-07-19 14:49 ` Rafael J. Wysocki 2013-07-19 14:49 ` Rafael J. Wysocki 2013-07-19 18:08 ` Tim Chen 2013-07-19 18:08 ` Tim Chen 2013-07-19 21:38 ` Rafael J. Wysocki 2013-07-19 21:38 ` Rafael J. Wysocki 2013-07-19 23:16 ` Greg Kroah-Hartman 2013-07-19 23:16 ` Greg Kroah-Hartman 2013-07-19 23:21 ` H. Peter Anvin 2013-07-19 23:24 ` Herbert Xu 2013-07-19 23:24 ` Herbert Xu 2013-07-19 23:37 ` Tim Chen 2013-07-19 23:37 ` Tim Chen 2013-07-20 1:31 ` Tim Chen 2013-07-20 1:31 ` Tim Chen 2013-07-20 2:19 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency Tetsuo Handa 2013-07-20 2:19 ` Tetsuo Handa 2013-07-20 5:30 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Herbert Xu 2013-07-20 5:30 ` Herbert Xu 2013-07-20 5:56 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to moduledependency Tetsuo Handa 2013-07-20 5:56 ` Tetsuo Handa 2013-09-11 11:43 ` [3.12-rc1] Dependency on module-init-tools >= 3.11 ? Tetsuo Handa 2013-09-12 4:26 ` Herbert Xu 2013-09-12 5:03 ` Tetsuo Handa 2013-09-12 5:28 ` Herbert Xu 2013-09-12 5:28 ` Herbert Xu 2013-09-12 10:20 ` Tetsuo Handa 2013-09-12 10:20 ` Tetsuo Handa 2013-09-12 10:29 ` Herbert Xu 2013-09-12 10:29 ` Herbert Xu 2013-09-12 14:26 ` Waiman Long 2013-09-12 14:26 ` Waiman Long 2013-09-13 13:27 ` Tetsuo Handa 2013-09-13 13:27 ` Tetsuo Handa 2013-09-12 11:12 ` Arthur Marsh 2013-07-19 23:26 ` [PATCH 3.11-rc1] crypto: Fix boot failure due to module dependency Greg Kroah-Hartman 2013-07-19 23:28 ` H. Peter Anvin 2013-07-20 0:00 ` Rafael J. Wysocki 2013-07-20 0:00 ` Rafael J. Wysocki 2013-07-20 3:06 ` Rafael J. Wysocki 2013-07-20 3:06 ` Rafael J. Wysocki 2013-07-20 9:51 ` Rafael J. Wysocki 2013-07-20 9:51 ` Rafael J. Wysocki 2013-07-20 11:01 ` Rafael J. Wysocki 2013-07-20 11:01 ` Rafael J. Wysocki 2013-07-18 23:44 ` H. Peter Anvin 2013-07-19 12:57 ` Rafael J. Wysocki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.