Re: [net-next 1/4] gve: Add basic driver framework for Compute Engine Virtual NIC

From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Catherine Sullivan <csully@google.com>
Cc: netdev@vger.kernel.org, Sagi Shahar <sagis@google.com>,
	Jon Olson <jonolson@google.com>,
	Willem de Bruijn <willemb@google.com>,
	Luigi Rizzo <lrizzo@google.com>
Subject: Re: [net-next 1/4] gve: Add basic driver framework for Compute Engine Virtual NIC
Date: Wed, 26 Jun 2019 16:08:32 -0700	[thread overview]
Message-ID: <20190626160832.3f191a53@cakuba.netronome.com> (raw)
In-Reply-To: <20190626185251.205687-2-csully@google.com>

On Wed, 26 Jun 2019 11:52:48 -0700, Catherine Sullivan wrote:
> Add a driver framework for the Compute Engine Virtual NIC that will be
> available in the future.
> 
> At this point the only functionality is loading the driver.
> 
> Signed-off-by: Catherine Sullivan <csully@google.com>
> Signed-off-by: Sagi Shahar <sagis@google.com>
> Signed-off-by: Jon Olson <jonolson@google.com>
> Acked-by: Willem de Bruijn <willemb@google.com>
> Reviewed-by: Luigi Rizzo <lrizzo@google.com>
> ---

> +if NET_VENDOR_GOOGLE
> +
> +config GVE
> +	tristate "Google Virtual NIC (gVNIC) support"
> +	depends on (PCI_MSI && X86)

We usually prefer for drivers not to depend on the platform unless
really necessary, but IDK how that applies to the curious new world 
of NICs nobody can buy :)

> +	help
> +	  This driver supports Google Virtual NIC (gVNIC)"
> +
> +	  To compile this driver as a module, choose M here.
> +	  The module will be called gve.
> +
> +endif #NET_VENDOR_GOOGLE

> +void gve_adminq_release(struct gve_priv *priv)
> +{
> +	int i;
> +
> +	/* Tell the device the adminq is leaving */
> +	writel(0x0, &priv->reg_bar0->adminq_pfn);
> +	for (i = 0; i < GVE_MAX_ADMINQ_RELEASE_CHECK; i++) {
> +		if (!readl(&priv->reg_bar0->adminq_pfn)) {
> +			gve_clear_device_rings_ok(priv);
> +			gve_clear_device_resources_ok(priv);
> +			gve_clear_admin_queue_ok(priv);
> +			return;
> +		}
> +		msleep(GVE_ADMINQ_SLEEP_LEN);
> +	}
> +	/* If this is reached the device is unrecoverable and still holding
> +	 * memory. Anything other than a BUG risks memory corruption.
> +	 */
> +	WARN(1, "Unrecoverable platform error!");
> +	BUG();

Please don't add BUG()s to the kernel.  You're probably better off
spinning for ever in the loop above.  Also if there is an IOMMU in 
the way the device won't be able to mess with the memory.

> +}
> +

> diff --git a/drivers/net/ethernet/google/gve/gve_size_assert.h b/drivers/net/ethernet/google/gve/gve_size_assert.h
> new file mode 100644
> index 000000000000..a58422d4f16e
> --- /dev/null
> +++ b/drivers/net/ethernet/google/gve/gve_size_assert.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT)
> + * Google virtual Ethernet (gve) driver
> + *
> + * Copyright (C) 2015-2019 Google, Inc.
> + */
> +
> +#ifndef _GVE_ASSERT_H_
> +#define _GVE_ASSERT_H_
> +#define GVE_ASSERT_SIZE(tag, type, size) \
> +	static void gve_assert_size_ ## type(void) __attribute__((used)); \
> +	static inline void gve_assert_size_ ## type(void) \
> +	{ \
> +		BUILD_BUG_ON(sizeof(tag type) != (size)); \
> +	}
> +#endif /* _GVE_ASSERT_H_ */

Please use static_assert() directly in your struct size checks.