[PATCH] RDMA/siw: Add missing Kconfig selections

[PATCH] RDMA/siw: Add missing Kconfig selections

[Tom Talpey]

The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.

In addition, it improperly "depends on" LIBCRC32C, this should be a
"select", similar to net/sctp and others. As a dependency, SIW fails
to appear in generic configurations.

Signed-off-by: Tom Talpey <tom@talpey.com>
---
  drivers/infiniband/sw/siw/Kconfig | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/Kconfig 
b/drivers/infiniband/sw/siw/Kconfig
index 1b5105cbabae..81b70a3eeb87 100644
--- a/drivers/infiniband/sw/siw/Kconfig
+++ b/drivers/infiniband/sw/siw/Kconfig
@@ -1,7 +1,10 @@
  config RDMA_SIW
         tristate "Software RDMA over TCP/IP (iWARP) driver"
-       depends on INET && INFINIBAND && LIBCRC32C
+       depends on INET && INFINIBAND
         depends on INFINIBAND_VIRT_DMA
+       select LIBCRC32C
+       select CRYPTO
+       select CRYPTO_CRC32C
         help
         This driver implements the iWARP RDMA transport over
         the Linux TCP/IP network stack. It enables a system with a

RE: [PATCH] RDMA/siw: Add missing Kconfig selections

[Bernard Metzler]

> -----Original Message-----
> From: Tom Talpey <tom@talpey.com>
> Sent: Wednesday, 31 August 2022 18:31
> To: Jason Gunthorpe <jgg@nvidia.com>; Leon Romanovsky <leonro@nvidia.com>;
> linux-rdma@vger.kernel.org; Bernard Metzler <BMT@zurich.ibm.com>
> Subject: [EXTERNAL] [PATCH] RDMA/siw: Add missing Kconfig selections
> 
> The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.
> 
> In addition, it improperly "depends on" LIBCRC32C, this should be a
> "select", similar to net/sctp and others. As a dependency, SIW fails
> to appear in generic configurations.
> 

Many thanks for taking care, Tom. Indeed, siw currently
only 'accidentally' benefits from having the two CRYPTO
dependencies selected by other code.
And 'depends' was not correct either, since it limits
siw visibility in kernel configurations.


Thank you!


Acked-by: Bernard Metzler <bmt@zurich.ibm.com>


> Signed-off-by: Tom Talpey <tom@talpey.com>
> ---
>   drivers/infiniband/sw/siw/Kconfig | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/siw/Kconfig
> b/drivers/infiniband/sw/siw/Kconfig
> index 1b5105cbabae..81b70a3eeb87 100644
> --- a/drivers/infiniband/sw/siw/Kconfig
> +++ b/drivers/infiniband/sw/siw/Kconfig
> @@ -1,7 +1,10 @@
>   config RDMA_SIW
>          tristate "Software RDMA over TCP/IP (iWARP) driver"
> -       depends on INET && INFINIBAND && LIBCRC32C
> +       depends on INET && INFINIBAND
>          depends on INFINIBAND_VIRT_DMA
> +       select LIBCRC32C
> +       select CRYPTO
> +       select CRYPTO_CRC32C
>          help
>          This driver implements the iWARP RDMA transport over
>          the Linux TCP/IP network stack. It enables a system with a

Re: [PATCH] RDMA/siw: Add missing Kconfig selections

[Leon Romanovsky]

On Wed, Aug 31, 2022 at 12:30:48PM -0400, Tom Talpey wrote:
> The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.
> 
> In addition, it improperly "depends on" LIBCRC32C, this should be a
> "select", similar to net/sctp and others. As a dependency, SIW fails
> to appear in generic configurations.
> 
> Signed-off-by: Tom Talpey <tom@talpey.com>
> ---
>  drivers/infiniband/sw/siw/Kconfig | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/siw/Kconfig
> b/drivers/infiniband/sw/siw/Kconfig

