Re: [RFC PATCH net-next 2/2] sfc: report 4-tuple UDP hashing to ethtool, if it's enabled

[Edward Cree]

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

RE: [RFC PATCH net-next 2/2] sfc: report 4-tuple UDP hashing to ethtool, if it's enabled

[David Laight]

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

Re: [RFC PATCH net-next 2/2] sfc: report 4-tuple UDP hashing to ethtool, if it's enabled

[Mintz, Yuval]

>  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?