diff mbox series

CIFS: SMBD: fix configurations with INFINIBAND=m

Message ID 20171219101327.2165797-1-arnd@arndb.de
State New
Headers show
Series CIFS: SMBD: fix configurations with INFINIBAND=m | expand

Commit Message

Arnd Bergmann Dec. 19, 2017, 10:12 a.m. UTC 
A built-in SMB file system cannot link against a modular
infiniband core module:

fs/cifs/smbdirect.o: In function `smbd_destroy_rdma_work':
smbdirect.c:(.text+0x28e3): undefined reference to `ib_drain_qp'
smbdirect.c:(.text+0x2915): undefined reference to `rdma_destroy_qp'
smbdirect.c:(.text+0x2d08): undefined reference to `ib_free_cq'
smbdirect.c:(.text+0x2d3d): undefined reference to `ib_free_cq'
smbdirect.c:(.text+0x2d6f): undefined reference to `ib_dealloc_pd'
smbdirect.c:(.text+0x2d9a): undefined reference to `rdma_destroy_id'
fs/cifs/smbdirect.o: In function `smbd_disconnect_rdma_work':
smbdirect.c:(.text+0x3d3e): undefined reference to `rdma_disconnect'
fs/cifs/smbdirect.o: In function `_smbd_get_connection':
smbdirect.c:(.text+0x5bc2): undefined reference to `rdma_create_id'
smbdirect.c:(.text+0x5cc5): undefined reference to `rdma_resolve_addr'
smbdirect.c:(.text+0x5cda): undefined reference to `rdma_destroy_id'
smbdirect.c:(.text+0x5da7): undefined reference to `rdma_destroy_id'
smbdirect.c:(.text+0x5e89): undefined reference to `rdma_resolve_route'
smbdirect.c:(.text+0x5f07): undefined reference to `rdma_destroy_id'

This changes the Kconfig dependency to enforce selecting one of the
valid configurations.

Fixes: bbc50d2ad317 ("CIFS: SMBD: Introduce kernel config option CONFIG_CIFS_SMB_DIRECT")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 fs/cifs/Kconfig | 1 +
 1 file changed, 1 insertion(+)

-- 
2.9.0

Comments

Stefan Metzmacher Dec. 19, 2017, 10:33 a.m. UTC | #1 
Hi Arnd,

> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig

> index 500fd69fb58b..3bfc55c08bef 100644

> --- a/fs/cifs/Kconfig

> +++ b/fs/cifs/Kconfig

> @@ -199,6 +199,7 @@ config CIFS_SMB311

>  config CIFS_SMB_DIRECT

>  	bool "SMB Direct support (Experimental)"

>  	depends on CIFS && INFINIBAND

> +	depends on CIFS=m || INFINIBAND=y

>  	help

>  	  Enables SMB Direct experimental support for SMB 3.0, 3.02 and 3.1.1.

>  	  SMB Direct allows transferring SMB packets over RDMA. If unsure,


Is this really correct? Should CIFS_SMB_DIRECT be allowed with:

CIFS=n and INFINIBAND=y ???
or
CIFS=m and INFINIBAND=n ???

I guess a more complex logic should be used here
or am I missing something?

metze
Arnd Bergmann Dec. 19, 2017, 10:56 a.m. UTC | #2 
On Tue, Dec 19, 2017 at 11:33 AM, Stefan Metzmacher <metze@samba.org> wrote:
> Hi Arnd,

>

>> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig

>> index 500fd69fb58b..3bfc55c08bef 100644

>> --- a/fs/cifs/Kconfig

>> +++ b/fs/cifs/Kconfig

>> @@ -199,6 +199,7 @@ config CIFS_SMB311

>>  config CIFS_SMB_DIRECT

>>       bool "SMB Direct support (Experimental)"

>>       depends on CIFS && INFINIBAND

>> +     depends on CIFS=m || INFINIBAND=y

>>       help

