* Re: [PATCH] VirtioConsole support.
From: Greg KH @ 2011-10-28 16:27 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: xen-devel, Benjamin Herrenschmidt, Miche Baker-Harvey,
linux-kernel, virtualization, Anton Blanchard, Amit Shah
In-Reply-To: <20111027215535.GA5671@phenom.dumpdata.com>
On Thu, Oct 27, 2011 at 05:55:35PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 27, 2011 at 02:43:22PM -0700, Miche Baker-Harvey wrote:
> > Multiple HVC console terminals enabled.
>
> Miche,
>
> You might want to flesh out the description a bit. Perhaps include
> such details as : "fixes the infinite loop that git commit XX
> caused". Perhaps include some of the serial console output.
>
> Maybe also change the title - the patch name (which is what shows up
> if you do 'git log --oneline') is 'VirtioConsole support.' which is
> not very informative.
Yes, all of these need to be resolved before I can accept this.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] VirtioConsole support.
From: Stephen Boyd @ 2011-10-28 18:19 UTC (permalink / raw)
To: Joe Perches
Cc: xen-devel, Konrad Rzeszutek Wilk, Benjamin Herrenschmidt,
Greg Kroah-Hartman, Miche Baker-Harvey, linux-kernel,
virtualization, Anton Blanchard, Amit Shah
In-Reply-To: <1319770558.2529.20.camel@Joe-Laptop>
On 10/27/11 19:55, Joe Perches wrote:
>
> @@ -845,6 +857,19 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
> kref_init(&hp->kref);
>
> INIT_WORK(&hp->tty_resize, hvc_set_winsz);
> + /*
> + * make each console its own struct console.
> + * No need to do allocation and copy under lock.
> + */
> + cp = kzalloc(sizeof(*cp), GFP_KERNEL);
> + if (!cp) {
> + kfree(hp);
> + mutex_unlock(&hvc_ports_mutex);
> + return ERR_PTR(-ENOMEM);
> + }
> + memcpy(cp, &hvc_console, sizeof(*cp));
> The kzalloc should be kmalloc as the allocated
> memory is immediately overwritten.
Even better would be kmemdup().
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Jiri Kosina @ 2011-10-28 18:47 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: Greg KH, linux-kernel, devel, virtualization, ohering,
Dmitry Torokhov
In-Reply-To: <1319645321-9770-1-git-send-email-kys@microsoft.com>
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
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-28 19:50 UTC (permalink / raw)
To: Jiri Kosina
Cc: Greg KH, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Dmitry Torokhov
In-Reply-To: <alpine.LNX.2.00.1110282026360.3541@pobox.suse.cz>
> -----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
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Greg KH @ 2011-10-28 20:03 UTC (permalink / raw)
To: KY Srinivasan
Cc: Jiri Kosina, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Dmitry Torokhov
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA531D4@TK5EX14MBXC122.redmond.corp.microsoft.com>
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
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-28 20:28 UTC (permalink / raw)
To: Greg KH
Cc: Jiri Kosina, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Dmitry Torokhov
In-Reply-To: <20111028200315.GA30918@suse.de>
> -----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
^ permalink raw reply
* [PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver
From: K. Y. Srinivasan @ 2011-10-28 22:10 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, joe,
dmitry.torokhov, jkosina
Cc: K. Y. Srinivasan
Further cleanup to move the mouse driver out of staging.
This patch-set addresses the comments I have received from Jiri, Joe and Greg.
Thank you for taking the time to review the code.
Regards,
K. Y
^ permalink raw reply
* [PATCH 1/3] Staging: hv: mousevsc: Address some style issues
From: K. Y. Srinivasan @ 2011-10-28 22:11 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, joe,
dmitry.torokhov, jkosina
Cc: K. Y. Srinivasan
In-Reply-To: <1319839847-28959-1-git-send-email-kys@microsoft.com>
Deal with some style related issues. Also get rid of an unused macro.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 16 ++++++----------
1 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index dd42411..7c7449b 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -115,7 +115,6 @@ struct synthhid_input_report {
#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,
@@ -146,6 +145,7 @@ struct mousevsc_prt_msg {
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 */
@@ -156,7 +156,6 @@ struct mousevsc_dev {
unsigned char *report_desc;
u32 report_desc_size;
struct hv_input_dev_info hid_dev_info;
- bool connected;
struct hid_device *hid_device;
};
@@ -359,9 +358,9 @@ static void mousevsc_on_channel_callback(void *context)
bufferlen = bytes_recvd;
buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
- if (buffer == NULL) {
+ if (!buffer)
return;
- }
+
break;
}
} while (1);
@@ -376,7 +375,6 @@ static int mousevsc_connect_to_vsp(struct hv_device *device)
struct mousevsc_prt_msg *request;
struct mousevsc_prt_msg *response;
-
request = &input_dev->protocol_req;
memset(request, 0, sizeof(struct mousevsc_prt_msg));
@@ -388,7 +386,6 @@ static int mousevsc_connect_to_vsp(struct hv_device *device)
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) +
@@ -396,11 +393,11 @@ static int mousevsc_connect_to_vsp(struct hv_device *device)
(unsigned long)request,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret != 0)
+ if (ret)
goto cleanup;
t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
- if (t == 0) {
+ if (!t) {
ret = -ETIMEDOUT;
goto cleanup;
}
@@ -415,7 +412,7 @@ static int mousevsc_connect_to_vsp(struct hv_device *device)
}
t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
- if (t == 0) {
+ if (!t) {
ret = -ETIMEDOUT;
goto cleanup;
}
@@ -487,7 +484,6 @@ static int mousevsc_probe(struct hv_device *device,
return ret;
}
-
ret = mousevsc_connect_to_vsp(device);
if (ret != 0)
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/3] Staging: hv: mousevsc: Add a check to prevent memory corruption
From: K. Y. Srinivasan @ 2011-10-28 22:11 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, joe,
dmitry.torokhov, jkosina
Cc: K. Y. Srinivasan
In-Reply-To: <1319839888-29000-1-git-send-email-kys@microsoft.com>
Add a check to prevent memory corruption.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 7c7449b..c22f729 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -274,6 +274,18 @@ static void mousevsc_on_receive(struct hv_device *device,
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));
--
1.7.4.1
^ permalink raw reply related
* [PATCH 3/3] Staging: hv: mousevsc: Use the KBUILD_MODNAME macro
From: K. Y. Srinivasan @ 2011-10-28 22:11 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, joe,
dmitry.torokhov, jkosina
Cc: K. Y. Srinivasan
In-Reply-To: <1319839888-29000-1-git-send-email-kys@microsoft.com>
Use the KBUILD_MODNAME macro.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index c22f729..2c2e1b4 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -578,7 +578,7 @@ static const struct hv_vmbus_device_id id_table[] = {
MODULE_DEVICE_TABLE(vmbus, id_table);
static struct hv_driver mousevsc_drv = {
- .name = "mousevsc",
+ .name = KBUILD_MODNAME,
.id_table = id_table,
.probe = mousevsc_probe,
.remove = mousevsc_remove,
--
1.7.4.1
^ permalink raw reply related
* [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: K. Y. Srinivasan @ 2011-10-28 22:35 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering, joe,
dmitry.torokhov, jkosina
Cc: 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
^ permalink raw reply related
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Jesper Juhl @ 2011-10-28 22:54 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering, joe,
dmitry.torokhov, jkosina
In-Reply-To: <1319841316-29136-1-git-send-email-kys@microsoft.com>
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.
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: KY Srinivasan @ 2011-10-28 23:10 UTC (permalink / raw)
To: James Bottomley
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, linux-scsi@vger.kernel.org, hch@infradead.org,
Haiyang Zhang
In-Reply-To: <1318865202.4794.21.camel@dabdike.int.hansenpartnership.com>
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, October 17, 2011 11:27 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> linux-scsi@vger.kernel.org; hch@infradead.org; Haiyang Zhang
> Subject: RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of
> staging
>
> On Mon, 2011-10-17 at 15:15 +0000, KY Srinivasan wrote:
>
> > As I deal with the review comments now, do you want me to get these
> > changes
> > applied first to Greg's tree before you would consider moving this
> > driver to drivers/scsi
> > directory, or could you move the driver as it is in Greg's tree under
> > staging and I could give
> > you patches against the moved driver.
>
> Either way is fine by me. The best route for this is to get the staging
> update into Linus head, then work on the actual SCSI problems. I'd like
> other SCSI people besides me to review it.
Christoph had reviewed this code sometime back and I think I have addressed the
issues he had raised. These patches have already been applied by Greg to his
staging tree.
>
> > > OK, so as I read the code, what it can't handle is fragments except at
> > > the ends. As long as everything always transfers in multiples of 4k,
> > > the problem will never occur. This means that all devices you present
> > > need to tell the OS they have 4k sectors. I *think* you can do this by
> > > setting blk_limits_io_min() in the driver ... but you'll have to test
> > > this. The minimum sector size gets set also in sd.c when we probe the
> > > disk. I think that won't override blk_limits_io_min(), but please test
> > > (we don't have any SCSI drivers which have ever used this setting).
> > >
> > > The way to test, is to read back the /sys/block/<dev>/queue/ settings
> > > when presenting a 512 byte sector disk. (And the way to activate your
> > > bounce code would be to create a filesystem with a < PAGE_SIZE block
> > > size).
>
> > In any event, I will try your suggestion though.
>
> Just to be clear: Setting the minimum I/O size is completely well
> tested: it's how we handle 4k sector drives. What hasn't been tested is
> the ability of the *driver* to enforce the minimum using
> blk_limits_io_min(). As long at that doesn't get overridden by sd when
> it sees a 512b drive, the rest of the block paths (those which actually
> enforce the minimum) have all been well tested.
>
James, I used the blk_limits_io_min() to 4k and yet I see I/O requests coming in
to the driver that need to be handled with the bounce buffers. So I am not quite
sure how to avoid having the bounce buffer handling code. This is with a file system
with 2k block size.
Other than this, I have addressed all of your other comments. I am going to send these
patches out to the staging tree. Do you want me to send out a patch moving the driver
out of staging for your review as well. Let me know.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver
From: Greg KH @ 2011-10-29 4:49 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering, joe,
dmitry.torokhov, jkosina
In-Reply-To: <1319839847-28959-1-git-send-email-kys@microsoft.com>
On Fri, Oct 28, 2011 at 03:10:47PM -0700, K. Y. Srinivasan wrote:
> Further cleanup to move the mouse driver out of staging.
> This patch-set addresses the comments I have received from Jiri, Joe and Greg.
> Thank you for taking the time to review the code.
Please note, all of this work is too late for the 3.2 merge, as it
needed to be completed weeks ago for that to happen. And, as we are in
the middle of the merge, our time allocated to reviewing new patches is
quite limited.
So you might have a lot better success in just waiting a few weeks so
that we then have the time to review and address all of these cleanup
patches and requests to move the hyperv drivers.
I know I'm not going to be accepting any such patches from anyone other
than pure-bugfixes for the next 2-3 weeks at the least.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Dan Carpenter @ 2011-10-29 6:32 UTC (permalink / raw)
To: Jesper Juhl
Cc: gregkh, jkosina, dmitry.torokhov, linux-kernel, ohering,
virtualization, joe, devel
In-Reply-To: <alpine.LNX.2.00.1110290041500.31615@swampdragon.chaosbits.net>
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
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Greg KH @ 2011-10-29 6:34 UTC (permalink / raw)
To: KY Srinivasan
Cc: Jiri Kosina, Dmitry Torokhov, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA53219@TK5EX14MBXC122.redmond.corp.microsoft.com>
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
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-29 14:09 UTC (permalink / raw)
To: Greg KH
Cc: Jiri Kosina, Dmitry Torokhov, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
devel@linuxdriverproject.org
In-Reply-To: <20111029063409.GB2207@suse.de>
> -----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
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Jiri Kosina @ 2011-10-29 17:05 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH, Dmitry Torokhov, ohering@suse.com,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA53219@TK5EX14MBXC122.redmond.corp.microsoft.com>
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
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-30 16:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Greg KH, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Dmitry Torokhov
In-Reply-To: <alpine.LNX.2.00.1110291904190.3541@pobox.suse.cz>
> -----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
^ permalink raw reply
* RE: [PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver
From: KY Srinivasan @ 2011-10-30 16:16 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, joe@perches.com, dmitry.torokhov@gmail.com,
jkosina@suse.cz
In-Reply-To: <20111029044952.GA1061@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Saturday, October 29, 2011 12:50 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; dmitry.torokhov@gmail.com; jkosina@suse.cz
> Subject: Re: [PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver
>
> On Fri, Oct 28, 2011 at 03:10:47PM -0700, K. Y. Srinivasan wrote:
> > Further cleanup to move the mouse driver out of staging.
> > This patch-set addresses the comments I have received from Jiri, Joe and Greg.
> > Thank you for taking the time to review the code.
>
> Please note, all of this work is too late for the 3.2 merge, as it
> needed to be completed weeks ago for that to happen. And, as we are in
> the middle of the merge, our time allocated to reviewing new patches is
> quite limited.
>
> So you might have a lot better success in just waiting a few weeks so
> that we then have the time to review and address all of these cleanup
> patches and requests to move the hyperv drivers.
>
> I know I'm not going to be accepting any such patches from anyone other
> than pure-bugfixes for the next 2-3 weeks at the least.
Thanks Greg. I want to be close to being "first in line" when you are ready to accept patches!
These patches really have addressed the issues Jiri, you and other found as part
of the review process. I am hoping Jiri will be able to pick up the patch to move the
driver out of staging.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-30 16:16 UTC (permalink / raw)
To: Dan Carpenter, Jesper Juhl
Cc: 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
In-Reply-To: <20111029063240.GH14881@longonot.mountain>
> -----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
^ permalink raw reply
* [PATCHv4] virtio-blk: use ida to allocate disk index
From: Michael S. Tsirkin @ 2011-10-30 19:29 UTC (permalink / raw)
To: Jens Axboe
Cc: kvm, Greg Kroah-Hartman, linux-kernel, virtualization, Tejun Heo
Based on a patch by Mark Wu <dwu@redhat.com>
Current index allocation in virtio-blk is based on a monotonically
increasing variable "index". This means we'll run out of numbers
after a while. It also could cause confusion about the disk
name in the case of hot-plugging disks.
Change virtio-blk to use ida to allocate index, instead.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Changes from Mark's versions:
use the new ida_simple_get
error handling cleanup
fix user after free
Works fine for me.
Jens, could you merge this for 3.2?
That is, unless Rusty complains shortly ...
drivers/block/virtio_blk.c | 30 ++++++++++++++++++++++++------
1 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..e7a5750 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,10 +8,13 @@
#include <linux/scatterlist.h>
#include <linux/string_helpers.h>
#include <scsi/scsi_cmnd.h>
+#include <linux/idr.h>
#define PART_BITS 4
-static int major, index;
+static int major;
+static DEFINE_IDA(vd_index_ida);
+
struct workqueue_struct *virtblk_wq;
struct virtio_blk
@@ -35,6 +38,9 @@ struct virtio_blk
/* What host tells us, plus 2 for header & tailer. */
unsigned int sg_elems;
+ /* Ida index - used to track minor number allocations. */
+ int index;
+
/* Scatterlist: can be too big for stack. */
struct scatterlist sg[/*sg_elems*/];
};
@@ -276,6 +282,11 @@ static int index_to_minor(int index)
return index << PART_BITS;
}
+static int minor_to_index(int minor)
+{
+ return minor >> PART_BITS;
+}
+
static ssize_t virtblk_serial_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -341,14 +352,17 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
{
struct virtio_blk *vblk;
struct request_queue *q;
- int err;
+ int err, index;
u64 cap;
u32 v, blk_size, sg_elems, opt_io_size;
u16 min_io_size;
u8 physical_block_exp, alignment_offset;
- if (index_to_minor(index) >= 1 << MINORBITS)
- return -ENOSPC;
+ err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
+ GFP_KERNEL);
+ if (err < 0)
+ goto out;
+ index = err;
/* We need to know how many segments before we allocate. */
err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -365,7 +379,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
sizeof(vblk->sg[0]) * sg_elems, GFP_KERNEL);
if (!vblk) {
err = -ENOMEM;
- goto out;
+ goto out_free_index;
}
INIT_LIST_HEAD(&vblk->reqs);
@@ -421,7 +435,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
vblk->disk->private_data = vblk;
vblk->disk->fops = &virtblk_fops;
vblk->disk->driverfs_dev = &vdev->dev;
- index++;
+ vblk->index = index;
/* configure queue flush support */
if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
@@ -516,6 +530,8 @@ out_free_vq:
vdev->config->del_vqs(vdev);
out_free_vblk:
kfree(vblk);
+out_free_index:
+ ida_simple_remove(&vd_index_ida, index);
out:
return err;
}
@@ -523,6 +539,7 @@ out:
static void __devexit virtblk_remove(struct virtio_device *vdev)
{
struct virtio_blk *vblk = vdev->priv;
+ int index = vblk->index;
flush_work(&vblk->config_work);
@@ -538,6 +555,7 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
mempool_destroy(vblk->pool);
vdev->config->del_vqs(vdev);
kfree(vblk);
+ ida_simple_remove(&vd_index_ida, index);
}
static const struct virtio_device_id id_table[] = {
--
1.7.5.53.gc233e
^ permalink raw reply related
* Re: [PATCHv4] virtio-blk: use ida to allocate disk index
From: Jens Axboe @ 2011-10-31 7:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, Greg Kroah-Hartman, linux-kernel, virtualization, Tejun Heo
In-Reply-To: <20111030192959.GA31143@redhat.com>
On 2011-10-30 20:29, Michael S. Tsirkin wrote:
> Based on a patch by Mark Wu <dwu@redhat.com>
>
> Current index allocation in virtio-blk is based on a monotonically
> increasing variable "index". This means we'll run out of numbers
> after a while. It also could cause confusion about the disk
> name in the case of hot-plugging disks.
> Change virtio-blk to use ida to allocate index, instead.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Changes from Mark's versions:
> use the new ida_simple_get
> error handling cleanup
> fix user after free
>
> Works fine for me.
>
> Jens, could you merge this for 3.2?
> That is, unless Rusty complains shortly ...
Yep, tentatively added.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] VirtioConsole support.
From: Amit Shah @ 2011-10-31 9:55 UTC (permalink / raw)
To: Miche Baker-Harvey
Cc: xen-devel, Konrad Rzeszutek Wilk, Benjamin Herrenschmidt,
Greg Kroah-Hartman, linux-kernel, virtualization, Anton Blanchard
In-Reply-To: <1319740793-2187-1-git-send-email-miche@google.com>
On (Thu) 27 Oct 2011 [11:39:53], Miche Baker-Harvey wrote:
> Multiple HVC console terminals enabled.
>
> Serialize device and port open and initialization. Added a mutex
> which gates the handling of control messages in virtio_console.c.
> This includes adding and removing ports, and making opened ports be
> consoles. Extended the use of the prvdata spinlock to cover some missed
> modifications to prvdata.next_vtermno.
>
> I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting
> for the driver to be ready, and for the one-time call to hvc_init(). It
> had been the case that this was sometimes being called mulitple times, and
> partially setup state was being used by the second caller of hvc_alloc().
>
> Make separate struct console* for each new port. There was a single static
> struct console* hvc_console, to be used for early printk. We aren't doing
> early printk, but more importantly, there is no code to multiplex on that
> one console. Multiple virtio_console ports were "sharing" this, which was
> disasterous since both the index and the flags for the console are stored
> there. The console struct is remembered in the hvc_struct, and it is
> deallocated when the hvc_struct is deallocated.
>
> ------------------
>
> Konrad, thanks for trying this out on Xen.
> This is working in my KVM environment, letting me start multiple
> virtio_consoles with getty's on them, but I'm really not sure how
> all the console pieces fit together yet. Feedback is welcome.
>
> Signed-off-by: Miche Baker-Harvey <miche@google.com>
> ---
> drivers/char/virtio_console.c | 22 +++++++++++++++++++---
> drivers/tty/hvc/hvc_console.c | 39 +++++++++++++++++++++++++++++++++------
> drivers/tty/hvc/hvc_console.h | 1 +
> 3 files changed, 53 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fb68b12..e819d46 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -24,6 +24,7 @@
> #include <linux/fs.h>
> #include <linux/init.h>
> #include <linux/list.h>
> +#include <linux/mutex.h>
> #include <linux/poll.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> @@ -95,6 +96,11 @@ struct console {
> u32 vtermno;
> };
>
> +/* serialize the handling of control messages, which includes
> + * the initialization of the virtio_consoles.
> + */
Comments in this file are of the type
/*
* ...
*/
(others below are fine.)
> +static DEFINE_MUTEX(virtio_console_mutex);
> +
> struct port_buffer {
> char *buf;
>
> @@ -979,8 +985,14 @@ int init_port_console(struct port *port)
> * pointers. The final argument is the output buffer size: we
> * can do any size, so we put PAGE_SIZE here.
> */
> - port->cons.vtermno = pdrvdata.next_vtermno;
> + spin_lock_irq(&pdrvdata_lock);
> + port->cons.vtermno = pdrvdata.next_vtermno++;
> + spin_unlock_irq(&pdrvdata_lock);
This needs to be decremented in case of an error below, or you just
need the locking around the existing usage.
>
> + /*
> + * xxx Use index 0 for now assuming there is no early HVC, since
> + * we don't support it.
> + */
correction: x86 doesn't have early HVC console support yet, but s390
(and likely ppc) use this today.
I think it's safe to use 0 for the early console, it's not likely we
have anything else using hvc consoles till this point.
> port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
> if (IS_ERR(port->cons.hvc)) {
> ret = PTR_ERR(port->cons.hvc);
> @@ -990,7 +1002,6 @@ int init_port_console(struct port *port)
> return ret;
> }
> spin_lock_irq(&pdrvdata_lock);
> - pdrvdata.next_vtermno++;
> list_add_tail(&port->cons.list, &pdrvdata.consoles);
> spin_unlock_irq(&pdrvdata_lock);
> port->guest_connected = true;
> @@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev,
> int err;
>
> cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
> -
unrelated, please drop.
> port = find_port_by_id(portdev, cpkt->id);
> if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
> /* No valid header at start of buffer. Drop it. */
> @@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev,
> return;
> }
>
> + /*
> + * These are rare initialization-time events that should be
> + * serialized.
> + */
> + mutex_lock(&virtio_console_mutex);
> switch (cpkt->event) {
> case VIRTIO_CONSOLE_PORT_ADD:
> if (port) {
> @@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev,
> }
> break;
> }
> + mutex_unlock(&virtio_console_mutex);
Not really rare init-time events; ports can be hot-plugged.
BTW what does serialising just these add events gain us? I think if
this is necessary, the mutex might be necessary for other operations
too.
I'll leave the review of hvc_console bits to others.
Overall, I think you need to split this patch up into 3-4 parts, one
fixing the spinlock usage around next_vtermno above, another adding
the mutex, and the others dealing with hvc_console.c.
Thanks,
Amit
^ permalink raw reply
* Re: [PATCH 0/8] virtio: console: Fixes, cleanups, stats for bytes transferred
From: Amit Shah @ 2011-10-31 10:19 UTC (permalink / raw)
To: Rusty Russell; +Cc: Juan Quintela, Virtualization List
In-Reply-To: <cover.1315985553.git.amit.shah@redhat.com>
On (Wed) 14 Sep 2011 [13:06:38], Amit Shah wrote:
> Hello,
>
> These patches are mostly cleanups (patches 1, 4, 5, 6, 7). Patches 2
> and 3 fix cases which will be exposed when S4 support is enabled.
>
> Patch 8 adds stats for bytes received, sent and discarded for each
> port. These are added for debugging data loss issues (rather,
> pointing out they don't exist). Closing a port while data transfer is
> ongoing resulted in discarding of any pending data in the vq, which is
> the expected case. Adding these stats just helps showing there's no
> data loss, but things are working as they were designed.
>
> Please apply.
Hey Rusty,
Can you push this series to Linus for 3.2? Thanks!
Amit
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox