Re: [PATCH net-next v5 1/2] gve: Adding a new AdminQ command to verify driver

[Leon Romanovsky <leon@kernel.org>]

On Thu, Nov 17, 2022 at 08:27:00AM -0800, Jeroen de Borst wrote:
> Check whether the driver is compatible with the device
> presented.
> 
> Signed-off-by: Jeroen de Borst <jeroendb@google.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
>  drivers/net/ethernet/google/gve/gve.h        |  1 +
>  drivers/net/ethernet/google/gve/gve_adminq.c | 21 +++++++-
>  drivers/net/ethernet/google/gve/gve_adminq.h | 49 ++++++++++++++++++
>  drivers/net/ethernet/google/gve/gve_main.c   | 52 ++++++++++++++++++++
>  4 files changed, 122 insertions(+), 1 deletion(-)

<...>

> +enum gve_driver_capbility {
> +	gve_driver_capability_gqi_qpl = 0,
> +	gve_driver_capability_gqi_rda = 1,
> +	gve_driver_capability_dqo_qpl = 2, /* reserved for future use */
> +	gve_driver_capability_dqo_rda = 3,
> +};
> +
> +#define GVE_CAP1(a) BIT((int)a)
> +#define GVE_CAP2(a) BIT(((int)a) - 64)
> +#define GVE_CAP3(a) BIT(((int)a) - 128)
> +#define GVE_CAP4(a) BIT(((int)a) - 192)
> +
> +#define GVE_DRIVER_CAPABILITY_FLAGS1 \
> +	(GVE_CAP1(gve_driver_capability_gqi_qpl) | \
> +	 GVE_CAP1(gve_driver_capability_gqi_rda) | \
> +	 GVE_CAP1(gve_driver_capability_dqo_rda))

I never understood it why people do it.

You created named enum gve_driver_capbility, but nothing in the code
uses this name and you use the values as bits, which later you cast them
to int.

It will be much saner, if you use anonymous enum, which is int in
C-world and you won't need any (int) casting when you call to BIT().

BTW, you don't need this casting anyway and it will be much better if you
use >> for bit operations and not "- 64|128|192".

Thanks