I don't know how you created the patch and sent it later, but it broken.
1. diff --git line above was broken to two lines. It causes to the
following error:
➜  kernel git:(wip/leon-for-next) ✗ git am -s ./20220831_tom_rdma_siw_add_missing_kconfig_selections.mbx
Applying: RDMA/siw: Add missing Kconfig selections
error: git diff header lacks filename information when removing 1 leading pathname component (line 6)
Patch failed at 0001 RDMA/siw: Add missing Kconfig selections
....
2. You changed tabs to white spaces in the patch itself.
➜  kernel git:(wip/leon-for-next) ✗ git am --reject -s ./20220831_tom_rdma_siw_add_missing_kconfig_selections.mbx
Applying: RDMA/siw: Add missing Kconfig selections
Checking patch drivers/infiniband/sw/siw/Kconfig...
error: while searching for:
 config RDMA_SIW
        tristate "Software RDMA over TCP/IP (iWARP) driver"
       depends on INET && INFINIBAND && LIBCRC32C
        depends on INFINIBAND_VIRT_DMA
        help
        This driver implements the iWARP RDMA transport over
        the Linux TCP/IP network stack. It enables a system with a

error: patch failed: drivers/infiniband/sw/siw/Kconfig:1
Applying patch drivers/infiniband/sw/siw/Kconfig with 1 reject...
Rejected hunk #1.
Patch failed at 0001 RDMA/siw: Add missing Kconfig selections

I fixed everything and applied to -next.

Thanks

Re: [PATCH] RDMA/siw: Add missing Kconfig selections

[Jason Gunthorpe]

On Wed, Aug 31, 2022 at 12:30:48PM -0400, Tom Talpey wrote:
> The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.
> 
> In addition, it improperly "depends on" LIBCRC32C, this should be a
> "select", similar to net/sctp and others. As a dependency, SIW fails
> to appear in generic configurations.
> 
> Signed-off-by: Tom Talpey <tom@talpey.com>
> ---
>  drivers/infiniband/sw/siw/Kconfig | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/siw/Kconfig
> b/drivers/infiniband/sw/siw/Kconfig
> index 1b5105cbabae..81b70a3eeb87 100644
> --- a/drivers/infiniband/sw/siw/Kconfig
> +++ b/drivers/infiniband/sw/siw/Kconfig
> @@ -1,7 +1,10 @@
>  config RDMA_SIW
>         tristate "Software RDMA over TCP/IP (iWARP) driver"
> -       depends on INET && INFINIBAND && LIBCRC32C
> +       depends on INET && INFINIBAND
>         depends on INFINIBAND_VIRT_DMA
> +       select LIBCRC32C
> +       select CRYPTO
> +       select CRYPTO_CRC32C

This is against the kconfig instructions Documentation/kbuild/kconfig-language.rst:

  Note:
        select should be used with care. select will force
        a symbol to a value without visiting the dependencies.
        By abusing select you are able to select a symbol FOO even
        if FOO depends on BAR that is not set.
        In general use select only for non-visible symbols
        (no prompts anywhere) and for symbols with no dependencies.
        That will limit the usefulness but on the other hand avoid
        the illegal configurations all over.

None of them meet that criteria even though other places do abuse
select like this as well.

It looked fine to me the way it was, you are supposed to have to
select libcrc32c manually to make siw appear, and it already brings in
the other symbols.

Jason

Re: [PATCH] RDMA/siw: Add missing Kconfig selections

[Leon Romanovsky]

On Thu, Sep 01, 2022 at 09:25:39AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 31, 2022 at 12:30:48PM -0400, Tom Talpey wrote:
> > The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.
> > 
> > In addition, it improperly "depends on" LIBCRC32C, this should be a
> > "select", similar to net/sctp and others. As a dependency, SIW fails
> > to appear in generic configurations.
> > 
> > Signed-off-by: Tom Talpey <tom@talpey.com>
> > ---
> >  drivers/infiniband/sw/siw/Kconfig | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/sw/siw/Kconfig
> > b/drivers/infiniband/sw/siw/Kconfig
> > index 1b5105cbabae..81b70a3eeb87 100644
> > --- a/drivers/infiniband/sw/siw/Kconfig
> > +++ b/drivers/infiniband/sw/siw/Kconfig
> > @@ -1,7 +1,10 @@
> >  config RDMA_SIW
> >         tristate "Software RDMA over TCP/IP (iWARP) driver"
> > -       depends on INET && INFINIBAND && LIBCRC32C
> > +       depends on INET && INFINIBAND
> >         depends on INFINIBAND_VIRT_DMA
> > +       select LIBCRC32C
> > +       select CRYPTO
> > +       select CRYPTO_CRC32C
> 
> This is against the kconfig instructions Documentation/kbuild/kconfig-language.rst:
> 
>   Note:
>         select should be used with care. select will force
>         a symbol to a value without visiting the dependencies.
>         By abusing select you are able to select a symbol FOO even
>         if FOO depends on BAR that is not set.
>         In general use select only for non-visible symbols
>         (no prompts anywhere) and for symbols with no dependencies.
>         That will limit the usefulness but on the other hand avoid
>         the illegal configurations all over.
> 
> None of them meet that criteria even though other places do abuse
> select like this as well.
> 
> It looked fine to me the way it was, you are supposed to have to
> select libcrc32c manually to make siw appear, and it already brings in
> the other symbols.

