[PATCH] idpf: fix building without IPv4

[PATCH] idpf: fix building without IPv4

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The newly added offload code fails to link when IPv4 networking is disabled:

arm-linux-gnueabi-ld: drivers/net/ethernet/intel/idpf/idpf_txrx.o: in function `idpf_vport_splitq_napi_poll':
idpf_txrx.c:(.text+0x7a20): undefined reference to `tcp_gro_complete'

Add complile-time checks for both CONFIG_INET (ipv4) and CONFIG_IPV6
in order to drop the corresponding code when the features are unavailable.
This should also help produce slightly better output for IPv4-only
kernel builds, if anyone still uses those.

Fixes: 3a8845af66edb ("idpf: add RX splitq napi poll support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index 6fa79898c42c5..140c1ad3e0679 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -2770,8 +2770,10 @@ static void idpf_rx_csum(struct idpf_queue *rxq, struct sk_buff *skb,
 	if (!(csum_bits->l3l4p))
 		return;
 
-	ipv4 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
-	ipv6 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
+	ipv4 = IS_ENABLED(CONFIG_INET) &&
+	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
+	ipv6 = IS_ENABLED(CONFIG_IPV6) &&
+	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
 
 	if (ipv4 && (csum_bits->ipe || csum_bits->eipe))
 		goto checksum_fail;
@@ -2870,8 +2872,10 @@ static int idpf_rx_rsc(struct idpf_queue *rxq, struct sk_buff *skb,
 	if (unlikely(!rsc_seg_len))
 		return -EINVAL;
 
-	ipv4 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
-	ipv6 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
+	ipv4 = IS_ENABLED(CONFIG_INET) &&
+	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
+	ipv6 = IS_ENABLED(CONFIG_IPV6) &&
+	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
 
 	if (unlikely(!(ipv4 ^ ipv6)))
 		return -EINVAL;
-- 
2.39.2

Re: [PATCH] idpf: fix building without IPv4

[Tony Nguyen]

On 9/25/2023 8:58 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The newly added offload code fails to link when IPv4 networking is disabled:
> 
> arm-linux-gnueabi-ld: drivers/net/ethernet/intel/idpf/idpf_txrx.o: in function `idpf_vport_splitq_napi_poll':
> idpf_txrx.c:(.text+0x7a20): undefined reference to `tcp_gro_complete'
> 
> Add complile-time checks for both CONFIG_INET (ipv4) and CONFIG_IPV6
> in order to drop the corresponding code when the features are unavailable.
> This should also help produce slightly better output for IPv4-only
> kernel builds, if anyone still uses those.

Hi Arnd,

Also, a pending patch for this [1], however, this does look a bit more 
efficient. Adding Olek as he's author on the other patch.

netdev maintainers,

If this is the version that does get picked up, did you want to take it 
directly to close out the compile issues?

Acked-by: Tony Nguyen <anthony.l.nguyen@intel.com>

> Fixes: 3a8845af66edb ("idpf: add RX splitq napi poll support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/net/ethernet/intel/idpf/idpf_txrx.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 6fa79898c42c5..140c1ad3e0679 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> @@ -2770,8 +2770,10 @@ static void idpf_rx_csum(struct idpf_queue *rxq, struct sk_buff *skb,
>   	if (!(csum_bits->l3l4p))
>   		return;
>   
> -	ipv4 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
> -	ipv6 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
> +	ipv4 = IS_ENABLED(CONFIG_INET) &&
> +	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
> +	ipv6 = IS_ENABLED(CONFIG_IPV6) &&
> +	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
>   
>   	if (ipv4 && (csum_bits->ipe || csum_bits->eipe))
>   		goto checksum_fail;
> @@ -2870,8 +2872,10 @@ static int idpf_rx_rsc(struct idpf_queue *rxq, struct sk_buff *skb,
>   	if (unlikely(!rsc_seg_len))
>   		return -EINVAL;
>   
> -	ipv4 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
> -	ipv6 = IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
> +	ipv4 = IS_ENABLED(CONFIG_INET) &&
> +	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV4);
> +	ipv6 = IS_ENABLED(CONFIG_IPV6) &&
> +	       IDPF_RX_PTYPE_TO_IPV(decoded, IDPF_RX_PTYPE_OUTER_IPV6);
>   
>   	if (unlikely(!(ipv4 ^ ipv6)))
>   		return -EINVAL;

[1] 
https://lore.kernel.org/netdev/20230921125936.1621191-1-aleksander.lobakin@intel.com/

Re: [PATCH] idpf: fix building without IPv4

[Jakub Kicinski]

On Mon, 25 Sep 2023 10:05:03 -0700 Tony Nguyen wrote:
> Also, a pending patch for this [1], however, this does look a bit more 
> efficient. Adding Olek as he's author on the other patch.
> 
> netdev maintainers,
> 
> If this is the version that does get picked up, did you want to take it 
> directly to close out the compile issues?

Sorry for the delays. Should we not add a !INET static inline wrapper
for tcp_gro_complete()? Seems a bit backwards to me to make drivers
suffer and think about such a preposterous config :S

$ git grep tcp_gro_complete -- drivers/
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:        tcp_gro_complete(skb);
drivers/net/ethernet/broadcom/bnxt/bnxt.c:              tcp_gro_complete(skb);
drivers/net/ethernet/intel/idpf/idpf_txrx.c:    tcp_gro_complete(skb);
drivers/net/ethernet/qlogic/qede/qede_fp.c:     tcp_gro_complete(skb);

We have 4 drivers which need ifdefs already and the number will only
grow with GRO-HW spreading.

Re: [PATCH] idpf: fix building without IPv4

[Arnd Bergmann]

On Wed, Oct 4, 2023, at 00:43, Jakub Kicinski wrote:
> On Mon, 25 Sep 2023 10:05:03 -0700 Tony Nguyen wrote:
>> Also, a pending patch for this [1], however, this does look a bit more 
>> efficient. Adding Olek as he's author on the other patch.
>> 
>> netdev maintainers,
>> 
>> If this is the version that does get picked up, did you want to take it 
>> directly to close out the compile issues?
>
> Sorry for the delays. Should we not add a !INET static inline wrapper
> for tcp_gro_complete()? Seems a bit backwards to me to make drivers
> suffer and think about such a preposterous config :S
>
> $ git grep tcp_gro_complete -- drivers/
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c:        tcp_gro_complete(skb);
> drivers/net/ethernet/broadcom/bnxt/bnxt.c:              tcp_gro_complete(skb);
> drivers/net/ethernet/intel/idpf/idpf_txrx.c:    tcp_gro_complete(skb);
> drivers/net/ethernet/qlogic/qede/qede_fp.c:     tcp_gro_complete(skb);
>
> We have 4 drivers which need ifdefs already and the number will only
> grow with GRO-HW spreading.

That sounds good to me, but it's better if someone that understands
this code patch better than me writes the stub helpers, to ensure
all callers have sensible behavior in that configuration.

I also had a brief look at who might be using kernels without CONFIG_INET.
In the kernel source tree, there are 19 defconfig files that completely
enable CONFIG_NET, which means that both INET and ETHERNET are always
turned off as well.

There are four configs that enable CONFIG_NET but not CONFIG_INET:

arch/arm/configs/spear3xx_defconfig
arch/arm/configs/spear6xx_defconfig
arch/m68k/configs/virt_defconfig
arch/s390/configs/zfcpdump_defconfig

I'm confident that the two arm configs are a mistake, as these are
regular embedded SoCs with on-chip ethernet that is enabled in
the config but almost certainly has no other use. The virt defconfig
lost CONFIG_INET after commit d7385ba13771 ("9p: Remove INET
dependency") added an 'imply INET'. This sounds like a bad idea,
since it messes up the 'defconfig' logic when a leaf driver enables
an entire subsystem.

The s390 zfcpdump defconfig looks like a legitimate case for
disabling INET, but it's not that size constrained and it might
not actually need CONFIG_NET either.

So overall, it seems there is no real need to support CONFIG_NET=y
with CONFIG_INET=n and we could just make them be the same and
avoid bugs like this. In theory we could also go the opposite way
and try to make INET a tristate symbol that can live in a loadable
module like all other network protocols. This would be nice
conceptually and for smaller vmlinux files (some systems are
much more limited in the size of their boot partition than their
RAM and rootfs), but would clearly cause way more build failures.

      Arnd

Re: [PATCH] idpf: fix building without IPv4

[Alexander Lobakin]

From: Tony Nguyen <anthony.l.nguyen@intel.com>
Date: Mon, 25 Sep 2023 10:05:03 -0700

> 
> On 9/25/2023 8:58 AM, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> The newly added offload code fails to link when IPv4 networking is
>> disabled:
>>
>> arm-linux-gnueabi-ld: drivers/net/ethernet/intel/idpf/idpf_txrx.o: in
>> function `idpf_vport_splitq_napi_poll':
>> idpf_txrx.c:(.text+0x7a20): undefined reference to `tcp_gro_complete'
>>
>> Add complile-time checks for both CONFIG_INET (ipv4) and CONFIG_IPV6
>> in order to drop the corresponding code when the features are
>> unavailable.
>> This should also help produce slightly better output for IPv4-only
>> kernel builds, if anyone still uses those.
> 
> Hi Arnd,
> 
> Also, a pending patch for this [1], however, this does look a bit more

I agree Arnd's version is more efficient, the other question is why my
patch has been hanging in your tree for 2 weeks, although I asked to
take it directly to fix automated builds ASAP xD

> efficient. Adding Olek as he's author on the other patch.
> 
> netdev maintainers,
> 
> If this is the version that does get picked up, did you want to take it
> directly to close out the compile issues?

[...]

Thanks,
Olek