>>         Enables SMB Direct experimental support for SMB 3.0, 3.02 and 3.1.1.

>>         SMB Direct allows transferring SMB packets over RDMA. If unsure,

>

> Is this really correct? Should CIFS_SMB_DIRECT be allowed with:

>

> CIFS=n and INFINIBAND=y ???

> or

> CIFS=m and INFINIBAND=n ???

>

> I guess a more complex logic should be used here

> or am I missing something?


The two ones you listed are prohibited by the existing
'depends on CIFS && INFINIBAND' dependency.

We could rephrase the dependency as

depends on (CIFS=y && INFINIBAND=y) || \
            (CIFS=m && INFINIBAND=y) || \
            (CIFS=m && INFINIBAND=m)

which has the same effect as

      depends on CIFS && INFINIBAND
      depends on CIFS=m || INFINIBAND=y

but I don't think that adds any clarity.

      Arnd
Stefan Metzmacher Dec. 19, 2017, 11:01 a.m. UTC | #3 
Am 19.12.2017 um 11:56 schrieb Arnd Bergmann via samba-technical:
> On Tue, Dec 19, 2017 at 11:33 AM, Stefan Metzmacher <metze@samba.org> wrote:

>> Hi Arnd,

>>

>>> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig

>>> index 500fd69fb58b..3bfc55c08bef 100644

>>> --- a/fs/cifs/Kconfig

>>> +++ b/fs/cifs/Kconfig

>>> @@ -199,6 +199,7 @@ config CIFS_SMB311

>>>  config CIFS_SMB_DIRECT

>>>       bool "SMB Direct support (Experimental)"

>>>       depends on CIFS && INFINIBAND

>>> +     depends on CIFS=m || INFINIBAND=y

>>>       help

>>>         Enables SMB Direct experimental support for SMB 3.0, 3.02 and 3.1.1.

>>>         SMB Direct allows transferring SMB packets over RDMA. If unsure,

>>

>> Is this really correct? Should CIFS_SMB_DIRECT be allowed with:

>>

>> CIFS=n and INFINIBAND=y ???

>> or

>> CIFS=m and INFINIBAND=n ???

>>

>> I guess a more complex logic should be used here

>> or am I missing something?

> 

> The two ones you listed are prohibited by the existing

> 'depends on CIFS && INFINIBAND' dependency.

> 

> We could rephrase the dependency as

> 

> depends on (CIFS=y && INFINIBAND=y) || \

>             (CIFS=m && INFINIBAND=y) || \

>             (CIFS=m && INFINIBAND=m)

> 

> which has the same effect as

> 

>       depends on CIFS && INFINIBAND

>       depends on CIFS=m || INFINIBAND=y

> 

> but I don't think that adds any clarity.


Thanks for the clarification!

I somehow assumed the patch has been:


-      depends on CIFS && INFINIBAND
+      depends on CIFS=m || INFINIBAND=y

instead of:
       depends on CIFS && INFINIBAND
+      depends on CIFS=m || INFINIBAND=y

metze
Jean Delvare Dec. 19, 2017, 12:39 p.m. UTC | #4 
On Tue, 19 Dec 2017 11:12:57 +0100, Arnd Bergmann wrote:
> A built-in SMB file system cannot link against a modular

> infiniband core module:

> 

> fs/cifs/smbdirect.o: In function `smbd_destroy_rdma_work':

> smbdirect.c:(.text+0x28e3): undefined reference to `ib_drain_qp'

> smbdirect.c:(.text+0x2915): undefined reference to `rdma_destroy_qp'

> smbdirect.c:(.text+0x2d08): undefined reference to `ib_free_cq'

> smbdirect.c:(.text+0x2d3d): undefined reference to `ib_free_cq'

> smbdirect.c:(.text+0x2d6f): undefined reference to `ib_dealloc_pd'

> smbdirect.c:(.text+0x2d9a): undefined reference to `rdma_destroy_id'

> fs/cifs/smbdirect.o: In function `smbd_disconnect_rdma_work':

