| Message ID | 20220304211524.10706-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next?] net: fungible: fix multiple build problems | expand |
On Fri, Mar 4, 2022 at 1:15 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > It is currently possible to have CONFIG_FUN_ETH=y and CONFIG_TLS=m. This combination is allowed if TLS_DEVICE=n, which I suspect you have in your config. I don't think you can get the combination otherwise. In that case the driver doesn't have TLS support and doesn't need anything from the TLS module. The compile problems you see I think come from the next item. > This causes build errors. Therefore FUN_ETH should > depend on TLS || TLS=n > > TLS_DEVICE is a bool symbol, so there is no need to test it as > depends on TLS && TLS_DEVICE || TLS_DEVICE=n > > And due to rules of precedence, the above means > depends on (TLS && TLS_DEVICE) || TLS_DEVICE=n > but that's probably not what was meant here. More likely it > should have been > depends on TLS && (TLS_DEVICE || TLS_DEVICE=n) > > That no longer matters. > > Also, gcc 7.5 does not handle the "C language" vs. #ifdef preprocessor > usage of IS_ENABLED() very well -- it is causing compile errors. I believe this is the source of the errors you see but it's not the compiler's fault or something specific to 7.5.0. The errors are because when IS_ENABLED() is false some of the symbols in the "if" are undefined and the compiler checks them regardless. > $ gcc --version > gcc (SUSE Linux) 7.5.0 > > And then funeth uses sbitmap, so it should select SBITMAP in order > to prevent build errors. Indeed. I think the "select SBITMAP" should be added to "config FUN_CORE" though as that is the module using sbitmap. > Fixes these build errors: > > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c: In function ‘write_pkt_desc’: > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:13: error: implicit declaration of function ‘tls_driver_ctx’ [-Werror=implicit-function-declaration] > tls_ctx = tls_driver_ctx(skb->sk, TLS_OFFLOAD_CTX_DIR_TX); > ^~~~~~~~~~~~~~ > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:37: error: ‘TLS_OFFLOAD_CTX_DIR_TX’ undeclared (first use in this function); did you mean ‘BPF_OFFLOAD_MAP_FREE’? > tls_ctx = tls_driver_ctx(skb->sk, TLS_OFFLOAD_CTX_DIR_TX); > ^~~~~~~~~~~~~~~~~~~~~~ > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:37: note: each undeclared identifier is reported only once for each function it appears in > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:245:23: error: dereferencing pointer to incomplete type ‘struct fun_ktls_tx_ctx’ > tls->tlsid = tls_ctx->tlsid; > ^~ > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c: In function ‘fun_start_xmit’: > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:310:6: error: implicit declaration of function ‘tls_is_sk_tx_device_offloaded’ [-Werror=implicit-function-declaration] > tls_is_sk_tx_device_offloaded(skb->sk)) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:311:9: error: implicit declaration of function ‘fun_tls_tx’; did you mean ‘fun_xdp_tx’? [-Werror=implicit-function-declaration] > skb = fun_tls_tx(skb, q, &tls_len); > ^~~~~~~~~~ > ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:311:7: warning: assignment makes pointer from integer without a cast [-Wint-conversion] > skb = fun_tls_tx(skb, q, &tls_len); > ^ > > and > > ERROR: modpost: "__sbitmap_queue_get" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! > ERROR: modpost: "sbitmap_finish_wait" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! > ERROR: modpost: "sbitmap_queue_clear" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! > ERROR: modpost: "sbitmap_prepare_to_wait" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! > ERROR: modpost: "sbitmap_queue_init_node" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! > ERROR: modpost: "sbitmap_queue_wake_all" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! > > #Fixes: not-merged-yet ("X") > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Cc: Dimitris Michailidis <dmichail@fungible.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > --- > drivers/net/ethernet/fungible/funeth/Kconfig | 3 ++- > drivers/net/ethernet/fungible/funeth/funeth_tx.c | 9 ++++++--- > 2 files changed, 8 insertions(+), 4 deletions(-) > > --- mmotm-2022-0303-2124.orig/drivers/net/ethernet/fungible/funeth/Kconfig > +++ mmotm-2022-0303-2124/drivers/net/ethernet/fungible/funeth/Kconfig > @@ -6,9 +6,10 @@ > config FUN_ETH > tristate "Fungible Ethernet device driver" > depends on PCI && PCI_MSI > - depends on TLS && TLS_DEVICE || TLS_DEVICE=n > + depends on TLS || TLS=n > select NET_DEVLINK > select FUN_CORE > + select SBITMAP > help > This driver supports the Ethernet functionality of Fungible adapters. > It works with both physical and virtual functions. > --- mmotm-2022-0303-2124.orig/drivers/net/ethernet/fungible/funeth/funeth_tx.c > +++ mmotm-2022-0303-2124/drivers/net/ethernet/fungible/funeth/funeth_tx.c > @@ -234,7 +234,8 @@ static unsigned int write_pkt_desc(struc > fun_dataop_gl_init(gle, 0, 0, lens[i], addrs[i]); > } > > - if (IS_ENABLED(CONFIG_TLS_DEVICE) && unlikely(tls_len)) { > +#if IS_ENABLED(CONFIG_TLS_DEVICE) > + if (unlikely(tls_len)) { > struct fun_eth_tls *tls = (struct fun_eth_tls *)gle; > struct fun_ktls_tx_ctx *tls_ctx; > > @@ -250,6 +251,7 @@ static unsigned int write_pkt_desc(struc > q->stats.tx_tls_pkts += 1 + extra_pkts; > u64_stats_update_end(&q->syncp); > } > +#endif > > u64_stats_update_begin(&q->syncp); > q->stats.tx_bytes += skb->len + extra_bytes; > @@ -306,12 +308,13 @@ netdev_tx_t fun_start_xmit(struct sk_buf > unsigned int tls_len = 0; > unsigned int ndesc; > > - if (IS_ENABLED(CONFIG_TLS_DEVICE) && skb->sk && > - tls_is_sk_tx_device_offloaded(skb->sk)) { > +#if IS_ENABLED(CONFIG_TLS_DEVICE) > + if (skb->sk && tls_is_sk_tx_device_offloaded(skb->sk)) { > skb = fun_tls_tx(skb, q, &tls_len); > if (unlikely(!skb)) > goto dropped; > } > +#endif > > ndesc = write_pkt_desc(skb, q, tls_len); > if (unlikely(!ndesc)) {
On Fri, 4 Mar 2022 16:39:49 -0800 Dimitris Michailidis wrote: > > Also, gcc 7.5 does not handle the "C language" vs. #ifdef preprocessor > > usage of IS_ENABLED() very well -- it is causing compile errors. > > I believe this is the source of the errors you see but it's not the compiler's > fault or something specific to 7.5.0. The errors are because when > IS_ENABLED() is false some of the symbols in the "if" are undefined and the > compiler checks them regardless. The compiler will need at least a declaration, right? tls_driver_ctx() is pretty stupidly hidden under an ifdef, for reasons which I cannot recall. Maybe we can take it out? Same thing could work for fun_tls_tx(): return skb; } +#else +struct sk_buff *fun_tls_tx(struct sk_buff *skb, struct funeth_txq *q, + unsigned int *tls_len); #endif no?
On Fri, Mar 4, 2022 at 7:41 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 4 Mar 2022 16:39:49 -0800 Dimitris Michailidis wrote: > > > Also, gcc 7.5 does not handle the "C language" vs. #ifdef preprocessor > > > usage of IS_ENABLED() very well -- it is causing compile errors. > > > > I believe this is the source of the errors you see but it's not the compiler's > > fault or something specific to 7.5.0. The errors are because when > > IS_ENABLED() is false some of the symbols in the "if" are undefined and the > > compiler checks them regardless. > > The compiler will need at least a declaration, right? tls_driver_ctx() > is pretty stupidly hidden under an ifdef, for reasons which I cannot > recall. Maybe we can take it out? > > Same thing could work for fun_tls_tx(): > > return skb; > } > +#else > +struct sk_buff *fun_tls_tx(struct sk_buff *skb, struct funeth_txq *q, > + unsigned int *tls_len); > #endif > > no? Yes, including net/tls.h and funeth_ktls.h unconditionally along with providing dummy definitions for the symbols involved in these errors when CONFIG_TLS_DEVICE=n would also solve the problem. I'd prefer this over adding #ifdef to have the compiler check the code regardless of configuration.
--- mmotm-2022-0303-2124.orig/drivers/net/ethernet/fungible/funeth/Kconfig +++ mmotm-2022-0303-2124/drivers/net/ethernet/fungible/funeth/Kconfig @@ -6,9 +6,10 @@ config FUN_ETH tristate "Fungible Ethernet device driver" depends on PCI && PCI_MSI - depends on TLS && TLS_DEVICE || TLS_DEVICE=n + depends on TLS || TLS=n select NET_DEVLINK select FUN_CORE + select SBITMAP help This driver supports the Ethernet functionality of Fungible adapters. It works with both physical and virtual functions. --- mmotm-2022-0303-2124.orig/drivers/net/ethernet/fungible/funeth/funeth_tx.c +++ mmotm-2022-0303-2124/drivers/net/ethernet/fungible/funeth/funeth_tx.c @@ -234,7 +234,8 @@ static unsigned int write_pkt_desc(struc fun_dataop_gl_init(gle, 0, 0, lens[i], addrs[i]); } - if (IS_ENABLED(CONFIG_TLS_DEVICE) && unlikely(tls_len)) { +#if IS_ENABLED(CONFIG_TLS_DEVICE) + if (unlikely(tls_len)) { struct fun_eth_tls *tls = (struct fun_eth_tls *)gle; struct fun_ktls_tx_ctx *tls_ctx; @@ -250,6 +251,7 @@ static unsigned int write_pkt_desc(struc q->stats.tx_tls_pkts += 1 + extra_pkts; u64_stats_update_end(&q->syncp); } +#endif u64_stats_update_begin(&q->syncp); q->stats.tx_bytes += skb->len + extra_bytes; @@ -306,12 +308,13 @@ netdev_tx_t fun_start_xmit(struct sk_buf unsigned int tls_len = 0; unsigned int ndesc; - if (IS_ENABLED(CONFIG_TLS_DEVICE) && skb->sk && - tls_is_sk_tx_device_offloaded(skb->sk)) { +#if IS_ENABLED(CONFIG_TLS_DEVICE) + if (skb->sk && tls_is_sk_tx_device_offloaded(skb->sk)) { skb = fun_tls_tx(skb, q, &tls_len); if (unlikely(!skb)) goto dropped; } +#endif ndesc = write_pkt_desc(skb, q, tls_len); if (unlikely(!ndesc)) {
It is currently possible to have CONFIG_FUN_ETH=y and CONFIG_TLS=m. This causes build errors. Therefore FUN_ETH should depend on TLS || TLS=n TLS_DEVICE is a bool symbol, so there is no need to test it as depends on TLS && TLS_DEVICE || TLS_DEVICE=n And due to rules of precedence, the above means depends on (TLS && TLS_DEVICE) || TLS_DEVICE=n but that's probably not what was meant here. More likely it should have been depends on TLS && (TLS_DEVICE || TLS_DEVICE=n) That no longer matters. Also, gcc 7.5 does not handle the "C language" vs. #ifdef preprocessor usage of IS_ENABLED() very well -- it is causing compile errors. $ gcc --version gcc (SUSE Linux) 7.5.0 And then funeth uses sbitmap, so it should select SBITMAP in order to prevent build errors. Fixes these build errors: ../drivers/net/ethernet/fungible/funeth/funeth_tx.c: In function ‘write_pkt_desc’: ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:13: error: implicit declaration of function ‘tls_driver_ctx’ [-Werror=implicit-function-declaration] tls_ctx = tls_driver_ctx(skb->sk, TLS_OFFLOAD_CTX_DIR_TX); ^~~~~~~~~~~~~~ ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:37: error: ‘TLS_OFFLOAD_CTX_DIR_TX’ undeclared (first use in this function); did you mean ‘BPF_OFFLOAD_MAP_FREE’? tls_ctx = tls_driver_ctx(skb->sk, TLS_OFFLOAD_CTX_DIR_TX); ^~~~~~~~~~~~~~~~~~~~~~ ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:244:37: note: each undeclared identifier is reported only once for each function it appears in ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:245:23: error: dereferencing pointer to incomplete type ‘struct fun_ktls_tx_ctx’ tls->tlsid = tls_ctx->tlsid; ^~ ../drivers/net/ethernet/fungible/funeth/funeth_tx.c: In function ‘fun_start_xmit’: ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:310:6: error: implicit declaration of function ‘tls_is_sk_tx_device_offloaded’ [-Werror=implicit-function-declaration] tls_is_sk_tx_device_offloaded(skb->sk)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:311:9: error: implicit declaration of function ‘fun_tls_tx’; did you mean ‘fun_xdp_tx’? [-Werror=implicit-function-declaration] skb = fun_tls_tx(skb, q, &tls_len); ^~~~~~~~~~ ../drivers/net/ethernet/fungible/funeth/funeth_tx.c:311:7: warning: assignment makes pointer from integer without a cast [-Wint-conversion] skb = fun_tls_tx(skb, q, &tls_len); ^ and ERROR: modpost: "__sbitmap_queue_get" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! ERROR: modpost: "sbitmap_finish_wait" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! ERROR: modpost: "sbitmap_queue_clear" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! ERROR: modpost: "sbitmap_prepare_to_wait" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! ERROR: modpost: "sbitmap_queue_init_node" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! ERROR: modpost: "sbitmap_queue_wake_all" [drivers/net/ethernet/fungible/funcore/funcore.ko] undefined! #Fixes: not-merged-yet ("X") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Cc: Dimitris Michailidis <dmichail@fungible.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> --- drivers/net/ethernet/fungible/funeth/Kconfig | 3 ++- drivers/net/ethernet/fungible/funeth/funeth_tx.c | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-)