| Message ID | 8341c601-674a-74ff-c6dd-689c19b3ce7f@solarflare.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
> info->data = 0; > switch (info->flow_type) { > + case UDP_V4_FLOW: > + if (efx->rx_hash_udp_4tuple) > + /* fall through */ > + /* else fall further! */ > case TCP_V4_FLOW: > - info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; > + info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; > /* fall through */ > - case UDP_V4_FLOW: > case SCTP_V4_FLOW: > case AH_ESP_V4_FLOW: > case IPV4_FLOW: > info->data |= RXH_IP_SRC | RXH_IP_DST; > min_revision = EFX_REV_FALCON_B0; > break; Well, you sure fulfilled your cover letter's promise. ;-) Do you really prefer this conditional mayham over copy-pasting some lines?
From: Edward Cree > Sent: 27 September 2016 17:36 ... > + case UDP_V4_FLOW: > + if (efx->rx_hash_udp_4tuple) > + /* fall through */ > + /* else fall further! */ If you invert the above and add a goto... if (!efx->rx_hash_udp_4tuple) goto set_ip; > case TCP_V4_FLOW: > - info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; > + info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; > /* fall through */ > - case UDP_V4_FLOW: > case SCTP_V4_FLOW: > case AH_ESP_V4_FLOW: > case IPV4_FLOW: set_ip: > info->data |= RXH_IP_SRC | RXH_IP_DST; > min_revision = EFX_REV_FALCON_B0; > break; It might look better. David
On 28/09/16 10:12, David Laight wrote: > If you invert the above and add a goto... > if (!efx->rx_hash_udp_4tuple) > goto set_ip; I don't mind gotos... >> case SCTP_V4_FLOW: >> case AH_ESP_V4_FLOW: >> case IPV4_FLOW: > set_ip: ...but this adds a label where we effectively already have one. I wish C allowed goto case labels. > It might look better. > David It just bugs me that it would have this unnecessary goto and label. Alternate ways to maybe make it look better, or not: * Remove the /* else fall further */ comment, does this make the indentation more or less confusing? * Include braces on the if, even though there's only one statement inside. Also, how strong are people's reaction to this? If it's just "I personally wouldn't do it that way", then I'm tempted to go ahead anyway. But if it's "NAK NAK NAK burn the heretic", that's another matter. -Ed
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c index 9f6d769..e61807e 100644 --- a/drivers/net/ethernet/sfc/ef10.c +++ b/drivers/net/ethernet/sfc/ef10.c @@ -2319,8 +2319,10 @@ static void efx_ef10_set_rss_flags(struct efx_nic *efx, u32 context) flags |= RSS_MODE_HASH_PORTS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV4_RSS_MODE_LBN; flags |= RSS_MODE_HASH_PORTS << MC_CMD_RSS_CONTEXT_GET_FLAGS_OUT_UDP_IPV6_RSS_MODE_LBN; MCDI_SET_DWORD(inbuf, RSS_CONTEXT_SET_FLAGS_IN_FLAGS, flags); - efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_SET_FLAGS, inbuf, sizeof(inbuf), - NULL, 0, NULL); + if (!efx_mcdi_rpc(efx, MC_CMD_RSS_CONTEXT_SET_FLAGS, inbuf, sizeof(inbuf), + NULL, 0, NULL)) + /* Succeeded, so UDP 4-tuple is now enabled */ + efx->rx_hash_udp_4tuple = true; } static int efx_ef10_alloc_rss_context(struct efx_nic *efx, u32 *context, diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c index 445ccdb..4a58899 100644 --- a/drivers/net/ethernet/sfc/ethtool.c +++ b/drivers/net/ethernet/sfc/ethtool.c @@ -968,20 +968,26 @@ efx_ethtool_get_rxnfc(struct net_device *net_dev, info->data = 0; switch (info->flow_type) { + case UDP_V4_FLOW: + if (efx->rx_hash_udp_4tuple) + /* fall through */ + /* else fall further! */ case TCP_V4_FLOW: - info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; + info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; /* fall through */ - case UDP_V4_FLOW: case SCTP_V4_FLOW: case AH_ESP_V4_FLOW: case IPV4_FLOW: info->data |= RXH_IP_SRC | RXH_IP_DST; min_revision = EFX_REV_FALCON_B0; break; + case UDP_V6_FLOW: + if (efx->rx_hash_udp_4tuple) + /* fall through */ + /* else fall further! */ case TCP_V6_FLOW: - info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; + info->data |= RXH_L4_B_0_1 | RXH_L4_B_2_3; /* fall through */ - case UDP_V6_FLOW: case SCTP_V6_FLOW: case AH_ESP_V6_FLOW: case IPV6_FLOW: diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index 99d8c82..fec51c4 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -853,6 +853,7 @@ struct vfdi_status; * @rx_hash_key: Toeplitz hash key for RSS * @rx_indir_table: Indirection table for RSS * @rx_scatter: Scatter mode enabled for receives + * @rx_hash_udp_4tuple: UDP 4-tuple hashing enabled * @int_error_count: Number of internal errors seen recently * @int_error_expire: Time at which error count will be expired * @irq_soft_enabled: Are IRQs soft-enabled? If not, IRQ handler will @@ -990,6 +991,7 @@ struct efx_nic { u8 rx_hash_key[40]; u32 rx_indir_table[128]; bool rx_scatter; + bool rx_hash_udp_4tuple; unsigned int_error_count; unsigned long int_error_expire;
Signed-off-by: Edward Cree <ecree@solarflare.com> --- drivers/net/ethernet/sfc/ef10.c | 6 ++++-- drivers/net/ethernet/sfc/ethtool.c | 14 ++++++++++---- drivers/net/ethernet/sfc/net_driver.h | 2 ++ 3 files changed, 16 insertions(+), 6 deletions(-)