Re: [Intel-wired-lan] [PATCH net-next] iavf: remove double underscores from states names

Re: [Intel-wired-lan] [PATCH net-next] iavf: remove double underscores from states names

[Alexander Lobakin]

From: Ahmed Zaki <ahmed.zaki@intel.com>
Date: Wed, 19 Apr 2023 17:17:51 -0600

+ netdev

Please add netdev ML next time you send networking stuff, not only Jakub :D

> No need to prepend some states names with "__", especially that others do
> not have these underscores.

What's the value of this change, apart from 400 locs diffstat? I don't
say having prefixes for general-purpose definitions is correct, but...
Dunno. Would be nice to have as a part of some series with assorted
improvements, but not standalone. Or it's just me =\

> 
> Link: https://lore.kernel.org/netdev/20230406130415.1aa1d1cd@kernel.org
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>

[...]

> @@ -468,36 +468,36 @@ extern char iavf_driver_name[];
>  static inline const char *iavf_state_str(enum iavf_state_t state)
>  {
>  	switch (state) {
> -	case __IAVF_STARTUP:
> -		return "__IAVF_STARTUP";
> -	case __IAVF_REMOVE:
> -		return "__IAVF_REMOVE";
> -	case __IAVF_INIT_VERSION_CHECK:
> -		return "__IAVF_INIT_VERSION_CHECK";
> -	case __IAVF_INIT_GET_RESOURCES:
> -		return "__IAVF_INIT_GET_RESOURCES";
> -	case __IAVF_INIT_EXTENDED_CAPS:
> -		return "__IAVF_INIT_EXTENDED_CAPS";
> -	case __IAVF_INIT_CONFIG_ADAPTER:
> -		return "__IAVF_INIT_CONFIG_ADAPTER";
> -	case __IAVF_INIT_SW:
> -		return "__IAVF_INIT_SW";
> -	case __IAVF_INIT_FAILED:
> -		return "__IAVF_INIT_FAILED";
> -	case __IAVF_RESETTING:
> -		return "__IAVF_RESETTING";
> -	case __IAVF_COMM_FAILED:
> -		return "__IAVF_COMM_FAILED";
> -	case __IAVF_DOWN:
> -		return "__IAVF_DOWN";
> -	case __IAVF_DOWN_PENDING:
> -		return "__IAVF_DOWN_PENDING";
> -	case __IAVF_TESTING:
> -		return "__IAVF_TESTING";
> -	case __IAVF_RUNNING:
> -		return "__IAVF_RUNNING";
> +	case IAVF_STARTUP:
> +		return "IAVF_STARTUP";
> +	case IAVF_REMOVE:
> +		return "IAVF_REMOVE";
> +	case IAVF_INIT_VERSION_CHECK:
> +		return "IAVF_INIT_VERSION_CHECK";
> +	case IAVF_INIT_GET_RESOURCES:
> +		return "IAVF_INIT_GET_RESOURCES";
> +	case IAVF_INIT_EXTENDED_CAPS:
> +		return "IAVF_INIT_EXTENDED_CAPS";
> +	case IAVF_INIT_CONFIG_ADAPTER:
> +		return "IAVF_INIT_CONFIG_ADAPTER";
> +	case IAVF_INIT_SW:
> +		return "IAVF_INIT_SW";
> +	case IAVF_INIT_FAILED:
> +		return "IAVF_INIT_FAILED";
> +	case IAVF_RESETTING:
> +		return "IAVF_RESETTING";
> +	case IAVF_COMM_FAILED:
> +		return "IAVF_COMM_FAILED";
> +	case IAVF_DOWN:
> +		return "IAVF_DOWN";
> +	case IAVF_DOWN_PENDING:
> +		return "IAVF_DOWN_PENDING";
> +	case IAVF_TESTING:
> +		return "IAVF_TESTING";
> +	case IAVF_RUNNING:
> +		return "IAVF_RUNNING";
>  	default:
> -		return "__IAVF_UNKNOWN_STATE";
> +		return "IAVF_UNKNOWN_STATE";
>  	}
>  }

If you want to do something really useful, pls replace that non-sense
with O(1) array lookup:

iavf_something.c:

const char * const iavf_states[] = {
#define __IAVF_STATE_STR(s)	[IAVF_##s]	= "IAVF_" #s
	__IAVF_STATE_STR(STARTUP),
	__IAVF_STATE_STR(REMOVE),
...
	__IAVF_STATE_STR(RUNNING),
#undef __IAVF_STATE_STR
	"IAVF_UNKNOWN_STATE",
}

iavf.h:

extern const char * const iavf_states[];

static inline const char *iavf_state_str(enum iavf_state_t state)
{
	if (unlikely(state > IAVF_STATE_RUNNING))
		state = IAVF_STATE_RUNNING + 1;

	// ^ or better just add `IAVF_UNKNOWN_STATE` to the enum
	// to not open-code `RUNNING + 1`

	return iavf_states[state];
}

You can even just copy'n'paste that and make a patch, I think it would
work :D
And attach bloat-o-meter output to the commit message, I'd like to watch
the result :3

[...]

Thanks,
Olek

Re: [Intel-wired-lan] [PATCH net-next] iavf: remove double underscores from states names

[Jakub Kicinski]

On Thu, 20 Apr 2023 16:33:54 +0200 Alexander Lobakin wrote:
> > No need to prepend some states names with "__", especially that others do
> > not have these underscores.  
> 
> What's the value of this change, apart from 400 locs diffstat? I don't
> say having prefixes for general-purpose definitions is correct, but...
> Dunno. Would be nice to have as a part of some series with assorted
> improvements, but not standalone. Or it's just me =\

Yeah, I'm not sure it's bad enough to justify the code churn, either.
Up to you folks at Intel.