From: Lee Jones <lee.jones@linaro.org> To: Herbert Xu <herbert@gondor.apana.org.au> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, "David S . Miller" <davem@davemloft.net>, linux-crypto@vger.kernel.org Subject: Re: [PATCH 1/1] crypto: ux500: hash: Add namespacing to hash_init() Date: Tue, 30 Jun 2020 08:07:27 +0100 [thread overview] Message-ID: <20200630070727.GD1179328@dell> (raw) In-Reply-To: <20200630041029.GA20892@gondor.apana.org.au> 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. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
WARNING: multiple messages have this Message-ID (diff)
From: Lee Jones <lee.jones@linaro.org> To: Herbert Xu <herbert@gondor.apana.org.au> Cc: linux-crypto@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "David S . Miller" <davem@davemloft.net> Subject: Re: [PATCH 1/1] crypto: ux500: hash: Add namespacing to hash_init() Date: Tue, 30 Jun 2020 08:07:27 +0100 [thread overview] Message-ID: <20200630070727.GD1179328@dell> (raw) In-Reply-To: <20200630041029.GA20892@gondor.apana.org.au> 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. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-06-30 7:07 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-29 12:30 [PATCH 1/1] crypto: ux500: hash: Add namespacing to hash_init() Lee Jones 2020-06-29 12:30 ` Lee Jones 2020-06-29 13:58 ` Linus Walleij 2020-06-29 13:58 ` Linus Walleij 2020-06-30 4:10 ` Herbert Xu 2020-06-30 4:10 ` Herbert Xu 2020-06-30 7:07 ` Lee Jones [this message] 2020-06-30 7:07 ` Lee Jones 2020-07-07 8:26 ` Lee Jones 2020-07-07 8:26 ` Lee Jones 2020-07-09 12:53 ` Herbert Xu 2020-07-09 12:53 ` Herbert Xu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200630070727.GD1179328@dell \ --to=lee.jones@linaro.org \ --cc=davem@davemloft.net \ --cc=herbert@gondor.apana.org.au \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.