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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y3qCnJi2D/WOK0BG@unreal>
Date:   Sun, 20 Nov 2022 21:40:12 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Jeroen de Borst <jeroendb@...gle.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        jesse.brandeburg@...el.com
Subject: Re: [PATCH net-next v5 1/2] gve: Adding a new AdminQ command to
 verify driver

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@...gle.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@...el.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