Re: [PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL_MIC

Re: [PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL_MIC

[Vegard Nossum]

On 10/13/21 2:26 PM, Greg Kroah-Hartman wrote:
> On Mon, Oct 11, 2021 at 05:29:41PM +0200, Vegard Nossum wrote:
>> Fix the following build/link errors:
>>
>>   ld: drivers/staging/ks7010/ks_hostif.o: in function `michael_mic.constprop.0':
>>   ks_hostif.c:(.text+0x95b): undefined reference to `crypto_alloc_shash'
>>   ld: ks_hostif.c:(.text+0x97a): undefined reference to `crypto_shash_setkey'
>>   ld: ks_hostif.c:(.text+0xa13): undefined reference to `crypto_shash_update'
>>   ld: ks_hostif.c:(.text+0xa28): undefined reference to `crypto_shash_update'
>>   ld: ks_hostif.c:(.text+0xa48): undefined reference to `crypto_shash_finup'
>>   ld: ks_hostif.c:(.text+0xa6d): undefined reference to `crypto_destroy_tfm'
>>
>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>> ---
>>  drivers/staging/ks7010/Kconfig | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/staging/ks7010/Kconfig b/drivers/staging/ks7010/Kconfig
>> index 0987fdc2f70db..8ea6c09286798 100644
>> --- a/drivers/staging/ks7010/Kconfig
>> +++ b/drivers/staging/ks7010/Kconfig
>> @@ -5,6 +5,9 @@ config KS7010
>>  	select WIRELESS_EXT
>>  	select WEXT_PRIV
>>  	select FW_LOADER
>> +	select CRYPTO
>> +	select CRYPTO_HASH
>> +	select CRYPTO_MICHAEL_MIC
> 
> Let's try to rely on 'depend' and not 'select' please.

I used 'select' because it seemed to be the established pattern for
these options. Compare:

$ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO$' | wc --lines
1

$ find -name '*Kconfig*' | xargs git grep 'select CRYPTO$' | wc --lines
66

$ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO' | wc --lines
87

$ find -name '*Kconfig*' | xargs git grep 'select CRYPTO' | wc --lines
1005

That said, I have found several other cases where CRYPTO_* algorithms
are getting 'select'-ed without also selecting CRYPTO/CRYPTO_HASH, so I
definitely see the problem you're trying to address.

I've added some more people on Cc to see if there is a consensus on the
best way to do this for the CRYPTO* options going forwards. Thoughts,
anybody?


Vegard

Re: [PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL_MIC

[Arnd Bergmann]

On Wed, Oct 13, 2021 at 2:51 PM Vegard Nossum <vegard.nossum@oracle.com> wrote:
> On 10/13/21 2:26 PM, Greg Kroah-Hartman wrote:
> > On Mon, Oct 11, 2021 at 05:29:41PM +0200, Vegard Nossum wrote:
> >> Fix the following build/link errors:
> >>
> >>   ld: drivers/staging/ks7010/ks_hostif.o: in function `michael_mic.constprop.0':
> >>   ks_hostif.c:(.text+0x95b): undefined reference to `crypto_alloc_shash'
> >>   ld: ks_hostif.c:(.text+0x97a): undefined reference to `crypto_shash_setkey'
> >>   ld: ks_hostif.c:(.text+0xa13): undefined reference to `crypto_shash_update'
> >>   ld: ks_hostif.c:(.text+0xa28): undefined reference to `crypto_shash_update'
> >>   ld: ks_hostif.c:(.text+0xa48): undefined reference to `crypto_shash_finup'
> >>   ld: ks_hostif.c:(.text+0xa6d): undefined reference to `crypto_destroy_tfm'
> >>
> >> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> >> ---
> >>  drivers/staging/ks7010/Kconfig | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/staging/ks7010/Kconfig b/drivers/staging/ks7010/Kconfig
> >> index 0987fdc2f70db..8ea6c09286798 100644
> >> --- a/drivers/staging/ks7010/Kconfig
> >> +++ b/drivers/staging/ks7010/Kconfig
> >> @@ -5,6 +5,9 @@ config KS7010
> >>      select WIRELESS_EXT
> >>      select WEXT_PRIV
> >>      select FW_LOADER
> >> +    select CRYPTO
> >> +    select CRYPTO_HASH
> >> +    select CRYPTO_MICHAEL_MIC
> >
> > Let's try to rely on 'depend' and not 'select' please.
>
> I used 'select' because it seemed to be the established pattern for
> these options.

Yes, this is the correct way to do it here. In general, using "depends on"
is better than "select", especially when crossing subsystem boundaries,
however mixing the two is what really hurts because of the circular
dependencies you'd get.

The only way we could use 'depends on' here would be to change all
the other drivers to do the same.

> Compare:
>
> $ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO$' | wc --lines
> 1
>
> $ find -name '*Kconfig*' | xargs git grep 'select CRYPTO$' | wc --lines
> 66
>
> $ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO' | wc --lines
> 87
>
> $ find -name '*Kconfig*' | xargs git grep 'select CRYPTO' | wc --lines
> 1005
>
> That said, I have found several other cases where CRYPTO_* algorithms
> are getting 'select'-ed without also selecting CRYPTO/CRYPTO_HASH, so I
> definitely see the problem you're trying to address.
>
> I've added some more people on Cc to see if there is a consensus on the
> best way to do this for the CRYPTO* options going forwards. Thoughts,
> anybody?

I don't think there is much point in trying to change the
existing pattern for crypto. We might want to change some of those
that use 'depends on' at the moment for consistency, though most of
those look correct as well.

        Arnd

Re: [PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL_MIC

[Greg Kroah-Hartman]

On Wed, Oct 13, 2021 at 03:03:41PM +0200, Arnd Bergmann wrote:
> On Wed, Oct 13, 2021 at 2:51 PM Vegard Nossum <vegard.nossum@oracle.com> wrote:
> > On 10/13/21 2:26 PM, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 11, 2021 at 05:29:41PM +0200, Vegard Nossum wrote:
> > >> Fix the following build/link errors:
> > >>
> > >>   ld: drivers/staging/ks7010/ks_hostif.o: in function `michael_mic.constprop.0':
> > >>   ks_hostif.c:(.text+0x95b): undefined reference to `crypto_alloc_shash'
> > >>   ld: ks_hostif.c:(.text+0x97a): undefined reference to `crypto_shash_setkey'
> > >>   ld: ks_hostif.c:(.text+0xa13): undefined reference to `crypto_shash_update'
> > >>   ld: ks_hostif.c:(.text+0xa28): undefined reference to `crypto_shash_update'
> > >>   ld: ks_hostif.c:(.text+0xa48): undefined reference to `crypto_shash_finup'
> > >>   ld: ks_hostif.c:(.text+0xa6d): undefined reference to `crypto_destroy_tfm'
> > >>
> > >> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> > >> ---
> > >>  drivers/staging/ks7010/Kconfig | 3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/drivers/staging/ks7010/Kconfig b/drivers/staging/ks7010/Kconfig
> > >> index 0987fdc2f70db..8ea6c09286798 100644
> > >> --- a/drivers/staging/ks7010/Kconfig
> > >> +++ b/drivers/staging/ks7010/Kconfig
> > >> @@ -5,6 +5,9 @@ config KS7010
> > >>      select WIRELESS_EXT
> > >>      select WEXT_PRIV
> > >>      select FW_LOADER
> > >> +    select CRYPTO
> > >> +    select CRYPTO_HASH
> > >> +    select CRYPTO_MICHAEL_MIC
> > >
> > > Let's try to rely on 'depend' and not 'select' please.
> >
> > I used 'select' because it seemed to be the established pattern for
> > these options.
> 
> Yes, this is the correct way to do it here. In general, using "depends on"
> is better than "select", especially when crossing subsystem boundaries,
> however mixing the two is what really hurts because of the circular
> dependencies you'd get.
> 
> The only way we could use 'depends on' here would be to change all
> the other drivers to do the same.
> 
> > Compare:
> >
> > $ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO$' | wc --lines
> > 1
> >
> > $ find -name '*Kconfig*' | xargs git grep 'select CRYPTO$' | wc --lines
> > 66
> >
> > $ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO' | wc --lines
> > 87
> >
> > $ find -name '*Kconfig*' | xargs git grep 'select CRYPTO' | wc --lines
> > 1005
> >
> > That said, I have found several other cases where CRYPTO_* algorithms
> > are getting 'select'-ed without also selecting CRYPTO/CRYPTO_HASH, so I
> > definitely see the problem you're trying to address.
> >
> > I've added some more people on Cc to see if there is a consensus on the
> > best way to do this for the CRYPTO* options going forwards. Thoughts,
> > anybody?
> 
> I don't think there is much point in trying to change the
> existing pattern for crypto. We might want to change some of those
> that use 'depends on' at the moment for consistency, though most of
> those look correct as well.

Ok, I'll take this as-is then.

thanks,

greg k-h