On Thu, May 07, 2015 at 08:06:35PM +0200, Paul Bolle wrote:
> On Thu, 2015-05-07 at 10:42 +0200, Paul Bolle wrote:
> > On Wed, 2015-05-06 at 11:33 +0800, Herbert Xu wrote:
> > > On Tue, May 05, 2015 at 05:44:21PM -0700, Luis R. Rodriguez wrote:
> > > > From: "Luis R. Rodriguez" <mcgrof at suse.com>
> > > >
> > > > We're going to add firmware module signing support, but when we do
> > > > this we end up with the following recursive dependency. Fix this by
> > > > just depending on FW_LOADER, which is typically always enabled
> > > > anyway.
> > > >
> > > > mcgrof at ergon ~/linux-next (git::master)$ make allnoconfig
> > > > scripts/kconfig/conf --allnoconfig Kconfig
> > > > crypto/Kconfig:15:error: recursive dependency detected!
> > > > crypto/Kconfig:15: symbol CRYPTO is selected by SYSDATA_SIG
> > > > init/Kconfig:1880: symbol SYSDATA_SIG is selected by FIRMWARE_SIG
> > > > drivers/base/Kconfig:88: symbol FIRMWARE_SIG depends on FW_LOADER
> > > > drivers/base/Kconfig:80: symbol FW_LOADER is selected by CRYPTO_DEV_QAT
> > > > drivers/crypto/qat/Kconfig:1: symbol CRYPTO_DEV_QAT is selected by CRYPTO_DEV_QAT_DH895xCC
> > > > drivers/crypto/qat/Kconfig:13: symbol CRYPTO_DEV_QAT_DH895xCC depends on CRYPTO
> > >
> > > This doesn't look like a real cycle to me so perhaps we can fix
> > > kbuild to understand this?
> >
> > (Dependency circles involving selects still hurt my brain.)
> >
> > Perhaps Luis should have another look at 02/12. See that patch adds this
> > Kconfig entry to init/Kconfig:
> > config SYSDATA_SIG
> > def_bool y
> > select SYSTEM_TRUSTED_KEYRING
> > select KEYS
> > select CRYPTO
> > select ASYMMETRIC_KEY_TYPE
> > select ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> > select PUBLIC_KEY_ALGO_RSA
> > select ASN1
> > select OID_REGISTRY
> > select X509_CERTIFICATE_PARSER
> >
> > As far as I can see this is not enclosed in anything that adds any
> > dependencies. So that basically means that SYSDATA_SIG will always be
> > set, for all architectures (because I think all arches source
> > init/Kconfig). That should make it a pretty pointless symbol (except for
> > the fact that it does trigger all those selects).
> >
> > The same patch also adds
> > select SYSDATA_SIG
> >
> > to the entry for MODULE_SIG. But to me that looks like a nop, because
> > SYSDATA_SIG will be set anyhow. So, but this is just I guess, the
> > problem might go away if
> > def_bool y
> >
> > is changed to just
> > bool
>
> No, it doesn't. But the change I propose still makes sense, anyway.
Thanks, yeah that should be fixed to def_bool n. I had tried def_bool n
before my submission and that didn't fix it, for some reason I forgot to
ammend that change.
> > (Note that I haven't actually tested anything here, and it wouldn't be
> > the first time my reasoning about Kconfig patches is completely off.)
>
> Hear, hear!
>
> > And, whatever the value of my analysis, adding a Kconfig problem in
> > 02/12 just to fix it in 03/12 is a bit silly. I think the patches should
> > be squashed if the problem can't be solved any other way.
>
> It seems the circular dependency warning is triggered by 5/12.
>
> And, having now fiddled a bit with this series, I think the approach
> taken in this patch might actually be preferable treewide.
>
> See, FW_LOADER is 'y' unless EXPERT is set and one goes to the trouble
> of setting FW_LOADER to 'n'. So in the 100+ places where FW_LOADER is
> selected, that is done for, almost always, no immediate benefit.
> Changing those places to use
> depends on FW_LOADER
>
> should have no effect, I think. Except for the EXPERT people not wanting
> FW_LOADER. But that would be putting the burden where it belongs, I'd
> say.
I think this is a correct assessment but only because FW_LOADER is
exposed as optional via EXPERT mode. We can go on a cursade for that
if folks are OK with that.
> Am I missing something here?
Yeah, I think this issue is deeper and *must* be fixed as otherwise
we can't later do more complex intersection dependencies. What we
do *for now* -- perhaps my patch is OK then given your assessemnt,
but for our TODO item, we must keep track that we need to fix this.
Let me explain.
Based on a closer look at the qat Kconfig file I think the issue
might be that for some reason kbuild is assuming that a symbol's
select's and their own dependencies are in and of themselves
related dependencies, that is incorrect. Although the request_firmware()
call is done within the common code (CRYPTO_DEV_QAT), if just for
testing purposes of my point we move "select FW_LOADER" to
CRYPTO_DEV_QAT_DH895xCC the dependency issue becomes clearer:
mcgrof at ergon ~/linux-next (git::your-recursive-qat-mom)$ make allnoconfig
scripts/kconfig/conf --allnoconfig Kconfig
crypto/Kconfig:15:error: recursive dependency detected!
crypto/Kconfig:15: symbol CRYPTO is selected by SYSDATA_SIG
init/Kconfig:1880: symbol SYSDATA_SIG is selected by FIRMWARE_SIG
drivers/base/Kconfig:88: symbol FIRMWARE_SIG depends on FW_LOADER
drivers/base/Kconfig:80: symbol FW_LOADER is selected by CRYPTO_DEV_QAT_DH895xCC
drivers/crypto/qat/Kconfig:12: symbol CRYPTO_DEV_QAT_DH895xCC depends on CRYPTO
#
# configuration written to .config
#
I'm saying that *if* the request_firmware() call was in
CRYPTO_DEV_QAT_DH895xCC it would be wrong for kbuild to think
that CRYPTO is somehow a dependency for FW_LOADER, although
the above does not say that I think I thinks that as otherwise
I cannot see why this would be considered a recursive dependency.
This issue can be made clear by just removing as a test
CRYPTO_DEV_QAT all together and having CRYPTO_DEV_QAT_DH895xCC
select CRYPTO:
config CRYPTO_DEV_QAT_DH895xCC
tristate "Support for Intel(R) DH895xCC"
depends on X86 && PCI
default n
select CRYPTO
select FW_LOADER
help
Support for Intel(R) DH895xcc with Intel(R) QuickAssist Technology
for accelerating crypto and compression workloads.
To compile this as a module, choose M here: the module
will be called qat_dh895xcc.
mcgrof at ergon ~/linux-next (git::your-recursive-qat-mom)$ make allnoconfig
scripts/kconfig/conf --allnoconfig Kconfig
crypto/Kconfig:15:error: recursive dependency detected!
crypto/Kconfig:15: symbol CRYPTO is selected by SYSDATA_SIG
init/Kconfig:1880: symbol SYSDATA_SIG is selected by FIRMWARE_SIG
drivers/base/Kconfig:88: symbol FIRMWARE_SIG depends on FW_LOADER
drivers/base/Kconfig:80: symbol FW_LOADER is selected by CRYPTO_DEV_QAT_DH895xCC
drivers/crypto/qat/Kconfig:1: symbol CRYPTO_DEV_QAT_DH895xCC depends on CRYPTO
#
# configuration written to .config
#
So it should not mean that if CRYPTO_DEV_QAT_DH895xCC's selects something
that that select depends on other of CRYPTO_DEV_QAT_DH895xCC's
selects. For instance in this case it would would mean that we
could not negate a feature that other drivers that selected FW_LOADER
enabled.
Here's a simple test Kconfig entry one can use to test this:
Let's say rock climbers hate locker rooms, but swimmer need them. We can
then have:
config GYM
tristate
default n
config LOCKER
tristate
default n
depends on GYM
config SWIMMING
tristate
default n
select GYM
select LOCKER
config ROCK_CLIMBING
tristate
default n
depends on !LOCKER
select GYM
Kbuild seems to believe that because swimmers need lockers that rock climbers
need them too. That is obviously not true.
mcgrof at ergon ~/linux-next (git::your-swimming-dad)$ make allnoconfig
scripts/kconfig/conf --allnoconfig Kconfig
drivers/crypto/qat/Kconfig:25:error: recursive dependency detected!
drivers/crypto/qat/Kconfig:25: symbol GYM is selected by ROCK_CLIMBING
drivers/crypto/qat/Kconfig:40: symbol ROCK_CLIMBING depends on LOCKER
drivers/crypto/qat/Kconfig:29: symbol LOCKER depends on GYM
#
# configuration written to .config
#
So kbuild does not accept intersection of a feature as a possible outlet
for a dependency, it wants things very atomic. In the FW_SIG case we do
want to enable FW_LOADER but not have all drivers require CRYPTO. The
issue is created because kbuild thinks FW_LOADER depends on CRYPTO
given that a driver selects it.
Luis
