[PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[K. Y. Srinivasan]

The hv_mouse.c implements a hid compliant mouse driver for use on a 
Hyper-V based system. This driver is currently in the staging area and
as part of the effort to move this driver out of staging, I had posted
the driver code for community review a few weeks ago. This current patch
addresses all the review comments I have gotten to date.

As per Greg's suggestion, this patch does not get rid of the code from
the staging area. Once the mouse driver lands under the hid directory,
we will cleanup the staging directory.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hid/Kconfig    |    6 +
 drivers/hid/Makefile   |    1 +
 drivers/hid/hv_mouse.c |  592 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 599 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hv_mouse.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 1130a89..068dd97 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -613,6 +613,12 @@ config HID_ZYDACRON
 	---help---
 	Support for Zydacron remote control.
 
+config HYPERV_MOUSE
+	tristate "Microsoft Hyper-V mouse driver"
+	depends on HYPERV
+	---help---
+	  Select this option to enable the Hyper-V mouse driver.
+
 endmenu
 
 endif # HID_SUPPORT
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0a0a38e..436ac2e 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
 obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
 obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
 obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
+obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o
 
 obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/
diff --git a/drivers/hid/hv_mouse.c b/drivers/hid/hv_mouse.c
new file mode 100644
index 0000000..dd42411
--- /dev/null
+++ b/drivers/hid/hv_mouse.c
@@ -0,0 +1,592 @@
+/*
+ *  Copyright (c) 2009, Citrix Systems, Inc.
+ *  Copyright (c) 2010, Microsoft Corporation.
+ *  Copyright (c) 2011, Novell Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms and conditions of the GNU General Public License,
+ *  version 2, as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ *  more details.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/completion.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/hiddev.h>
+#include <linux/hyperv.h>
+
+
+struct hv_input_dev_info {
+	unsigned int size;
+	unsigned short vendor;
+	unsigned short product;
+	unsigned short version;
+	unsigned short reserved[11];
+};
+
+/* The maximum size of a synthetic input message. */
+#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
+
+/*
+ * Current version
+ *
+ * History:
+ * Beta, RC < 2008/1/22        1,0
+ * RC > 2008/1/22              2,0
+ */
+#define SYNTHHID_INPUT_VERSION_MAJOR	2
+#define SYNTHHID_INPUT_VERSION_MINOR	0
+#define SYNTHHID_INPUT_VERSION		(SYNTHHID_INPUT_VERSION_MINOR | \
+					 (SYNTHHID_INPUT_VERSION_MAJOR << 16))
+
+
+#pragma pack(push, 1)
+/*
+ * Message types in the synthetic input protocol
+ */
+enum synthhid_msg_type {
+	SYNTH_HID_PROTOCOL_REQUEST,
+	SYNTH_HID_PROTOCOL_RESPONSE,
+	SYNTH_HID_INITIAL_DEVICE_INFO,
+	SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
+	SYNTH_HID_INPUT_REPORT,
+	SYNTH_HID_MAX
+};
+
+/*
+ * Basic message structures.
+ */
+struct synthhid_msg_hdr {
+	enum synthhid_msg_type type;
+	u32 size;
+};
+
+struct synthhid_msg {
+	struct synthhid_msg_hdr header;
+	char data[1]; /* Enclosed message */
+};
+
+union synthhid_version {
+	struct {
+		u16 minor_version;
+		u16 major_version;
+	};
+	u32 version;
+};
+
+/*
+ * Protocol messages
+ */
+struct synthhid_protocol_request {
+	struct synthhid_msg_hdr header;
+	union synthhid_version version_requested;
+};
+
+struct synthhid_protocol_response {
+	struct synthhid_msg_hdr header;
+	union synthhid_version version_requested;
+	unsigned char approved;
+};
+
+struct synthhid_device_info {
+	struct synthhid_msg_hdr header;
+	struct hv_input_dev_info hid_dev_info;
+	struct hid_descriptor hid_descriptor;
+};
+
+struct synthhid_device_info_ack {
+	struct synthhid_msg_hdr header;
+	unsigned char reserved;
+};
+
+struct synthhid_input_report {
+	struct synthhid_msg_hdr header;
+	char buffer[1];
+};
+
+#pragma pack(pop)
+
+#define INPUTVSC_SEND_RING_BUFFER_SIZE		(10*PAGE_SIZE)
+#define INPUTVSC_RECV_RING_BUFFER_SIZE		(10*PAGE_SIZE)
+
+#define NBITS(x) (((x)/BITS_PER_LONG)+1)
+
+enum pipe_prot_msg_type {
+	PIPE_MESSAGE_INVALID,
+	PIPE_MESSAGE_DATA,
+	PIPE_MESSAGE_MAXIMUM
+};
+
+
+struct pipe_prt_msg {
+	enum pipe_prot_msg_type type;
+	u32 size;
+	char data[1];
+};
+
+struct  mousevsc_prt_msg {
+	enum pipe_prot_msg_type type;
+	u32 size;
+	union {
+		struct synthhid_protocol_request request;
+		struct synthhid_protocol_response response;
+		struct synthhid_device_info_ack ack;
+	};
+};
+
+/*
+ * Represents an mousevsc device
+ */
+struct mousevsc_dev {
+	struct hv_device	*device;
+	bool			init_complete;
+	struct mousevsc_prt_msg	protocol_req;
+	struct mousevsc_prt_msg	protocol_resp;
+	/* Synchronize the request/response if needed */
+	struct completion	wait_event;
+	int			dev_info_status;
+
+	struct hid_descriptor	*hid_desc;
+	unsigned char		*report_desc;
+	u32			report_desc_size;
+	struct hv_input_dev_info hid_dev_info;
+	bool			connected;
+	struct hid_device       *hid_device;
+};
+
+
+static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
+{
+	struct mousevsc_dev *input_dev;
+
+	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
+
+	if (!input_dev)
+		return NULL;
+
+	input_dev->device = device;
+	hv_set_drvdata(device, input_dev);
+	init_completion(&input_dev->wait_event);
+
+	return input_dev;
+}
+
+static void free_input_device(struct mousevsc_dev *device)
+{
+	kfree(device->hid_desc);
+	kfree(device->report_desc);
+	hv_set_drvdata(device->device, NULL);
+	kfree(device);
+}
+
+
+static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
+				struct synthhid_device_info *device_info)
+{
+	int ret = 0;
+	struct hid_descriptor *desc;
+	struct mousevsc_prt_msg ack;
+
+	/* Assume success for now */
+	input_device->dev_info_status = 0;
+
+	memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
+		sizeof(struct hv_input_dev_info));
+
+	desc = &device_info->hid_descriptor;
+	WARN_ON(desc->bLength == 0);
+
+	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
+
+	if (!input_device->hid_desc)
+		goto cleanup;
+
+	memcpy(input_device->hid_desc, desc, desc->bLength);
+
+	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
+	if (input_device->report_desc_size == 0)
+		goto cleanup;
+	input_device->report_desc = kzalloc(input_device->report_desc_size,
+					  GFP_ATOMIC);
+
+	if (!input_device->report_desc)
+		goto cleanup;
+
+	memcpy(input_device->report_desc,
+	       ((unsigned char *)desc) + desc->bLength,
+	       desc->desc[0].wDescriptorLength);
+
+	/* Send the ack */
+	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
+
+	ack.type = PIPE_MESSAGE_DATA;
+	ack.size = sizeof(struct synthhid_device_info_ack);
+
+	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
+	ack.ack.header.size = 1;
+	ack.ack.reserved = 0;
+
+	ret = vmbus_sendpacket(input_device->device->channel,
+			&ack,
+			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
+			sizeof(struct synthhid_device_info_ack),
+			(unsigned long)&ack,
+			VM_PKT_DATA_INBAND,
+			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (ret != 0)
+		goto cleanup;
+
+	complete(&input_device->wait_event);
+
+	return;
+
+cleanup:
+	kfree(input_device->hid_desc);
+	input_device->hid_desc = NULL;
+
+	kfree(input_device->report_desc);
+	input_device->report_desc = NULL;
+
+	input_device->dev_info_status = -1;
+	complete(&input_device->wait_event);
+}
+
+static void mousevsc_on_receive(struct hv_device *device,
+				struct vmpacket_descriptor *packet)
+{
+	struct pipe_prt_msg *pipe_msg;
+	struct synthhid_msg *hid_msg;
+	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+	struct synthhid_input_report *input_report;
+
+	pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
+						(packet->offset8 << 3));
+
+	if (pipe_msg->type != PIPE_MESSAGE_DATA)
+		return;
+
+	hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
+
+	switch (hid_msg->header.type) {
+	case SYNTH_HID_PROTOCOL_RESPONSE:
+		memcpy(&input_dev->protocol_resp, pipe_msg,
+		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
+		       sizeof(unsigned char));
+		complete(&input_dev->wait_event);
+		break;
+
+	case SYNTH_HID_INITIAL_DEVICE_INFO:
+		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
+
+		/*
+		 * Parse out the device info into device attr,
+		 * hid desc and report desc
+		 */
+		mousevsc_on_receive_device_info(input_dev,
+			(struct synthhid_device_info *)&pipe_msg->data[0]);
+		break;
+	case SYNTH_HID_INPUT_REPORT:
+		input_report =
+			(struct synthhid_input_report *)&pipe_msg->data[0];
+		if (!input_dev->init_complete)
+			break;
+		hid_input_report(input_dev->hid_device,
+				HID_INPUT_REPORT, input_report->buffer,
+				input_report->header.size, 1);
+		break;
+	default:
+		pr_err("unsupported hid msg type - type %d len %d",
+		       hid_msg->header.type, hid_msg->header.size);
+		break;
+	}
+
+}
+
+static void mousevsc_on_channel_callback(void *context)
+{
+	const int packet_size = 0x100;
+	int ret;
+	struct hv_device *device = context;
+	u32 bytes_recvd;
+	u64 req_id;
+	struct vmpacket_descriptor *desc;
+	unsigned char	*buffer;
+	int	bufferlen = packet_size;
+
+	buffer = kmalloc(bufferlen, GFP_ATOMIC);
+	if (!buffer)
+		return;
+
+	do {
+		ret = vmbus_recvpacket_raw(device->channel, buffer,
+					bufferlen, &bytes_recvd, &req_id);
+
+		switch (ret) {
+		case 0:
+			if (bytes_recvd <= 0) {
+				kfree(buffer);
+				return;
+			}
+			desc = (struct vmpacket_descriptor *)buffer;
+
+			switch (desc->type) {
+			case VM_PKT_COMP:
+				break;
+
+			case VM_PKT_DATA_INBAND:
+				mousevsc_on_receive(device, desc);
+				break;
+
+			default:
+				pr_err("unhandled packet type %d, tid %llx len %d\n",
+				   desc->type,
+				   req_id,
+				   bytes_recvd);
+				break;
+			}
+
+			break;
+
+		case -ENOBUFS:
+			kfree(buffer);
+			/* Handle large packet */
+			bufferlen = bytes_recvd;
+			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+
+			if (buffer == NULL) {
+				return;
+			}
+			break;
+		}
+	} while (1);
+
+}
+
+static int mousevsc_connect_to_vsp(struct hv_device *device)
+{
+	int ret = 0;
+	int t;
+	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+	struct mousevsc_prt_msg *request;
+	struct mousevsc_prt_msg *response;
+
+
+	request = &input_dev->protocol_req;
+
+	memset(request, 0, sizeof(struct mousevsc_prt_msg));
+
+	request->type = PIPE_MESSAGE_DATA;
+	request->size = sizeof(struct synthhid_protocol_request);
+
+	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
+	request->request.header.size = sizeof(unsigned int);
+	request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
+
+
+	ret = vmbus_sendpacket(device->channel, request,
+				sizeof(struct pipe_prt_msg) -
+				sizeof(unsigned char) +
+				sizeof(struct synthhid_protocol_request),
+				(unsigned long)request,
+				VM_PKT_DATA_INBAND,
+				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (ret != 0)
+		goto cleanup;
+
+	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+	if (t == 0) {
+		ret = -ETIMEDOUT;
+		goto cleanup;
+	}
+
+	response = &input_dev->protocol_resp;
+
+	if (!response->response.approved) {
+		pr_err("synthhid protocol request failed (version %d)\n",
+		       SYNTHHID_INPUT_VERSION);
+		ret = -ENODEV;
+		goto cleanup;
+	}
+
+	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+	if (t == 0) {
+		ret = -ETIMEDOUT;
+		goto cleanup;
+	}
+
+	/*
+	 * We should have gotten the device attr, hid desc and report
+	 * desc at this point
+	 */
+	if (input_dev->dev_info_status)
+		ret = -ENOMEM;
+
+cleanup:
+
+	return ret;
+}
+
+static int mousevsc_hid_open(struct hid_device *hid)
+{
+	return 0;
+}
+
+static int mousevsc_hid_start(struct hid_device *hid)
+{
+	return 0;
+}
+
+static void mousevsc_hid_close(struct hid_device *hid)
+{
+}
+
+static void mousevsc_hid_stop(struct hid_device *hid)
+{
+}
+
+static struct hid_ll_driver mousevsc_ll_driver = {
+	.open = mousevsc_hid_open,
+	.close = mousevsc_hid_close,
+	.start = mousevsc_hid_start,
+	.stop = mousevsc_hid_stop,
+};
+
+static struct hid_driver mousevsc_hid_driver;
+
+static int mousevsc_probe(struct hv_device *device,
+			const struct hv_vmbus_device_id *dev_id)
+{
+	int ret = 0;
+	struct mousevsc_dev *input_dev;
+	struct hid_device *hid_dev;
+
+	input_dev = alloc_input_device(device);
+
+	if (!input_dev)
+		return -ENOMEM;
+
+	input_dev->init_complete = false;
+
+	ret = vmbus_open(device->channel,
+		INPUTVSC_SEND_RING_BUFFER_SIZE,
+		INPUTVSC_RECV_RING_BUFFER_SIZE,
+		NULL,
+		0,
+		mousevsc_on_channel_callback,
+		device
+		);
+
+	if (ret != 0) {
+		free_input_device(input_dev);
+		return ret;
+	}
+
+
+	ret = mousevsc_connect_to_vsp(device);
+
+	if (ret != 0)
+		goto probe_err0;
+
+	/* workaround SA-167 */
+	if (input_dev->report_desc[14] == 0x25)
+		input_dev->report_desc[14] = 0x29;
+
+	hid_dev = hid_allocate_device();
+	if (IS_ERR(hid_dev)) {
+		ret = PTR_ERR(hid_dev);
+		goto probe_err0;
+	}
+
+	hid_dev->ll_driver = &mousevsc_ll_driver;
+	hid_dev->driver = &mousevsc_hid_driver;
+	hid_dev->bus = BUS_VIRTUAL;
+	hid_dev->vendor = input_dev->hid_dev_info.vendor;
+	hid_dev->product = input_dev->hid_dev_info.product;
+	hid_dev->version = input_dev->hid_dev_info.version;
+	input_dev->hid_device = hid_dev;
+
+	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
+
+	ret = hid_parse_report(hid_dev, input_dev->report_desc,
+				input_dev->report_desc_size);
+
+	if (ret) {
+		hid_err(hid_dev, "parse failed\n");
+		goto probe_err1;
+	}
+
+	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
+
+	if (ret) {
+		hid_err(hid_dev, "hw start failed\n");
+		goto probe_err1;
+	}
+
+	input_dev->connected = true;
+	input_dev->init_complete = true;
+
+	return ret;
+
+probe_err1:
+	hid_destroy_device(hid_dev);
+
+probe_err0:
+	vmbus_close(device->channel);
+	free_input_device(input_dev);
+	return ret;
+}
+
+
+static int mousevsc_remove(struct hv_device *dev)
+{
+	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
+
+	vmbus_close(dev->channel);
+
+	if (input_dev->connected) {
+		hidinput_disconnect(input_dev->hid_device);
+		input_dev->connected = false;
+		hid_destroy_device(input_dev->hid_device);
+	}
+
+	free_input_device(input_dev);
+
+	return 0;
+}
+
+static const struct hv_vmbus_device_id id_table[] = {
+	/* Mouse guid */
+	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
+		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct  hv_driver mousevsc_drv = {
+	.name = "mousevsc",
+	.id_table = id_table,
+	.probe = mousevsc_probe,
+	.remove = mousevsc_remove,
+};
+
+static int __init mousevsc_init(void)
+{
+	return vmbus_driver_register(&mousevsc_drv);
+}
+
+static void __exit mousevsc_exit(void)
+{
+	vmbus_driver_unregister(&mousevsc_drv);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_VERSION(HV_DRV_VERSION);
+module_init(mousevsc_init);
+module_exit(mousevsc_exit);
-- 
1.7.4.1

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Olaf Hering]

On Wed, Oct 26, K. Y. Srinivasan wrote:

> +static struct  hv_driver mousevsc_drv = {
> +	.name = "mousevsc",

This should be KBUILD_MODNAME.

Olaf

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Thursday, October 27, 2011 6:37 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> dmitry.torokhov@gmail.com; jkosina@suse.cz
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Wed, Oct 26, K. Y. Srinivasan wrote:
> 
> > +static struct  hv_driver mousevsc_drv = {
> > +	.name = "mousevsc",
> 
> This should be KBUILD_MODNAME.

Thanks Olaf. Once I get some more comments (especially from Jiri), I will re-spin this patch
addressing all the comments I have gotten to date.

Regards,

K. Y 

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Jiri Kosina]

On Wed, 26 Oct 2011, K. Y. Srinivasan wrote:

> The hv_mouse.c implements a hid compliant mouse driver for use on a 
> Hyper-V based system. This driver is currently in the staging area and
> as part of the effort to move this driver out of staging, I had posted
> the driver code for community review a few weeks ago. This current patch
> addresses all the review comments I have gotten to date.
> 
> As per Greg's suggestion, this patch does not get rid of the code from
> the staging area. Once the mouse driver lands under the hid directory,
> we will cleanup the staging directory.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>

K.Y,,

first, thanks a lot for your efforts on working on this driver. Porting it 
to HID infrastructure definitely is a huge and proper step.

> ---
>  drivers/hid/Kconfig    |    6 +
>  drivers/hid/Makefile   |    1 +
>  drivers/hid/hv_mouse.c |  592 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 599 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hid/hv_mouse.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 1130a89..068dd97 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -613,6 +613,12 @@ config HID_ZYDACRON
>  	---help---
>  	Support for Zydacron remote control.
>  
> +config HYPERV_MOUSE
> +	tristate "Microsoft Hyper-V mouse driver"
> +	depends on HYPERV
> +	---help---
> +	  Select this option to enable the Hyper-V mouse driver.
> +
>  endmenu
>  
>  endif # HID_SUPPORT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 0a0a38e..436ac2e 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
>  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
>  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
>  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> +obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o

I'd prefer a bit to follow the current naming of the drivers in 
drivers/hid directory ... all the device-specific (vendor-specific) 
drivers currently are called hid-<vendor> or similar.

We could talk about changing this naming scheme, but before we reach any 
conclusion on this, I'd prefer this to stay for all drivers if possible.

>  obj-$(CONFIG_USB_HID)		+= usbhid/
>  obj-$(CONFIG_USB_MOUSE)		+= usbhid/
> diff --git a/drivers/hid/hv_mouse.c b/drivers/hid/hv_mouse.c
> new file mode 100644
> index 0000000..dd42411
> --- /dev/null
> +++ b/drivers/hid/hv_mouse.c
> @@ -0,0 +1,592 @@
> +/*
> + *  Copyright (c) 2009, Citrix Systems, Inc.
> + *  Copyright (c) 2010, Microsoft Corporation.
> + *  Copyright (c) 2011, Novell Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms and conditions of the GNU General Public License,
> + *  version 2, as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope it will be useful, but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + *  more details.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/completion.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/hiddev.h>
> +#include <linux/hyperv.h>
> +
> +
> +struct hv_input_dev_info {
> +	unsigned int size;
> +	unsigned short vendor;
> +	unsigned short product;
> +	unsigned short version;
> +	unsigned short reserved[11];
> +};
> +
> +/* The maximum size of a synthetic input message. */
> +#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
> +
> +/*
> + * Current version
> + *
> + * History:
> + * Beta, RC < 2008/1/22        1,0
> + * RC > 2008/1/22              2,0
> + */
> +#define SYNTHHID_INPUT_VERSION_MAJOR	2
> +#define SYNTHHID_INPUT_VERSION_MINOR	0
> +#define SYNTHHID_INPUT_VERSION		(SYNTHHID_INPUT_VERSION_MINOR | \
> +					 (SYNTHHID_INPUT_VERSION_MAJOR << 16))
> +
> +
> +#pragma pack(push, 1)
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synthhid_msg_type {
> +	SYNTH_HID_PROTOCOL_REQUEST,
> +	SYNTH_HID_PROTOCOL_RESPONSE,
> +	SYNTH_HID_INITIAL_DEVICE_INFO,
> +	SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> +	SYNTH_HID_INPUT_REPORT,
> +	SYNTH_HID_MAX
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synthhid_msg_hdr {
> +	enum synthhid_msg_type type;
> +	u32 size;
> +};
> +
> +struct synthhid_msg {
> +	struct synthhid_msg_hdr header;
> +	char data[1]; /* Enclosed message */
> +};
> +
> +union synthhid_version {
> +	struct {
> +		u16 minor_version;
> +		u16 major_version;
> +	};
> +	u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synthhid_protocol_request {
> +	struct synthhid_msg_hdr header;
> +	union synthhid_version version_requested;
> +};
> +
> +struct synthhid_protocol_response {
> +	struct synthhid_msg_hdr header;
> +	union synthhid_version version_requested;
> +	unsigned char approved;
> +};
> +
> +struct synthhid_device_info {
> +	struct synthhid_msg_hdr header;
> +	struct hv_input_dev_info hid_dev_info;
> +	struct hid_descriptor hid_descriptor;
> +};
> +
> +struct synthhid_device_info_ack {
> +	struct synthhid_msg_hdr header;
> +	unsigned char reserved;
> +};
> +
> +struct synthhid_input_report {
> +	struct synthhid_msg_hdr header;
> +	char buffer[1];
> +};
> +
> +#pragma pack(pop)
> +
> +#define INPUTVSC_SEND_RING_BUFFER_SIZE		(10*PAGE_SIZE)
> +#define INPUTVSC_RECV_RING_BUFFER_SIZE		(10*PAGE_SIZE)

Where does the '10' constant come from? Is it completely arbitrary, or 
does it some real background?

> +
> +#define NBITS(x) (((x)/BITS_PER_LONG)+1)

Where is this macro needed? (I wouldn't notice normally, but I have 
wondered why you can't use BIT_WORD and found that I can't seem to find a 
place where this macro would actually be used :) ).

> +
> +enum pipe_prot_msg_type {
> +	PIPE_MESSAGE_INVALID,
> +	PIPE_MESSAGE_DATA,
> +	PIPE_MESSAGE_MAXIMUM
> +};
> +
> +
> +struct pipe_prt_msg {
> +	enum pipe_prot_msg_type type;
> +	u32 size;
> +	char data[1];
> +};
> +
> +struct  mousevsc_prt_msg {
> +	enum pipe_prot_msg_type type;
> +	u32 size;
> +	union {
> +		struct synthhid_protocol_request request;
> +		struct synthhid_protocol_response response;
> +		struct synthhid_device_info_ack ack;
> +	};
> +};
> +
> +/*
> + * Represents an mousevsc device
> + */
> +struct mousevsc_dev {
> +	struct hv_device	*device;
> +	bool			init_complete;
> +	struct mousevsc_prt_msg	protocol_req;
> +	struct mousevsc_prt_msg	protocol_resp;
> +	/* Synchronize the request/response if needed */
> +	struct completion	wait_event;
> +	int			dev_info_status;
> +
> +	struct hid_descriptor	*hid_desc;
> +	unsigned char		*report_desc;
> +	u32			report_desc_size;
> +	struct hv_input_dev_info hid_dev_info;
> +	bool			connected;
> +	struct hid_device       *hid_device;
> +};
> +
> +
> +static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
> +{
> +	struct mousevsc_dev *input_dev;
> +
> +	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> +
> +	if (!input_dev)
> +		return NULL;
> +
> +	input_dev->device = device;
> +	hv_set_drvdata(device, input_dev);
> +	init_completion(&input_dev->wait_event);
> +
> +	return input_dev;
> +}
> +
> +static void free_input_device(struct mousevsc_dev *device)
> +{
> +	kfree(device->hid_desc);
> +	kfree(device->report_desc);
> +	hv_set_drvdata(device->device, NULL);
> +	kfree(device);
> +}
> +
> +
> +static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
> +				struct synthhid_device_info *device_info)
> +{
> +	int ret = 0;
> +	struct hid_descriptor *desc;
> +	struct mousevsc_prt_msg ack;
> +
> +	/* Assume success for now */
> +	input_device->dev_info_status = 0;
> +
> +	memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> +		sizeof(struct hv_input_dev_info));
> +
> +	desc = &device_info->hid_descriptor;
> +	WARN_ON(desc->bLength == 0);
> +
> +	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> +
> +	if (!input_device->hid_desc)
> +		goto cleanup;
> +
> +	memcpy(input_device->hid_desc, desc, desc->bLength);
> +
> +	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> +	if (input_device->report_desc_size == 0)
> +		goto cleanup;
> +	input_device->report_desc = kzalloc(input_device->report_desc_size,
> +					  GFP_ATOMIC);
> +
> +	if (!input_device->report_desc)
> +		goto cleanup;
> +
> +	memcpy(input_device->report_desc,
> +	       ((unsigned char *)desc) + desc->bLength,
> +	       desc->desc[0].wDescriptorLength);
> +
> +	/* Send the ack */
> +	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> +
> +	ack.type = PIPE_MESSAGE_DATA;
> +	ack.size = sizeof(struct synthhid_device_info_ack);
> +
> +	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> +	ack.ack.header.size = 1;
> +	ack.ack.reserved = 0;
> +
> +	ret = vmbus_sendpacket(input_device->device->channel,
> +			&ack,
> +			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> +			sizeof(struct synthhid_device_info_ack),
> +			(unsigned long)&ack,
> +			VM_PKT_DATA_INBAND,
> +			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +	if (ret != 0)
> +		goto cleanup;
> +
> +	complete(&input_device->wait_event);
> +
> +	return;
> +
> +cleanup:
> +	kfree(input_device->hid_desc);
> +	input_device->hid_desc = NULL;
> +
> +	kfree(input_device->report_desc);
> +	input_device->report_desc = NULL;
> +
> +	input_device->dev_info_status = -1;
> +	complete(&input_device->wait_event);
> +}
> +
> +static void mousevsc_on_receive(struct hv_device *device,
> +				struct vmpacket_descriptor *packet)
> +{
> +	struct pipe_prt_msg *pipe_msg;
> +	struct synthhid_msg *hid_msg;
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> +	struct synthhid_input_report *input_report;
> +
> +	pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
> +						(packet->offset8 << 3));
> +
> +	if (pipe_msg->type != PIPE_MESSAGE_DATA)
> +		return;
> +
> +	hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
> +
> +	switch (hid_msg->header.type) {
> +	case SYNTH_HID_PROTOCOL_RESPONSE:
> +		memcpy(&input_dev->protocol_resp, pipe_msg,
> +		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
> +		       sizeof(unsigned char));

Is there any guarantee anywhere that packet doesn't get corrupted (either 
maliciously or not)?

Seems to me like we are taking it 'raw' in mousevsc_on_channel_callback() 
without any sanity checking.

If not, second argument of this memcpy() could easily overflow, causing 
quite some memory corruption, right?

Actually the question of sanity of the raw packet is much more general one 
throughout this driver. I am not very familiar with things in drivers/hv, 
hence the question.

> +		complete(&input_dev->wait_event);
> +		break;
> +
> +	case SYNTH_HID_INITIAL_DEVICE_INFO:
> +		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> +
> +		/*
> +		 * Parse out the device info into device attr,
> +		 * hid desc and report desc
> +		 */
> +		mousevsc_on_receive_device_info(input_dev,
> +			(struct synthhid_device_info *)&pipe_msg->data[0]);
> +		break;
> +	case SYNTH_HID_INPUT_REPORT:
> +		input_report =
> +			(struct synthhid_input_report *)&pipe_msg->data[0];
> +		if (!input_dev->init_complete)
> +			break;
> +		hid_input_report(input_dev->hid_device,
> +				HID_INPUT_REPORT, input_report->buffer,
> +				input_report->header.size, 1);
> +		break;
> +	default:
> +		pr_err("unsupported hid msg type - type %d len %d",
> +		       hid_msg->header.type, hid_msg->header.size);
> +		break;
> +	}
> +
> +}
> +
> +static void mousevsc_on_channel_callback(void *context)
> +{
> +	const int packet_size = 0x100;
> +	int ret;
> +	struct hv_device *device = context;
> +	u32 bytes_recvd;
> +	u64 req_id;
> +	struct vmpacket_descriptor *desc;
> +	unsigned char	*buffer;
> +	int	bufferlen = packet_size;
> +
> +	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> +	if (!buffer)
> +		return;
> +
> +	do {
> +		ret = vmbus_recvpacket_raw(device->channel, buffer,
> +					bufferlen, &bytes_recvd, &req_id);
> +
> +		switch (ret) {
> +		case 0:
> +			if (bytes_recvd <= 0) {
> +				kfree(buffer);
> +				return;
> +			}
> +			desc = (struct vmpacket_descriptor *)buffer;
> +
> +			switch (desc->type) {
> +			case VM_PKT_COMP:
> +				break;
> +
> +			case VM_PKT_DATA_INBAND:
> +				mousevsc_on_receive(device, desc);
> +				break;
> +
> +			default:
> +				pr_err("unhandled packet type %d, tid %llx len %d\n",
> +				   desc->type,
> +				   req_id,
> +				   bytes_recvd);
> +				break;
> +			}
> +
> +			break;
> +
> +		case -ENOBUFS:
> +			kfree(buffer);
> +			/* Handle large packet */
> +			bufferlen = bytes_recvd;
> +			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> +
> +			if (buffer == NULL) {

I'd prefer if (!buffer) here, but not a big deal.

> +				return;
> +			}
> +			break;
> +		}
> +	} while (1);

Again, not being familiar with 'hv' infrastructure at all, this inifite 
loop makes me to ask -- in what context does it run? Why do we need it?

It seems to me that it's some kind of infinite loop for event-driven 
programming, which is not something we do in kernel (outside of kernel 
threads, perhaps, with very careful handling).

> +
> +}
> +
> +static int mousevsc_connect_to_vsp(struct hv_device *device)
> +{
> +	int ret = 0;
> +	int t;
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> +	struct mousevsc_prt_msg *request;
> +	struct mousevsc_prt_msg *response;
> +
> +

No need for two spaces here.

> +	request = &input_dev->protocol_req;
> +
> +	memset(request, 0, sizeof(struct mousevsc_prt_msg));
> +
> +	request->type = PIPE_MESSAGE_DATA;
> +	request->size = sizeof(struct synthhid_protocol_request);
> +
> +	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> +	request->request.header.size = sizeof(unsigned int);
> +	request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
> +
> +

Here as well.

> +	ret = vmbus_sendpacket(device->channel, request,
> +				sizeof(struct pipe_prt_msg) -
> +				sizeof(unsigned char) +
> +				sizeof(struct synthhid_protocol_request),
> +				(unsigned long)request,
> +				VM_PKT_DATA_INBAND,
> +				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +	if (ret != 0)

Just if (ret) ... ?

> +		goto cleanup;
> +
> +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> +	if (t == 0) {

if (!t)

> +		ret = -ETIMEDOUT;
> +		goto cleanup;
> +	}
> +
> +	response = &input_dev->protocol_resp;
> +
> +	if (!response->response.approved) {
> +		pr_err("synthhid protocol request failed (version %d)\n",
> +		       SYNTHHID_INPUT_VERSION);
> +		ret = -ENODEV;
> +		goto cleanup;
> +	}
> +
> +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> +	if (t == 0) {
> +		ret = -ETIMEDOUT;
> +		goto cleanup;
> +	}
> +
> +	/*
> +	 * We should have gotten the device attr, hid desc and report
> +	 * desc at this point
> +	 */
> +	if (input_dev->dev_info_status)
> +		ret = -ENOMEM;
> +
> +cleanup:
> +
> +	return ret;
> +}
> +
> +static int mousevsc_hid_open(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static int mousevsc_hid_start(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static void mousevsc_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +static void mousevsc_hid_stop(struct hid_device *hid)
> +{
> +}
> +
> +static struct hid_ll_driver mousevsc_ll_driver = {
> +	.open = mousevsc_hid_open,
> +	.close = mousevsc_hid_close,
> +	.start = mousevsc_hid_start,
> +	.stop = mousevsc_hid_stop,
> +};
> +
> +static struct hid_driver mousevsc_hid_driver;
> +
> +static int mousevsc_probe(struct hv_device *device,
> +			const struct hv_vmbus_device_id *dev_id)
> +{
> +	int ret = 0;
> +	struct mousevsc_dev *input_dev;
> +	struct hid_device *hid_dev;
> +
> +	input_dev = alloc_input_device(device);
> +
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->init_complete = false;
> +
> +	ret = vmbus_open(device->channel,
> +		INPUTVSC_SEND_RING_BUFFER_SIZE,
> +		INPUTVSC_RECV_RING_BUFFER_SIZE,
> +		NULL,
> +		0,
> +		mousevsc_on_channel_callback,
> +		device
> +		);
> +
> +	if (ret != 0) {
> +		free_input_device(input_dev);
> +		return ret;
> +	}
> +
> +
> +	ret = mousevsc_connect_to_vsp(device);
> +
> +	if (ret != 0)
> +		goto probe_err0;
> +
> +	/* workaround SA-167 */
> +	if (input_dev->report_desc[14] == 0x25)
> +		input_dev->report_desc[14] = 0x29;
> +
> +	hid_dev = hid_allocate_device();
> +	if (IS_ERR(hid_dev)) {
> +		ret = PTR_ERR(hid_dev);
> +		goto probe_err0;
> +	}
> +
> +	hid_dev->ll_driver = &mousevsc_ll_driver;
> +	hid_dev->driver = &mousevsc_hid_driver;
> +	hid_dev->bus = BUS_VIRTUAL;
> +	hid_dev->vendor = input_dev->hid_dev_info.vendor;
> +	hid_dev->product = input_dev->hid_dev_info.product;
> +	hid_dev->version = input_dev->hid_dev_info.version;
> +	input_dev->hid_device = hid_dev;
> +
> +	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> +
> +	ret = hid_parse_report(hid_dev, input_dev->report_desc,
> +				input_dev->report_desc_size);
> +
> +	if (ret) {
> +		hid_err(hid_dev, "parse failed\n");
> +		goto probe_err1;
> +	}
> +
> +	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
> +
> +	if (ret) {
> +		hid_err(hid_dev, "hw start failed\n");
> +		goto probe_err1;
> +	}
> +
> +	input_dev->connected = true;
> +	input_dev->init_complete = true;
> +
> +	return ret;
> +
> +probe_err1:
> +	hid_destroy_device(hid_dev);
> +
> +probe_err0:
> +	vmbus_close(device->channel);
> +	free_input_device(input_dev);
> +	return ret;
> +}
> +
> +
> +static int mousevsc_remove(struct hv_device *dev)
> +{
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> +
> +	vmbus_close(dev->channel);
> +
> +	if (input_dev->connected) {
> +		hidinput_disconnect(input_dev->hid_device);
> +		input_dev->connected = false;
> +		hid_destroy_device(input_dev->hid_device);
> +	}
> +
> +	free_input_device(input_dev);
> +
> +	return 0;
> +}
> +
> +static const struct hv_vmbus_device_id id_table[] = {
> +	/* Mouse guid */
> +	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> +		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(vmbus, id_table);
> +
> +static struct  hv_driver mousevsc_drv = {
> +	.name = "mousevsc",
> +	.id_table = id_table,
> +	.probe = mousevsc_probe,
> +	.remove = mousevsc_remove,
> +};
> +
> +static int __init mousevsc_init(void)
> +{
> +	return vmbus_driver_register(&mousevsc_drv);
> +}
> +
> +static void __exit mousevsc_exit(void)
> +{
> +	vmbus_driver_unregister(&mousevsc_drv);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(HV_DRV_VERSION);
> +module_init(mousevsc_init);
> +module_exit(mousevsc_exit);

Thanks again for porting this to HID bus, I am looking forward to having 
this finalized.

-- 
Jiri Kosina
SUSE Labs

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Jiri Kosina [mailto:jkosina@suse.cz]
> Sent: Friday, October 28, 2011 2:47 PM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; Dmitry Torokhov
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Wed, 26 Oct 2011, K. Y. Srinivasan wrote:
> 
> > The hv_mouse.c implements a hid compliant mouse driver for use on a
> > Hyper-V based system. This driver is currently in the staging area and
> > as part of the effort to move this driver out of staging, I had posted
> > the driver code for community review a few weeks ago. This current patch
> > addresses all the review comments I have gotten to date.
> >
> > As per Greg's suggestion, this patch does not get rid of the code from
> > the staging area. Once the mouse driver lands under the hid directory,
> > we will cleanup the staging directory.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> 
> K.Y,,
> 
> first, thanks a lot for your efforts on working on this driver. Porting it
> to HID infrastructure definitely is a huge and proper step.
> 
> > ---
> >  drivers/hid/Kconfig    |    6 +
> >  drivers/hid/Makefile   |    1 +
> >  drivers/hid/hv_mouse.c |  592
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 599 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/hid/hv_mouse.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 1130a89..068dd97 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -613,6 +613,12 @@ config HID_ZYDACRON
> >  	---help---
> >  	Support for Zydacron remote control.
> >
> > +config HYPERV_MOUSE
> > +	tristate "Microsoft Hyper-V mouse driver"
> > +	depends on HYPERV
> > +	---help---
> > +	  Select this option to enable the Hyper-V mouse driver.
> > +
> >  endmenu
> >
> >  endif # HID_SUPPORT
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 0a0a38e..436ac2e 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
> >  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
> >  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
> >  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> > +obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o
> 
> I'd prefer a bit to follow the current naming of the drivers in
> drivers/hid directory ... all the device-specific (vendor-specific)
> drivers currently are called hid-<vendor> or similar.
> 
> We could talk about changing this naming scheme, but before we reach any
> conclusion on this, I'd prefer this to stay for all drivers if possible.

Do you want the driver module to conform to the naming scheme you have?
Greg, in the past has resisted changing driver names, but I have no objection 
to conforming to the naming convention you have.
 
> 
> >  obj-$(CONFIG_USB_HID)		+= usbhid/
> >  obj-$(CONFIG_USB_MOUSE)		+= usbhid/
> > diff --git a/drivers/hid/hv_mouse.c b/drivers/hid/hv_mouse.c
> > new file mode 100644
> > index 0000000..dd42411
> > --- /dev/null
> > +++ b/drivers/hid/hv_mouse.c
> > @@ -0,0 +1,592 @@
> > +/*
> > + *  Copyright (c) 2009, Citrix Systems, Inc.
> > + *  Copyright (c) 2010, Microsoft Corporation.
> > + *  Copyright (c) 2011, Novell Inc.
> > + *
> > + *  This program is free software; you can redistribute it and/or modify it
> > + *  under the terms and conditions of the GNU General Public License,
> > + *  version 2, as published by the Free Software Foundation.
> > + *
> > + *  This program is distributed in the hope it will be useful, but WITHOUT
> > + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or
> > + *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> for
> > + *  more details.
> > + */
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/completion.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/hiddev.h>
> > +#include <linux/hyperv.h>
> > +
> > +
> > +struct hv_input_dev_info {
> > +	unsigned int size;
> > +	unsigned short vendor;
> > +	unsigned short product;
> > +	unsigned short version;
> > +	unsigned short reserved[11];
> > +};
> > +
> > +/* The maximum size of a synthetic input message. */
> > +#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
> > +
> > +/*
> > + * Current version
> > + *
> > + * History:
> > + * Beta, RC < 2008/1/22        1,0
> > + * RC > 2008/1/22              2,0
> > + */
> > +#define SYNTHHID_INPUT_VERSION_MAJOR	2
> > +#define SYNTHHID_INPUT_VERSION_MINOR	0
> > +#define SYNTHHID_INPUT_VERSION
> 	(SYNTHHID_INPUT_VERSION_MINOR | \
> > +					 (SYNTHHID_INPUT_VERSION_MAJOR <<
> 16))
> > +
> > +
> > +#pragma pack(push, 1)
> > +/*
> > + * Message types in the synthetic input protocol
> > + */
> > +enum synthhid_msg_type {
> > +	SYNTH_HID_PROTOCOL_REQUEST,
> > +	SYNTH_HID_PROTOCOL_RESPONSE,
> > +	SYNTH_HID_INITIAL_DEVICE_INFO,
> > +	SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> > +	SYNTH_HID_INPUT_REPORT,
> > +	SYNTH_HID_MAX
> > +};
> > +
> > +/*
> > + * Basic message structures.
> > + */
> > +struct synthhid_msg_hdr {
> > +	enum synthhid_msg_type type;
> > +	u32 size;
> > +};
> > +
> > +struct synthhid_msg {
> > +	struct synthhid_msg_hdr header;
> > +	char data[1]; /* Enclosed message */
> > +};
> > +
> > +union synthhid_version {
> > +	struct {
> > +		u16 minor_version;
> > +		u16 major_version;
> > +	};
> > +	u32 version;
> > +};
> > +
> > +/*
> > + * Protocol messages
> > + */
> > +struct synthhid_protocol_request {
> > +	struct synthhid_msg_hdr header;
> > +	union synthhid_version version_requested;
> > +};
> > +
> > +struct synthhid_protocol_response {
> > +	struct synthhid_msg_hdr header;
> > +	union synthhid_version version_requested;
> > +	unsigned char approved;
> > +};
> > +
> > +struct synthhid_device_info {
> > +	struct synthhid_msg_hdr header;
> > +	struct hv_input_dev_info hid_dev_info;
> > +	struct hid_descriptor hid_descriptor;
> > +};
> > +
> > +struct synthhid_device_info_ack {
> > +	struct synthhid_msg_hdr header;
> > +	unsigned char reserved;
> > +};
> > +
> > +struct synthhid_input_report {
> > +	struct synthhid_msg_hdr header;
> > +	char buffer[1];
> > +};
> > +
> > +#pragma pack(pop)
> > +
> > +#define INPUTVSC_SEND_RING_BUFFER_SIZE		(10*PAGE_SIZE)
> > +#define INPUTVSC_RECV_RING_BUFFER_SIZE		(10*PAGE_SIZE)
> 
> Where does the '10' constant come from? Is it completely arbitrary, or
> does it some real background?

There is nothing magical about this number; this number, I think is used on 
the Windows guests and so we are using it here as well.

> 
> > +
> > +#define NBITS(x) (((x)/BITS_PER_LONG)+1)
> 
> Where is this macro needed? (I wouldn't notice normally, but I have
> wondered why you can't use BIT_WORD and found that I can't seem to find a
> place where this macro would actually be used :) ).

If it is not used, I will get rid of  it.

> 
> > +
> > +enum pipe_prot_msg_type {
> > +	PIPE_MESSAGE_INVALID,
> > +	PIPE_MESSAGE_DATA,
> > +	PIPE_MESSAGE_MAXIMUM
> > +};
> > +
> > +
> > +struct pipe_prt_msg {
> > +	enum pipe_prot_msg_type type;
> > +	u32 size;
> > +	char data[1];
> > +};
> > +
> > +struct  mousevsc_prt_msg {
> > +	enum pipe_prot_msg_type type;
> > +	u32 size;
> > +	union {
> > +		struct synthhid_protocol_request request;
> > +		struct synthhid_protocol_response response;
> > +		struct synthhid_device_info_ack ack;
> > +	};
> > +};
> > +
> > +/*
> > + * Represents an mousevsc device
> > + */
> > +struct mousevsc_dev {
> > +	struct hv_device	*device;
> > +	bool			init_complete;
> > +	struct mousevsc_prt_msg	protocol_req;
> > +	struct mousevsc_prt_msg	protocol_resp;
> > +	/* Synchronize the request/response if needed */
> > +	struct completion	wait_event;
> > +	int			dev_info_status;
> > +
> > +	struct hid_descriptor	*hid_desc;
> > +	unsigned char		*report_desc;
> > +	u32			report_desc_size;
> > +	struct hv_input_dev_info hid_dev_info;
> > +	bool			connected;
> > +	struct hid_device       *hid_device;
> > +};
> > +
> > +
> > +static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
> > +{
> > +	struct mousevsc_dev *input_dev;
> > +
> > +	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> > +
> > +	if (!input_dev)
> > +		return NULL;
> > +
> > +	input_dev->device = device;
> > +	hv_set_drvdata(device, input_dev);
> > +	init_completion(&input_dev->wait_event);
> > +
> > +	return input_dev;
> > +}
> > +
> > +static void free_input_device(struct mousevsc_dev *device)
> > +{
> > +	kfree(device->hid_desc);
> > +	kfree(device->report_desc);
> > +	hv_set_drvdata(device->device, NULL);
> > +	kfree(device);
> > +}
> > +
> > +
> > +static void mousevsc_on_receive_device_info(struct mousevsc_dev
> *input_device,
> > +				struct synthhid_device_info *device_info)
> > +{
> > +	int ret = 0;
> > +	struct hid_descriptor *desc;
> > +	struct mousevsc_prt_msg ack;
> > +
> > +	/* Assume success for now */
> > +	input_device->dev_info_status = 0;
> > +
> > +	memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> > +		sizeof(struct hv_input_dev_info));
> > +
> > +	desc = &device_info->hid_descriptor;
> > +	WARN_ON(desc->bLength == 0);
> > +
> > +	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> > +
> > +	if (!input_device->hid_desc)
> > +		goto cleanup;
> > +
> > +	memcpy(input_device->hid_desc, desc, desc->bLength);
> > +
> > +	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> > +	if (input_device->report_desc_size == 0)
> > +		goto cleanup;
> > +	input_device->report_desc = kzalloc(input_device->report_desc_size,
> > +					  GFP_ATOMIC);
> > +
> > +	if (!input_device->report_desc)
> > +		goto cleanup;
> > +
> > +	memcpy(input_device->report_desc,
> > +	       ((unsigned char *)desc) + desc->bLength,
> > +	       desc->desc[0].wDescriptorLength);
> > +
> > +	/* Send the ack */
> > +	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > +	ack.type = PIPE_MESSAGE_DATA;
> > +	ack.size = sizeof(struct synthhid_device_info_ack);
> > +
> > +	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> > +	ack.ack.header.size = 1;
> > +	ack.ack.reserved = 0;
> > +
> > +	ret = vmbus_sendpacket(input_device->device->channel,
> > +			&ack,
> > +			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> > +			sizeof(struct synthhid_device_info_ack),
> > +			(unsigned long)&ack,
> > +			VM_PKT_DATA_INBAND,
> > +
> 	VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +	if (ret != 0)
> > +		goto cleanup;
> > +
> > +	complete(&input_device->wait_event);
> > +
> > +	return;
> > +
> > +cleanup:
> > +	kfree(input_device->hid_desc);
> > +	input_device->hid_desc = NULL;
> > +
> > +	kfree(input_device->report_desc);
> > +	input_device->report_desc = NULL;
> > +
> > +	input_device->dev_info_status = -1;
> > +	complete(&input_device->wait_event);
> > +}
> > +
> > +static void mousevsc_on_receive(struct hv_device *device,
> > +				struct vmpacket_descriptor *packet)
> > +{
> > +	struct pipe_prt_msg *pipe_msg;
> > +	struct synthhid_msg *hid_msg;
> > +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> > +	struct synthhid_input_report *input_report;
> > +
> > +	pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
> > +						(packet->offset8 << 3));
> > +
> > +	if (pipe_msg->type != PIPE_MESSAGE_DATA)
> > +		return;
> > +
> > +	hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
> > +
> > +	switch (hid_msg->header.type) {
> > +	case SYNTH_HID_PROTOCOL_RESPONSE:
> > +		memcpy(&input_dev->protocol_resp, pipe_msg,
> > +		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
> > +		       sizeof(unsigned char));
> 
> Is there any guarantee anywhere that packet doesn't get corrupted (either
> maliciously or not)?
> 
> Seems to me like we are taking it 'raw' in mousevsc_on_channel_callback()
> without any sanity checking.
> 
> If not, second argument of this memcpy() could easily overflow, causing
> quite some memory corruption, right?
> 
> Actually the question of sanity of the raw packet is much more general one
> throughout this driver. I am not very familiar with things in drivers/hv,
> hence the question.

The guest cannot survive a malicious host; so I think it is safe to say that the 
guest can assume the host is following the protocol.

> 
> > +		complete(&input_dev->wait_event);
> > +		break;
> > +
> > +	case SYNTH_HID_INITIAL_DEVICE_INFO:
> > +		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> > +
> > +		/*
> > +		 * Parse out the device info into device attr,
> > +		 * hid desc and report desc
> > +		 */
> > +		mousevsc_on_receive_device_info(input_dev,
> > +			(struct synthhid_device_info *)&pipe_msg->data[0]);
> > +		break;
> > +	case SYNTH_HID_INPUT_REPORT:
> > +		input_report =
> > +			(struct synthhid_input_report *)&pipe_msg->data[0];
> > +		if (!input_dev->init_complete)
> > +			break;
> > +		hid_input_report(input_dev->hid_device,
> > +				HID_INPUT_REPORT, input_report->buffer,
> > +				input_report->header.size, 1);
> > +		break;
> > +	default:
> > +		pr_err("unsupported hid msg type - type %d len %d",
> > +		       hid_msg->header.type, hid_msg->header.size);
> > +		break;
> > +	}
> > +
> > +}
> > +
> > +static void mousevsc_on_channel_callback(void *context)
> > +{
> > +	const int packet_size = 0x100;
> > +	int ret;
> > +	struct hv_device *device = context;
> > +	u32 bytes_recvd;
> > +	u64 req_id;
> > +	struct vmpacket_descriptor *desc;
> > +	unsigned char	*buffer;
> > +	int	bufferlen = packet_size;
> > +
> > +	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> > +	if (!buffer)
> > +		return;
> > +
> > +	do {
> > +		ret = vmbus_recvpacket_raw(device->channel, buffer,
> > +					bufferlen, &bytes_recvd, &req_id);
> > +
> > +		switch (ret) {
> > +		case 0:
> > +			if (bytes_recvd <= 0) {
> > +				kfree(buffer);
> > +				return;
> > +			}
> > +			desc = (struct vmpacket_descriptor *)buffer;
> > +
> > +			switch (desc->type) {
> > +			case VM_PKT_COMP:
> > +				break;
> > +
> > +			case VM_PKT_DATA_INBAND:
> > +				mousevsc_on_receive(device, desc);
> > +				break;
> > +
> > +			default:
> > +				pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > +				   desc->type,
> > +				   req_id,
> > +				   bytes_recvd);
> > +				break;
> > +			}
> > +
> > +			break;
> > +
> > +		case -ENOBUFS:
> > +			kfree(buffer);
> > +			/* Handle large packet */
> > +			bufferlen = bytes_recvd;
> > +			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> > +
> > +			if (buffer == NULL) {
> 
> I'd prefer if (!buffer) here, but not a big deal.
> 
> > +				return;
> > +			}
> > +			break;
> > +		}
> > +	} while (1);
> 
> Again, not being familiar with 'hv' infrastructure at all, this inifite
> loop makes me to ask -- in what context does it run? Why do we need it?
> 
> It seems to me that it's some kind of infinite loop for event-driven
> programming, which is not something we do in kernel (outside of kernel
> threads, perhaps, with very careful handling).

This loop is invoked in a tasklet context. The agreed upon protocol between the
guest and the host is that the consumer of a ring buffer (in this case the guest) will
process all available data before returning. This permits signaling optimizations between
the producer and the consumer. For instance, if the consumer is still active as seen by the 
producer (based on the read index seen by the producer), as the producer enqueues additional
data, the producer does not have to signal the consumer.
 
> 
> > +
> > +}
> > +
> > +static int mousevsc_connect_to_vsp(struct hv_device *device)
> > +{
> > +	int ret = 0;
> > +	int t;
> > +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> > +	struct mousevsc_prt_msg *request;
> > +	struct mousevsc_prt_msg *response;
> > +
> > +
> 
> No need for two spaces here.
> 
> > +	request = &input_dev->protocol_req;
> > +
> > +	memset(request, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > +	request->type = PIPE_MESSAGE_DATA;
> > +	request->size = sizeof(struct synthhid_protocol_request);
> > +
> > +	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> > +	request->request.header.size = sizeof(unsigned int);
> > +	request->request.version_requested.version =
> SYNTHHID_INPUT_VERSION;
> > +
> > +
> 
> Here as well.
> 
> > +	ret = vmbus_sendpacket(device->channel, request,
> > +				sizeof(struct pipe_prt_msg) -
> > +				sizeof(unsigned char) +
> > +				sizeof(struct synthhid_protocol_request),
> > +				(unsigned long)request,
> > +				VM_PKT_DATA_INBAND,
> > +
> 	VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +	if (ret != 0)
> 
> Just if (ret) ... ?
> 
> > +		goto cleanup;
> > +
> > +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > +	if (t == 0) {
> 
> if (!t)
> 
> > +		ret = -ETIMEDOUT;
> > +		goto cleanup;
> > +	}
> > +
> > +	response = &input_dev->protocol_resp;
> > +
> > +	if (!response->response.approved) {
> > +		pr_err("synthhid protocol request failed (version %d)\n",
> > +		       SYNTHHID_INPUT_VERSION);
> > +		ret = -ENODEV;
> > +		goto cleanup;
> > +	}
> > +
> > +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > +	if (t == 0) {
> > +		ret = -ETIMEDOUT;
> > +		goto cleanup;
> > +	}
> > +
> > +	/*
> > +	 * We should have gotten the device attr, hid desc and report
> > +	 * desc at this point
> > +	 */
> > +	if (input_dev->dev_info_status)
> > +		ret = -ENOMEM;
> > +
> > +cleanup:
> > +
> > +	return ret;
> > +}
> > +
> > +static int mousevsc_hid_open(struct hid_device *hid)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int mousevsc_hid_start(struct hid_device *hid)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void mousevsc_hid_close(struct hid_device *hid)
> > +{
> > +}
> > +
> > +static void mousevsc_hid_stop(struct hid_device *hid)
> > +{
> > +}
> > +
> > +static struct hid_ll_driver mousevsc_ll_driver = {
> > +	.open = mousevsc_hid_open,
> > +	.close = mousevsc_hid_close,
> > +	.start = mousevsc_hid_start,
> > +	.stop = mousevsc_hid_stop,
> > +};
> > +
> > +static struct hid_driver mousevsc_hid_driver;
> > +
> > +static int mousevsc_probe(struct hv_device *device,
> > +			const struct hv_vmbus_device_id *dev_id)
> > +{
> > +	int ret = 0;
> > +	struct mousevsc_dev *input_dev;
> > +	struct hid_device *hid_dev;
> > +
> > +	input_dev = alloc_input_device(device);
> > +
> > +	if (!input_dev)
> > +		return -ENOMEM;
> > +
> > +	input_dev->init_complete = false;
> > +
> > +	ret = vmbus_open(device->channel,
> > +		INPUTVSC_SEND_RING_BUFFER_SIZE,
> > +		INPUTVSC_RECV_RING_BUFFER_SIZE,
> > +		NULL,
> > +		0,
> > +		mousevsc_on_channel_callback,
> > +		device
> > +		);
> > +
> > +	if (ret != 0) {
> > +		free_input_device(input_dev);
> > +		return ret;
> > +	}
> > +
> > +
> > +	ret = mousevsc_connect_to_vsp(device);
> > +
> > +	if (ret != 0)
> > +		goto probe_err0;
> > +
> > +	/* workaround SA-167 */
> > +	if (input_dev->report_desc[14] == 0x25)
> > +		input_dev->report_desc[14] = 0x29;
> > +
> > +	hid_dev = hid_allocate_device();
> > +	if (IS_ERR(hid_dev)) {
> > +		ret = PTR_ERR(hid_dev);
> > +		goto probe_err0;
> > +	}
> > +
> > +	hid_dev->ll_driver = &mousevsc_ll_driver;
> > +	hid_dev->driver = &mousevsc_hid_driver;
> > +	hid_dev->bus = BUS_VIRTUAL;
> > +	hid_dev->vendor = input_dev->hid_dev_info.vendor;
> > +	hid_dev->product = input_dev->hid_dev_info.product;
> > +	hid_dev->version = input_dev->hid_dev_info.version;
> > +	input_dev->hid_device = hid_dev;
> > +
> > +	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> > +
> > +	ret = hid_parse_report(hid_dev, input_dev->report_desc,
> > +				input_dev->report_desc_size);
> > +
> > +	if (ret) {
> > +		hid_err(hid_dev, "parse failed\n");
> > +		goto probe_err1;
> > +	}
> > +
> > +	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT |
> HID_CONNECT_HIDDEV);
> > +
> > +	if (ret) {
> > +		hid_err(hid_dev, "hw start failed\n");
> > +		goto probe_err1;
> > +	}
> > +
> > +	input_dev->connected = true;
> > +	input_dev->init_complete = true;
> > +
> > +	return ret;
> > +
> > +probe_err1:
> > +	hid_destroy_device(hid_dev);
> > +
> > +probe_err0:
> > +	vmbus_close(device->channel);
> > +	free_input_device(input_dev);
> > +	return ret;
> > +}
> > +
> > +
> > +static int mousevsc_remove(struct hv_device *dev)
> > +{
> > +	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> > +
> > +	vmbus_close(dev->channel);
> > +
> > +	if (input_dev->connected) {
> > +		hidinput_disconnect(input_dev->hid_device);
> > +		input_dev->connected = false;
> > +		hid_destroy_device(input_dev->hid_device);
> > +	}
> > +
> > +	free_input_device(input_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct hv_vmbus_device_id id_table[] = {
> > +	/* Mouse guid */
> > +	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> > +		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(vmbus, id_table);
> > +
> > +static struct  hv_driver mousevsc_drv = {
> > +	.name = "mousevsc",
> > +	.id_table = id_table,
> > +	.probe = mousevsc_probe,
> > +	.remove = mousevsc_remove,
> > +};
> > +
> > +static int __init mousevsc_init(void)
> > +{
> > +	return vmbus_driver_register(&mousevsc_drv);
> > +}
> > +
> > +static void __exit mousevsc_exit(void)
> > +{
> > +	vmbus_driver_unregister(&mousevsc_drv);
> > +}
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION(HV_DRV_VERSION);
> > +module_init(mousevsc_init);
> > +module_exit(mousevsc_exit);
> 
> Thanks again for porting this to HID bus, I am looking forward to having
> this finalized.

Thanks Jiri for taking the time to review. I will address all the issues you have 
raised and re-spin the patch. If it is ok with you, I will not change the driver name 
in this go around as I want to hear from Greg and Olaf before I do that. Would this
prevent you from taking this driver out of staging. I could make the name change after
the driver has exited the staging area.

Regards,

K. Y

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Greg KH]

On Fri, Oct 28, 2011 at 07:50:59PM +0000, KY Srinivasan wrote:
> > >  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
> > >  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
> > >  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> > > +obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o
> > 
> > I'd prefer a bit to follow the current naming of the drivers in
> > drivers/hid directory ... all the device-specific (vendor-specific)
> > drivers currently are called hid-<vendor> or similar.
> > 
> > We could talk about changing this naming scheme, but before we reach any
> > conclusion on this, I'd prefer this to stay for all drivers if possible.
> 
> Do you want the driver module to conform to the naming scheme you have?
> Greg, in the past has resisted changing driver names, but I have no objection 
> to conforming to the naming convention you have.

I recommend following the proper naming scheme.  As this driver is
auto-loaded when the virtual hardware is seen, the name really doesn't
matter at all.

So, how about 'hid-hyperv' or 'hid-hv'?

> > > +	switch (hid_msg->header.type) {
> > > +	case SYNTH_HID_PROTOCOL_RESPONSE:
> > > +		memcpy(&input_dev->protocol_resp, pipe_msg,
> > > +		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
> > > +		       sizeof(unsigned char));
> > 
> > Is there any guarantee anywhere that packet doesn't get corrupted (either
> > maliciously or not)?
> > 
> > Seems to me like we are taking it 'raw' in mousevsc_on_channel_callback()
> > without any sanity checking.
> > 
> > If not, second argument of this memcpy() could easily overflow, causing
> > quite some memory corruption, right?
> > 
> > Actually the question of sanity of the raw packet is much more general one
> > throughout this driver. I am not very familiar with things in drivers/hv,
> > hence the question.
> 
> The guest cannot survive a malicious host; so I think it is safe to say that the 
> guest can assume the host is following the protocol.

That's not good for a very large number of reasons, not the least being
that we have no idea how secure the hyperv hypervisor is, so making it
so that there isn't an obvious hole into linux through it, would be a
good idea.

And yes, I'd say the same thing if this was a KVM or Xen driver as well.
Please be very defensive in this area of the code, especially as there
are no performance issues here.

> > > +				return;
> > > +			}
> > > +			break;
> > > +		}
> > > +	} while (1);
> > 
> > Again, not being familiar with 'hv' infrastructure at all, this inifite
> > loop makes me to ask -- in what context does it run? Why do we need it?
> > 
> > It seems to me that it's some kind of infinite loop for event-driven
> > programming, which is not something we do in kernel (outside of kernel
> > threads, perhaps, with very careful handling).
> 
> This loop is invoked in a tasklet context. The agreed upon protocol between the
> guest and the host is that the consumer of a ring buffer (in this case the guest) will
> process all available data before returning. This permits signaling optimizations between
> the producer and the consumer. For instance, if the consumer is still active as seen by the 
> producer (based on the read index seen by the producer), as the producer enqueues additional
> data, the producer does not have to signal the consumer.

That's great, but again, be defensive and at least have a "time out"
way to get out of this loop in case something goes wrong.  It will be
triggered some day, by someone, I guarantee it.

thanks,

greg k-h

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Friday, October 28, 2011 4:03 PM
> To: KY Srinivasan
> Cc: Jiri Kosina; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; Dmitry Torokhov
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Fri, Oct 28, 2011 at 07:50:59PM +0000, KY Srinivasan wrote:
> > > >  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
> > > >  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
> > > >  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> > > > +obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o
> > >
> > > I'd prefer a bit to follow the current naming of the drivers in
> > > drivers/hid directory ... all the device-specific (vendor-specific)
> > > drivers currently are called hid-<vendor> or similar.
> > >
> > > We could talk about changing this naming scheme, but before we reach any
> > > conclusion on this, I'd prefer this to stay for all drivers if possible.
> >
> > Do you want the driver module to conform to the naming scheme you have?
> > Greg, in the past has resisted changing driver names, but I have no objection
> > to conforming to the naming convention you have.
> 
> I recommend following the proper naming scheme.  As this driver is
> auto-loaded when the virtual hardware is seen, the name really doesn't
> matter at all.
> 
> So, how about 'hid-hyperv' or 'hid-hv'?

I will go with hid-hv  if there are no objections.
> 
> > > > +	switch (hid_msg->header.type) {
> > > > +	case SYNTH_HID_PROTOCOL_RESPONSE:
> > > > +		memcpy(&input_dev->protocol_resp, pipe_msg,
> > > > +		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
> > > > +		       sizeof(unsigned char));
> > >
> > > Is there any guarantee anywhere that packet doesn't get corrupted (either
> > > maliciously or not)?
> > >
> > > Seems to me like we are taking it 'raw' in mousevsc_on_channel_callback()
> > > without any sanity checking.
> > >
> > > If not, second argument of this memcpy() could easily overflow, causing
> > > quite some memory corruption, right?
> > >
> > > Actually the question of sanity of the raw packet is much more general one
> > > throughout this driver. I am not very familiar with things in drivers/hv,
> > > hence the question.
> >
> > The guest cannot survive a malicious host; so I think it is safe to say that the
> > guest can assume the host is following the protocol.
> 
> That's not good for a very large number of reasons, not the least being
> that we have no idea how secure the hyperv hypervisor is, so making it
> so that there isn't an obvious hole into linux through it, would be a
> good idea.
> 
> And yes, I'd say the same thing if this was a KVM or Xen driver as well.
> Please be very defensive in this area of the code, especially as there
> are no performance issues here.

In the chain of trust, the hypervisor and the host are the foundations
as far as the guest is concerned, since both the hypervisor and the host
can affect the guest in ways that the guest has no obvious way to protect itself.
If the hypervisor/host have security holes, there is not much you can do in the guest
to deal with it. 
In this case, I can add checks but I am not sure how useful it is.

> 
> > > > +				return;
> > > > +			}
> > > > +			break;
> > > > +		}
> > > > +	} while (1);
> > >
> > > Again, not being familiar with 'hv' infrastructure at all, this inifite
> > > loop makes me to ask -- in what context does it run? Why do we need it?
> > >
> > > It seems to me that it's some kind of infinite loop for event-driven
> > > programming, which is not something we do in kernel (outside of kernel
> > > threads, perhaps, with very careful handling).
> >
> > This loop is invoked in a tasklet context. The agreed upon protocol between the
> > guest and the host is that the consumer of a ring buffer (in this case the guest)
> will
> > process all available data before returning. This permits signaling optimizations
> between
> > the producer and the consumer. For instance, if the consumer is still active as
> seen by the
> > producer (based on the read index seen by the producer), as the producer
> enqueues additional
> > data, the producer does not have to signal the consumer.
> 
> That's great, but again, be defensive and at least have a "time out"
> way to get out of this loop in case something goes wrong.  It will be
> triggered some day, by someone, I guarantee it.