He took his snippet from RXE.

Thanks

> 
> Jason

Re: [PATCH] RDMA/siw: Add missing Kconfig selections

[Tom Talpey]

On 9/1/2022 8:32 AM, Leon Romanovsky wrote:
> On Thu, Sep 01, 2022 at 09:25:39AM -0300, Jason Gunthorpe wrote:
>> On Wed, Aug 31, 2022 at 12:30:48PM -0400, Tom Talpey wrote:
>>> The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.
>>>
>>> In addition, it improperly "depends on" LIBCRC32C, this should be a
>>> "select", similar to net/sctp and others. As a dependency, SIW fails
>>> to appear in generic configurations.
>>>
>>> Signed-off-by: Tom Talpey <tom@talpey.com>
>>> ---
>>>   drivers/infiniband/sw/siw/Kconfig | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/sw/siw/Kconfig
>>> b/drivers/infiniband/sw/siw/Kconfig
>>> index 1b5105cbabae..81b70a3eeb87 100644
>>> --- a/drivers/infiniband/sw/siw/Kconfig
>>> +++ b/drivers/infiniband/sw/siw/Kconfig
>>> @@ -1,7 +1,10 @@
>>>   config RDMA_SIW
>>>          tristate "Software RDMA over TCP/IP (iWARP) driver"
>>> -       depends on INET && INFINIBAND && LIBCRC32C
>>> +       depends on INET && INFINIBAND
>>>          depends on INFINIBAND_VIRT_DMA
>>> +       select LIBCRC32C
>>> +       select CRYPTO
>>> +       select CRYPTO_CRC32C
>>
>> This is against the kconfig instructions Documentation/kbuild/kconfig-language.rst:
>>
>>    Note:
>>          select should be used with care. select will force
>>          a symbol to a value without visiting the dependencies.
>>          By abusing select you are able to select a symbol FOO even
>>          if FOO depends on BAR that is not set.
>>          In general use select only for non-visible symbols
>>          (no prompts anywhere) and for symbols with no dependencies.
>>          That will limit the usefulness but on the other hand avoid
>>          the illegal configurations all over.
>>
>> None of them meet that criteria even though other places do abuse
>> select like this as well.
>>
>> It looked fine to me the way it was, you are supposed to have to
>> select libcrc32c manually to make siw appear, and it already brings in
>> the other symbols.
> 
> He took his snippet from RXE.

RXE, net/sctp and many others, actually. It seems backwards for a
subsystem to depend on a library, shouldn't libraries be there for
selecting? If that's invalid, there are a LOT of subsystems to fix.

Leon, thanks for fixing up the patch. As you observed, I couldn't
git-send-email and had to shuffle it to another machine, and not
surprisingly I messed it up. Appreciate your help.

Tom.

Re: [PATCH] RDMA/siw: Add missing Kconfig selections

[Jason Gunthorpe]