> smbdirect.c:(.text+0x3d3e): undefined reference to `rdma_disconnect'

> fs/cifs/smbdirect.o: In function `_smbd_get_connection':

> smbdirect.c:(.text+0x5bc2): undefined reference to `rdma_create_id'

> smbdirect.c:(.text+0x5cc5): undefined reference to `rdma_resolve_addr'

> smbdirect.c:(.text+0x5cda): undefined reference to `rdma_destroy_id'

> smbdirect.c:(.text+0x5da7): undefined reference to `rdma_destroy_id'

> smbdirect.c:(.text+0x5e89): undefined reference to `rdma_resolve_route'

> smbdirect.c:(.text+0x5f07): undefined reference to `rdma_destroy_id'

> 

> This changes the Kconfig dependency to enforce selecting one of the

> valid configurations.

> 

> Fixes: bbc50d2ad317 ("CIFS: SMBD: Introduce kernel config option CONFIG_CIFS_SMB_DIRECT")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  fs/cifs/Kconfig | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig

> index 500fd69fb58b..3bfc55c08bef 100644

> --- a/fs/cifs/Kconfig

> +++ b/fs/cifs/Kconfig

> @@ -199,6 +199,7 @@ config CIFS_SMB311

>  config CIFS_SMB_DIRECT

>  	bool "SMB Direct support (Experimental)"

>  	depends on CIFS && INFINIBAND

> +	depends on CIFS=m || INFINIBAND=y

>  	help

>  	  Enables SMB Direct experimental support for SMB 3.0, 3.02 and 3.1.1.

>  	  SMB Direct allows transferring SMB packets over RDMA. If unsure,


Looks good to me.

Reviewed-by: Jean Delvare <jdelvare@suse.de>


-- 
Jean Delvare
SUSE L3 Support
Long Li Dec. 19, 2017, 9:21 p.m. UTC | #5 
>  	depends on CIFS && INFINIBAND

> +	depends on CIFS=m || INFINIBAND=y


How about we change them to

        depends on CIFS=m && INFINIBAND || CIFS=y && INFINIBAND=y

This makes it easy to read.
Stefan Metzmacher Dec. 20, 2017, 6:34 a.m. UTC | #6 
Am 19.12.2017 um 22:21 schrieb Long Li via samba-technical:
>>  	depends on CIFS && INFINIBAND

>> +	depends on CIFS=m || INFINIBAND=y

> 

> How about we change them to

> 

>         depends on CIFS=m && INFINIBAND || CIFS=y && INFINIBAND=y

> 

> This makes it easy to read.


I like it :-)

metze
Arnd Bergmann Dec. 20, 2017, 8:17 a.m. UTC | #7 
On Tue, Dec 19, 2017 at 10:21 PM, Long Li <longli@microsoft.com> wrote:
>>       depends on CIFS && INFINIBAND

>> +     depends on CIFS=m || INFINIBAND=y

>

> How about we change them to

>

>         depends on CIFS=m && INFINIBAND || CIFS=y && INFINIBAND=y

>

> This makes it easy to read.


Yes, that seems fine. I would normally group them using () to avoid
any confusion with operator precedence, but your version is
also correct, so just pick the one you like best.

       Arnd
diff mbox series

Patch

diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
index 500fd69fb58b..3bfc55c08bef 100644
--- a/fs/cifs/Kconfig
+++ b/fs/cifs/Kconfig
@@ -199,6 +199,7 @@  config CIFS_SMB311
 config CIFS_SMB_DIRECT
 	bool "SMB Direct support (Experimental)"
 	depends on CIFS && INFINIBAND
+	depends on CIFS=m || INFINIBAND=y
 	help
 	  Enables SMB Direct experimental support for SMB 3.0, 3.02 and 3.1.1.
 	  SMB Direct allows transferring SMB packets over RDMA. If unsure,