[PATCH net-next 0/3] sfc: further EF100 encap TSO features

[PATCH net-next 0/3] sfc: further EF100 encap TSO features

[Edward Cree]

This series adds support for GRE and GRE_CSUM TSO on EF100 NICs, as
 well as improving the handling of UDP tunnel TSO.

Edward Cree (3):
  sfc: extend bitfield macros to 19 fields
  sfc: correctly support non-partial GSO_UDP_TUNNEL_CSUM on EF100
  sfc: support GRE TSO on EF100

 drivers/net/ethernet/sfc/bitfield.h  | 26 +++++++++++++++++++++-----
 drivers/net/ethernet/sfc/ef100_nic.c |  8 ++++++--
 drivers/net/ethernet/sfc/ef100_tx.c  | 12 ++++++++++--
 3 files changed, 37 insertions(+), 9 deletions(-)

[PATCH net-next 1/3] sfc: extend bitfield macros to 19 fields

[Edward Cree]

Our TSO descriptors got even more fussy.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/bitfield.h | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/sfc/bitfield.h b/drivers/net/ethernet/sfc/bitfield.h
index 64731eb5dd56..1f981dfe4bdc 100644
--- a/drivers/net/ethernet/sfc/bitfield.h
+++ b/drivers/net/ethernet/sfc/bitfield.h
@@ -289,7 +289,9 @@ typedef union efx_oword {
 				 field14, value14,			\
 				 field15, value15,			\
 				 field16, value16,			\
-				 field17, value17)			\
+				 field17, value17,			\
+				 field18, value18,			\
+				 field19, value19)			\
 	(EFX_INSERT_FIELD_NATIVE((min), (max), field1, (value1)) |	\
 	 EFX_INSERT_FIELD_NATIVE((min), (max), field2, (value2)) |	\
 	 EFX_INSERT_FIELD_NATIVE((min), (max), field3, (value3)) |	\
@@ -306,7 +308,9 @@ typedef union efx_oword {
 	 EFX_INSERT_FIELD_NATIVE((min), (max), field14, (value14)) |	\
 	 EFX_INSERT_FIELD_NATIVE((min), (max), field15, (value15)) |	\
 	 EFX_INSERT_FIELD_NATIVE((min), (max), field16, (value16)) |	\
-	 EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17)))
+	 EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17)) |	\
+	 EFX_INSERT_FIELD_NATIVE((min), (max), field18, (value18)) |	\
+	 EFX_INSERT_FIELD_NATIVE((min), (max), field19, (value19)))
 
 #define EFX_INSERT_FIELDS64(...)				\
 	cpu_to_le64(EFX_INSERT_FIELDS_NATIVE(__VA_ARGS__))