I am not sure what you are recommending here Greg; the host mandates that
we exit this loop only when there is nothing more to be read; that exit path is in the
current code. If we exit this code for any other reason (such as based on a timeout)
we will not be awakened by the host when in fact there is data to be read.

Regards,

K. Y 

[PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[K. Y. Srinivasan]

The hv_mouse.c implements a hid compliant mouse driver for use on a
Hyper-V based system. This driver is currently in the staging area and
as part of the effort to move this driver out of staging, I had posted
the driver code for community review a few weeks ago. This current patch
addresses all the review comments I have gotten to date. All the relevant
patches have already been submitted to the staging tree as well.

As per Greg's suggestion, this patch does not get rid of the code from
the staging area. Once the mouse driver lands under the hid directory,
we will cleanup the staging directory.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hid/Kconfig      |    6 +
 drivers/hid/Makefile     |    1 +
 drivers/hid/hid-hyperv.c |  600 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 607 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hid-hyperv.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 1130a89..068dd97 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -613,6 +613,12 @@ config HID_ZYDACRON
 	---help---
 	Support for Zydacron remote control.
 
+config HYPERV_MOUSE
+	tristate "Microsoft Hyper-V mouse driver"
+	depends on HYPERV
+	---help---
+	  Select this option to enable the Hyper-V mouse driver.
+
 endmenu
 
 endif # HID_SUPPORT
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0a0a38e..e683b8c 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
 obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
 obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
 obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
+obj-$(CONFIG_HYPERV_MOUSE)	+= hid-hyperv.o
 
 obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
new file mode 100644
index 0000000..2c2e1b4
--- /dev/null
+++ b/drivers/hid/hid-hyperv.c
@@ -0,0 +1,600 @@
+/*
+ *  Copyright (c) 2009, Citrix Systems, Inc.
+ *  Copyright (c) 2010, Microsoft Corporation.
+ *  Copyright (c) 2011, Novell Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms and conditions of the GNU General Public License,
+ *  version 2, as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ *  more details.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/completion.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/hiddev.h>
+#include <linux/hyperv.h>
+
+
+struct hv_input_dev_info {
+	unsigned int size;
+	unsigned short vendor;
+	unsigned short product;
+	unsigned short version;
+	unsigned short reserved[11];
+};
+
+/* The maximum size of a synthetic input message. */
+#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
+
+/*
+ * Current version
+ *
+ * History:
+ * Beta, RC < 2008/1/22        1,0
+ * RC > 2008/1/22              2,0
+ */
+#define SYNTHHID_INPUT_VERSION_MAJOR	2
+#define SYNTHHID_INPUT_VERSION_MINOR	0
+#define SYNTHHID_INPUT_VERSION		(SYNTHHID_INPUT_VERSION_MINOR | \
+					 (SYNTHHID_INPUT_VERSION_MAJOR << 16))
+
+
+#pragma pack(push, 1)
+/*
+ * Message types in the synthetic input protocol
+ */
+enum synthhid_msg_type {
+	SYNTH_HID_PROTOCOL_REQUEST,
+	SYNTH_HID_PROTOCOL_RESPONSE,
+	SYNTH_HID_INITIAL_DEVICE_INFO,
+	SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
+	SYNTH_HID_INPUT_REPORT,
+	SYNTH_HID_MAX
+};
+
+/*
+ * Basic message structures.
+ */
+struct synthhid_msg_hdr {
+	enum synthhid_msg_type type;
+	u32 size;
+};
+
+struct synthhid_msg {
+	struct synthhid_msg_hdr header;
+	char data[1]; /* Enclosed message */
+};
+
+union synthhid_version {
+	struct {
+		u16 minor_version;
+		u16 major_version;
+	};
+	u32 version;
+};
+
+/*
+ * Protocol messages
+ */
+struct synthhid_protocol_request {
+	struct synthhid_msg_hdr header;
+	union synthhid_version version_requested;
+};
+
+struct synthhid_protocol_response {
+	struct synthhid_msg_hdr header;
+	union synthhid_version version_requested;
+	unsigned char approved;
+};
+
+struct synthhid_device_info {
+	struct synthhid_msg_hdr header;
+	struct hv_input_dev_info hid_dev_info;
+	struct hid_descriptor hid_descriptor;
+};
+
+struct synthhid_device_info_ack {
+	struct synthhid_msg_hdr header;
+	unsigned char reserved;
+};
+
+struct synthhid_input_report {
+	struct synthhid_msg_hdr header;
+	char buffer[1];
+};
+
+#pragma pack(pop)
+
+#define INPUTVSC_SEND_RING_BUFFER_SIZE		(10*PAGE_SIZE)
+#define INPUTVSC_RECV_RING_BUFFER_SIZE		(10*PAGE_SIZE)
+
+
+enum pipe_prot_msg_type {
+	PIPE_MESSAGE_INVALID,
+	PIPE_MESSAGE_DATA,
+	PIPE_MESSAGE_MAXIMUM
+};
+
+
+struct pipe_prt_msg {
+	enum pipe_prot_msg_type type;
+	u32 size;
+	char data[1];
+};
+
+struct  mousevsc_prt_msg {
+	enum pipe_prot_msg_type type;
+	u32 size;
+	union {
+		struct synthhid_protocol_request request;
+		struct synthhid_protocol_response response;
+		struct synthhid_device_info_ack ack;
+	};
+};
+
+/*
+ * Represents an mousevsc device
+ */
+struct mousevsc_dev {
+	struct hv_device	*device;
+	bool			init_complete;
+	bool			connected;
+	struct mousevsc_prt_msg	protocol_req;
+	struct mousevsc_prt_msg	protocol_resp;
+	/* Synchronize the request/response if needed */
+	struct completion	wait_event;
+	int			dev_info_status;
+
+	struct hid_descriptor	*hid_desc;
+	unsigned char		*report_desc;
+	u32			report_desc_size;
+	struct hv_input_dev_info hid_dev_info;
+	struct hid_device       *hid_device;
+};
+
+
+static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
+{
+	struct mousevsc_dev *input_dev;
+
+	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
+
+	if (!input_dev)
+		return NULL;
+
+	input_dev->device = device;
+	hv_set_drvdata(device, input_dev);
+	init_completion(&input_dev->wait_event);
+
+	return input_dev;
+}
+
+static void free_input_device(struct mousevsc_dev *device)
+{
+	kfree(device->hid_desc);
+	kfree(device->report_desc);
+	hv_set_drvdata(device->device, NULL);
+	kfree(device);
+}
+
+
+static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
+				struct synthhid_device_info *device_info)
+{
+	int ret = 0;
+	struct hid_descriptor *desc;
+	struct mousevsc_prt_msg ack;
+
+	/* Assume success for now */
+	input_device->dev_info_status = 0;
+
+	memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
+		sizeof(struct hv_input_dev_info));
+
+	desc = &device_info->hid_descriptor;
+	WARN_ON(desc->bLength == 0);
+
+	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
+
+	if (!input_device->hid_desc)
+		goto cleanup;
+
+	memcpy(input_device->hid_desc, desc, desc->bLength);
+
+	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
+	if (input_device->report_desc_size == 0)
+		goto cleanup;
+	input_device->report_desc = kzalloc(input_device->report_desc_size,
+					  GFP_ATOMIC);
+
+	if (!input_device->report_desc)
+		goto cleanup;
+
+	memcpy(input_device->report_desc,
+	       ((unsigned char *)desc) + desc->bLength,
+	       desc->desc[0].wDescriptorLength);
+
+	/* Send the ack */
+	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
+
+	ack.type = PIPE_MESSAGE_DATA;
+	ack.size = sizeof(struct synthhid_device_info_ack);
+
+	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
+	ack.ack.header.size = 1;
+	ack.ack.reserved = 0;
+
+	ret = vmbus_sendpacket(input_device->device->channel,
+			&ack,
+			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
+			sizeof(struct synthhid_device_info_ack),
+			(unsigned long)&ack,
+			VM_PKT_DATA_INBAND,
+			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (ret != 0)
+		goto cleanup;
+
+	complete(&input_device->wait_event);
+
+	return;
+
+cleanup:
+	kfree(input_device->hid_desc);
+	input_device->hid_desc = NULL;
+
+	kfree(input_device->report_desc);
+	input_device->report_desc = NULL;
+
+	input_device->dev_info_status = -1;
+	complete(&input_device->wait_event);
+}
+
+static void mousevsc_on_receive(struct hv_device *device,
+				struct vmpacket_descriptor *packet)
+{
+	struct pipe_prt_msg *pipe_msg;
+	struct synthhid_msg *hid_msg;
+	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+	struct synthhid_input_report *input_report;
+
+	pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
+						(packet->offset8 << 3));
+
+	if (pipe_msg->type != PIPE_MESSAGE_DATA)
+		return;
+
+	hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
+
+	switch (hid_msg->header.type) {
+	case SYNTH_HID_PROTOCOL_RESPONSE:
+		/*
+		 * While it will be impossible for us to protect against
+		 * malicious/buggy hypervisor/host, add a check here to
+		 * ensure we don't corrupt memory.
+		 */
+		if ((pipe_msg->size + sizeof(struct pipe_prt_msg)
+			- sizeof(unsigned char))
+			> sizeof(struct mousevsc_prt_msg)) {
+			WARN_ON(1);
+			break;
+		}
+
+		memcpy(&input_dev->protocol_resp, pipe_msg,
+		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
+		       sizeof(unsigned char));
+		complete(&input_dev->wait_event);
+		break;
+
+	case SYNTH_HID_INITIAL_DEVICE_INFO:
+		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
+
+		/*
+		 * Parse out the device info into device attr,
+		 * hid desc and report desc
+		 */
+		mousevsc_on_receive_device_info(input_dev,
+			(struct synthhid_device_info *)&pipe_msg->data[0]);
+		break;
+	case SYNTH_HID_INPUT_REPORT:
+		input_report =
+			(struct synthhid_input_report *)&pipe_msg->data[0];
+		if (!input_dev->init_complete)
+			break;
+		hid_input_report(input_dev->hid_device,
+				HID_INPUT_REPORT, input_report->buffer,
+				input_report->header.size, 1);
+		break;
+	default:
+		pr_err("unsupported hid msg type - type %d len %d",
+		       hid_msg->header.type, hid_msg->header.size);
+		break;
+	}
+
+}
+
+static void mousevsc_on_channel_callback(void *context)
+{
+	const int packet_size = 0x100;
+	int ret;
+	struct hv_device *device = context;
+	u32 bytes_recvd;
+	u64 req_id;
+	struct vmpacket_descriptor *desc;
+	unsigned char	*buffer;
+	int	bufferlen = packet_size;
+
+	buffer = kmalloc(bufferlen, GFP_ATOMIC);
+	if (!buffer)
+		return;
+
+	do {
+		ret = vmbus_recvpacket_raw(device->channel, buffer,
+					bufferlen, &bytes_recvd, &req_id);
+
+		switch (ret) {
+		case 0:
+			if (bytes_recvd <= 0) {
+				kfree(buffer);
+				return;
+			}
+			desc = (struct vmpacket_descriptor *)buffer;
+
+			switch (desc->type) {
+			case VM_PKT_COMP:
+				break;
+
+			case VM_PKT_DATA_INBAND:
+				mousevsc_on_receive(device, desc);
+				break;
+
+			default:
+				pr_err("unhandled packet type %d, tid %llx len %d\n",
+				   desc->type,
+				   req_id,
+				   bytes_recvd);
+				break;
+			}
+
+			break;
+
+		case -ENOBUFS:
+			kfree(buffer);
+			/* Handle large packet */
+			bufferlen = bytes_recvd;
+			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+
+			if (!buffer)
+				return;
+
+			break;
+		}
+	} while (1);
+
+}
+
+static int mousevsc_connect_to_vsp(struct hv_device *device)
+{
+	int ret = 0;
+	int t;
+	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+	struct mousevsc_prt_msg *request;
+	struct mousevsc_prt_msg *response;
+
+	request = &input_dev->protocol_req;
+
+	memset(request, 0, sizeof(struct mousevsc_prt_msg));
+
+	request->type = PIPE_MESSAGE_DATA;
+	request->size = sizeof(struct synthhid_protocol_request);
+
+	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
+	request->request.header.size = sizeof(unsigned int);
+	request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
+
+	ret = vmbus_sendpacket(device->channel, request,
+				sizeof(struct pipe_prt_msg) -
+				sizeof(unsigned char) +
+				sizeof(struct synthhid_protocol_request),
+				(unsigned long)request,
+				VM_PKT_DATA_INBAND,
+				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (ret)
+		goto cleanup;
+
+	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+	if (!t) {
+		ret = -ETIMEDOUT;
+		goto cleanup;
+	}
+
+	response = &input_dev->protocol_resp;
+
+	if (!response->response.approved) {
+		pr_err("synthhid protocol request failed (version %d)\n",
+		       SYNTHHID_INPUT_VERSION);
+		ret = -ENODEV;
+		goto cleanup;
+	}
+
+	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+	if (!t) {
+		ret = -ETIMEDOUT;
+		goto cleanup;
+	}
+
+	/*
+	 * We should have gotten the device attr, hid desc and report
+	 * desc at this point
+	 */
+	if (input_dev->dev_info_status)
+		ret = -ENOMEM;
+
+cleanup:
+
+	return ret;
+}
+
+static int mousevsc_hid_open(struct hid_device *hid)
+{
+	return 0;
+}
+
+static int mousevsc_hid_start(struct hid_device *hid)
+{
+	return 0;
+}
+
+static void mousevsc_hid_close(struct hid_device *hid)
+{
+}
+
+static void mousevsc_hid_stop(struct hid_device *hid)
+{
+}
+
+static struct hid_ll_driver mousevsc_ll_driver = {
+	.open = mousevsc_hid_open,
+	.close = mousevsc_hid_close,
+	.start = mousevsc_hid_start,
+	.stop = mousevsc_hid_stop,
+};
+
+static struct hid_driver mousevsc_hid_driver;
+
+static int mousevsc_probe(struct hv_device *device,
+			const struct hv_vmbus_device_id *dev_id)
+{
+	int ret = 0;
+	struct mousevsc_dev *input_dev;
+	struct hid_device *hid_dev;
+
+	input_dev = alloc_input_device(device);
+
+	if (!input_dev)
+		return -ENOMEM;
+
+	input_dev->init_complete = false;
+
+	ret = vmbus_open(device->channel,
+		INPUTVSC_SEND_RING_BUFFER_SIZE,
+		INPUTVSC_RECV_RING_BUFFER_SIZE,
+		NULL,
+		0,
+		mousevsc_on_channel_callback,
+		device
+		);
+
+	if (ret != 0) {
+		free_input_device(input_dev);
+		return ret;
+	}
+
+	ret = mousevsc_connect_to_vsp(device);
+
+	if (ret != 0)
+		goto probe_err0;
+
+	/* workaround SA-167 */
+	if (input_dev->report_desc[14] == 0x25)
+		input_dev->report_desc[14] = 0x29;
+
+	hid_dev = hid_allocate_device();
+	if (IS_ERR(hid_dev)) {
+		ret = PTR_ERR(hid_dev);
+		goto probe_err0;
+	}
+
+	hid_dev->ll_driver = &mousevsc_ll_driver;
+	hid_dev->driver = &mousevsc_hid_driver;
+	hid_dev->bus = BUS_VIRTUAL;
+	hid_dev->vendor = input_dev->hid_dev_info.vendor;
+	hid_dev->product = input_dev->hid_dev_info.product;
+	hid_dev->version = input_dev->hid_dev_info.version;
+	input_dev->hid_device = hid_dev;
+
+	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
+
+	ret = hid_parse_report(hid_dev, input_dev->report_desc,
+				input_dev->report_desc_size);
+
+	if (ret) {
+		hid_err(hid_dev, "parse failed\n");
+		goto probe_err1;
+	}
+
+	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
+
+	if (ret) {
+		hid_err(hid_dev, "hw start failed\n");
+		goto probe_err1;
+	}
+
+	input_dev->connected = true;
+	input_dev->init_complete = true;
+
+	return ret;
+
+probe_err1:
+	hid_destroy_device(hid_dev);
+
+probe_err0:
+	vmbus_close(device->channel);
+	free_input_device(input_dev);
+	return ret;
+}
+
+
+static int mousevsc_remove(struct hv_device *dev)
+{
+	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
+
+	vmbus_close(dev->channel);
+
+	if (input_dev->connected) {
+		hidinput_disconnect(input_dev->hid_device);
+		input_dev->connected = false;
+		hid_destroy_device(input_dev->hid_device);
+	}
+
+	free_input_device(input_dev);
+
+	return 0;
+}
+
+static const struct hv_vmbus_device_id id_table[] = {
+	/* Mouse guid */
+	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
+		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct  hv_driver mousevsc_drv = {
+	.name = KBUILD_MODNAME,
+	.id_table = id_table,
+	.probe = mousevsc_probe,
+	.remove = mousevsc_remove,
+};
+
+static int __init mousevsc_init(void)
+{
+	return vmbus_driver_register(&mousevsc_drv);
+}
+
+static void __exit mousevsc_exit(void)
+{
+	vmbus_driver_unregister(&mousevsc_drv);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_VERSION(HV_DRV_VERSION);
+module_init(mousevsc_init);
+module_exit(mousevsc_exit);
-- 
1.7.4.1

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Jesper Juhl]

On Fri, 28 Oct 2011, K. Y. Srinivasan wrote:

> The hv_mouse.c implements a hid compliant mouse driver for use on a
> Hyper-V based system. This driver is currently in the staging area and
> as part of the effort to move this driver out of staging, I had posted
> the driver code for community review a few weeks ago. This current patch
> addresses all the review comments I have gotten to date. All the relevant
> patches have already been submitted to the staging tree as well.
> 
> As per Greg's suggestion, this patch does not get rid of the code from
> the staging area. Once the mouse driver lands under the hid directory,
> we will cleanup the staging directory.
> 

I have a few (trivial) comments below...


> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hid/Kconfig      |    6 +
>  drivers/hid/Makefile     |    1 +
>  drivers/hid/hid-hyperv.c |  600 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 607 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hid/hid-hyperv.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 1130a89..068dd97 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -613,6 +613,12 @@ config HID_ZYDACRON
>  	---help---
>  	Support for Zydacron remote control.
>  
> +config HYPERV_MOUSE
> +	tristate "Microsoft Hyper-V mouse driver"
> +	depends on HYPERV
> +	---help---
> +	  Select this option to enable the Hyper-V mouse driver.
> +

I believe that just plain "help" rather than "--help--" is more common 
these days, but I admit that I'm not certain.

>  endmenu
>  
>  endif # HID_SUPPORT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 0a0a38e..e683b8c 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
>  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
>  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
>  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> +obj-$(CONFIG_HYPERV_MOUSE)	+= hid-hyperv.o
>  
>  obj-$(CONFIG_USB_HID)		+= usbhid/
>  obj-$(CONFIG_USB_MOUSE)		+= usbhid/
> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
> new file mode 100644
> index 0000000..2c2e1b4
> --- /dev/null
> +++ b/drivers/hid/hid-hyperv.c
> @@ -0,0 +1,600 @@
> +/*
> + *  Copyright (c) 2009, Citrix Systems, Inc.
> + *  Copyright (c) 2010, Microsoft Corporation.
> + *  Copyright (c) 2011, Novell Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms and conditions of the GNU General Public License,
> + *  version 2, as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope it will be useful, but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + *  more details.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/completion.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/hiddev.h>
> +#include <linux/hyperv.h>
> +
> +
> +struct hv_input_dev_info {
> +	unsigned int size;
> +	unsigned short vendor;
> +	unsigned short product;
> +	unsigned short version;
> +	unsigned short reserved[11];
> +};
> +
> +/* The maximum size of a synthetic input message. */
> +#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
> +
> +/*
> + * Current version
> + *
> + * History:
> + * Beta, RC < 2008/1/22        1,0
> + * RC > 2008/1/22              2,0
> + */

Is this comment really relevant? Shouldn't it just go away?


> +#define SYNTHHID_INPUT_VERSION_MAJOR	2
> +#define SYNTHHID_INPUT_VERSION_MINOR	0
> +#define SYNTHHID_INPUT_VERSION		(SYNTHHID_INPUT_VERSION_MINOR | \
> +					 (SYNTHHID_INPUT_VERSION_MAJOR << 16))
> +
> +
> +#pragma pack(push, 1)
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synthhid_msg_type {
> +	SYNTH_HID_PROTOCOL_REQUEST,
> +	SYNTH_HID_PROTOCOL_RESPONSE,
> +	SYNTH_HID_INITIAL_DEVICE_INFO,
> +	SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> +	SYNTH_HID_INPUT_REPORT,
> +	SYNTH_HID_MAX
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synthhid_msg_hdr {
> +	enum synthhid_msg_type type;
> +	u32 size;
> +};
> +
> +struct synthhid_msg {
> +	struct synthhid_msg_hdr header;
> +	char data[1]; /* Enclosed message */
> +};
> +
> +union synthhid_version {
> +	struct {
> +		u16 minor_version;
> +		u16 major_version;
> +	};
> +	u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synthhid_protocol_request {
> +	struct synthhid_msg_hdr header;
> +	union synthhid_version version_requested;
> +};
> +
> +struct synthhid_protocol_response {
> +	struct synthhid_msg_hdr header;
> +	union synthhid_version version_requested;
> +	unsigned char approved;
> +};
> +
> +struct synthhid_device_info {
> +	struct synthhid_msg_hdr header;
> +	struct hv_input_dev_info hid_dev_info;
> +	struct hid_descriptor hid_descriptor;
> +};
> +
> +struct synthhid_device_info_ack {
> +	struct synthhid_msg_hdr header;
> +	unsigned char reserved;
> +};
> +
> +struct synthhid_input_report {
> +	struct synthhid_msg_hdr header;
> +	char buffer[1];
> +};
> +
> +#pragma pack(pop)
> +
> +#define INPUTVSC_SEND_RING_BUFFER_SIZE		(10*PAGE_SIZE)
> +#define INPUTVSC_RECV_RING_BUFFER_SIZE		(10*PAGE_SIZE)
> +
> +
> +enum pipe_prot_msg_type {
> +	PIPE_MESSAGE_INVALID,
> +	PIPE_MESSAGE_DATA,
> +	PIPE_MESSAGE_MAXIMUM
> +};
> +
> +
> +struct pipe_prt_msg {
> +	enum pipe_prot_msg_type type;
> +	u32 size;
> +	char data[1];
> +};
> +
> +struct  mousevsc_prt_msg {
> +	enum pipe_prot_msg_type type;
> +	u32 size;
> +	union {
> +		struct synthhid_protocol_request request;
> +		struct synthhid_protocol_response response;
> +		struct synthhid_device_info_ack ack;
> +	};
> +};
> +
> +/*
> + * Represents an mousevsc device
> + */
> +struct mousevsc_dev {
> +	struct hv_device	*device;
> +	bool			init_complete;
> +	bool			connected;
> +	struct mousevsc_prt_msg	protocol_req;
> +	struct mousevsc_prt_msg	protocol_resp;
> +	/* Synchronize the request/response if needed */
> +	struct completion	wait_event;
> +	int			dev_info_status;
> +
> +	struct hid_descriptor	*hid_desc;
> +	unsigned char		*report_desc;
> +	u32			report_desc_size;
> +	struct hv_input_dev_info hid_dev_info;
> +	struct hid_device       *hid_device;
> +};
> +
> +
> +static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
> +{
> +	struct mousevsc_dev *input_dev;
> +
> +	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> +
> +	if (!input_dev)
> +		return NULL;
> +
> +	input_dev->device = device;
> +	hv_set_drvdata(device, input_dev);
> +	init_completion(&input_dev->wait_event);
> +
> +	return input_dev;
> +}
> +
> +static void free_input_device(struct mousevsc_dev *device)
> +{
> +	kfree(device->hid_desc);
> +	kfree(device->report_desc);
> +	hv_set_drvdata(device->device, NULL);
> +	kfree(device);
> +}
> +
> +
> +static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
> +				struct synthhid_device_info *device_info)
> +{
> +	int ret = 0;
> +	struct hid_descriptor *desc;
> +	struct mousevsc_prt_msg ack;
> +
> +	/* Assume success for now */
> +	input_device->dev_info_status = 0;
> +
> +	memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> +		sizeof(struct hv_input_dev_info));
> +
> +	desc = &device_info->hid_descriptor;
> +	WARN_ON(desc->bLength == 0);
> +
> +	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> +
> +	if (!input_device->hid_desc)
> +		goto cleanup;
> +
> +	memcpy(input_device->hid_desc, desc, desc->bLength);
> +
> +	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> +	if (input_device->report_desc_size == 0)
> +		goto cleanup;
> +	input_device->report_desc = kzalloc(input_device->report_desc_size,
> +					  GFP_ATOMIC);
> +
> +	if (!input_device->report_desc)
> +		goto cleanup;
> +
> +	memcpy(input_device->report_desc,
> +	       ((unsigned char *)desc) + desc->bLength,
> +	       desc->desc[0].wDescriptorLength);
> +
> +	/* Send the ack */
> +	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> +
> +	ack.type = PIPE_MESSAGE_DATA;
> +	ack.size = sizeof(struct synthhid_device_info_ack);
> +
> +	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> +	ack.ack.header.size = 1;
> +	ack.ack.reserved = 0;
> +
> +	ret = vmbus_sendpacket(input_device->device->channel,
> +			&ack,
> +			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> +			sizeof(struct synthhid_device_info_ack),
> +			(unsigned long)&ack,
> +			VM_PKT_DATA_INBAND,
> +			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +	if (ret != 0)
> +		goto cleanup;
> +
> +	complete(&input_device->wait_event);
> +
> +	return;
> +
> +cleanup:
> +	kfree(input_device->hid_desc);
> +	input_device->hid_desc = NULL;
> +
> +	kfree(input_device->report_desc);
> +	input_device->report_desc = NULL;
> +
> +	input_device->dev_info_status = -1;
> +	complete(&input_device->wait_event);
> +}
> +
> +static void mousevsc_on_receive(struct hv_device *device,
> +				struct vmpacket_descriptor *packet)
> +{
> +	struct pipe_prt_msg *pipe_msg;
> +	struct synthhid_msg *hid_msg;
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> +	struct synthhid_input_report *input_report;
> +
> +	pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
> +						(packet->offset8 << 3));
> +
> +	if (pipe_msg->type != PIPE_MESSAGE_DATA)
> +		return;
> +
> +	hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
> +
> +	switch (hid_msg->header.type) {
> +	case SYNTH_HID_PROTOCOL_RESPONSE:
> +		/*
> +		 * While it will be impossible for us to protect against
> +		 * malicious/buggy hypervisor/host, add a check here to
> +		 * ensure we don't corrupt memory.
> +		 */
> +		if ((pipe_msg->size + sizeof(struct pipe_prt_msg)
> +			- sizeof(unsigned char))
> +			> sizeof(struct mousevsc_prt_msg)) {
> +			WARN_ON(1);
> +			break;
> +		}
> +
> +		memcpy(&input_dev->protocol_resp, pipe_msg,
> +		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
> +		       sizeof(unsigned char));
> +		complete(&input_dev->wait_event);
> +		break;
> +
> +	case SYNTH_HID_INITIAL_DEVICE_INFO:
> +		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> +
> +		/*
> +		 * Parse out the device info into device attr,
> +		 * hid desc and report desc
> +		 */
> +		mousevsc_on_receive_device_info(input_dev,
> +			(struct synthhid_device_info *)&pipe_msg->data[0]);
> +		break;
> +	case SYNTH_HID_INPUT_REPORT:
> +		input_report =
> +			(struct synthhid_input_report *)&pipe_msg->data[0];
> +		if (!input_dev->init_complete)
> +			break;
> +		hid_input_report(input_dev->hid_device,
> +				HID_INPUT_REPORT, input_report->buffer,
> +				input_report->header.size, 1);
> +		break;
> +	default:
> +		pr_err("unsupported hid msg type - type %d len %d",
> +		       hid_msg->header.type, hid_msg->header.size);
> +		break;
> +	}
> +
> +}
> +
> +static void mousevsc_on_channel_callback(void *context)
> +{
> +	const int packet_size = 0x100;
> +	int ret;
> +	struct hv_device *device = context;
> +	u32 bytes_recvd;
> +	u64 req_id;
> +	struct vmpacket_descriptor *desc;
> +	unsigned char	*buffer;

Kill the whitespace between "char" and "*buffer".

> +	int	bufferlen = packet_size;

Kill the extra spaces between "int" and "bufferlen" here.


> +
> +	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> +	if (!buffer)
> +		return;
> +
> +	do {
> +		ret = vmbus_recvpacket_raw(device->channel, buffer,
> +					bufferlen, &bytes_recvd, &req_id);
> +
> +		switch (ret) {
> +		case 0:
> +			if (bytes_recvd <= 0) {
> +				kfree(buffer);
> +				return;
> +			}
> +			desc = (struct vmpacket_descriptor *)buffer;
> +
> +			switch (desc->type) {
> +			case VM_PKT_COMP:
> +				break;
> +
> +			case VM_PKT_DATA_INBAND:
> +				mousevsc_on_receive(device, desc);
> +				break;
> +
> +			default:
> +				pr_err("unhandled packet type %d, tid %llx len %d\n",
> +				   desc->type,
> +				   req_id,
> +				   bytes_recvd);

Why not:

                                pr_err("unhandled packet type %d, tid %llx 
                                       len %d\n", desc->type, req_id, bytes_recvd);
> +				break;
> +			}
> +
> +			break;
> +
> +		case -ENOBUFS:
> +			kfree(buffer);
> +			/* Handle large packet */
> +			bufferlen = bytes_recvd;
> +			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> +
> +			if (!buffer)
> +				return;
> +
> +			break;
> +		}
> +	} while (1);
> +
> +}
> +
> +static int mousevsc_connect_to_vsp(struct hv_device *device)
> +{
> +	int ret = 0;

Pointless to explicitly initialize 'ret' to 0 when there is no way you 
could return from the function before initializing 'ret' with 
'vmbus_sendpacket()' below.

> +	int t;
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> +	struct mousevsc_prt_msg *request;
> +	struct mousevsc_prt_msg *response;
> +
> +	request = &input_dev->protocol_req;
> +
> +	memset(request, 0, sizeof(struct mousevsc_prt_msg));
> +
> +	request->type = PIPE_MESSAGE_DATA;
> +	request->size = sizeof(struct synthhid_protocol_request);
> +
> +	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> +	request->request.header.size = sizeof(unsigned int);
> +	request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
> +
> +	ret = vmbus_sendpacket(device->channel, request,
> +				sizeof(struct pipe_prt_msg) -
> +				sizeof(unsigned char) +
> +				sizeof(struct synthhid_protocol_request),
> +				(unsigned long)request,
> +				VM_PKT_DATA_INBAND,
> +				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +	if (ret)
> +		goto cleanup;
> +
> +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> +	if (!t) {
> +		ret = -ETIMEDOUT;
> +		goto cleanup;
> +	}
> +
> +	response = &input_dev->protocol_resp;
> +
> +	if (!response->response.approved) {
> +		pr_err("synthhid protocol request failed (version %d)\n",
> +		       SYNTHHID_INPUT_VERSION);
> +		ret = -ENODEV;
> +		goto cleanup;
> +	}
> +
> +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> +	if (!t) {
> +		ret = -ETIMEDOUT;
> +		goto cleanup;
> +	}
> +
> +	/*
> +	 * We should have gotten the device attr, hid desc and report
> +	 * desc at this point
> +	 */
> +	if (input_dev->dev_info_status)
> +		ret = -ENOMEM;
> +
> +cleanup:
> +
Pretty pointless newline...

> +	return ret;
> +}
> +
> +static int mousevsc_hid_open(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static int mousevsc_hid_start(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static void mousevsc_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +static void mousevsc_hid_stop(struct hid_device *hid)
> +{
> +}
> +
> +static struct hid_ll_driver mousevsc_ll_driver = {
> +	.open = mousevsc_hid_open,
> +	.close = mousevsc_hid_close,
> +	.start = mousevsc_hid_start,
> +	.stop = mousevsc_hid_stop,
> +};
> +
> +static struct hid_driver mousevsc_hid_driver;
> +
> +static int mousevsc_probe(struct hv_device *device,
> +			const struct hv_vmbus_device_id *dev_id)
> +{
> +	int ret = 0;

Why init 'ret' to 0 here? It is always initialized again later before 
being used...

> +	struct mousevsc_dev *input_dev;
> +	struct hid_device *hid_dev;
> +
> +	input_dev = alloc_input_device(device);
> +
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->init_complete = false;
> +
> +	ret = vmbus_open(device->channel,
> +		INPUTVSC_SEND_RING_BUFFER_SIZE,
> +		INPUTVSC_RECV_RING_BUFFER_SIZE,
> +		NULL,
> +		0,
> +		mousevsc_on_channel_callback,
> +		device
> +		);
> +
> +	if (ret != 0) {
> +		free_input_device(input_dev);
> +		return ret;
> +	}
> +
> +	ret = mousevsc_connect_to_vsp(device);
> +
> +	if (ret != 0)
> +		goto probe_err0;
> +
> +	/* workaround SA-167 */
> +	if (input_dev->report_desc[14] == 0x25)
> +		input_dev->report_desc[14] = 0x29;
> +
> +	hid_dev = hid_allocate_device();
> +	if (IS_ERR(hid_dev)) {
> +		ret = PTR_ERR(hid_dev);
> +		goto probe_err0;
> +	}
> +
> +	hid_dev->ll_driver = &mousevsc_ll_driver;
> +	hid_dev->driver = &mousevsc_hid_driver;
> +	hid_dev->bus = BUS_VIRTUAL;
> +	hid_dev->vendor = input_dev->hid_dev_info.vendor;
> +	hid_dev->product = input_dev->hid_dev_info.product;
> +	hid_dev->version = input_dev->hid_dev_info.version;
> +	input_dev->hid_device = hid_dev;
> +
> +	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> +
> +	ret = hid_parse_report(hid_dev, input_dev->report_desc,
> +				input_dev->report_desc_size);
> +
> +	if (ret) {
> +		hid_err(hid_dev, "parse failed\n");
> +		goto probe_err1;
> +	}
> +
> +	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
> +
> +	if (ret) {
> +		hid_err(hid_dev, "hw start failed\n");
> +		goto probe_err1;
> +	}
> +
> +	input_dev->connected = true;
> +	input_dev->init_complete = true;
> +
> +	return ret;
> +
> +probe_err1:
> +	hid_destroy_device(hid_dev);
> +
> +probe_err0:
> +	vmbus_close(device->channel);
> +	free_input_device(input_dev);
> +	return ret;
> +}
> +
> +
> +static int mousevsc_remove(struct hv_device *dev)
> +{
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> +
> +	vmbus_close(dev->channel);
> +
> +	if (input_dev->connected) {
> +		hidinput_disconnect(input_dev->hid_device);
> +		input_dev->connected = false;
> +		hid_destroy_device(input_dev->hid_device);
> +	}
> +
> +	free_input_device(input_dev);
> +
> +	return 0;
> +}
> +
> +static const struct hv_vmbus_device_id id_table[] = {
> +	/* Mouse guid */
> +	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> +		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(vmbus, id_table);
> +
> +static struct  hv_driver mousevsc_drv = {
> +	.name = KBUILD_MODNAME,
> +	.id_table = id_table,
> +	.probe = mousevsc_probe,
> +	.remove = mousevsc_remove,
> +};
> +
> +static int __init mousevsc_init(void)
> +{
> +	return vmbus_driver_register(&mousevsc_drv);
> +}
> +
> +static void __exit mousevsc_exit(void)
> +{
> +	vmbus_driver_unregister(&mousevsc_drv);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(HV_DRV_VERSION);
> +module_init(mousevsc_init);
> +module_exit(mousevsc_exit);
> 

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Dan Carpenter]

On Sat, Oct 29, 2011 at 12:54:34AM +0200, Jesper Juhl wrote:
> > +			default:
> > +				pr_err("unhandled packet type %d, tid %llx len %d\n",
> > +				   desc->type,
> > +				   req_id,
> > +				   bytes_recvd);
> 
> Why not:
> 
>                                 pr_err("unhandled packet type %d, tid %llx 
>                                        len %d\n", desc->type, req_id, bytes_recvd);

Because then the printk would be messed up?  Your final printed
string would look like:

"unhandled packet type %d, tid %llx 
                                       len %d\n"

Don't break strings up across lines because it breaks grep.  If K. Y.
wants to put all the parameters on one line instead of three that
would probably be better, but in the end who cares?

regards,
dan carpenter

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Greg KH]

On Fri, Oct 28, 2011 at 08:28:11PM +0000, KY Srinivasan wrote:
> > > The guest cannot survive a malicious host; so I think it is safe to say that the
> > > guest can assume the host is following the protocol.
> > 
> > That's not good for a very large number of reasons, not the least being
> > that we have no idea how secure the hyperv hypervisor is, so making it
> > so that there isn't an obvious hole into linux through it, would be a
> > good idea.
> > 
> > And yes, I'd say the same thing if this was a KVM or Xen driver as well.
> > Please be very defensive in this area of the code, especially as there
> > are no performance issues here.
> 
> In the chain of trust, the hypervisor and the host are the foundations
> as far as the guest is concerned, since both the hypervisor and the host
> can affect the guest in ways that the guest has no obvious way to protect itself.

That's true.

> If the hypervisor/host have security holes, there is not much you can do in the guest
> to deal with it. 
> In this case, I can add checks but I am not sure how useful it is.

I would prefer to see them here, just to be safe, it can not hurt,
right?


greg k-h

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Saturday, October 29, 2011 2:34 AM
> To: KY Srinivasan
> Cc: Jiri Kosina; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; Dmitry Torokhov
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Fri, Oct 28, 2011 at 08:28:11PM +0000, KY Srinivasan wrote:
> > > > The guest cannot survive a malicious host; so I think it is safe to say that the
> > > > guest can assume the host is following the protocol.
> > >
> > > That's not good for a very large number of reasons, not the least being
> > > that we have no idea how secure the hyperv hypervisor is, so making it
> > > so that there isn't an obvious hole into linux through it, would be a
> > > good idea.
> > >
> > > And yes, I'd say the same thing if this was a KVM or Xen driver as well.
> > > Please be very defensive in this area of the code, especially as there
> > > are no performance issues here.
> >
> > In the chain of trust, the hypervisor and the host are the foundations
> > as far as the guest is concerned, since both the hypervisor and the host
> > can affect the guest in ways that the guest has no obvious way to protect itself.
> 
> That's true.
> 
> > If the hypervisor/host have security holes, there is not much you can do in the
> guest
> > to deal with it.
> > In this case, I can add checks but I am not sure how useful it is.
> 
> I would prefer to see them here, just to be safe, it can not hurt,
> right?

I have added a check in the patches I sent out yesterday.

Regards,

K. Y
> 
> 
> greg k-h

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Jiri Kosina]

On Fri, 28 Oct 2011, KY Srinivasan wrote:

> > > > >  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
> > > > >  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
> > > > >  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> > > > > +obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o
> > > >
> > > > I'd prefer a bit to follow the current naming of the drivers in
> > > > drivers/hid directory ... all the device-specific (vendor-specific)
> > > > drivers currently are called hid-<vendor> or similar.
> > > >
> > > > We could talk about changing this naming scheme, but before we reach any
> > > > conclusion on this, I'd prefer this to stay for all drivers if possible.
> > >
> > > Do you want the driver module to conform to the naming scheme you have?
> > > Greg, in the past has resisted changing driver names, but I have no objection
> > > to conforming to the naming convention you have.
> > 
> > I recommend following the proper naming scheme.  As this driver is
> > auto-loaded when the virtual hardware is seen, the name really doesn't
> > matter at all.
> > 
> > So, how about 'hid-hyperv' or 'hid-hv'?
> 
> I will go with hid-hv  if there are no objections.

OK by me, thanks.

-- 
Jiri Kosina
SUSE Labs

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Jiri Kosina [mailto:jkosina@suse.cz]
> Sent: Saturday, October 29, 2011 1:05 PM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; Dmitry Torokhov
> Subject: RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Fri, 28 Oct 2011, KY Srinivasan wrote:
> 
> > > > > >  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
> > > > > >  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
> > > > > >  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> > > > > > +obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o
> > > > >
> > > > > I'd prefer a bit to follow the current naming of the drivers in
> > > > > drivers/hid directory ... all the device-specific (vendor-specific)
> > > > > drivers currently are called hid-<vendor> or similar.
> > > > >
> > > > > We could talk about changing this naming scheme, but before we reach
> any
> > > > > conclusion on this, I'd prefer this to stay for all drivers if possible.
> > > >
> > > > Do you want the driver module to conform to the naming scheme you
> have?
> > > > Greg, in the past has resisted changing driver names, but I have no
> objection
> > > > to conforming to the naming convention you have.
> > >
> > > I recommend following the proper naming scheme.  As this driver is
> > > auto-loaded when the virtual hardware is seen, the name really doesn't
> > > matter at all.
> > >
> > > So, how about 'hid-hyperv' or 'hid-hv'?
> >
> > I will go with hid-hv  if there are no objections.
> 
> OK by me, thanks.

I am sorry I changed my mind and named it hid-hyperv. The patch I sent out
has this name. I hope it is ok with you.

Regards,

K. Y

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Saturday, October 29, 2011 2:33 AM
> To: Jesper Juhl
> Cc: KY Srinivasan; dmitry.torokhov@gmail.com; jkosina@suse.cz;
> gregkh@suse.de; ohering@suse.com; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org; joe@perches.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Sat, Oct 29, 2011 at 12:54:34AM +0200, Jesper Juhl wrote:
> > > +			default:
> > > +				pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > > +				   desc->type,
> > > +				   req_id,
> > > +				   bytes_recvd);
> >
> > Why not:
> >
> >                                 pr_err("unhandled packet type %d, tid %llx
> >                                        len %d\n", desc->type, req_id, bytes_recvd);
> 
> Because then the printk would be messed up?  Your final printed
> string would look like:
> 
> "unhandled packet type %d, tid %llx
>                                        len %d\n"
> 
> Don't break strings up across lines because it breaks grep.  If K. Y.
> wants to put all the parameters on one line instead of three that
> would probably be better, but in the end who cares?
> 
Thanks Dan.

Regards,

K. Y

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Jesper Juhl]

On Sat, 29 Oct 2011, Dan Carpenter wrote:

> On Sat, Oct 29, 2011 at 12:54:34AM +0200, Jesper Juhl wrote:
> > > +			default:
> > > +				pr_err("unhandled packet type %d, tid %llx len %d\n",
> > > +				   desc->type,
> > > +				   req_id,
> > > +				   bytes_recvd);
> > 
> > Why not:
> > 
> >                                 pr_err("unhandled packet type %d, tid %llx 
> >                                        len %d\n", desc->type, req_id, bytes_recvd);
> 
> Because then the printk would be messed up?  Your final printed
> string would look like:
> 
> "unhandled packet type %d, tid %llx 
>                                        len %d\n"
> 
> Don't break strings up across lines because it breaks grep.  If K. Y.
> wants to put all the parameters on one line instead of three that
> would probably be better, but in the end who cares?
> 
Right, so I obviously "fat fingered" that and should have read my email 
once more before sending. *But* the point really was just the "put all the 
parameters on one line rather than 3" bit...

As for who cares; well, I cared enough to actually read the patch and send 
a reply, and I thought we needed more reviewers... When I review something 
I comment on everything I spot from bugs to trivial stuff - then it is 
up to the recipient to pick the things they want to address from the 
reply..

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Jesper Juhl [mailto:jj@chaosbits.net]
> Sent: Wednesday, November 02, 2011 6:59 PM
> To: Dan Carpenter
> Cc: KY Srinivasan; dmitry.torokhov@gmail.com; jkosina@suse.cz;
> gregkh@suse.de; ohering@suse.com; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org; joe@perches.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Sat, 29 Oct 2011, Dan Carpenter wrote:
> 
> > On Sat, Oct 29, 2011 at 12:54:34AM +0200, Jesper Juhl wrote:
> > > > +			default:
> > > > +				pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > > > +				   desc->type,
> > > > +				   req_id,
> > > > +				   bytes_recvd);
> > >
> > > Why not:
> > >
> > >                                 pr_err("unhandled packet type %d, tid %llx
> > >                                        len %d\n", desc->type, req_id, bytes_recvd);
> >
> > Because then the printk would be messed up?  Your final printed
> > string would look like:
> >
> > "unhandled packet type %d, tid %llx
> >                                        len %d\n"
> >
> > Don't break strings up across lines because it breaks grep.  If K. Y.
> > wants to put all the parameters on one line instead of three that
> > would probably be better, but in the end who cares?
> >
> Right, so I obviously "fat fingered" that and should have read my email
> once more before sending. *But* the point really was just the "put all the
> parameters on one line rather than 3" bit...
> 
> As for who cares; well, I cared enough to actually read the patch and send
> a reply, and I thought we needed more reviewers... When I review something
> I comment on everything I spot from bugs to trivial stuff - then it is
> up to the recipient to pick the things they want to address from the
> reply..

I am sure I will have to re-spin this. I will take care of it then.


Regards,

K. Y
 

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Dmitry Torokhov]

Hi KY,

On Fri, Oct 28, 2011 at 03:35:16PM -0700, K. Y. Srinivasan wrote:
> The hv_mouse.c implements a hid compliant mouse driver for use on a
> Hyper-V based system. This driver is currently in the staging area and
> as part of the effort to move this driver out of staging, I had posted
> the driver code for community review a few weeks ago. This current patch
> addresses all the review comments I have gotten to date. All the relevant
> patches have already been submitted to the staging tree as well.
> 
> As per Greg's suggestion, this patch does not get rid of the code from
> the staging area. Once the mouse driver lands under the hid directory,
> we will cleanup the staging directory.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hid/Kconfig      |    6 +
>  drivers/hid/Makefile     |    1 +
>  drivers/hid/hid-hyperv.c |  600 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 607 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hid/hid-hyperv.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 1130a89..068dd97 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -613,6 +613,12 @@ config HID_ZYDACRON
>  	---help---
>  	Support for Zydacron remote control.
>  
> +config HYPERV_MOUSE
> +	tristate "Microsoft Hyper-V mouse driver"
> +	depends on HYPERV
> +	---help---
> +	  Select this option to enable the Hyper-V mouse driver.
> +
>  endmenu
>  
>  endif # HID_SUPPORT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 0a0a38e..e683b8c 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
>  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
>  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
>  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
> +obj-$(CONFIG_HYPERV_MOUSE)	+= hid-hyperv.o
>  
>  obj-$(CONFIG_USB_HID)		+= usbhid/
>  obj-$(CONFIG_USB_MOUSE)		+= usbhid/
> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
> new file mode 100644
> index 0000000..2c2e1b4
> --- /dev/null
> +++ b/drivers/hid/hid-hyperv.c
> @@ -0,0 +1,600 @@
> +/*
> + *  Copyright (c) 2009, Citrix Systems, Inc.
> + *  Copyright (c) 2010, Microsoft Corporation.
> + *  Copyright (c) 2011, Novell Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms and conditions of the GNU General Public License,
> + *  version 2, as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope it will be useful, but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + *  more details.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/completion.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/hiddev.h>
> +#include <linux/hyperv.h>
> +
> +
> +struct hv_input_dev_info {
> +	unsigned int size;
> +	unsigned short vendor;
> +	unsigned short product;
> +	unsigned short version;
> +	unsigned short reserved[11];
> +};
> +
> +/* The maximum size of a synthetic input message. */
> +#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
> +
> +/*
> + * Current version
> + *
> + * History:
> + * Beta, RC < 2008/1/22        1,0
> + * RC > 2008/1/22              2,0
> + */
> +#define SYNTHHID_INPUT_VERSION_MAJOR	2
> +#define SYNTHHID_INPUT_VERSION_MINOR	0
> +#define SYNTHHID_INPUT_VERSION		(SYNTHHID_INPUT_VERSION_MINOR | \
> +					 (SYNTHHID_INPUT_VERSION_MAJOR << 16))
> +
> +
> +#pragma pack(push, 1)
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synthhid_msg_type {
> +	SYNTH_HID_PROTOCOL_REQUEST,
> +	SYNTH_HID_PROTOCOL_RESPONSE,
> +	SYNTH_HID_INITIAL_DEVICE_INFO,
> +	SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> +	SYNTH_HID_INPUT_REPORT,
> +	SYNTH_HID_MAX
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synthhid_msg_hdr {
> +	enum synthhid_msg_type type;
> +	u32 size;
> +};
> +
> +struct synthhid_msg {
> +	struct synthhid_msg_hdr header;
> +	char data[1]; /* Enclosed message */
> +};
> +
> +union synthhid_version {
> +	struct {
> +		u16 minor_version;
> +		u16 major_version;
> +	};
> +	u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synthhid_protocol_request {
> +	struct synthhid_msg_hdr header;
> +	union synthhid_version version_requested;
> +};
> +
> +struct synthhid_protocol_response {
> +	struct synthhid_msg_hdr header;
> +	union synthhid_version version_requested;
> +	unsigned char approved;
> +};
> +
> +struct synthhid_device_info {
> +	struct synthhid_msg_hdr header;
> +	struct hv_input_dev_info hid_dev_info;
> +	struct hid_descriptor hid_descriptor;
> +};
> +
> +struct synthhid_device_info_ack {
> +	struct synthhid_msg_hdr header;
> +	unsigned char reserved;
> +};
> +
> +struct synthhid_input_report {
> +	struct synthhid_msg_hdr header;
> +	char buffer[1];
> +};
> +
> +#pragma pack(pop)
> +
> +#define INPUTVSC_SEND_RING_BUFFER_SIZE		(10*PAGE_SIZE)
> +#define INPUTVSC_RECV_RING_BUFFER_SIZE		(10*PAGE_SIZE)
> +
> +
> +enum pipe_prot_msg_type {
> +	PIPE_MESSAGE_INVALID,
> +	PIPE_MESSAGE_DATA,
> +	PIPE_MESSAGE_MAXIMUM
> +};
> +
> +
> +struct pipe_prt_msg {
> +	enum pipe_prot_msg_type type;
> +	u32 size;
> +	char data[1];
> +};
> +
> +struct  mousevsc_prt_msg {
> +	enum pipe_prot_msg_type type;
> +	u32 size;
> +	union {
> +		struct synthhid_protocol_request request;
> +		struct synthhid_protocol_response response;
> +		struct synthhid_device_info_ack ack;
> +	};
> +};
> +
> +/*
> + * Represents an mousevsc device
> + */
> +struct mousevsc_dev {
> +	struct hv_device	*device;
> +	bool			init_complete;
> +	bool			connected;
> +	struct mousevsc_prt_msg	protocol_req;
> +	struct mousevsc_prt_msg	protocol_resp;
> +	/* Synchronize the request/response if needed */
> +	struct completion	wait_event;
> +	int			dev_info_status;
> +
> +	struct hid_descriptor	*hid_desc;
> +	unsigned char		*report_desc;
> +	u32			report_desc_size;
> +	struct hv_input_dev_info hid_dev_info;
> +	struct hid_device       *hid_device;
> +};
> +
> +
> +static struct mousevsc_dev *alloc_input_device(struct hv_device *device)

Can we call it mousevsc_alloc_device?

> +{
> +	struct mousevsc_dev *input_dev;
> +
> +	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> +
> +	if (!input_dev)
> +		return NULL;
> +
> +	input_dev->device = device;
> +	hv_set_drvdata(device, input_dev);
> +	init_completion(&input_dev->wait_event);
> +
> +	return input_dev;
> +}
> +
> +static void free_input_device(struct mousevsc_dev *device)

Can we call it mousevsc_free_device?

> +{
> +	kfree(device->hid_desc);
> +	kfree(device->report_desc);
> +	hv_set_drvdata(device->device, NULL);
> +	kfree(device);
> +}
> +
> +
> +static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
> +				struct synthhid_device_info *device_info)
> +{
> +	int ret = 0;
> +	struct hid_descriptor *desc;
> +	struct mousevsc_prt_msg ack;

This could probably be:

	static const mousevsc_prt_msg ack = {
		.type = PIPE_MESSAGE_DATA,
		.size = sizeof(struct synthhid_device_info_ack),
		.ack = {
			.header = {
				.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
				.size = 1,
			},
			.reserved = 0,
		},
	};


> +
> +	/* Assume success for now */
> +	input_device->dev_info_status = 0;
> +
> +	memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> +		sizeof(struct hv_input_dev_info));
> +

or simply:

	input_device->hid_dev_info = device_info->hid_dev_info;

> +	desc = &device_info->hid_descriptor;
> +	WARN_ON(desc->bLength == 0);

Should also goto cleanup as such descriptor is not usable.

> +
> +	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> +
> +	if (!input_device->hid_desc)

if you do

		input_device->dev_info_status = -ENOMEM;

you could use it later instead of defaulting to -ENOMEM.

> +		goto cleanup;
> +
> +	memcpy(input_device->hid_desc, desc, desc->bLength);
> +
> +	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> +	if (input_device->report_desc_size == 0)

		input_device->dev_info_status = -EINVAL;

> +		goto cleanup;
> +	input_device->report_desc = kzalloc(input_device->report_desc_size,
> +					  GFP_ATOMIC);
> +
> +	if (!input_device->report_desc)

		input_device->dev_info_status = -ENOMEM;

> +		goto cleanup;
> +
> +	memcpy(input_device->report_desc,
> +	       ((unsigned char *)desc) + desc->bLength,
> +	       desc->desc[0].wDescriptorLength);
> +
> +	/* Send the ack */
> +	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> +
> +	ack.type = PIPE_MESSAGE_DATA;
> +	ack.size = sizeof(struct synthhid_device_info_ack);
> +
> +	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> +	ack.ack.header.size = 1;
> +	ack.ack.reserved = 0;
> +
> +	ret = vmbus_sendpacket(input_device->device->channel,
> +			&ack,
> +			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> +			sizeof(struct synthhid_device_info_ack),
> +			(unsigned long)&ack,
> +			VM_PKT_DATA_INBAND,
> +			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +	if (ret != 0)

		input_device->dev_info_status = ret;

> +		goto cleanup;
> +
> +	complete(&input_device->wait_event);
> +
> +	return;
> +
> +cleanup:
> +	kfree(input_device->hid_desc);
> +	input_device->hid_desc = NULL;
> +
> +	kfree(input_device->report_desc);
> +	input_device->report_desc = NULL;

Do you have to clean it up here? You still need to clean it up in main
unwind path so consolidate both error and success path and just signal
completion and let main code figure it all out.

> +
> +	input_device->dev_info_status = -1;
> +	complete(&input_device->wait_event);
> +}
> +
> +static void mousevsc_on_receive(struct hv_device *device,
> +				struct vmpacket_descriptor *packet)
> +{
> +	struct pipe_prt_msg *pipe_msg;
> +	struct synthhid_msg *hid_msg;
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> +	struct synthhid_input_report *input_report;
> +
> +	pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
> +						(packet->offset8 << 3));
> +
> +	if (pipe_msg->type != PIPE_MESSAGE_DATA)
> +		return;
> +
> +	hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
> +
> +	switch (hid_msg->header.type) {
> +	case SYNTH_HID_PROTOCOL_RESPONSE:
> +		/*
> +		 * While it will be impossible for us to protect against
> +		 * malicious/buggy hypervisor/host, add a check here to
> +		 * ensure we don't corrupt memory.
> +		 */
> +		if ((pipe_msg->size + sizeof(struct pipe_prt_msg)
> +			- sizeof(unsigned char))
> +			> sizeof(struct mousevsc_prt_msg)) {
> +			WARN_ON(1);
> +			break;
> +		}
> +
> +		memcpy(&input_dev->protocol_resp, pipe_msg,
> +		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
> +		       sizeof(unsigned char));
> +		complete(&input_dev->wait_event);
> +		break;
> +
> +	case SYNTH_HID_INITIAL_DEVICE_INFO:
> +		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> +
> +		/*
> +		 * Parse out the device info into device attr,
> +		 * hid desc and report desc
> +		 */
> +		mousevsc_on_receive_device_info(input_dev,
> +			(struct synthhid_device_info *)&pipe_msg->data[0]);

&pipe_msg->data[0] is the same as pipe_msg->data, but the latter is
shorter and cleaner.

> +		break;
> +	case SYNTH_HID_INPUT_REPORT:
> +		input_report =
> +			(struct synthhid_input_report *)&pipe_msg->data[0];
> +		if (!input_dev->init_complete)
> +			break;
> +		hid_input_report(input_dev->hid_device,
> +				HID_INPUT_REPORT, input_report->buffer,
> +				input_report->header.size, 1);
> +		break;
> +	default:
> +		pr_err("unsupported hid msg type - type %d len %d",
> +		       hid_msg->header.type, hid_msg->header.size);
> +		break;
> +	}
> +
> +}
> +
> +static void mousevsc_on_channel_callback(void *context)
> +{
> +	const int packet_size = 0x100;
> +	int ret;
> +	struct hv_device *device = context;
> +	u32 bytes_recvd;
> +	u64 req_id;
> +	struct vmpacket_descriptor *desc;
> +	unsigned char	*buffer;
> +	int	bufferlen = packet_size;
> +
> +	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> +	if (!buffer)
> +		return;
> +
> +	do {
> +		ret = vmbus_recvpacket_raw(device->channel, buffer,
> +					bufferlen, &bytes_recvd, &req_id);
> +
> +		switch (ret) {
> +		case 0:
> +			if (bytes_recvd <= 0) {
> +				kfree(buffer);
> +				return;
> +			}
> +			desc = (struct vmpacket_descriptor *)buffer;
> +
> +			switch (desc->type) {
> +			case VM_PKT_COMP:
> +				break;
> +
> +			case VM_PKT_DATA_INBAND:
> +				mousevsc_on_receive(device, desc);
> +				break;
> +
> +			default:
> +				pr_err("unhandled packet type %d, tid %llx len %d\n",
> +				   desc->type,
> +				   req_id,
> +				   bytes_recvd);
> +				break;
> +			}
> +
> +			break;
> +
> +		case -ENOBUFS:
> +			kfree(buffer);
> +			/* Handle large packet */
> +			bufferlen = bytes_recvd;
> +			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);

Instead of potentially ever-increasing buffer that you also allocate
(and it looks like leaking on every callback invocation) can you just
repeat the read if you know that there are more data and use single
pre-allocated buffer?

> +
> +			if (!buffer)
> +				return;
> +
> +			break;
> +		}
> +	} while (1);
> +
> +}
> +
> +static int mousevsc_connect_to_vsp(struct hv_device *device)
> +{
> +	int ret = 0;
> +	int t;
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> +	struct mousevsc_prt_msg *request;
> +	struct mousevsc_prt_msg *response;
> +
> +	request = &input_dev->protocol_req;
> +
> +	memset(request, 0, sizeof(struct mousevsc_prt_msg));
> +
> +	request->type = PIPE_MESSAGE_DATA;
> +	request->size = sizeof(struct synthhid_protocol_request);
> +
> +	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> +	request->request.header.size = sizeof(unsigned int);
> +	request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
> 

This is constant data; just do the same as with ack in
on_receive_device_info. It does not need to be a part of input_dev, does
it?

> +	ret = vmbus_sendpacket(device->channel, request,
> +				sizeof(struct pipe_prt_msg) -
> +				sizeof(unsigned char) +
> +				sizeof(struct synthhid_protocol_request),
> +				(unsigned long)request,
> +				VM_PKT_DATA_INBAND,
> +				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +	if (ret)
> +		goto cleanup;
> +
> +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> +	if (!t) {
> +		ret = -ETIMEDOUT;
> +		goto cleanup;
> +	}
> +
> +	response = &input_dev->protocol_resp;
> +
> +	if (!response->response.approved) {
> +		pr_err("synthhid protocol request failed (version %d)\n",
> +		       SYNTHHID_INPUT_VERSION);
> +		ret = -ENODEV;
> +		goto cleanup;
> +	}
> +
> +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);

5 seconds? That's a long time of HV to respond...

> +	if (!t) {
> +		ret = -ETIMEDOUT;
> +		goto cleanup;
> +	}
> +
> +	/*
> +	 * We should have gotten the device attr, hid desc and report
> +	 * desc at this point
> +	 */
> +	if (input_dev->dev_info_status)
> +		ret = -ENOMEM;

Just do

	ret = input_dev->dev_info_status;

unconditionally.

> +
> +cleanup:
> +
> +	return ret;
> +}
> +
> +static int mousevsc_hid_open(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static int mousevsc_hid_start(struct hid_device *hid)
> +{
> +	return 0;
> +}
> +
> +static void mousevsc_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +static void mousevsc_hid_stop(struct hid_device *hid)
> +{
> +}
> +
> +static struct hid_ll_driver mousevsc_ll_driver = {
> +	.open = mousevsc_hid_open,
> +	.close = mousevsc_hid_close,
> +	.start = mousevsc_hid_start,
> +	.stop = mousevsc_hid_stop,
> +};
> +
> +static struct hid_driver mousevsc_hid_driver;
> +
> +static int mousevsc_probe(struct hv_device *device,
> +			const struct hv_vmbus_device_id *dev_id)
> +{
> +	int ret = 0;
> +	struct mousevsc_dev *input_dev;
> +	struct hid_device *hid_dev;
> +
> +	input_dev = alloc_input_device(device);
> +
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->init_complete = false;

Should this also be in alloc_input_device()? 

> +
> +	ret = vmbus_open(device->channel,
> +		INPUTVSC_SEND_RING_BUFFER_SIZE,
> +		INPUTVSC_RECV_RING_BUFFER_SIZE,
> +		NULL,
> +		0,
> +		mousevsc_on_channel_callback,
> +		device
> +		);
> +

This blank line is extra IMO.

> +	if (ret != 0) {
> +		free_input_device(input_dev);
> +		return ret;

Please do not mix error unwinding with gotos and unwinding in error
handling branches.

> +	}
> +
> +	ret = mousevsc_connect_to_vsp(device);
> +

This blank line is extra IMO.

> +	if (ret != 0)
> +		goto probe_err0;
> +
> +	/* workaround SA-167 */
> +	if (input_dev->report_desc[14] == 0x25)
> +		input_dev->report_desc[14] = 0x29;
> +
> +	hid_dev = hid_allocate_device();
> +	if (IS_ERR(hid_dev)) {
> +		ret = PTR_ERR(hid_dev);
> +		goto probe_err0;
> +	}
> +
> +	hid_dev->ll_driver = &mousevsc_ll_driver;
> +	hid_dev->driver = &mousevsc_hid_driver;

You are not really hid driver; you are more of a "provider" so why do
you need to set hid_dev->driver in addition to hid_dev->ll_driver?

> +	hid_dev->bus = BUS_VIRTUAL;
> +	hid_dev->vendor = input_dev->hid_dev_info.vendor;
> +	hid_dev->product = input_dev->hid_dev_info.product;
> +	hid_dev->version = input_dev->hid_dev_info.version;
> +	input_dev->hid_device = hid_dev;
> +
> +	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");

strlcpy?

> +
> +	ret = hid_parse_report(hid_dev, input_dev->report_desc,
> +				input_dev->report_desc_size);
> +
> +	if (ret) {
> +		hid_err(hid_dev, "parse failed\n");
> +		goto probe_err1;
> +	}
> +
> +	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);

Why do you need to call hid_hw_start instead of letting HID core figure
it out for you?

> +
> +	if (ret) {
> +		hid_err(hid_dev, "hw start failed\n");
> +		goto probe_err1;
> +	}
> +
> +	input_dev->connected = true;
> +	input_dev->init_complete = true;
> +
> +	return ret;
> +
> +probe_err1:
> +	hid_destroy_device(hid_dev);
> +
> +probe_err0:
> +	vmbus_close(device->channel);
> +	free_input_device(input_dev);
> +	return ret;
> +}
> +
> +
> +static int mousevsc_remove(struct hv_device *dev)
> +{
> +	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> +
> +	vmbus_close(dev->channel);
> +
> +	if (input_dev->connected) {

Can we even get here if input_dev->connected == false?

> +		hidinput_disconnect(input_dev->hid_device);

You did not explicitly connect hidinput; leave disconnecting to the HID
core as well.

It looks like all that is needed is:

static int mousevsc_remove(struct hv_device *dev)
{
	struct mousevsc_dev *mouse = hv_get_drvdata(dev);

	vmbus_close(dev->channel);
	hid_destroy_device(mouse->hid_device);
	mousevcs_free_device(mouse);

	return 0;
}

> +		input_dev->connected = false;
> +		hid_destroy_device(input_dev->hid_device);
> +	}
> +
> +	free_input_device(input_dev);
> +
> +	return 0;
> +}
> +
> +static const struct hv_vmbus_device_id id_table[] = {
> +	/* Mouse guid */
> +	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> +		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(vmbus, id_table);
> +
> +static struct  hv_driver mousevsc_drv = {
> +	.name = KBUILD_MODNAME,
> +	.id_table = id_table,
> +	.probe = mousevsc_probe,
> +	.remove = mousevsc_remove,
> +};
> +
> +static int __init mousevsc_init(void)
> +{
> +	return vmbus_driver_register(&mousevsc_drv);
> +}
> +
> +static void __exit mousevsc_exit(void)
> +{
> +	vmbus_driver_unregister(&mousevsc_drv);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(HV_DRV_VERSION);
> +module_init(mousevsc_init);
> +module_exit(mousevsc_exit);
> -- 
> 1.7.4.1
> 

Thanks.

-- 
Dmitry

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Saturday, November 05, 2011 2:48 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> joe@perches.com; jkosina@suse.cz
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> Hi KY,

Dimitry,

Let me begin by thanking you for taking the time to review. I have incorporated
pretty much all your suggestions. These changes have cleaned up the code 
considerably. I will send the patch out that incorporates these changes shortly. I will
also send out the next version of the patch to move the mouse driver out of staging.
More details in-line.
 
> Can we call it mousevsc_alloc_device?
Done.
> 
> > +{
> > +	struct mousevsc_dev *input_dev;
> > +
> > +	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> > +
> > +	if (!input_dev)
> > +		return NULL;
> > +
> > +	input_dev->device = device;
> > +	hv_set_drvdata(device, input_dev);
> > +	init_completion(&input_dev->wait_event);
> > +
> > +	return input_dev;
> > +}
> > +
> > +static void free_input_device(struct mousevsc_dev *device)
> 
> Can we call it mousevsc_free_device?

Done.

> This could probably be:
> 
> 	static const mousevsc_prt_msg ack = {
> 		.type = PIPE_MESSAGE_DATA,
> 		.size = sizeof(struct synthhid_device_info_ack),
> 		.ack = {
> 			.header = {
> 				.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> 				.size = 1,
> 			},
> 			.reserved = 0,
> 		},
> 	};
> 

Done.

> 
> > +
> > +	/* Assume success for now */
> > +	input_device->dev_info_status = 0;
> > +
> > +	memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> > +		sizeof(struct hv_input_dev_info));
> > +
> 
> or simply:
> 
> 	input_device->hid_dev_info = device_info->hid_dev_info;

Done.

> 
> > +	desc = &device_info->hid_descriptor;
> > +	WARN_ON(desc->bLength == 0);
> 
> Should also goto cleanup as such descriptor is not usable.

Done.

> 
> > +
> > +	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> > +
> > +	if (!input_device->hid_desc)
> 
> if you do
> 
> 		input_device->dev_info_status = -ENOMEM;
> 
> you could use it later instead of defaulting to -ENOMEM.
> 
> > +		goto cleanup;
> > +
> > +	memcpy(input_device->hid_desc, desc, desc->bLength);
> > +
> > +	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> > +	if (input_device->report_desc_size == 0)
> 
> 		input_device->dev_info_status = -EINVAL;
> 
> > +		goto cleanup;
> > +	input_device->report_desc = kzalloc(input_device->report_desc_size,
> > +					  GFP_ATOMIC);
> > +
> > +	if (!input_device->report_desc)
> 
> 		input_device->dev_info_status = -ENOMEM;
> 
> > +		goto cleanup;
> > +
> > +	memcpy(input_device->report_desc,
> > +	       ((unsigned char *)desc) + desc->bLength,
> > +	       desc->desc[0].wDescriptorLength);
> > +
> > +	/* Send the ack */
> > +	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > +	ack.type = PIPE_MESSAGE_DATA;
> > +	ack.size = sizeof(struct synthhid_device_info_ack);
> > +
> > +	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> > +	ack.ack.header.size = 1;
> > +	ack.ack.reserved = 0;
> > +
> > +	ret = vmbus_sendpacket(input_device->device->channel,
> > +			&ack,
> > +			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> > +			sizeof(struct synthhid_device_info_ack),
> > +			(unsigned long)&ack,
> > +			VM_PKT_DATA_INBAND,
> > +
> 	VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +	if (ret != 0)
> 
> 		input_device->dev_info_status = ret;
> 
> > +		goto cleanup;
> > +
> > +	complete(&input_device->wait_event);
> > +
> > +	return;
> > +
> > +cleanup:
> > +	kfree(input_device->hid_desc);
> > +	input_device->hid_desc = NULL;
> > +
> > +	kfree(input_device->report_desc);
> > +	input_device->report_desc = NULL;
> 
> Do you have to clean it up here? You still need to clean it up in main
> unwind path so consolidate both error and success path and just signal
> completion and let main code figure it all out.


Done.

> > +
> > +static void mousevsc_on_channel_callback(void *context)
> > +{
> > +	const int packet_size = 0x100;
> > +	int ret;
> > +	struct hv_device *device = context;
> > +	u32 bytes_recvd;
> > +	u64 req_id;
> > +	struct vmpacket_descriptor *desc;
> > +	unsigned char	*buffer;
> > +	int	bufferlen = packet_size;
> > +
> > +	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> > +	if (!buffer)
> > +		return;
> > +
> > +	do {
> > +		ret = vmbus_recvpacket_raw(device->channel, buffer,
> > +					bufferlen, &bytes_recvd, &req_id);
> > +
> > +		switch (ret) {
> > +		case 0:
> > +			if (bytes_recvd <= 0) {
> > +				kfree(buffer);
> > +				return;
> > +			}
> > +			desc = (struct vmpacket_descriptor *)buffer;
> > +
> > +			switch (desc->type) {
> > +			case VM_PKT_COMP:
> > +				break;
> > +
> > +			case VM_PKT_DATA_INBAND:
> > +				mousevsc_on_receive(device, desc);
> > +				break;
> > +
> > +			default:
> > +				pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > +				   desc->type,
> > +				   req_id,
> > +				   bytes_recvd);
> > +				break;
> > +			}
> > +
> > +			break;
> > +
> > +		case -ENOBUFS:
> > +			kfree(buffer);
> > +			/* Handle large packet */
> > +			bufferlen = bytes_recvd;
> > +			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> 
> Instead of potentially ever-increasing buffer that you also allocate
> (and it looks like leaking on every callback invocation) can you just
> repeat the read if you know that there are more data and use single
> pre-allocated buffer?

The ring-buffer protocol is such that we need to consume the full message.
Also, why do you say we are leaking memory?

> 
> > +
> > +			if (!buffer)
> > +				return;
> > +
> > +			break;
> > +		}
> > +	} while (1);
> > +
> > +}
> > +
> > +static int mousevsc_connect_to_vsp(struct hv_device *device)
> > +{
> > +	int ret = 0;
> > +	int t;
> > +	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> > +	struct mousevsc_prt_msg *request;
> > +	struct mousevsc_prt_msg *response;
> > +
> > +	request = &input_dev->protocol_req;
> > +
> > +	memset(request, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > +	request->type = PIPE_MESSAGE_DATA;
> > +	request->size = sizeof(struct synthhid_protocol_request);
> > +
> > +	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> > +	request->request.header.size = sizeof(unsigned int);
> > +	request->request.version_requested.version =
> SYNTHHID_INPUT_VERSION;
> >
> 
> This is constant data; just do the same as with ack in
> on_receive_device_info. It does not need to be a part of input_dev, does
> it?

Done.

> 
> > +	ret = vmbus_sendpacket(device->channel, request,
> > +				sizeof(struct pipe_prt_msg) -
> > +				sizeof(unsigned char) +
> > +				sizeof(struct synthhid_protocol_request),
> > +				(unsigned long)request,
> > +				VM_PKT_DATA_INBAND,
> > +
> 	VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +	if (ret)
> > +		goto cleanup;
> > +
> > +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > +	if (!t) {
> > +		ret = -ETIMEDOUT;
> > +		goto cleanup;
> > +	}
> > +
> > +	response = &input_dev->protocol_resp;
> > +
> > +	if (!response->response.approved) {
> > +		pr_err("synthhid protocol request failed (version %d)\n",
> > +		       SYNTHHID_INPUT_VERSION);
> > +		ret = -ENODEV;
> > +		goto cleanup;
> > +	}
> > +
> > +	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> 
> 5 seconds? That's a long time of HV to respond...

Well, this is a safe number! We never wait this long.

> 
> > +	if (!t) {
> > +		ret = -ETIMEDOUT;
> > +		goto cleanup;
> > +	}
> > +
> > +	/*
> > +	 * We should have gotten the device attr, hid desc and report
> > +	 * desc at this point
> > +	 */
> > +	if (input_dev->dev_info_status)
> > +		ret = -ENOMEM;
> 
> Just do
> 
> 	ret = input_dev->dev_info_status;

Done.

> 
> unconditionally.
> 
> > +
> > +cleanup:
> > +
> > +	return ret;
> > +}
> > +
> > +static int mousevsc_hid_open(struct hid_device *hid)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int mousevsc_hid_start(struct hid_device *hid)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void mousevsc_hid_close(struct hid_device *hid)
> > +{
> > +}
> > +
> > +static void mousevsc_hid_stop(struct hid_device *hid)
> > +{
> > +}
> > +
> > +static struct hid_ll_driver mousevsc_ll_driver = {
> > +	.open = mousevsc_hid_open,
> > +	.close = mousevsc_hid_close,
> > +	.start = mousevsc_hid_start,
> > +	.stop = mousevsc_hid_stop,
> > +};
> > +
> > +static struct hid_driver mousevsc_hid_driver;
> > +
> > +static int mousevsc_probe(struct hv_device *device,
> > +			const struct hv_vmbus_device_id *dev_id)
> > +{
> > +	int ret = 0;
> > +	struct mousevsc_dev *input_dev;
> > +	struct hid_device *hid_dev;
> > +
> > +	input_dev = alloc_input_device(device);
> > +
> > +	if (!input_dev)
> > +		return -ENOMEM;
> > +
> > +	input_dev->init_complete = false;
> 
> Should this also be in alloc_input_device()?

Done.

> 
> > +
> > +	ret = vmbus_open(device->channel,
> > +		INPUTVSC_SEND_RING_BUFFER_SIZE,
> > +		INPUTVSC_RECV_RING_BUFFER_SIZE,
> > +		NULL,
> > +		0,
> > +		mousevsc_on_channel_callback,
> > +		device
> > +		);
> > +
> 
> This blank line is extra IMO.
> 
> > +	if (ret != 0) {
> > +		free_input_device(input_dev);
> > +		return ret;
> 
> Please do not mix error unwinding with gotos and unwinding in error
> handling branches.

Done.
> 
> > +	}
> > +
> > +	ret = mousevsc_connect_to_vsp(device);
> > +
> 
> This blank line is extra IMO.
> 
> > +	if (ret != 0)
> > +		goto probe_err0;
> > +
> > +	/* workaround SA-167 */
> > +	if (input_dev->report_desc[14] == 0x25)
> > +		input_dev->report_desc[14] = 0x29;
> > +
> > +	hid_dev = hid_allocate_device();
> > +	if (IS_ERR(hid_dev)) {
> > +		ret = PTR_ERR(hid_dev);
> > +		goto probe_err0;
> > +	}
> > +
> > +	hid_dev->ll_driver = &mousevsc_ll_driver;
> > +	hid_dev->driver = &mousevsc_hid_driver;
> 
> You are not really hid driver; you are more of a "provider" so why do
> you need to set hid_dev->driver in addition to hid_dev->ll_driver?
> 
True, but hid_parse_report() expects that the driver field be set; so I
need to fake this.
 
> > +	hid_dev->bus = BUS_VIRTUAL;
> > +	hid_dev->vendor = input_dev->hid_dev_info.vendor;
> > +	hid_dev->product = input_dev->hid_dev_info.product;
> > +	hid_dev->version = input_dev->hid_dev_info.version;
> > +	input_dev->hid_device = hid_dev;
> > +
> > +	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> 
> strlcpy?
> 
> > +
> > +	ret = hid_parse_report(hid_dev, input_dev->report_desc,
> > +				input_dev->report_desc_size);
> > +
> > +	if (ret) {
> > +		hid_err(hid_dev, "parse failed\n");
> > +		goto probe_err1;
> > +	}
> > +
> > +	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT |
> HID_CONNECT_HIDDEV);
> 
> Why do you need to call hid_hw_start instead of letting HID core figure
> it out for you?

I am not a hid expert; but all  hid low level drivers appear to do this.
Initially, I was directly invoking hid_connect() directly and based on your
Input, I chose to use hid_hw_start() which all other drivers are using.

> 
> > +
> > +	if (ret) {
> > +		hid_err(hid_dev, "hw start failed\n");
> > +		goto probe_err1;
> > +	}
> > +
> > +	input_dev->connected = true;
> > +	input_dev->init_complete = true;
> > +
> > +	return ret;
> > +
> > +probe_err1:
> > +	hid_destroy_device(hid_dev);
> > +
> > +probe_err0:
> > +	vmbus_close(device->channel);
> > +	free_input_device(input_dev);
> > +	return ret;
> > +}
> > +
> > +
> > +static int mousevsc_remove(struct hv_device *dev)
> > +{
> > +	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> > +
> > +	vmbus_close(dev->channel);
> > +
> > +	if (input_dev->connected) {
> 
> Can we even get here if input_dev->connected == false?
> 
> > +		hidinput_disconnect(input_dev->hid_device);
> 
> You did not explicitly connect hidinput; leave disconnecting to the HID
> core as well.
> 
> It looks like all that is needed is:
> 
> static int mousevsc_remove(struct hv_device *dev)
> {
> 	struct mousevsc_dev *mouse = hv_get_drvdata(dev);
> 
> 	vmbus_close(dev->channel);
> 	hid_destroy_device(mouse->hid_device);
> 	mousevcs_free_device(mouse);
> 
> 	return 0;
> }

Done.

> 
> > +		input_dev->connected = false;
> > +		hid_destroy_device(input_dev->hid_device);
> > +	}
> > +
> > +	free_input_device(input_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct hv_vmbus_device_id id_table[] = {
> > +	/* Mouse guid */
> > +	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> > +		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(vmbus, id_table);
> > +
> > +static struct  hv_driver mousevsc_drv = {
> > +	.name = KBUILD_MODNAME,
> > +	.id_table = id_table,
> > +	.probe = mousevsc_probe,
> > +	.remove = mousevsc_remove,
> > +};
> > +
> > +static int __init mousevsc_init(void)
> > +{
> > +	return vmbus_driver_register(&mousevsc_drv);
> > +}
> > +
> > +static void __exit mousevsc_exit(void)
> > +{
> > +	vmbus_driver_unregister(&mousevsc_drv);
> > +}
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION(HV_DRV_VERSION);
> > +module_init(mousevsc_init);
> > +module_exit(mousevsc_exit);
> > --
> > 1.7.4.1
> >
> 
> Thanks.

Thank you!

K. Y

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Dmitry Torokhov]

Hi K. Y,

On Mon, Nov 07, 2011 at 01:04:53AM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Saturday, November 05, 2011 2:48 AM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > joe@perches.com; jkosina@suse.cz
> > Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> > 
> > Hi KY,
> 
> Dimitry,
> 
> Let me begin by thanking you for taking the time to review. I have incorporated
> pretty much all your suggestions.

Thank you very much for considering my suggestions.

> > 
> > Instead of potentially ever-increasing buffer that you also allocate
> > (and it looks like leaking on every callback invocation) can you just
> > repeat the read if you know that there are more data and use single
> > pre-allocated buffer?
> 
> The ring-buffer protocol is such that we need to consume the full message.
> Also, why do you say we are leaking memory?

Ah, OK, I see, we keep reading until read returns 0-sized reply and then
we free the buffer... Never mind then.

> > > +
> > > +	hid_dev->ll_driver = &mousevsc_ll_driver;
> > > +	hid_dev->driver = &mousevsc_hid_driver;
> > 
> > You are not really hid driver; you are more of a "provider" so why do
> > you need to set hid_dev->driver in addition to hid_dev->ll_driver?
> > 
> True, but hid_parse_report() expects that the driver field be set; so I
> need to fake this.

If you supply .parse() method for your mousevsc_ll_driver structure and
call hid_parse_report() from it then HID core will set the default
driver and call parse at appropriate time.

>  
> > > +	hid_dev->bus = BUS_VIRTUAL;
> > > +	hid_dev->vendor = input_dev->hid_dev_info.vendor;
> > > +	hid_dev->product = input_dev->hid_dev_info.product;
> > > +	hid_dev->version = input_dev->hid_dev_info.version;
> > > +	input_dev->hid_device = hid_dev;
> > > +
> > > +	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> > 
> > strlcpy?
> > 
> > > +
> > > +	ret = hid_parse_report(hid_dev, input_dev->report_desc,
> > > +				input_dev->report_desc_size);
> > > +
> > > +	if (ret) {
> > > +		hid_err(hid_dev, "parse failed\n");
> > > +		goto probe_err1;
> > > +	}
> > > +
> > > +	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT |
> > HID_CONNECT_HIDDEV);
> > 
> > Why do you need to call hid_hw_start instead of letting HID core figure
> > it out for you?
> 
> I am not a hid expert; but all  hid low level drivers appear to do this.
> Initially, I was directly invoking hid_connect() directly and based on your
> Input, I chose to use hid_hw_start() which all other drivers are using.

Note that the users of hid_hw_start() actually are not low level
drivers, such as usbhid or bluetooth hidp, but higher-level drivers,
such as hid-wacom, hid-a4tech, etc. Since your driver is a low-level
driver (a provider so to speak) it should not call hid_hw_start() on its
own but rather wait for the hid code to do it.

Still, I am not a HID expert either so I'll defer to Jiri here.

Thanks.

-- 
Dmitry

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, November 07, 2011 12:51 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> joe@perches.com; jkosina@suse.cz
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> Hi K. Y,
> 
> On Mon, Nov 07, 2011 at 01:04:53AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > > Sent: Saturday, November 05, 2011 2:48 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > joe@perches.com; jkosina@suse.cz
> > > Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> > >
> > > Hi KY,
> >
> > Dimitry,
> >
> > Let me begin by thanking you for taking the time to review. I have incorporated
> > pretty much all your suggestions.
> 
> Thank you very much for considering my suggestions.

> 
> > >
> > > Instead of potentially ever-increasing buffer that you also allocate
> > > (and it looks like leaking on every callback invocation) can you just
> > > repeat the read if you know that there are more data and use single
> > > pre-allocated buffer?
> >
> > The ring-buffer protocol is such that we need to consume the full message.
> > Also, why do you say we are leaking memory?
> 
> Ah, OK, I see, we keep reading until read returns 0-sized reply and then
> we free the buffer... Never mind then.
> 
> > > > +
> > > > +	hid_dev->ll_driver = &mousevsc_ll_driver;
> > > > +	hid_dev->driver = &mousevsc_hid_driver;
> > >
> > > You are not really hid driver; you are more of a "provider" so why do
> > > you need to set hid_dev->driver in addition to hid_dev->ll_driver?
> > >
> > True, but hid_parse_report() expects that the driver field be set; so I
> > need to fake this.
> 
> If you supply .parse() method for your mousevsc_ll_driver structure and
> call hid_parse_report() from it then HID core will set the default
> driver and call parse at appropriate time.

Would it be ok if I were to make this change after Jiri accepts the driver out of staging?

> 
> >
> > > > +	hid_dev->bus = BUS_VIRTUAL;
> > > > +	hid_dev->vendor = input_dev->hid_dev_info.vendor;
> > > > +	hid_dev->product = input_dev->hid_dev_info.product;
> > > > +	hid_dev->version = input_dev->hid_dev_info.version;
> > > > +	input_dev->hid_device = hid_dev;
> > > > +
> > > > +	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> > >
> > > strlcpy?
> > >
> > > > +
> > > > +	ret = hid_parse_report(hid_dev, input_dev->report_desc,
> > > > +				input_dev->report_desc_size);
> > > > +
> > > > +	if (ret) {
> > > > +		hid_err(hid_dev, "parse failed\n");
> > > > +		goto probe_err1;
> > > > +	}
> > > > +
> > > > +	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT |
> > > HID_CONNECT_HIDDEV);
> > >
> > > Why do you need to call hid_hw_start instead of letting HID core figure
> > > it out for you?
> >
> > I am not a hid expert; but all  hid low level drivers appear to do this.
> > Initially, I was directly invoking hid_connect() directly and based on your
> > Input, I chose to use hid_hw_start() which all other drivers are using.
> 
> Note that the users of hid_hw_start() actually are not low level
> drivers, such as usbhid or bluetooth hidp, but higher-level drivers,
> such as hid-wacom, hid-a4tech, etc. Since your driver is a low-level
> driver (a provider so to speak) it should not call hid_hw_start() on its
> own but rather wait for the hid code to do it.
> 
> Still, I am not a HID expert either so I'll defer to Jiri here.

I look forward to Jiri's ruling here. On a different note, you had asked me to
make a couple of structures const structures that were statically initialized - the 
ack structure and the request structure. I am running into some issues doing that.
The low level ring buffer code uses  struct scatterlists and  for some reason the behavior 
of dynamically allocated structures (as well as stack variables) is different when it comes to
the following transformation: VA ->Page->VA. For dynamically allocated data this transformation
gives us the original VA; while for module global data, this transformation gives me a different VA.
And so, after I made the change you had recommended (making the two variables static variables),
the driver does not work and I have tracked this problem to the transformation noted earlier. Is this 
transformation not valid for static data? 

If it is ok with you, I will send the patch out with all the other cleanup that you had suggested.

Regards,

K. Y

[PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[K. Y. Srinivasan]

The file  hid-hyperv.c implements a hid compliant mouse driver for use on a
Hyper-V based system. This driver is currently in the staging area and
as part of the effort to move this driver out of staging, I had posted
the driver code for community review a few weeks ago. This current patch
addresses all the review comments I have gotten to date. All the relevant
patches have already been submitted to the staging tree as well.

As per Greg's suggestion, this patch does not get rid of the code from
the staging area. Once the mouse driver lands under the hid directory,
we will cleanup the staging directory.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hid/Kconfig      |    6 +
 drivers/hid/Makefile     |    1 +
 drivers/hid/hid-hyperv.c |  582 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 589 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hid-hyperv.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 1130a89..d77ac36 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -613,6 +613,12 @@ config HID_ZYDACRON
 	---help---
 	Support for Zydacron remote control.
 
+config HYPERV_MOUSE
+	tristate "Microsoft Hyper-V mouse driver"
+	depends on HYPERV
+	---help---
+	Select this option to enable the Hyper-V mouse driver.
+
 endmenu
 
 endif # HID_SUPPORT
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0a0a38e..e683b8c 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
 obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
 obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
 obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
+obj-$(CONFIG_HYPERV_MOUSE)	+= hid-hyperv.o
 
 obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
new file mode 100644
index 0000000..d503cbb
--- /dev/null
+++ b/drivers/hid/hid-hyperv.c
@@ -0,0 +1,582 @@
+/*
+ *  Copyright (c) 2009, Citrix Systems, Inc.
+ *  Copyright (c) 2010, Microsoft Corporation.
+ *  Copyright (c) 2011, Novell Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms and conditions of the GNU General Public License,
+ *  version 2, as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ *  more details.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/completion.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/hiddev.h>
+#include <linux/hyperv.h>
+
+
+struct hv_input_dev_info {
+	unsigned int size;
+	unsigned short vendor;
+	unsigned short product;
+	unsigned short version;
+	unsigned short reserved[11];
+};
+
+/* The maximum size of a synthetic input message. */
+#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
+
+/*
+ * Current version
+ *
+ * History:
+ * Beta, RC < 2008/1/22        1,0
+ * RC > 2008/1/22              2,0
+ */
+#define SYNTHHID_INPUT_VERSION_MAJOR	2
+#define SYNTHHID_INPUT_VERSION_MINOR	0
+#define SYNTHHID_INPUT_VERSION		(SYNTHHID_INPUT_VERSION_MINOR | \
+					 (SYNTHHID_INPUT_VERSION_MAJOR << 16))
+
+
+#pragma pack(push, 1)
+/*
+ * Message types in the synthetic input protocol
+ */
+enum synthhid_msg_type {
+	SYNTH_HID_PROTOCOL_REQUEST,
+	SYNTH_HID_PROTOCOL_RESPONSE,
+	SYNTH_HID_INITIAL_DEVICE_INFO,
+	SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
+	SYNTH_HID_INPUT_REPORT,
+	SYNTH_HID_MAX
+};
+
+/*
+ * Basic message structures.
+ */
+struct synthhid_msg_hdr {
+	enum synthhid_msg_type type;
+	u32 size;
+};
+
+struct synthhid_msg {
+	struct synthhid_msg_hdr header;
+	char data[1]; /* Enclosed message */
+};
+
+union synthhid_version {
+	struct {
+		u16 minor_version;
+		u16 major_version;
+	};
+	u32 version;
+};
+
+/*
+ * Protocol messages
+ */
+struct synthhid_protocol_request {
+	struct synthhid_msg_hdr header;
+	union synthhid_version version_requested;
+};
+
+struct synthhid_protocol_response {
+	struct synthhid_msg_hdr header;
+	union synthhid_version version_requested;
+	unsigned char approved;
+};
+
+struct synthhid_device_info {
+	struct synthhid_msg_hdr header;
+	struct hv_input_dev_info hid_dev_info;
+	struct hid_descriptor hid_descriptor;
+};
+
+struct synthhid_device_info_ack {
+	struct synthhid_msg_hdr header;
+	unsigned char reserved;
+};
+
+struct synthhid_input_report {
+	struct synthhid_msg_hdr header;
+	char buffer[1];
+};
+
+#pragma pack(pop)
+
+#define INPUTVSC_SEND_RING_BUFFER_SIZE		(10*PAGE_SIZE)
+#define INPUTVSC_RECV_RING_BUFFER_SIZE		(10*PAGE_SIZE)
+
+
+enum pipe_prot_msg_type {
+	PIPE_MESSAGE_INVALID,
+	PIPE_MESSAGE_DATA,
+	PIPE_MESSAGE_MAXIMUM
+};
+
+
+struct pipe_prt_msg {
+	enum pipe_prot_msg_type type;
+	u32 size;
+	char data[1];
+};
+
+struct  mousevsc_prt_msg {
+	enum pipe_prot_msg_type type;
+	u32 size;
+	union {
+		struct synthhid_protocol_request request;
+		struct synthhid_protocol_response response;
+		struct synthhid_device_info_ack ack;
+	};
+};
+
+/*
+ * Represents an mousevsc device
+ */
+struct mousevsc_dev {
+	struct hv_device	*device;
+	bool			init_complete;
+	bool			connected;
+	struct mousevsc_prt_msg	protocol_req;
+	struct mousevsc_prt_msg	protocol_resp;
+	/* Synchronize the request/response if needed */
+	struct completion	wait_event;
+	int			dev_info_status;
+
+	struct hid_descriptor	*hid_desc;
+	unsigned char		*report_desc;
+	u32			report_desc_size;
+	struct hv_input_dev_info hid_dev_info;
+	struct hid_device       *hid_device;
+};
+
+
+static struct mousevsc_dev *mousevsc_alloc_device(struct hv_device *device)
+{
+	struct mousevsc_dev *input_dev;
+
+	input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
+
+	if (!input_dev)
+		return NULL;
+
+	input_dev->device = device;
+	hv_set_drvdata(device, input_dev);
+	init_completion(&input_dev->wait_event);
+	input_dev->init_complete = false;
+
+	return input_dev;
+}
+
+static void mousevsc_free_device(struct mousevsc_dev *device)
+{
+	kfree(device->hid_desc);
+	kfree(device->report_desc);
+	hv_set_drvdata(device->device, NULL);
+	kfree(device);
+}
+
+static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
+				struct synthhid_device_info *device_info)
+{
+	int ret = 0;
+	struct hid_descriptor *desc;
+	struct mousevsc_prt_msg ack;
+
+	input_device->dev_info_status = -ENOMEM;
+
+	input_device->hid_dev_info = device_info->hid_dev_info;
+	desc = &device_info->hid_descriptor;
+	if (desc->bLength == 0)
+		goto cleanup;
+
+	input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
+
+	if (!input_device->hid_desc)
+		goto cleanup;
+
+	memcpy(input_device->hid_desc, desc, desc->bLength);
+
+	input_device->report_desc_size = desc->desc[0].wDescriptorLength;
+	if (input_device->report_desc_size == 0) {
+		input_device->dev_info_status = -EINVAL;
+		goto cleanup;
+	}
+
+	input_device->report_desc = kzalloc(input_device->report_desc_size,
+					  GFP_ATOMIC);
+
+	if (!input_device->report_desc) {
+		input_device->dev_info_status = -ENOMEM;
+		goto cleanup;
+	}
+
+	memcpy(input_device->report_desc,
+	       ((unsigned char *)desc) + desc->bLength,
+	       desc->desc[0].wDescriptorLength);
+
+	/* Send the ack */
+	memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
+
+	ack.type = PIPE_MESSAGE_DATA;
+	ack.size = sizeof(struct synthhid_device_info_ack);
+
+	ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
+	ack.ack.header.size = 1;
+	ack.ack.reserved = 0;
+
+	ret = vmbus_sendpacket(input_device->device->channel,
+			&ack,
+			sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
+			sizeof(struct synthhid_device_info_ack),
+			(unsigned long)&ack,
+			VM_PKT_DATA_INBAND,
+			VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+
+	if (!ret)
+		input_device->dev_info_status = 0;
+
+cleanup:
+	complete(&input_device->wait_event);
+
+	return;
+}
+
+static void mousevsc_on_receive(struct hv_device *device,
+				struct vmpacket_descriptor *packet)
+{
+	struct pipe_prt_msg *pipe_msg;
+	struct synthhid_msg *hid_msg;
+	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+	struct synthhid_input_report *input_report;
+
+	pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
+						(packet->offset8 << 3));
+
+	if (pipe_msg->type != PIPE_MESSAGE_DATA)
+		return;
+
+	hid_msg = (struct synthhid_msg *)pipe_msg->data;
+
+	switch (hid_msg->header.type) {
+	case SYNTH_HID_PROTOCOL_RESPONSE:
+		/*
+		 * While it will be impossible for us to protect against
+		 * malicious/buggy hypervisor/host, add a check here to
+		 * ensure we don't corrupt memory.
+		 */
+		if ((pipe_msg->size + sizeof(struct pipe_prt_msg)
+			- sizeof(unsigned char))
+			> sizeof(struct mousevsc_prt_msg)) {
+			WARN_ON(1);
+			break;
+		}
+
+		memcpy(&input_dev->protocol_resp, pipe_msg,
+		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
+		       sizeof(unsigned char));
+		complete(&input_dev->wait_event);
+		break;
+
+	case SYNTH_HID_INITIAL_DEVICE_INFO:
+		WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
+
+		/*
+		 * Parse out the device info into device attr,
+		 * hid desc and report desc
+		 */
+		mousevsc_on_receive_device_info(input_dev,
+			(struct synthhid_device_info *)pipe_msg->data);
+		break;
+	case SYNTH_HID_INPUT_REPORT:
+		input_report =
+			(struct synthhid_input_report *)pipe_msg->data;
+		if (!input_dev->init_complete)
+			break;
+		hid_input_report(input_dev->hid_device,
+				HID_INPUT_REPORT, input_report->buffer,
+				input_report->header.size, 1);
+		break;
+	default:
+		pr_err("unsupported hid msg type - type %d len %d",
+		       hid_msg->header.type, hid_msg->header.size);
+		break;
+	}
+
+}
+
+static void mousevsc_on_channel_callback(void *context)
+{
+	const int packet_size = 0x100;
+	int ret;
+	struct hv_device *device = context;
+	u32 bytes_recvd;
+	u64 req_id;
+	struct vmpacket_descriptor *desc;
+	unsigned char	*buffer;
+	int	bufferlen = packet_size;
+
+	buffer = kmalloc(bufferlen, GFP_ATOMIC);
+	if (!buffer)
+		return;
+
+	do {
+		ret = vmbus_recvpacket_raw(device->channel, buffer,
+					bufferlen, &bytes_recvd, &req_id);
+
+		switch (ret) {
+		case 0:
+			if (bytes_recvd <= 0) {
+				kfree(buffer);
+				return;
+			}
+			desc = (struct vmpacket_descriptor *)buffer;
+
+			switch (desc->type) {
+			case VM_PKT_COMP:
+				break;
+
+			case VM_PKT_DATA_INBAND:
+				mousevsc_on_receive(device, desc);
+				break;
+
+			default:
+				pr_err("unhandled packet type %d, tid %llx len %d\n",
+					desc->type, req_id, bytes_recvd);
+				break;
+			}
+
+			break;
+
+		case -ENOBUFS:
+			kfree(buffer);
+			/* Handle large packet */
+			bufferlen = bytes_recvd;
+			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+
+			if (!buffer)
+				return;
+
+			break;
+		}
+	} while (1);
+
+}
+
+static int mousevsc_connect_to_vsp(struct hv_device *device)
+{
+	int ret = 0;
+	int t;
+	struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+	struct mousevsc_prt_msg *request;
+	struct mousevsc_prt_msg *response;
+
+	request = &input_dev->protocol_req;
+	memset(request, 0, sizeof(struct mousevsc_prt_msg));
+
+	request->type = PIPE_MESSAGE_DATA;
+	request->size = sizeof(struct synthhid_protocol_request);
+	request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
+	request->request.header.size = sizeof(unsigned int);
+	request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
+
+	ret = vmbus_sendpacket(device->channel, request,
+				sizeof(struct pipe_prt_msg) -
+				sizeof(unsigned char) +
+				sizeof(struct synthhid_protocol_request),
+				(unsigned long)request,
+				VM_PKT_DATA_INBAND,
+				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (ret)
+		goto cleanup;
+
+	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+	if (!t) {
+		ret = -ETIMEDOUT;
+		goto cleanup;
+	}
+
+	response = &input_dev->protocol_resp;
+
+	if (!response->response.approved) {
+		pr_err("synthhid protocol request failed (version %d)\n",
+		       SYNTHHID_INPUT_VERSION);
+		ret = -ENODEV;
+		goto cleanup;
+	}
+
+	t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+	if (!t) {
+		ret = -ETIMEDOUT;
+		goto cleanup;
+	}
+
+	/*
+	 * We should have gotten the device attr, hid desc and report
+	 * desc at this point
+	 */
+	ret = input_dev->dev_info_status;
+
+cleanup:
+	return ret;
+}
+
+static int mousevsc_hid_open(struct hid_device *hid)
+{
+	return 0;
+}
+
+static int mousevsc_hid_start(struct hid_device *hid)
+{
+	return 0;
+}
+
+static void mousevsc_hid_close(struct hid_device *hid)
+{
+}
+
+static void mousevsc_hid_stop(struct hid_device *hid)
+{
+}
+
+static struct hid_ll_driver mousevsc_ll_driver = {
+	.open = mousevsc_hid_open,
+	.close = mousevsc_hid_close,
+	.start = mousevsc_hid_start,
+	.stop = mousevsc_hid_stop,
+};
+
+static struct hid_driver mousevsc_hid_driver;
+
+static int mousevsc_probe(struct hv_device *device,
+			const struct hv_vmbus_device_id *dev_id)
+{
+	int ret;
+	struct mousevsc_dev *input_dev;
+	struct hid_device *hid_dev;
+
+	input_dev = mousevsc_alloc_device(device);
+
+	if (!input_dev)
+		return -ENOMEM;
+
+	ret = vmbus_open(device->channel,
+		INPUTVSC_SEND_RING_BUFFER_SIZE,
+		INPUTVSC_RECV_RING_BUFFER_SIZE,
+		NULL,
+		0,
+		mousevsc_on_channel_callback,
+		device
+		);
+
+	if (ret)
+		goto probe_err0;
+
+	ret = mousevsc_connect_to_vsp(device);
+
+	if (ret)
+		goto probe_err1;
+
+	/* workaround SA-167 */
+	if (input_dev->report_desc[14] == 0x25)
+		input_dev->report_desc[14] = 0x29;
+
+	hid_dev = hid_allocate_device();
+	if (IS_ERR(hid_dev)) {
+		ret = PTR_ERR(hid_dev);
+		goto probe_err1;
+	}
+
+	hid_dev->ll_driver = &mousevsc_ll_driver;
+	hid_dev->driver = &mousevsc_hid_driver;
+	hid_dev->bus = BUS_VIRTUAL;
+	hid_dev->vendor = input_dev->hid_dev_info.vendor;
+	hid_dev->product = input_dev->hid_dev_info.product;
+	hid_dev->version = input_dev->hid_dev_info.version;
+	input_dev->hid_device = hid_dev;
+
+	sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
+
+	ret = hid_parse_report(hid_dev, input_dev->report_desc,
+				input_dev->report_desc_size);
+
+	if (ret) {
+		hid_err(hid_dev, "parse failed\n");
+		goto probe_err2;
+	}
+
+	ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
+
+	if (ret) {
+		hid_err(hid_dev, "hw start failed\n");
+		goto probe_err2;
+	}
+
+	input_dev->connected = true;
+	input_dev->init_complete = true;
+
+	return ret;
+
+probe_err2:
+	hid_destroy_device(hid_dev);
+
+probe_err1:
+	vmbus_close(device->channel);
+
+probe_err0:
+	mousevsc_free_device(input_dev);
+
+	return ret;
+}
+
+
+static int mousevsc_remove(struct hv_device *dev)
+{
+	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
+
+	vmbus_close(dev->channel);
+	hid_destroy_device(input_dev->hid_device);
+	mousevsc_free_device(input_dev);
+
+	return 0;
+}
+
+static const struct hv_vmbus_device_id id_table[] = {
+	/* Mouse guid */
+	{ VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
+		       0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct  hv_driver mousevsc_drv = {
+	.name = KBUILD_MODNAME,
+	.id_table = id_table,
+	.probe = mousevsc_probe,
+	.remove = mousevsc_remove,
+};
+
+static int __init mousevsc_init(void)
+{
+	return vmbus_driver_register(&mousevsc_drv);
+}
+
+static void __exit mousevsc_exit(void)
+{
+	vmbus_driver_unregister(&mousevsc_drv);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_VERSION(HV_DRV_VERSION);
+module_init(mousevsc_init);
+module_exit(mousevsc_exit);
-- 
1.7.4.1

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Jiri Kosina]

On Sun, 6 Nov 2011, Dmitry Torokhov wrote:

> > I am not a hid expert; but all  hid low level drivers appear to do this.
> > Initially, I was directly invoking hid_connect() directly and based on your
> > Input, I chose to use hid_hw_start() which all other drivers are using.
> 
> Note that the users of hid_hw_start() actually are not low level
> drivers, such as usbhid or bluetooth hidp, but higher-level drivers,
> such as hid-wacom, hid-a4tech, etc. Since your driver is a low-level
> driver (a provider so to speak) it should not call hid_hw_start() on its
> own but rather wait for the hid code to do it.
> 
> Still, I am not a HID expert either so I'll defer to Jiri here.

Hi,

my understanding is that hv driver is actually a bit special in this 
respect -- it's actually all-in-one both low-level and high-level driver. 
I take it that there is not ever going to be a different high-level driver 
using low-level hv transport (is that correct, KY?), so this might indeed 
be an acceptable layout of the driver.

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Jiri Kosina]

On Wed, 9 Nov 2011, KY Srinivasan wrote:

> If it is ok with you, I will send the patch out with all the other 
> cleanup that you had suggested.

Yes please, Dmitry has done an excellent review here (as usual). So I will 
not be merging the latest patch you have sent just now and will wait for 
respin with Dmitry's comments incorporated.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[Dmitry Torokhov]

On Sun, Nov 13, 2011 at 09:01:28PM +0100, Jiri Kosina wrote:
> On Sun, 6 Nov 2011, Dmitry Torokhov wrote:
> 
> > > I am not a hid expert; but all  hid low level drivers appear to do this.
> > > Initially, I was directly invoking hid_connect() directly and based on your
> > > Input, I chose to use hid_hw_start() which all other drivers are using.
> > 
> > Note that the users of hid_hw_start() actually are not low level
> > drivers, such as usbhid or bluetooth hidp, but higher-level drivers,
> > such as hid-wacom, hid-a4tech, etc. Since your driver is a low-level
> > driver (a provider so to speak) it should not call hid_hw_start() on its
> > own but rather wait for the hid code to do it.
> > 
> > Still, I am not a HID expert either so I'll defer to Jiri here.
> 
> Hi,
> 
> my understanding is that hv driver is actually a bit special in this 
> respect -- it's actually all-in-one both low-level and high-level driver. 
> I take it that there is not ever going to be a different high-level driver 
> using low-level hv transport (is that correct, KY?), so this might indeed 
> be an acceptable layout of the driver.
> 

I actually do not see anything of a high-level driver in hv-mouse. It is
a pure transport driver that channels everything through hid-input...

Thanks.

-- 
Dmitry

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Jiri Kosina [mailto:jkosina@suse.cz]
> Sent: Sunday, November 13, 2011 3:01 PM
> To: Dmitry Torokhov
> Cc: KY Srinivasan; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> joe@perches.com
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Sun, 6 Nov 2011, Dmitry Torokhov wrote:
> 
> > > I am not a hid expert; but all  hid low level drivers appear to do this.
> > > Initially, I was directly invoking hid_connect() directly and based on your
> > > Input, I chose to use hid_hw_start() which all other drivers are using.
> >
> > Note that the users of hid_hw_start() actually are not low level
> > drivers, such as usbhid or bluetooth hidp, but higher-level drivers,
> > such as hid-wacom, hid-a4tech, etc. Since your driver is a low-level
> > driver (a provider so to speak) it should not call hid_hw_start() on its
> > own but rather wait for the hid code to do it.
> >
> > Still, I am not a HID expert either so I'll defer to Jiri here.
> 
> Hi,
> 
> my understanding is that hv driver is actually a bit special in this
> respect -- it's actually all-in-one both low-level and high-level driver.
> I take it that there is not ever going to be a different high-level driver
> using low-level hv transport (is that correct, KY?), so this might indeed
> be an acceptable layout of the driver.

You are right Jiri. While this is a low level transport driver, there will never be a another
higher level driver using this transport.

Regards,

K. Y

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Jiri Kosina [mailto:jkosina@suse.cz]
> Sent: Sunday, November 13, 2011 3:03 PM
> To: KY Srinivasan
> Cc: Dmitry Torokhov; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> joe@perches.com
> Subject: RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Wed, 9 Nov 2011, KY Srinivasan wrote:
> 
> > If it is ok with you, I will send the patch out with all the other
> > cleanup that you had suggested.
> 
> Yes please, Dmitry has done an excellent review here (as usual). So I will
> not be merging the latest patch you have sent just now and will wait for
> respin with Dmitry's comments incorporated.

I did send this patch out (with Dmitry's comments incorporated.
If you have not received them, I could re-send the patch. Let me know.

Regards,

K. Y

RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

[KY Srinivasan]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Sunday, November 13, 2011 7:48 PM
> To: Jiri Kosina
> Cc: KY Srinivasan; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> joe@perches.com
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> 
> On Sun, Nov 13, 2011 at 09:01:28PM +0100, Jiri Kosina wrote:
> > On Sun, 6 Nov 2011, Dmitry Torokhov wrote:
> >
> > > > I am not a hid expert; but all  hid low level drivers appear to do this.
> > > > Initially, I was directly invoking hid_connect() directly and based on your
> > > > Input, I chose to use hid_hw_start() which all other drivers are using.
> > >
> > > Note that the users of hid_hw_start() actually are not low level
> > > drivers, such as usbhid or bluetooth hidp, but higher-level drivers,
> > > such as hid-wacom, hid-a4tech, etc. Since your driver is a low-level
> > > driver (a provider so to speak) it should not call hid_hw_start() on its
> > > own but rather wait for the hid code to do it.
> > >
> > > Still, I am not a HID expert either so I'll defer to Jiri here.
> >
> > Hi,
> >
> > my understanding is that hv driver is actually a bit special in this
> > respect -- it's actually all-in-one both low-level and high-level driver.
> > I take it that there is not ever going to be a different high-level driver
> > using low-level hv transport (is that correct, KY?), so this might indeed
> > be an acceptable layout of the driver.
> >
> 
> I actually do not see anything of a high-level driver in hv-mouse. It is
> a pure transport driver that channels everything through hid-input...

The split architecture (in my opinion) makes sense if there are multiple consumers
of that transport. In this case though, there is not going to be any other drivers
using this transport. As you have noted, there really is not any code in this driver that
would qualify as being the high level driver. 

Regards,

K. Y