Re: [vmw_vmci 11/11] Apply the header code to make VMCI build

[Sam Ravnborg <sam@ravnborg.org>]

Hi Andrew.

A few things noted in the following..

> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 2661f6e..fe38c7a 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -517,4 +517,5 @@ source "drivers/misc/lis3lv02d/Kconfig"
>  source "drivers/misc/carma/Kconfig"
>  source "drivers/misc/altera-stapl/Kconfig"
>  source "drivers/misc/mei/Kconfig"
> +source "drivers/misc/vmw_vmci/Kconfig"
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 456972f..af9e413 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -51,3 +51,4 @@ obj-y				+= carma/
>  obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
>  obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
>  obj-$(CONFIG_INTEL_MEI)		+= mei/
> +obj-y				+= vmw_vmci/

Please use obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/

like we do in the other cases. This prevents us from visiting the directory
when this feature is not enabled.

> +++ b/drivers/misc/vmw_vmci/Makefile
> @@ -0,0 +1,43 @@
> +################################################################################
> +#
> +# Linux driver for VMware's VMCI device.
> +#
> +# Copyright (C) 2007-2012, VMware, Inc. All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License as published by the
> +# Free Software Foundation; version 2 of the License and no later version.
> +#
> +# This program is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> +# NON INFRINGEMENT.  See the GNU General Public License for more
> +# details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> +#
> +# The full GNU General Public License is included in this distribution in
> +# the file called "COPYING".
> +#
> +# Maintained by: Andrew Stiegmann <pv-drivers@vmware.com>
> +#
> +################################################################################
Lot's of boilerplate noise for such a simple file...

> +
> +#
> +# Makefile for the VMware VMCI
> +#
> +
> +obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci.o
> +
> +vmw_vmci-objs += vmci_context.o
> +vmw_vmci-objs += vmci_datagram.o
> +vmw_vmci-objs += vmci_doorbell.o
> +vmw_vmci-objs += vmci_driver.o
> +vmw_vmci-objs += vmci_event.o
> +vmw_vmci-objs += vmci_handle_array.o
> +vmw_vmci-objs += vmci_hash_table.o
> +vmw_vmci-objs += vmci_queue_pair.o
> +vmw_vmci-objs += vmci_resource.o
> +vmw_vmci-objs += vmci_route.o

please use:
vmw_vmci-y += vmci_context.o
vmw_vmci-y += vmci_datagram.o
vmw_vmci-y += vmci_doorbell.o

This is recommended these days and allows you to enable/disable
single files later using a config option.



> diff --git a/drivers/misc/vmw_vmci/vmci_common_int.h b/drivers/misc/vmw_vmci/vmci_common_int.h
> +
> +#ifndef _VMCI_COMMONINT_H_
> +#define _VMCI_COMMONINT_H_
> +
> +#include <linux/printk.h>
> +#include <linux/vmw_vmci_defs.h>

Use inverse chrismas tree here.
Longer include lines first, and soret alphabetically when
lines are of the same length.
This applies likely in many cases.

> +#include "vmci_handle_array.h"
> +
> +#define ASSERT(cond) BUG_ON(!(cond))
> +
> +#define CAN_BLOCK(_f) (!((_f) & VMCI_QPFLAG_NONBLOCK))
> +#define QP_PINNED(_f) ((_f) & VMCI_QPFLAG_PINNED)

Looks like poor obscufation.
Use a statis inline function if you need a helper for this.

> +
> +/*
> + * Utilility function that checks whether two entities are allowed
> + * to interact. If one of them is restricted, the other one must
> + * be trusted.
> + */
> +static inline bool vmci_deny_interaction(uint32_t partOne,
> +					 uint32_t partTwo)

The kernel types are u32 not uint32_t - these types belongs in user-space.

> +++ b/include/linux/vmw_vmci_api.h
> +
> +#ifndef __VMW_VMCI_API_H__
> +#define __VMW_VMCI_API_H__
> +
> +#include <linux/vmw_vmci_defs.h>
> +
> +#undef  VMCI_KERNEL_API_VERSION
> +#define VMCI_KERNEL_API_VERSION_2 2
> +#define VMCI_KERNEL_API_VERSION   VMCI_KERNEL_API_VERSION_2
> +
> +typedef void (VMCI_DeviceShutdownFn) (void *deviceRegistration, void *userData);
> +
> +bool VMCI_DeviceGet(uint32_t *apiVersion,
> +		    VMCI_DeviceShutdownFn *deviceShutdownCB,
> +		    void *userData, void **deviceRegistration);

The kernel style is to use lower_case for everything.
So this would become:

    vmci_device_get()

This is obviously a very general comment and applies everywhere.

	Sam