Hi K. Y.,
On Fri, Oct 14, 2011 at 11:08:27PM -0700, K. Y. Srinivasan wrote:
> In preparation for moving the mouse driver out of staging, seek community
> review of the mouse driver code.
>
Because it is a HID driver it should go through Jiri Kosina's tree
(CCed). Still, a few comments below.
> Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> ---
> drivers/hid/Kconfig | 6 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hv_mouse.c | 599 +++++++++++++++++++++++++++++++++++++++++
> drivers/staging/hv/Kconfig | 6 -
> drivers/staging/hv/Makefile | 1 -
> drivers/staging/hv/hv_mouse.c | 599 -----------------------------------------
> 6 files changed, 606 insertions(+), 606 deletions(-)
> create mode 100644 drivers/hid/hv_mouse.c
> delete mode 100644 drivers/staging/hv/hv_mouse.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 1130a89..f8b98b8 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..ccd39c7
> --- /dev/null
> +++ b/drivers/hid/hv_mouse.c
> @@ -0,0 +1,599 @@
> +/*
> + * 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/delay.h>
> +#include <linux/device.h>
> +#include <linux/workqueue.h>
Is this really needed?
> +#include <linux/sched.h>
You probably want completion.h instead.
> +#include <linux/wait.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;
> + unsigned char 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;
> + int connected;
bool?
> + 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);
> +
This blank line is extra.
> + 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;
That looks like a constant structure...
> +
> + 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 packetSize = 0x100;
> + int ret = 0;
> + struct hv_device *device = (struct hv_device *)context;
No need to cast.
> +
> + u32 bytes_recvd;
> + u64 req_id;
> + unsigned char packet[0x100];
Hmm, 100 bytes on stack... and are we in interrupt context by any
chance?
> + struct vmpacket_descriptor *desc;
> + unsigned char *buffer = packet;
> + int bufferlen = packetSize;
> +
> +
> + do {
> + ret = vmbus_recvpacket_raw(device->channel, buffer,
> + bufferlen, &bytes_recvd, &req_id);
> +
> + if (ret == 0) {
> + if (bytes_recvd > 0) {
> + 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;
> + }
> +
> + /* reset */
> + if (bufferlen > packetSize) {
> + kfree(buffer);
> +
> + buffer = packet;
> + bufferlen = packetSize;
> + }
> + } else {
> + if (bufferlen > packetSize) {
> + kfree(buffer);
> +
> + buffer = packet;
> + bufferlen = packetSize;
> + }
> + break;
> + }
> + } else if (ret == -ENOBUFS) {
> + /* Handle large packet */
> + bufferlen = bytes_recvd;
> + buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
> +
What happens if we receive even larger amount of data after allocating
the buffer?
> + if (buffer == NULL) {
> + buffer = packet;
> + bufferlen = packetSize;
> + break;
> + }
> + }
> + } while (1);
So we can be looping indefinitely here as long as other side keeps
sending data?
> +
> + return;
No naked returns please.
> +}
> +
> +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)",
> + SYNTHHID_INPUT_VERSION);
> + ret = -ENODEV;
> + goto cleanup;
> + }
> +
> + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
We just completed the wait for this completion, why are we waiting on
the same completion again?
> + 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;
-ENOMEM seems wrong.
> +
> +cleanup:
> +
> + return ret;
> +}
> +
> +static int mousevsc_hid_open(struct hid_device *hid)
> +{
> + return 0;
> +}
> +
> +static void mousevsc_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +static struct hid_ll_driver mousevsc_ll_driver = {
> + .open = mousevsc_hid_open,
> + .close = mousevsc_hid_close,
> +};
> +
> +static struct hid_driver mousevsc_hid_driver;
> +
> +static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
> +{
This is called from mousevsc_on_device_add() which is part of device
probe. Why it is split into separate function with bizzare arguments is
beyond me.
> + struct hid_device *hid_dev;
> + struct mousevsc_dev *input_device = hv_get_drvdata(dev);
> +
> + hid_dev = hid_allocate_device();
> + if (IS_ERR(hid_dev))
> + return;
This is not very helpful.
> +
> + hid_dev->ll_driver = &mousevsc_ll_driver;
> + hid_dev->driver = &mousevsc_hid_driver;
> +
> + if (hid_parse_report(hid_dev, packet, len))
> + return;
Neither is this.
> +
> + hid_dev->bus = BUS_VIRTUAL;
> + hid_dev->vendor = input_device->hid_dev_info.vendor;
> + hid_dev->product = input_device->hid_dev_info.product;
> + hid_dev->version = input_device->hid_dev_info.version;
> +
> + sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> +
> + if (!hidinput_connect(hid_dev, 0)) {
> + hid_dev->claimed |= HID_CLAIMED_INPUT;
Why do you force hidinput claim? Let the normal rules do it.
> +
> + input_device->connected = 1;
input_device->connected = true;
> +
> + }
> +
> + input_device->hid_device = hid_dev;
This assignment is probably too late.
> +}
> +
> +static int mousevsc_on_device_add(struct hv_device *device)
The only caller of this is mousevsc_probe() so why do you have it>
> +{
> + int ret = 0;
> + struct mousevsc_dev *input_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) {
> + vmbus_close(device->channel);
> + free_input_device(input_dev);
> + return ret;
> + }
> +
> +
> + /* workaround SA-167 */
> + if (input_dev->report_desc[14] == 0x25)
> + input_dev->report_desc[14] = 0x29;
> +
> + reportdesc_callback(device, input_dev->report_desc,
> + input_dev->report_desc_size);
> +
> + input_dev->init_complete = true;
> +
> + return ret;
> +}
> +
> +static int mousevsc_probe(struct hv_device *dev,
> + const struct hv_vmbus_device_id *dev_id)
> +{
> +
> + return mousevsc_on_device_add(dev);
> +
> +}
> +
> +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 = 0;
> + hid_destroy_device(input_dev->hid_device);
hid_destroy_device() should disconnect registered handlers for you; you
do not need to do that manually.
> + }
> +
> + free_input_device(input_dev);
Calling it input device is terribly confusing to me. I'd also freed it
inline (and used gotos to unwind errors in probe()).
> +
> + 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.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