@@ -348,7 +352,11 @@ typedef union efx_oword {
 #endif
 
 /* Populate an octword field with various numbers of arguments */
-#define EFX_POPULATE_OWORD_17 EFX_POPULATE_OWORD
+#define EFX_POPULATE_OWORD_19 EFX_POPULATE_OWORD
+#define EFX_POPULATE_OWORD_18(oword, ...) \
+	EFX_POPULATE_OWORD_19(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
+#define EFX_POPULATE_OWORD_17(oword, ...) \
+	EFX_POPULATE_OWORD_18(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
 #define EFX_POPULATE_OWORD_16(oword, ...) \
 	EFX_POPULATE_OWORD_17(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
 #define EFX_POPULATE_OWORD_15(oword, ...) \
@@ -391,7 +399,11 @@ typedef union efx_oword {
 			     EFX_DWORD_3, 0xffffffff)
 
 /* Populate a quadword field with various numbers of arguments */
-#define EFX_POPULATE_QWORD_17 EFX_POPULATE_QWORD
+#define EFX_POPULATE_QWORD_19 EFX_POPULATE_QWORD
+#define EFX_POPULATE_QWORD_18(qword, ...) \
+	EFX_POPULATE_QWORD_19(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
+#define EFX_POPULATE_QWORD_17(qword, ...) \
+	EFX_POPULATE_QWORD_18(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
 #define EFX_POPULATE_QWORD_16(qword, ...) \
 	EFX_POPULATE_QWORD_17(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
 #define EFX_POPULATE_QWORD_15(qword, ...) \
@@ -432,7 +444,11 @@ typedef union efx_oword {
 			     EFX_DWORD_1, 0xffffffff)
 
 /* Populate a dword field with various numbers of arguments */
-#define EFX_POPULATE_DWORD_17 EFX_POPULATE_DWORD
+#define EFX_POPULATE_DWORD_19 EFX_POPULATE_DWORD
+#define EFX_POPULATE_DWORD_18(dword, ...) \
+	EFX_POPULATE_DWORD_19(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
+#define EFX_POPULATE_DWORD_17(dword, ...) \
+	EFX_POPULATE_DWORD_18(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
 #define EFX_POPULATE_DWORD_16(dword, ...) \
 	EFX_POPULATE_DWORD_17(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
 #define EFX_POPULATE_DWORD_15(dword, ...) \

[PATCH net-next 2/3] sfc: correctly support non-partial GSO_UDP_TUNNEL_CSUM on EF100

[Edward Cree]

By asking the HW for the correct edits, we can make UDP tunnel TSO
 work without needing GSO_PARTIAL.  So don't specify it in our
 netdev->gso_partial_features.
However, retain GSO_PARTIAL support, as this will be used for other
 protocols later.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/ef100_nic.c | 1 -
 drivers/net/ethernet/sfc/ef100_tx.c  | 6 +++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
index cd93c5ffd45c..05d22220228a 100644
--- a/drivers/net/ethernet/sfc/ef100_nic.c
+++ b/drivers/net/ethernet/sfc/ef100_nic.c
@@ -190,7 +190,6 @@ static int efx_ef100_init_datapath_caps(struct efx_nic *efx)
 		net_dev->features |= tso;
 		net_dev->hw_features |= tso;
 		net_dev->hw_enc_features |= tso;
-		net_dev->gso_partial_features |= NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM;
 	}
 	efx->num_mac_stats = MCDI_WORD(outbuf,
 				       GET_CAPABILITIES_V4_OUT_MAC_STATS_NUM_STATS);
diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
index ad0ad9bad423..a9e045c54a79 100644
--- a/drivers/net/ethernet/sfc/ef100_tx.c
+++ b/drivers/net/ethernet/sfc/ef100_tx.c
@@ -196,6 +196,7 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
 	bool encap = skb->encapsulation;
 	u16 vlan_enable = 0;
 	struct tcphdr *tcp;
+	bool outer_csum;
 	u32 paylen;
 
 	if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID)
@@ -216,19 +217,21 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
 		tcp_offset = skb_transport_offset(skb);
 		outer_ip_offset = outer_l4_offset = 0;
 	}
+	outer_csum = skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM;
 
 	/* subtract TCP payload length from inner checksum */
 	tcp = (void *)skb->data + tcp_offset;
 	paylen = skb->len - tcp_offset;
 	csum_replace_by_diff(&tcp->check, (__force __wsum)htonl(paylen));
 
-	EFX_POPULATE_OWORD_17(*txd,
+	EFX_POPULATE_OWORD_19(*txd,
 			      ESF_GZ_TX_DESC_TYPE, ESE_GZ_TX_DESC_TYPE_TSO,
 			      ESF_GZ_TX_TSO_MSS, mss,
 			      ESF_GZ_TX_TSO_HDR_NUM_SEGS, 1,
 			      ESF_GZ_TX_TSO_PAYLOAD_NUM_SEGS, payload_segs,
 			      ESF_GZ_TX_TSO_HDR_LEN_W, buffer->len >> 1,
 			      ESF_GZ_TX_TSO_PAYLOAD_LEN, len,
+			      ESF_GZ_TX_TSO_CSO_OUTER_L4, outer_csum,
 			      ESF_GZ_TX_TSO_CSO_INNER_L4, 1,
 			      ESF_GZ_TX_TSO_INNER_L3_OFF_W, ip_offset >> 1,
 			      ESF_GZ_TX_TSO_INNER_L4_OFF_W, tcp_offset >> 1,
@@ -237,6 +240,7 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
 			      ESF_GZ_TX_TSO_OUTER_L3_OFF_W, outer_ip_offset >> 1,
 			      ESF_GZ_TX_TSO_OUTER_L4_OFF_W, outer_l4_offset >> 1,
 			      ESF_GZ_TX_TSO_ED_OUTER_UDP_LEN, encap && !gso_partial,
+			      ESF_GZ_TX_TSO_ED_OUTER_IP_LEN, encap && !gso_partial,
 			      ESF_GZ_TX_TSO_ED_OUTER_IP4_ID, encap ? mangleid :
 								     ESE_GZ_TX_DESC_IP4_ID_NO_OP,
 			      ESF_GZ_TX_TSO_VLAN_INSERT_EN, vlan_enable,

[PATCH net-next 3/3] sfc: support GRE TSO on EF100

[Edward Cree]

We can treat SKB_GSO_GRE almost exactly the same as UDP tunnels, except
 that we don't want to edit the outer UDP len (as there isn't one).
For SKB_GSO_GRE_CSUM, we have to use GSO_PARTIAL as the device doesn't
 support offload of non-UDP outer L4 checksums.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/ef100_nic.c | 7 ++++++-
 drivers/net/ethernet/sfc/ef100_tx.c  | 6 +++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
index 05d22220228a..518268ce2064 100644
--- a/drivers/net/ethernet/sfc/ef100_nic.c
+++ b/drivers/net/ethernet/sfc/ef100_nic.c
@@ -185,11 +185,16 @@ static int efx_ef100_init_datapath_caps(struct efx_nic *efx)
 	if (efx_ef100_has_cap(nic_data->datapath_caps2, TX_TSO_V3)) {
 		struct net_device *net_dev = efx->net_dev;
 		netdev_features_t tso = NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_GSO_PARTIAL |
-					NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM;
+					NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM |
+					NETIF_F_GSO_GRE | NETIF_F_GSO_GRE_CSUM;
 
 		net_dev->features |= tso;
 		net_dev->hw_features |= tso;
 		net_dev->hw_enc_features |= tso;
+		/* EF100 HW can only offload outer checksums if they are UDP,
+		 * so for GRE_CSUM we have to use GSO_PARTIAL.
+		 */
+		net_dev->gso_partial_features |= NETIF_F_GSO_GRE_CSUM;
 	}
 	efx->num_mac_stats = MCDI_WORD(outbuf,
 				       GET_CAPABILITIES_V4_OUT_MAC_STATS_NUM_STATS);
diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
index a9e045c54a79..26ef51d6b542 100644
--- a/drivers/net/ethernet/sfc/ef100_tx.c
+++ b/drivers/net/ethernet/sfc/ef100_tx.c
@@ -194,6 +194,7 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
 	u16 vlan_tci = skb_vlan_tag_get(skb);
 	u32 mss = skb_shinfo(skb)->gso_size;
 	bool encap = skb->encapsulation;
+	bool udp_encap = false;
 	u16 vlan_enable = 0;
 	struct tcphdr *tcp;
 	bool outer_csum;
@@ -212,6 +213,9 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
 		outer_l4_offset = skb_transport_offset(skb);
 		ip_offset = skb_inner_network_offset(skb);
 		tcp_offset = skb_inner_transport_offset(skb);
+		if (skb_shinfo(skb)->gso_type &
+		    (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM))
+			udp_encap = true;
 	} else {
 		ip_offset =  skb_network_offset(skb);
 		tcp_offset = skb_transport_offset(skb);
@@ -239,7 +243,7 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
 			      ESF_GZ_TX_TSO_ED_INNER_IP_LEN, 1,
 			      ESF_GZ_TX_TSO_OUTER_L3_OFF_W, outer_ip_offset >> 1,
 			      ESF_GZ_TX_TSO_OUTER_L4_OFF_W, outer_l4_offset >> 1,
-			      ESF_GZ_TX_TSO_ED_OUTER_UDP_LEN, encap && !gso_partial,
+			      ESF_GZ_TX_TSO_ED_OUTER_UDP_LEN, udp_encap && !gso_partial,
 			      ESF_GZ_TX_TSO_ED_OUTER_IP_LEN, encap && !gso_partial,
 			      ESF_GZ_TX_TSO_ED_OUTER_IP4_ID, encap ? mangleid :
 								     ESE_GZ_TX_DESC_IP4_ID_NO_OP,

Re: [PATCH net-next 0/3] sfc: further EF100 encap TSO features

[Martin Habets]

On Thu, Nov 12, 2020 at 03:18:01PM +0000, Edward Cree wrote:
> This series adds support for GRE and GRE_CSUM TSO on EF100 NICs, as
>  well as improving the handling of UDP tunnel TSO.
> 
> Edward Cree (3):
>   sfc: extend bitfield macros to 19 fields
>   sfc: correctly support non-partial GSO_UDP_TUNNEL_CSUM on EF100
>   sfc: support GRE TSO on EF100

Acked-by: Martin Habets <mhabets@solarflare.com>

Re: [PATCH net-next 1/3] sfc: extend bitfield macros to 19 fields

[Alexander Duyck]

On Thu, Nov 12, 2020 at 7:23 AM Edward Cree <ecree@solarflare.com> wrote:
>
> Our TSO descriptors got even more fussy.
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
>  drivers/net/ethernet/sfc/bitfield.h | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/sfc/bitfield.h b/drivers/net/ethernet/sfc/bitfield.h
> index 64731eb5dd56..1f981dfe4bdc 100644
> --- a/drivers/net/ethernet/sfc/bitfield.h
> +++ b/drivers/net/ethernet/sfc/bitfield.h
> @@ -289,7 +289,9 @@ typedef union efx_oword {
>                                  field14, value14,                      \
>                                  field15, value15,                      \
>                                  field16, value16,                      \
> -                                field17, value17)                      \
> +                                field17, value17,                      \
> +                                field18, value18,                      \
> +                                field19, value19)                      \
>         (EFX_INSERT_FIELD_NATIVE((min), (max), field1, (value1)) |      \
>          EFX_INSERT_FIELD_NATIVE((min), (max), field2, (value2)) |      \
>          EFX_INSERT_FIELD_NATIVE((min), (max), field3, (value3)) |      \
> @@ -306,7 +308,9 @@ typedef union efx_oword {
>          EFX_INSERT_FIELD_NATIVE((min), (max), field14, (value14)) |    \
>          EFX_INSERT_FIELD_NATIVE((min), (max), field15, (value15)) |    \
>          EFX_INSERT_FIELD_NATIVE((min), (max), field16, (value16)) |    \
> -        EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17)))
> +        EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17)) |    \
> +        EFX_INSERT_FIELD_NATIVE((min), (max), field18, (value18)) |    \
> +        EFX_INSERT_FIELD_NATIVE((min), (max), field19, (value19)))
>
>  #define EFX_INSERT_FIELDS64(...)                               \
>         cpu_to_le64(EFX_INSERT_FIELDS_NATIVE(__VA_ARGS__))
> @@ -348,7 +352,11 @@ typedef union efx_oword {
>  #endif
>
>  /* Populate an octword field with various numbers of arguments */
> -#define EFX_POPULATE_OWORD_17 EFX_POPULATE_OWORD
> +#define EFX_POPULATE_OWORD_19 EFX_POPULATE_OWORD
> +#define EFX_POPULATE_OWORD_18(oword, ...) \
> +       EFX_POPULATE_OWORD_19(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFX_POPULATE_OWORD_17(oword, ...) \
> +       EFX_POPULATE_OWORD_18(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>  #define EFX_POPULATE_OWORD_16(oword, ...) \
>         EFX_POPULATE_OWORD_17(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>  #define EFX_POPULATE_OWORD_15(oword, ...) \
> @@ -391,7 +399,11 @@ typedef union efx_oword {
>                              EFX_DWORD_3, 0xffffffff)
>
>  /* Populate a quadword field with various numbers of arguments */
> -#define EFX_POPULATE_QWORD_17 EFX_POPULATE_QWORD
> +#define EFX_POPULATE_QWORD_19 EFX_POPULATE_QWORD
> +#define EFX_POPULATE_QWORD_18(qword, ...) \
> +       EFX_POPULATE_QWORD_19(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFX_POPULATE_QWORD_17(qword, ...) \
> +       EFX_POPULATE_QWORD_18(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>  #define EFX_POPULATE_QWORD_16(qword, ...) \
>         EFX_POPULATE_QWORD_17(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>  #define EFX_POPULATE_QWORD_15(qword, ...) \
> @@ -432,7 +444,11 @@ typedef union efx_oword {
>                              EFX_DWORD_1, 0xffffffff)
>
>  /* Populate a dword field with various numbers of arguments */
> -#define EFX_POPULATE_DWORD_17 EFX_POPULATE_DWORD
> +#define EFX_POPULATE_DWORD_19 EFX_POPULATE_DWORD
> +#define EFX_POPULATE_DWORD_18(dword, ...) \
> +       EFX_POPULATE_DWORD_19(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFX_POPULATE_DWORD_17(dword, ...) \
> +       EFX_POPULATE_DWORD_18(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>  #define EFX_POPULATE_DWORD_16(dword, ...) \
>         EFX_POPULATE_DWORD_17(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>  #define EFX_POPULATE_DWORD_15(dword, ...) \
>

Are all these macros really needed? It seems like this is adding a
bunch of noise in order to add support for a few additional fields.
Wouldn't it be possible to just define the ones that are actually
needed and add multiple dummy values to fill in the gaps instead of
defining every macro between zero and 19? For example this patch set
adds an option for setting 18 fields, but from what I can tell it is
never used.

Re: [PATCH net-next 0/3] sfc: further EF100 encap TSO features

[Alexander Duyck]

On Thu, Nov 12, 2020 at 7:19 AM Edward Cree <ecree@solarflare.com> wrote:
>
> This series adds support for GRE and GRE_CSUM TSO on EF100 NICs, as
>  well as improving the handling of UDP tunnel TSO.
>
> Edward Cree (3):
>   sfc: extend bitfield macros to 19 fields
>   sfc: correctly support non-partial GSO_UDP_TUNNEL_CSUM on EF100
>   sfc: support GRE TSO on EF100
>
>  drivers/net/ethernet/sfc/bitfield.h  | 26 +++++++++++++++++++++-----
>  drivers/net/ethernet/sfc/ef100_nic.c |  8 ++++++--
>  drivers/net/ethernet/sfc/ef100_tx.c  | 12 ++++++++++--
>  3 files changed, 37 insertions(+), 9 deletions(-)
>

The code itself is mostly solid. The only issue I see is a question I
have about if we really need to be defining so many of the
"EFX_POPULATE_..." macros in patch 1 as it seems a bit gratuitous and
could likely be replaced by just adding multiple dummy fields to a
single macro instead of defining multiple macros to get to the end
goal.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Re: [PATCH net-next 0/3] sfc: further EF100 encap TSO features

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Thu, 12 Nov 2020 15:18:01 +0000 you wrote:
> This series adds support for GRE and GRE_CSUM TSO on EF100 NICs, as
>  well as improving the handling of UDP tunnel TSO.
> 
> Edward Cree (3):
>   sfc: extend bitfield macros to 19 fields
>   sfc: correctly support non-partial GSO_UDP_TUNNEL_CSUM on EF100
>   sfc: support GRE TSO on EF100
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] sfc: extend bitfield macros to 19 fields
    https://git.kernel.org/netdev/net-next/c/dc8d2512e697
  - [net-next,2/3] sfc: correctly support non-partial GSO_UDP_TUNNEL_CSUM on EF100
    https://git.kernel.org/netdev/net-next/c/42bfd69a9fdd
  - [net-next,3/3] sfc: support GRE TSO on EF100
    https://git.kernel.org/netdev/net-next/c/c5122cf58412

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 1/3] sfc: extend bitfield macros to 19 fields

[Edward Cree]

On 13/11/2020 19:06, Alexander Duyck wrote:
> On Thu, Nov 12, 2020 at 7:23 AM Edward Cree <ecree@solarflare.com> wrote:
>> @@ -348,7 +352,11 @@ typedef union efx_oword {
>>  #endif
>>
>>  /* Populate an octword field with various numbers of arguments */
>> -#define EFX_POPULATE_OWORD_17 EFX_POPULATE_OWORD
>> +#define EFX_POPULATE_OWORD_19 EFX_POPULATE_OWORD
>> +#define EFX_POPULATE_OWORD_18(oword, ...) \
>> +       EFX_POPULATE_OWORD_19(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>> +#define EFX_POPULATE_OWORD_17(oword, ...) \
>> +       EFX_POPULATE_OWORD_18(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>>  #define EFX_POPULATE_OWORD_16(oword, ...) \
>>         EFX_POPULATE_OWORD_17(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
>>  #define EFX_POPULATE_OWORD_15(oword, ...) \
> Are all these macros really needed? It seems like this is adding a
> bunch of noise in order to add support for a few additional fields.
> Wouldn't it be possible to just define the ones that are actually
> needed and add multiple dummy values to fill in the gaps instead of
> defining every macro between zero and 19? For example this patch set
> adds an option for setting 18 fields, but from what I can tell it is
> never used.
I guess the reasoningoriginally was that it's easier to read and
 v-lint if it's just n repetitions of the same pattern.  Whereas if
 there were jumps, it'd be more likely for a typo to slip through
 unnoticed and subtly corrupt all the values.
But tbh I don't know, it's been like that since the driver was added
 twelve years ago (8ceee660aacb) when it had all from 0 to 10.  All
 we've done since then is extend that pattern.

-ed

Re: [PATCH net-next 1/3] sfc: extend bitfield macros to 19 fields

[Alexander Duyck]

On Mon, Nov 16, 2020 at 4:27 AM Edward Cree <ecree@solarflare.com> wrote:
>
> On 13/11/2020 19:06, Alexander Duyck wrote:
> > On Thu, Nov 12, 2020 at 7:23 AM Edward Cree <ecree@solarflare.com> wrote:
> >> @@ -348,7 +352,11 @@ typedef union efx_oword {
> >>  #endif
> >>
> >>  /* Populate an octword field with various numbers of arguments */
> >> -#define EFX_POPULATE_OWORD_17 EFX_POPULATE_OWORD
> >> +#define EFX_POPULATE_OWORD_19 EFX_POPULATE_OWORD
> >> +#define EFX_POPULATE_OWORD_18(oword, ...) \
> >> +       EFX_POPULATE_OWORD_19(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> >> +#define EFX_POPULATE_OWORD_17(oword, ...) \
> >> +       EFX_POPULATE_OWORD_18(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> >>  #define EFX_POPULATE_OWORD_16(oword, ...) \
> >>         EFX_POPULATE_OWORD_17(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> >>  #define EFX_POPULATE_OWORD_15(oword, ...) \
> > Are all these macros really needed? It seems like this is adding a
> > bunch of noise in order to add support for a few additional fields.
> > Wouldn't it be possible to just define the ones that are actually
> > needed and add multiple dummy values to fill in the gaps instead of
> > defining every macro between zero and 19? For example this patch set
> > adds an option for setting 18 fields, but from what I can tell it is
> > never used.
> I guess the reasoningoriginally was that it's easier to read and
>  v-lint if it's just n repetitions of the same pattern.  Whereas if
>  there were jumps, it'd be more likely for a typo to slip through
>  unnoticed and subtly corrupt all the values.

I'm not sure the typo argument holds much water. The fact is it is
pretty easy to just count the variables doing something like the
following:
#define EFX_POPULATE_OWORD_10(oword, ...) \
        EFX_POPULATE_OWORD_19(oword, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              __VA_ARGS__)

Any change is basically update the 19 to whatever and add/subtract
lines using a simple copy/paste.

> But tbh I don't know, it's been like that since the driver was added
>  twelve years ago (8ceee660aacb) when it had all from 0 to 10.  All
>  we've done since then is extend that pattern.

The reason I bring it up is that it seems like it is dragging  a bunch
of macros that will likely never need 19 variables forward along with
it. For example the EFX_POPULATE_[DQ]WORD_<n> seems to only go as high
as 7. I'm not sure it makes much sense to keep defining new versions
of the macro when you could just be adding the needed lines to the 7
variable version of the macro and and come up with something that
looks like the definition of EFX_SET_OWORD where you could just add an
EFX_DUMMY_FIELD, 0, for each new variable added. The
EFX_POPULATE_OWORD_<X> goes all the way to 17 currently, however if we
exclude that the real distribution seems to be 1 - 10, with just the
one lone call to the 17 case which is becoming 19 with your patch.

The one issue I can think of is the fact that you will need the 17
variable version until you change it to the 19, but even then dropping
the 17 afterwards and adding the 2 additional sets of dummy variables
to the 10 should be straight forward and still pretty easy to review.