| Message ID | 20200629123003.1014387-1-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 95a62311edf3677c5b1d5ab74380dfa51e5f6b03 |
| Headers | show |
| Series | [1/1] crypto: ux500: hash: Add namespacing to hash_init() | expand |
On Mon, Jun 29, 2020 at 2:30 PM Lee Jones <lee.jones@linaro.org> wrote: > A recent change to the Regulator consumer API (which this driver > utilises) add prototypes for the some suspend functions. These > functions require including header file include/linux/suspend.h. > > The following tree of includes affecting this driver will be > present: > > In file included from include/linux/elevator.h:6, > from include/linux/blkdev.h:288, > from include/linux/blk-cgroup.h:23, > from include/linux/writeback.h:14, > from include/linux/memcontrol.h:22, > from include/linux/swap.h:9, > from include/linux/suspend.h:5, > from include/linux/regulator/consumer.h:35, > from drivers/crypto/ux500/hash/hash_core.c:28: > > include/linux/elevator.h pulls in include/linux/hashtable.h which > contains its own version of hash_init(). This confuses the build > system and results in the following error (amongst others): > > drivers/crypto/ux500/hash/hash_core.c:1362:19: error: passing argument 1 of '__hash_init' from incompatible pointer type [-Werror=incompatible-pointer-types] > 1362 | return hash_init(req); > > Fix this by namespacing the local hash_init() such that the > source of confusion is removed. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David S. Miller <davem@davemloft.net> > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Lee Jones <lee.jones@linaro.org> This looks reasonable. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Mon, Jun 29, 2020 at 01:30:03PM +0100, Lee Jones wrote: > A recent change to the Regulator consumer API (which this driver > utilises) add prototypes for the some suspend functions. These > functions require including header file include/linux/suspend.h. > > The following tree of includes affecting this driver will be > present: > > In file included from include/linux/elevator.h:6, > from include/linux/blkdev.h:288, > from include/linux/blk-cgroup.h:23, > from include/linux/writeback.h:14, > from include/linux/memcontrol.h:22, > from include/linux/swap.h:9, > from include/linux/suspend.h:5, > from include/linux/regulator/consumer.h:35, > from drivers/crypto/ux500/hash/hash_core.c:28: > > include/linux/elevator.h pulls in include/linux/hashtable.h which > contains its own version of hash_init(). This confuses the build > system and results in the following error (amongst others): > > drivers/crypto/ux500/hash/hash_core.c:1362:19: error: passing argument 1 of '__hash_init' from incompatible pointer type [-Werror=incompatible-pointer-types] > 1362 | return hash_init(req); > > Fix this by namespacing the local hash_init() such that the > source of confusion is removed. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David S. Miller <davem@davemloft.net> > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > > Ideally this should go into v5.8's -rcs else it runs the risk of > breaking when Linus pulls everything in for v5.9-rc1. I have no objections to this patch. However, I'd rather put it on a topic branch which you could pull rather than pushing it into 5.8 straight away. I also dislike pulling in the kitchen sink when all you need in consumer.h is the definition of suspend_state_t. A better solution would be to move the definition of suspend_state_t into linux/types.h and including that instead of suspend.h in consumer.h. Cheers,
On Tue, 30 Jun 2020, Herbert Xu wrote: > On Mon, Jun 29, 2020 at 01:30:03PM +0100, Lee Jones wrote: > > A recent change to the Regulator consumer API (which this driver > > utilises) add prototypes for the some suspend functions. These > > functions require including header file include/linux/suspend.h. > > > > The following tree of includes affecting this driver will be > > present: > > > > In file included from include/linux/elevator.h:6, > > from include/linux/blkdev.h:288, > > from include/linux/blk-cgroup.h:23, > > from include/linux/writeback.h:14, > > from include/linux/memcontrol.h:22, > > from include/linux/swap.h:9, > > from include/linux/suspend.h:5, > > from include/linux/regulator/consumer.h:35, > > from drivers/crypto/ux500/hash/hash_core.c:28: > > > > include/linux/elevator.h pulls in include/linux/hashtable.h which > > contains its own version of hash_init(). This confuses the build > > system and results in the following error (amongst others): > > > > drivers/crypto/ux500/hash/hash_core.c:1362:19: error: passing argument 1 of '__hash_init' from incompatible pointer type [-Werror=incompatible-pointer-types] > > 1362 | return hash_init(req); > > > > Fix this by namespacing the local hash_init() such that the > > source of confusion is removed. > > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > Cc: David S. Miller <davem@davemloft.net> > > Cc: linux-crypto@vger.kernel.org > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > > > Ideally this should go into v5.8's -rcs else it runs the risk of > > breaking when Linus pulls everything in for v5.9-rc1. [...] > I also dislike pulling in the kitchen sink when all you need in > consumer.h is the definition of suspend_state_t. A better solution > would be to move the definition of suspend_state_t into linux/types.h > and including that instead of suspend.h in consumer.h. IMHO, including (whole) headers into source/header files is the norm. Even if only a small portion is actually referenced. Very seldom do consumers of an API use more than a fraction of what is available. Whether it's a couple of function calls, a struct or a type. Pulling headers apart and placing items in more convenient places i.e. into headers which are more commonly included, messes with the compartmentalisation of subsystems and sounds like more of a hack than simply saying "to enable suspend functions we need to reference the suspend API" like we are here. > I have no objections to this patch. However, I'd rather put > it on a topic branch which you could pull rather than pushing > it into 5.8 straight away. An immutable branch sounds like a sensible solution. Thanks.
On Tue, 30 Jun 2020, Lee Jones wrote: > On Tue, 30 Jun 2020, Herbert Xu wrote: > > On Mon, Jun 29, 2020 at 01:30:03PM +0100, Lee Jones wrote: > > > A recent change to the Regulator consumer API (which this driver > > > utilises) add prototypes for the some suspend functions. These > > > functions require including header file include/linux/suspend.h. > > > > > > The following tree of includes affecting this driver will be > > > present: > > > > > > In file included from include/linux/elevator.h:6, > > > from include/linux/blkdev.h:288, > > > from include/linux/blk-cgroup.h:23, > > > from include/linux/writeback.h:14, > > > from include/linux/memcontrol.h:22, > > > from include/linux/swap.h:9, > > > from include/linux/suspend.h:5, > > > from include/linux/regulator/consumer.h:35, > > > from drivers/crypto/ux500/hash/hash_core.c:28: > > > > > > include/linux/elevator.h pulls in include/linux/hashtable.h which > > > contains its own version of hash_init(). This confuses the build > > > system and results in the following error (amongst others): > > > > > > drivers/crypto/ux500/hash/hash_core.c:1362:19: error: passing argument 1 of '__hash_init' from incompatible pointer type [-Werror=incompatible-pointer-types] > > > 1362 | return hash_init(req); > > > > > > Fix this by namespacing the local hash_init() such that the > > > source of confusion is removed. > > > > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > > Cc: David S. Miller <davem@davemloft.net> > > > Cc: linux-crypto@vger.kernel.org > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > --- > > > > > > Ideally this should go into v5.8's -rcs else it runs the risk of > > > breaking when Linus pulls everything in for v5.9-rc1. > > [...] > > > I have no objections to this patch. However, I'd rather put > > it on a topic branch which you could pull rather than pushing > > it into 5.8 straight away. > > An immutable branch sounds like a sensible solution. Thanks. Any movement on this Herbert?
On Mon, Jun 29, 2020 at 01:30:03PM +0100, Lee Jones wrote: > A recent change to the Regulator consumer API (which this driver > utilises) add prototypes for the some suspend functions. These > functions require including header file include/linux/suspend.h. > > The following tree of includes affecting this driver will be > present: > > In file included from include/linux/elevator.h:6, > from include/linux/blkdev.h:288, > from include/linux/blk-cgroup.h:23, > from include/linux/writeback.h:14, > from include/linux/memcontrol.h:22, > from include/linux/swap.h:9, > from include/linux/suspend.h:5, > from include/linux/regulator/consumer.h:35, > from drivers/crypto/ux500/hash/hash_core.c:28: > > include/linux/elevator.h pulls in include/linux/hashtable.h which > contains its own version of hash_init(). This confuses the build > system and results in the following error (amongst others): > > drivers/crypto/ux500/hash/hash_core.c:1362:19: error: passing argument 1 of '__hash_init' from incompatible pointer type [-Werror=incompatible-pointer-types] > 1362 | return hash_init(req); > > Fix this by namespacing the local hash_init() such that the > source of confusion is removed. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David S. Miller <davem@davemloft.net> > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > > Ideally this should go into v5.8's -rcs else it runs the risk of > breaking when Linus pulls everything in for v5.9-rc1. > > drivers/crypto/ux500/hash/hash_core.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) Patch applied to ux500 branch. Thanks.
diff --git a/drivers/crypto/ux500/hash/hash_core.c b/drivers/crypto/ux500/hash/hash_core.c index c24f2db8d5e83..a5ee8c2fb4e0b 100644 --- a/drivers/crypto/ux500/hash/hash_core.c +++ b/drivers/crypto/ux500/hash/hash_core.c @@ -545,7 +545,7 @@ static bool hash_dma_valid_data(struct scatterlist *sg, int datasize) * * Initialize structures. */ -static int hash_init(struct ahash_request *req) +static int ux500_hash_init(struct ahash_request *req) { struct crypto_ahash *tfm = crypto_ahash_reqtfm(req); struct hash_ctx *ctx = crypto_ahash_ctx(tfm); @@ -1359,7 +1359,7 @@ static int ahash_sha1_init(struct ahash_request *req) ctx->config.oper_mode = HASH_OPER_MODE_HASH; ctx->digestsize = SHA1_DIGEST_SIZE; - return hash_init(req); + return ux500_hash_init(req); } static int ahash_sha256_init(struct ahash_request *req) @@ -1372,7 +1372,7 @@ static int ahash_sha256_init(struct ahash_request *req) ctx->config.oper_mode = HASH_OPER_MODE_HASH; ctx->digestsize = SHA256_DIGEST_SIZE; - return hash_init(req); + return ux500_hash_init(req); } static int ahash_sha1_digest(struct ahash_request *req) @@ -1425,7 +1425,7 @@ static int hmac_sha1_init(struct ahash_request *req) ctx->config.oper_mode = HASH_OPER_MODE_HMAC; ctx->digestsize = SHA1_DIGEST_SIZE; - return hash_init(req); + return ux500_hash_init(req); } static int hmac_sha256_init(struct ahash_request *req) @@ -1438,7 +1438,7 @@ static int hmac_sha256_init(struct ahash_request *req) ctx->config.oper_mode = HASH_OPER_MODE_HMAC; ctx->digestsize = SHA256_DIGEST_SIZE; - return hash_init(req); + return ux500_hash_init(req); } static int hmac_sha1_digest(struct ahash_request *req) @@ -1515,7 +1515,7 @@ static struct hash_algo_template hash_algs[] = { .conf.algorithm = HASH_ALGO_SHA1, .conf.oper_mode = HASH_OPER_MODE_HASH, .hash = { - .init = hash_init, + .init = ux500_hash_init, .update = ahash_update, .final = ahash_final, .digest = ahash_sha1_digest, @@ -1538,7 +1538,7 @@ static struct hash_algo_template hash_algs[] = { .conf.algorithm = HASH_ALGO_SHA256, .conf.oper_mode = HASH_OPER_MODE_HASH, .hash = { - .init = hash_init, + .init = ux500_hash_init, .update = ahash_update, .final = ahash_final, .digest = ahash_sha256_digest, @@ -1561,7 +1561,7 @@ static struct hash_algo_template hash_algs[] = { .conf.algorithm = HASH_ALGO_SHA1, .conf.oper_mode = HASH_OPER_MODE_HMAC, .hash = { - .init = hash_init, + .init = ux500_hash_init, .update = ahash_update, .final = ahash_final, .digest = hmac_sha1_digest, @@ -1585,7 +1585,7 @@ static struct hash_algo_template hash_algs[] = { .conf.algorithm = HASH_ALGO_SHA256, .conf.oper_mode = HASH_OPER_MODE_HMAC, .hash = { - .init = hash_init, + .init = ux500_hash_init, .update = ahash_update, .final = ahash_final, .digest = hmac_sha256_digest,
A recent change to the Regulator consumer API (which this driver utilises) add prototypes for the some suspend functions. These functions require including header file include/linux/suspend.h. The following tree of includes affecting this driver will be present: In file included from include/linux/elevator.h:6, from include/linux/blkdev.h:288, from include/linux/blk-cgroup.h:23, from include/linux/writeback.h:14, from include/linux/memcontrol.h:22, from include/linux/swap.h:9, from include/linux/suspend.h:5, from include/linux/regulator/consumer.h:35, from drivers/crypto/ux500/hash/hash_core.c:28: include/linux/elevator.h pulls in include/linux/hashtable.h which contains its own version of hash_init(). This confuses the build system and results in the following error (amongst others): drivers/crypto/ux500/hash/hash_core.c:1362:19: error: passing argument 1 of '__hash_init' from incompatible pointer type [-Werror=incompatible-pointer-types] 1362 | return hash_init(req); Fix this by namespacing the local hash_init() such that the source of confusion is removed. Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: David S. Miller <davem@davemloft.net> Cc: linux-crypto@vger.kernel.org Signed-off-by: Lee Jones <lee.jones@linaro.org> --- Ideally this should go into v5.8's -rcs else it runs the risk of breaking when Linus pulls everything in for v5.9-rc1. drivers/crypto/ux500/hash/hash_core.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)