On Thu, Sep 01, 2022 at 08:50:12AM -0400, Tom Talpey wrote:
> On 9/1/2022 8:32 AM, Leon Romanovsky wrote:
> > On Thu, Sep 01, 2022 at 09:25:39AM -0300, Jason Gunthorpe wrote:
> > > On Wed, Aug 31, 2022 at 12:30:48PM -0400, Tom Talpey wrote:
> > > > The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.
> > > > 
> > > > In addition, it improperly "depends on" LIBCRC32C, this should be a
> > > > "select", similar to net/sctp and others. As a dependency, SIW fails
> > > > to appear in generic configurations.
> > > > 
> > > > Signed-off-by: Tom Talpey <tom@talpey.com>
> > > > ---
> > > >   drivers/infiniband/sw/siw/Kconfig | 5 ++++-
> > > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/infiniband/sw/siw/Kconfig
> > > > b/drivers/infiniband/sw/siw/Kconfig
> > > > index 1b5105cbabae..81b70a3eeb87 100644
> > > > --- a/drivers/infiniband/sw/siw/Kconfig
> > > > +++ b/drivers/infiniband/sw/siw/Kconfig
> > > > @@ -1,7 +1,10 @@
> > > >   config RDMA_SIW
> > > >          tristate "Software RDMA over TCP/IP (iWARP) driver"
> > > > -       depends on INET && INFINIBAND && LIBCRC32C
> > > > +       depends on INET && INFINIBAND
> > > >          depends on INFINIBAND_VIRT_DMA
> > > > +       select LIBCRC32C
> > > > +       select CRYPTO
> > > > +       select CRYPTO_CRC32C
> > > 
> > > This is against the kconfig instructions Documentation/kbuild/kconfig-language.rst:
> > > 
> > >    Note:
> > >          select should be used with care. select will force
> > >          a symbol to a value without visiting the dependencies.
> > >          By abusing select you are able to select a symbol FOO even
> > >          if FOO depends on BAR that is not set.
> > >          In general use select only for non-visible symbols
> > >          (no prompts anywhere) and for symbols with no dependencies.
> > >          That will limit the usefulness but on the other hand avoid
> > >          the illegal configurations all over.
> > > 
> > > None of them meet that criteria even though other places do abuse
> > > select like this as well.
> > > 
> > > It looked fine to me the way it was, you are supposed to have to
> > > select libcrc32c manually to make siw appear, and it already brings in
> > > the other symbols.
> > 
> > He took his snippet from RXE.
> 
> RXE, net/sctp and many others, actually. It seems backwards for a
> subsystem to depend on a library, shouldn't libraries be there for
> selecting? If that's invalid, there are a LOT of subsystems to fix.

kconfig is a mess unfortunately, and the crypto stuff is kind of
weirdly done to be both a library and a user selectable pluggable..

Jason

Re: [PATCH] RDMA/siw: Add missing Kconfig selections

[Tom Talpey]

On 9/1/2022 8:51 AM, Jason Gunthorpe wrote:
> On Thu, Sep 01, 2022 at 08:50:12AM -0400, Tom Talpey wrote:
>> On 9/1/2022 8:32 AM, Leon Romanovsky wrote:
>>> On Thu, Sep 01, 2022 at 09:25:39AM -0300, Jason Gunthorpe wrote:
>>>> On Wed, Aug 31, 2022 at 12:30:48PM -0400, Tom Talpey wrote:
>>>>> The SoftiWARP Kconfig is missing "select" for CRYPTO and CRYPTO_CRC32C.
>>>>>
>>>>> In addition, it improperly "depends on" LIBCRC32C, this should be a
>>>>> "select", similar to net/sctp and others. As a dependency, SIW fails
>>>>> to appear in generic configurations.
>>>>>
>>>>> Signed-off-by: Tom Talpey <tom@talpey.com>
>>>>> ---
>>>>>    drivers/infiniband/sw/siw/Kconfig | 5 ++++-
>>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/infiniband/sw/siw/Kconfig
>>>>> b/drivers/infiniband/sw/siw/Kconfig
>>>>> index 1b5105cbabae..81b70a3eeb87 100644
>>>>> --- a/drivers/infiniband/sw/siw/Kconfig
>>>>> +++ b/drivers/infiniband/sw/siw/Kconfig
>>>>> @@ -1,7 +1,10 @@
>>>>>    config RDMA_SIW
>>>>>           tristate "Software RDMA over TCP/IP (iWARP) driver"
>>>>> -       depends on INET && INFINIBAND && LIBCRC32C
>>>>> +       depends on INET && INFINIBAND
>>>>>           depends on INFINIBAND_VIRT_DMA
>>>>> +       select LIBCRC32C
>>>>> +       select CRYPTO
>>>>> +       select CRYPTO_CRC32C
>>>>
>>>> This is against the kconfig instructions Documentation/kbuild/kconfig-language.rst:
>>>>
>>>>     Note:
>>>>           select should be used with care. select will force
>>>>           a symbol to a value without visiting the dependencies.
>>>>           By abusing select you are able to select a symbol FOO even
>>>>           if FOO depends on BAR that is not set.
>>>>           In general use select only for non-visible symbols
>>>>           (no prompts anywhere) and for symbols with no dependencies.
>>>>           That will limit the usefulness but on the other hand avoid
>>>>           the illegal configurations all over.
>>>>
>>>> None of them meet that criteria even though other places do abuse
>>>> select like this as well.
>>>>
>>>> It looked fine to me the way it was, you are supposed to have to
>>>> select libcrc32c manually to make siw appear, and it already brings in
>>>> the other symbols.
>>>
>>> He took his snippet from RXE.
>>
>> RXE, net/sctp and many others, actually. It seems backwards for a
>> subsystem to depend on a library, shouldn't libraries be there for
>> selecting? If that's invalid, there are a LOT of subsystems to fix.
> 
> kconfig is a mess unfortunately, and the crypto stuff is kind of
> weirdly done to be both a library and a user selectable pluggable..

