[PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL_MIC

[PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL_MIC

[Vegard Nossum]

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
 	help
 	  This is a driver for KeyStream KS7010 based SDIO WIFI cards. It is
 	  found on at least later Spectec SDW-821 (FCC-ID "S2Y-WLAN-11G-K" only,
-- 
2.23.0.718.g5ad94255a8

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

[Greg Kroah-Hartman]

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(+)

What commit caused these issues?

thanks,

greg k-h

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

[Vegard Nossum]

On 10/11/21 5:37 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(+)
> 
> What commit caused these issues?
> 
> thanks,
> 
> greg k-h
> 

So I found this commit that appears to (attempt to) fix this issue already:

commit 3e5bc68fa596874500e8c718605aa44d5e42a34c
Author: YueHaibing <yuehaibing@huawei.com>
Date:   Fri Jun 21 15:24:55 2019 +0800

    staging: ks7010: Fix build error

    when CRYPTO is m and KS7010 is y, building fails:

    drivers/staging/ks7010/ks_hostif.o: In function
`michael_mic.constprop.13':
    ks_hostif.c:(.text+0x560): undefined reference to `crypto_alloc_shash'
    ks_hostif.c:(.text+0x580): undefined reference to `crypto_shash_setkey'
    ks_hostif.c:(.text+0x5e0): undefined reference to `crypto_destroy_tfm'
    ks_hostif.c:(.text+0x614): undefined reference to `crypto_shash_update'
    ks_hostif.c:(.text+0x62c): undefined reference to `crypto_shash_update'
    ks_hostif.c:(.text+0x648): undefined reference to `crypto_shash_finup'

    Add CRYPTO and CRYPTO_HASH dependencies to fix this.

    Reported-by: Hulk Robot <hulkci@huawei.com>
    Fixes: 8b523f20417d ("staging: ks7010: removed custom Michael MIC
implementation.")
    Signed-off-by: YueHaibing <yuehaibing@huawei.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Notes (mainline):
    Fixes: 8b523f20417d ("staging: ks7010: removed custom Michael MIC
implementation.") # v5.1-rc1
    Lore:
https://lore.kernel.org/r/20190621034221.36708-1-yuehaibing@huawei.com #
driverdev-devel, lkml

However, it was reverted in v5.2-rc6:

commit a4961427e74948ced9ddd40f3efb2a206b87e4c8
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Mon Jun 24 16:45:34 2019 +0800

    Revert "staging: ks7010: Fix build error"

    This reverts commit 3e5bc68fa596874500e8c718605aa44d5e42a34c as it
    causes build errors in linux-next.

    Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
    Cc: YueHaibing <yuehaibing@huawei.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Looks like this was the build error in question:

https://lore.kernel.org/all/20190624173855.3c188955@canb.auug.org.au/

To be honest I don't really see what the full recursive dependency cycle
is, but I think "select" should work instead -- Arnd?

Maybe we can add this:

Fixes: 8b523f20417d ("staging: ks7010: removed custom Michael MIC
implementation.")
Fixes: 3e5bc68fa5968 ("staging: ks7010: Fix build error")
Fixes: a4961427e7494 ("Revert "staging: ks7010: Fix build error"")


Vegard

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

[Greg Kroah-Hartman]

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.

thanks,

greg k-h

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