For both CRYPTO_CRC32C and  LIBCRC32C, in the unpatched tree, siw is
the outlier:

$ find . -name Kconfig -exec grep -H CRC32C '{}' \;
./crypto/Kconfig:config CRYPTO_CRC32C
./crypto/Kconfig:config CRYPTO_CRC32C_INTEL
./crypto/Kconfig:         support CRC32C implementation using hardware 
accelerated CRC32
./crypto/Kconfig:config CRYPTO_CRC32C_VPMSUM
./crypto/Kconfig:config CRYPTO_CRC32C_SPARC64
./crypto/Kconfig:       depends on CRYPTO_CRCT10DIF_VPMSUM && 
CRYPTO_CRC32C_VPMSUM
./drivers/block/drbd/Kconfig:   select LIBCRC32C
./drivers/block/Kconfig:        select LIBCRC32C
./drivers/infiniband/sw/siw/Kconfig:    depends on INET && INFINIBAND && 
LIBCRC32C
./drivers/md/Kconfig:   select LIBCRC32C
./drivers/md/persistent-data/Kconfig:       select LIBCRC32C
./drivers/net/ethernet/broadcom/Kconfig:        select LIBCRC32C
./drivers/net/ethernet/broadcom/Kconfig:        select LIBCRC32C
./drivers/net/ethernet/cavium/Kconfig:  select LIBCRC32C
./drivers/nvme/host/Kconfig:    select CRYPTO_CRC32C
./drivers/scsi/Kconfig: select CRYPTO_CRC32C
./drivers/target/iscsi/Kconfig: select CRYPTO_CRC32C
./drivers/target/iscsi/Kconfig: select CRYPTO_CRC32C_INTEL if X86
./fs/btrfs/Kconfig:     select CRYPTO_CRC32C
./fs/btrfs/Kconfig:     select LIBCRC32C
./fs/ceph/Kconfig:      select LIBCRC32C
./fs/erofs/Kconfig:     select LIBCRC32C
./fs/ext4/Kconfig:      select CRYPTO_CRC32C
./fs/gfs2/Kconfig:      select LIBCRC32C
./fs/jbd2/Kconfig:      select CRYPTO_CRC32C
./fs/xfs/Kconfig:       select LIBCRC32C
./lib/Kconfig:config LIBCRC32C
./lib/Kconfig:  select CRYPTO_CRC32C
./net/batman-adv/Kconfig:       select LIBCRC32C
./net/ceph/Kconfig:     select LIBCRC32C
./net/netfilter/ipvs/Kconfig:   select LIBCRC32C
./net/netfilter/Kconfig:        select LIBCRC32C
./net/netfilter/Kconfig:        select LIBCRC32C
./net/openvswitch/Kconfig:      select LIBCRC32C
./net/sched/Kconfig:    select LIBCRC32C
./net/sctp/Kconfig:     select LIBCRC32C
$