* [PATCH 0/6] Staging: hv: mousevsc: cleanup the mouse driver
From: K. Y. Srinivasan @ 2011-10-26 0:18 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan
As part of the effort to move the mouse driver out of staging, I had posted
the code for review by the community. This patch-set addresses the comments I
received. I would like to thank Dmitry and Joe for their comments and suggestions.
Regards,
K. Y
^ permalink raw reply
* [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: Haiyang Zhang
In-Reply-To: <1319588311-9935-1-git-send-email-kys@microsoft.com>
Make some state that is boolean in nature, a boolean variable.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index ccd39c7..15aa2ce 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
*/
struct mousevsc_dev {
struct hv_device *device;
- unsigned char init_complete;
+ bool init_complete;
struct mousevsc_prt_msg protocol_req;
struct mousevsc_prt_msg protocol_resp;
/* Synchronize the request/response if needed */
@@ -159,7 +159,7 @@ struct mousevsc_dev {
unsigned char *report_desc;
u32 report_desc_size;
struct hv_input_dev_info hid_dev_info;
- int connected;
+ bool connected;
struct hid_device *hid_device;
};
@@ -487,7 +487,7 @@ static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
if (!hidinput_connect(hid_dev, 0)) {
hid_dev->claimed |= HID_CLAIMED_INPUT;
- input_device->connected = 1;
+ input_device->connected = true;
}
@@ -558,7 +558,7 @@ static int mousevsc_remove(struct hv_device *dev)
if (input_dev->connected) {
hidinput_disconnect(input_dev->hid_device);
- input_dev->connected = 0;
+ input_dev->connected = false;
hid_destroy_device(input_dev->hid_device);
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/6] Staging: hv: mousevsc: Inline the code for mousevsc_on_device_add()
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1319588392-9982-1-git-send-email-kys@microsoft.com>
Inline the code for mousevsc_on_device_add() as this only used from
the function mousevsc_probe().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 10 ++--------
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 15aa2ce..0a5676c 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -494,7 +494,8 @@ static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
input_device->hid_device = hid_dev;
}
-static int mousevsc_on_device_add(struct hv_device *device)
+static int mousevsc_probe(struct hv_device *device,
+ const struct hv_vmbus_device_id *dev_id)
{
int ret = 0;
struct mousevsc_dev *input_dev;
@@ -542,13 +543,6 @@ static int mousevsc_on_device_add(struct hv_device *device)
return ret;
}
-static int mousevsc_probe(struct hv_device *dev,
- const struct hv_vmbus_device_id *dev_id)
-{
-
- return mousevsc_on_device_add(dev);
-
-}
static int mousevsc_remove(struct hv_device *dev)
{
--
1.7.4.1
^ permalink raw reply related
* [PATCH 3/6] Staging: hv: mousevsc: Inline the code for reportdesc_callback()
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1319588392-9982-1-git-send-email-kys@microsoft.com>
Inline the code for reportdesc_callback() as this function is called from
mousevsc_probe(). As part of this, cleanup the code in reportdesc_callback().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 93 +++++++++++++++++++++++-----------------
1 files changed, 53 insertions(+), 40 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 0a5676c..ed071b8 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -451,54 +451,34 @@ 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 void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
-{
- struct hid_device *hid_dev;
- struct mousevsc_dev *input_device = hv_get_drvdata(dev);
-
- hid_dev = hid_allocate_device();
- if (IS_ERR(hid_dev))
- return;
-
- hid_dev->ll_driver = &mousevsc_ll_driver;
- hid_dev->driver = &mousevsc_hid_driver;
-
- if (hid_parse_report(hid_dev, packet, len))
- return;
-
- hid_dev->bus = BUS_VIRTUAL;
- hid_dev->vendor = input_device->hid_dev_info.vendor;
- hid_dev->product = input_device->hid_dev_info.product;
- hid_dev->version = input_device->hid_dev_info.version;
-
- sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
-
- if (!hidinput_connect(hid_dev, 0)) {
- hid_dev->claimed |= HID_CLAIMED_INPUT;
-
- input_device->connected = true;
-
- }
-
- input_device->hid_device = hid_dev;
-}
-
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);
@@ -524,23 +504,56 @@ static int mousevsc_probe(struct hv_device *device,
ret = mousevsc_connect_to_vsp(device);
- if (ret != 0) {
- vmbus_close(device->channel);
- free_input_device(input_dev);
- return ret;
- }
-
+ if (ret != 0)
+ goto probe_err0;
/* workaround SA-167 */
if (input_dev->report_desc[14] == 0x25)
input_dev->report_desc[14] = 0x29;
- reportdesc_callback(device, input_dev->report_desc,
- input_dev->report_desc_size);
+ 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;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 4/6] Staging: hv: mousevsc: Cleanup mousevsc_on_channel_callback()
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1319588392-9982-1-git-send-email-kys@microsoft.com>
Cleanup mousevsc_on_channel_callback(). This is based on the code provided
by Joe Perches <joe@perches.com>.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 85 ++++++++++++++++++-----------------------
1 files changed, 37 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index ed071b8..b10466f 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -313,73 +313,62 @@ static void mousevsc_on_receive(struct hv_device *device,
static void mousevsc_on_channel_callback(void *context)
{
- const int packetSize = 0x100;
- int ret = 0;
- struct hv_device *device = (struct hv_device *)context;
-
+ const int packet_size = 0x100;
+ int ret;
+ struct hv_device *device = context;
u32 bytes_recvd;
u64 req_id;
- unsigned char packet[0x100];
struct vmpacket_descriptor *desc;
- unsigned char *buffer = packet;
- int bufferlen = packetSize;
+ 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);
- if (ret == 0) {
- if (bytes_recvd > 0) {
- desc = (struct vmpacket_descriptor *)buffer;
-
- switch (desc->type) {
- case VM_PKT_COMP:
- break;
-
- case VM_PKT_DATA_INBAND:
- mousevsc_on_receive(
- device, desc);
- break;
-
- default:
- pr_err("unhandled packet type %d, tid %llx len %d\n",
- desc->type,
- req_id,
- bytes_recvd);
- break;
- }
-
- /* reset */
- if (bufferlen > packetSize) {
- kfree(buffer);
-
- buffer = packet;
- bufferlen = packetSize;
- }
- } else {
- if (bufferlen > packetSize) {
- kfree(buffer);
-
- buffer = packet;
- bufferlen = packetSize;
- }
+ 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;
}
- } else if (ret == -ENOBUFS) {
+
+ break;
+
+ case -ENOBUFS:
+ kfree(buffer);
/* Handle large packet */
bufferlen = bytes_recvd;
- buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
+ buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
if (buffer == NULL) {
- buffer = packet;
- bufferlen = packetSize;
- break;
+ return;
}
+ break;
}
} while (1);
- return;
}
static int mousevsc_connect_to_vsp(struct hv_device *device)
--
1.7.4.1
^ permalink raw reply related
* [PATCH 5/6] Staging: hv: mousevsc: Add a new line to a debug string
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: Haiyang Zhang
In-Reply-To: <1319588392-9982-1-git-send-email-kys@microsoft.com>
Add a new line to a debug string.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@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 b10466f..21cd050 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -411,7 +411,7 @@ static int mousevsc_connect_to_vsp(struct hv_device *device)
response = &input_dev->protocol_resp;
if (!response->response.approved) {
- pr_err("synthhid protocol request failed (version %d)",
+ pr_err("synthhid protocol request failed (version %d)\n",
SYNTHHID_INPUT_VERSION);
ret = -ENODEV;
goto cleanup;
--
1.7.4.1
^ permalink raw reply related
* [PATCH 6/6] Staging: hv: mousevsc: Get rid of unnecessary include files
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1319588392-9982-1-git-send-email-kys@microsoft.com>
Get rid of unnecessary include files.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 21cd050..dd42411 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -14,11 +14,8 @@
*/
#include <linux/init.h>
#include <linux/module.h>
-#include <linux/delay.h>
#include <linux/device.h>
-#include <linux/workqueue.h>
-#include <linux/sched.h>
-#include <linux/wait.h>
+#include <linux/completion.h>
#include <linux/input.h>
#include <linux/hid.h>
#include <linux/hiddev.h>
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
From: Greg KH @ 2011-10-26 6:07 UTC (permalink / raw)
To: Joe Perches
Cc: K. Y. Srinivasan, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, Haiyang Zhang
In-Reply-To: <1319587427.14932.7.camel@Joe-Laptop>
On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > Make some state that is boolean in nature, a boolean variable.
> []
> > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> []
> @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > */
> > struct mousevsc_dev {
> > struct hv_device *device;
> > - unsigned char init_complete;
> > + bool init_complete;
> > struct mousevsc_prt_msg protocol_req;
> > struct mousevsc_prt_msg protocol_resp;
> > /* Synchronize the request/response if needed */
> > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > unsigned char *report_desc;
> > u32 report_desc_size;
> > struct hv_input_dev_info hid_dev_info;
> > - int connected;
> > + bool connected;
> > struct hid_device *hid_device;
> > };
>
> One of the bools should be moved to be adjacent to the other.
For a mouse driver, it really doesn't matter.
^ permalink raw reply
* [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: K. Y. Srinivasan @ 2011-10-26 16:08 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
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.
As per Greg's suggestion, this patch does not get rid of the code from
the staging area. Once the mouse driver lands under the hid directory,
we will cleanup the staging directory.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/hid/Kconfig | 6 +
drivers/hid/Makefile | 1 +
drivers/hid/hv_mouse.c | 592 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 599 insertions(+), 0 deletions(-)
create mode 100644 drivers/hid/hv_mouse.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 1130a89..068dd97 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -613,6 +613,12 @@ config HID_ZYDACRON
---help---
Support for Zydacron remote control.
+config HYPERV_MOUSE
+ tristate "Microsoft Hyper-V mouse driver"
+ depends on HYPERV
+ ---help---
+ Select this option to enable the Hyper-V mouse driver.
+
endmenu
endif # HID_SUPPORT
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0a0a38e..436ac2e 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON) += hid-zydacron.o
obj-$(CONFIG_HID_WACOM) += hid-wacom.o
obj-$(CONFIG_HID_WALTOP) += hid-waltop.o
obj-$(CONFIG_HID_WIIMOTE) += hid-wiimote.o
+obj-$(CONFIG_HYPERV_MOUSE) += hv_mouse.o
obj-$(CONFIG_USB_HID) += usbhid/
obj-$(CONFIG_USB_MOUSE) += usbhid/
diff --git a/drivers/hid/hv_mouse.c b/drivers/hid/hv_mouse.c
new file mode 100644
index 0000000..dd42411
--- /dev/null
+++ b/drivers/hid/hv_mouse.c
@@ -0,0 +1,592 @@
+/*
+ * Copyright (c) 2009, Citrix Systems, Inc.
+ * Copyright (c) 2010, Microsoft Corporation.
+ * Copyright (c) 2011, Novell Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/completion.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/hiddev.h>
+#include <linux/hyperv.h>
+
+
+struct hv_input_dev_info {
+ unsigned int size;
+ unsigned short vendor;
+ unsigned short product;
+ unsigned short version;
+ unsigned short reserved[11];
+};
+
+/* The maximum size of a synthetic input message. */
+#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
+
+/*
+ * Current version
+ *
+ * History:
+ * Beta, RC < 2008/1/22 1,0
+ * RC > 2008/1/22 2,0
+ */
+#define SYNTHHID_INPUT_VERSION_MAJOR 2
+#define SYNTHHID_INPUT_VERSION_MINOR 0
+#define SYNTHHID_INPUT_VERSION (SYNTHHID_INPUT_VERSION_MINOR | \
+ (SYNTHHID_INPUT_VERSION_MAJOR << 16))
+
+
+#pragma pack(push, 1)
+/*
+ * Message types in the synthetic input protocol
+ */
+enum synthhid_msg_type {
+ SYNTH_HID_PROTOCOL_REQUEST,
+ SYNTH_HID_PROTOCOL_RESPONSE,
+ SYNTH_HID_INITIAL_DEVICE_INFO,
+ SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
+ SYNTH_HID_INPUT_REPORT,
+ SYNTH_HID_MAX
+};
+
+/*
+ * Basic message structures.
+ */
+struct synthhid_msg_hdr {
+ enum synthhid_msg_type type;
+ u32 size;
+};
+
+struct synthhid_msg {
+ struct synthhid_msg_hdr header;
+ char data[1]; /* Enclosed message */
+};
+
+union synthhid_version {
+ struct {
+ u16 minor_version;
+ u16 major_version;
+ };
+ u32 version;
+};
+
+/*
+ * Protocol messages
+ */
+struct synthhid_protocol_request {
+ struct synthhid_msg_hdr header;
+ union synthhid_version version_requested;
+};
+
+struct synthhid_protocol_response {
+ struct synthhid_msg_hdr header;
+ union synthhid_version version_requested;
+ unsigned char approved;
+};
+
+struct synthhid_device_info {
+ struct synthhid_msg_hdr header;
+ struct hv_input_dev_info hid_dev_info;
+ struct hid_descriptor hid_descriptor;
+};
+
+struct synthhid_device_info_ack {
+ struct synthhid_msg_hdr header;
+ unsigned char reserved;
+};
+
+struct synthhid_input_report {
+ struct synthhid_msg_hdr header;
+ char buffer[1];
+};
+
+#pragma pack(pop)
+
+#define INPUTVSC_SEND_RING_BUFFER_SIZE (10*PAGE_SIZE)
+#define INPUTVSC_RECV_RING_BUFFER_SIZE (10*PAGE_SIZE)
+
+#define NBITS(x) (((x)/BITS_PER_LONG)+1)
+
+enum pipe_prot_msg_type {
+ PIPE_MESSAGE_INVALID,
+ PIPE_MESSAGE_DATA,
+ PIPE_MESSAGE_MAXIMUM
+};
+
+
+struct pipe_prt_msg {
+ enum pipe_prot_msg_type type;
+ u32 size;
+ char data[1];
+};
+
+struct mousevsc_prt_msg {
+ enum pipe_prot_msg_type type;
+ u32 size;
+ union {
+ struct synthhid_protocol_request request;
+ struct synthhid_protocol_response response;
+ struct synthhid_device_info_ack ack;
+ };
+};
+
+/*
+ * Represents an mousevsc device
+ */
+struct mousevsc_dev {
+ struct hv_device *device;
+ bool init_complete;
+ struct mousevsc_prt_msg protocol_req;
+ struct mousevsc_prt_msg protocol_resp;
+ /* Synchronize the request/response if needed */
+ struct completion wait_event;
+ int dev_info_status;
+
+ struct hid_descriptor *hid_desc;
+ unsigned char *report_desc;
+ u32 report_desc_size;
+ struct hv_input_dev_info hid_dev_info;
+ bool connected;
+ struct hid_device *hid_device;
+};
+
+
+static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
+{
+ struct mousevsc_dev *input_dev;
+
+ input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
+
+ if (!input_dev)
+ return NULL;
+
+ input_dev->device = device;
+ hv_set_drvdata(device, input_dev);
+ init_completion(&input_dev->wait_event);
+
+ return input_dev;
+}
+
+static void free_input_device(struct mousevsc_dev *device)
+{
+ kfree(device->hid_desc);
+ kfree(device->report_desc);
+ hv_set_drvdata(device->device, NULL);
+ kfree(device);
+}
+
+
+static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
+ struct synthhid_device_info *device_info)
+{
+ int ret = 0;
+ struct hid_descriptor *desc;
+ struct mousevsc_prt_msg ack;
+
+ /* Assume success for now */
+ input_device->dev_info_status = 0;
+
+ memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
+ sizeof(struct hv_input_dev_info));
+
+ desc = &device_info->hid_descriptor;
+ WARN_ON(desc->bLength == 0);
+
+ input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
+
+ if (!input_device->hid_desc)
+ goto cleanup;
+
+ memcpy(input_device->hid_desc, desc, desc->bLength);
+
+ input_device->report_desc_size = desc->desc[0].wDescriptorLength;
+ if (input_device->report_desc_size == 0)
+ goto cleanup;
+ input_device->report_desc = kzalloc(input_device->report_desc_size,
+ GFP_ATOMIC);
+
+ if (!input_device->report_desc)
+ goto cleanup;
+
+ memcpy(input_device->report_desc,
+ ((unsigned char *)desc) + desc->bLength,
+ desc->desc[0].wDescriptorLength);
+
+ /* Send the ack */
+ memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
+
+ ack.type = PIPE_MESSAGE_DATA;
+ ack.size = sizeof(struct synthhid_device_info_ack);
+
+ ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
+ ack.ack.header.size = 1;
+ ack.ack.reserved = 0;
+
+ ret = vmbus_sendpacket(input_device->device->channel,
+ &ack,
+ sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
+ sizeof(struct synthhid_device_info_ack),
+ (unsigned long)&ack,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret != 0)
+ goto cleanup;
+
+ complete(&input_device->wait_event);
+
+ return;
+
+cleanup:
+ kfree(input_device->hid_desc);
+ input_device->hid_desc = NULL;
+
+ kfree(input_device->report_desc);
+ input_device->report_desc = NULL;
+
+ input_device->dev_info_status = -1;
+ complete(&input_device->wait_event);
+}
+
+static void mousevsc_on_receive(struct hv_device *device,
+ struct vmpacket_descriptor *packet)
+{
+ struct pipe_prt_msg *pipe_msg;
+ struct synthhid_msg *hid_msg;
+ struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+ struct synthhid_input_report *input_report;
+
+ pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
+ (packet->offset8 << 3));
+
+ if (pipe_msg->type != PIPE_MESSAGE_DATA)
+ return;
+
+ hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
+
+ switch (hid_msg->header.type) {
+ case SYNTH_HID_PROTOCOL_RESPONSE:
+ memcpy(&input_dev->protocol_resp, pipe_msg,
+ pipe_msg->size + sizeof(struct pipe_prt_msg) -
+ sizeof(unsigned char));
+ complete(&input_dev->wait_event);
+ break;
+
+ case SYNTH_HID_INITIAL_DEVICE_INFO:
+ WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
+
+ /*
+ * Parse out the device info into device attr,
+ * hid desc and report desc
+ */
+ mousevsc_on_receive_device_info(input_dev,
+ (struct synthhid_device_info *)&pipe_msg->data[0]);
+ break;
+ case SYNTH_HID_INPUT_REPORT:
+ input_report =
+ (struct synthhid_input_report *)&pipe_msg->data[0];
+ if (!input_dev->init_complete)
+ break;
+ hid_input_report(input_dev->hid_device,
+ HID_INPUT_REPORT, input_report->buffer,
+ input_report->header.size, 1);
+ break;
+ default:
+ pr_err("unsupported hid msg type - type %d len %d",
+ hid_msg->header.type, hid_msg->header.size);
+ break;
+ }
+
+}
+
+static void mousevsc_on_channel_callback(void *context)
+{
+ const int packet_size = 0x100;
+ int ret;
+ struct hv_device *device = context;
+ u32 bytes_recvd;
+ u64 req_id;
+ struct vmpacket_descriptor *desc;
+ unsigned char *buffer;
+ int bufferlen = packet_size;
+
+ buffer = kmalloc(bufferlen, GFP_ATOMIC);
+ if (!buffer)
+ return;
+
+ do {
+ ret = vmbus_recvpacket_raw(device->channel, buffer,
+ bufferlen, &bytes_recvd, &req_id);
+
+ switch (ret) {
+ case 0:
+ if (bytes_recvd <= 0) {
+ kfree(buffer);
+ return;
+ }
+ desc = (struct vmpacket_descriptor *)buffer;
+
+ switch (desc->type) {
+ case VM_PKT_COMP:
+ break;
+
+ case VM_PKT_DATA_INBAND:
+ mousevsc_on_receive(device, desc);
+ break;
+
+ default:
+ pr_err("unhandled packet type %d, tid %llx len %d\n",
+ desc->type,
+ req_id,
+ bytes_recvd);
+ break;
+ }
+
+ break;
+
+ case -ENOBUFS:
+ kfree(buffer);
+ /* Handle large packet */
+ bufferlen = bytes_recvd;
+ buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+
+ if (buffer == NULL) {
+ return;
+ }
+ break;
+ }
+ } while (1);
+
+}
+
+static int mousevsc_connect_to_vsp(struct hv_device *device)
+{
+ int ret = 0;
+ int t;
+ struct mousevsc_dev *input_dev = hv_get_drvdata(device);
+ struct mousevsc_prt_msg *request;
+ struct mousevsc_prt_msg *response;
+
+
+ request = &input_dev->protocol_req;
+
+ memset(request, 0, sizeof(struct mousevsc_prt_msg));
+
+ request->type = PIPE_MESSAGE_DATA;
+ request->size = sizeof(struct synthhid_protocol_request);
+
+ request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
+ request->request.header.size = sizeof(unsigned int);
+ request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
+
+
+ ret = vmbus_sendpacket(device->channel, request,
+ sizeof(struct pipe_prt_msg) -
+ sizeof(unsigned char) +
+ sizeof(struct synthhid_protocol_request),
+ (unsigned long)request,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret != 0)
+ goto cleanup;
+
+ t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+ if (t == 0) {
+ ret = -ETIMEDOUT;
+ goto cleanup;
+ }
+
+ response = &input_dev->protocol_resp;
+
+ if (!response->response.approved) {
+ pr_err("synthhid protocol request failed (version %d)\n",
+ SYNTHHID_INPUT_VERSION);
+ ret = -ENODEV;
+ goto cleanup;
+ }
+
+ t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
+ if (t == 0) {
+ ret = -ETIMEDOUT;
+ goto cleanup;
+ }
+
+ /*
+ * We should have gotten the device attr, hid desc and report
+ * desc at this point
+ */
+ if (input_dev->dev_info_status)
+ ret = -ENOMEM;
+
+cleanup:
+
+ return ret;
+}
+
+static int mousevsc_hid_open(struct hid_device *hid)
+{
+ return 0;
+}
+
+static int mousevsc_hid_start(struct hid_device *hid)
+{
+ return 0;
+}
+
+static void mousevsc_hid_close(struct hid_device *hid)
+{
+}
+
+static void mousevsc_hid_stop(struct hid_device *hid)
+{
+}
+
+static struct hid_ll_driver mousevsc_ll_driver = {
+ .open = mousevsc_hid_open,
+ .close = mousevsc_hid_close,
+ .start = mousevsc_hid_start,
+ .stop = mousevsc_hid_stop,
+};
+
+static struct hid_driver mousevsc_hid_driver;
+
+static int mousevsc_probe(struct hv_device *device,
+ const struct hv_vmbus_device_id *dev_id)
+{
+ int ret = 0;
+ struct mousevsc_dev *input_dev;
+ struct hid_device *hid_dev;
+
+ input_dev = alloc_input_device(device);
+
+ if (!input_dev)
+ return -ENOMEM;
+
+ input_dev->init_complete = false;
+
+ ret = vmbus_open(device->channel,
+ INPUTVSC_SEND_RING_BUFFER_SIZE,
+ INPUTVSC_RECV_RING_BUFFER_SIZE,
+ NULL,
+ 0,
+ mousevsc_on_channel_callback,
+ device
+ );
+
+ if (ret != 0) {
+ free_input_device(input_dev);
+ return ret;
+ }
+
+
+ ret = mousevsc_connect_to_vsp(device);
+
+ if (ret != 0)
+ goto probe_err0;
+
+ /* workaround SA-167 */
+ if (input_dev->report_desc[14] == 0x25)
+ input_dev->report_desc[14] = 0x29;
+
+ hid_dev = hid_allocate_device();
+ if (IS_ERR(hid_dev)) {
+ ret = PTR_ERR(hid_dev);
+ goto probe_err0;
+ }
+
+ hid_dev->ll_driver = &mousevsc_ll_driver;
+ hid_dev->driver = &mousevsc_hid_driver;
+ hid_dev->bus = BUS_VIRTUAL;
+ hid_dev->vendor = input_dev->hid_dev_info.vendor;
+ hid_dev->product = input_dev->hid_dev_info.product;
+ hid_dev->version = input_dev->hid_dev_info.version;
+ input_dev->hid_device = hid_dev;
+
+ sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
+
+ ret = hid_parse_report(hid_dev, input_dev->report_desc,
+ input_dev->report_desc_size);
+
+ if (ret) {
+ hid_err(hid_dev, "parse failed\n");
+ goto probe_err1;
+ }
+
+ ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
+
+ if (ret) {
+ hid_err(hid_dev, "hw start failed\n");
+ goto probe_err1;
+ }
+
+ input_dev->connected = true;
+ input_dev->init_complete = true;
+
+ return ret;
+
+probe_err1:
+ hid_destroy_device(hid_dev);
+
+probe_err0:
+ vmbus_close(device->channel);
+ free_input_device(input_dev);
+ return ret;
+}
+
+
+static int mousevsc_remove(struct hv_device *dev)
+{
+ struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
+
+ vmbus_close(dev->channel);
+
+ if (input_dev->connected) {
+ hidinput_disconnect(input_dev->hid_device);
+ input_dev->connected = false;
+ hid_destroy_device(input_dev->hid_device);
+ }
+
+ free_input_device(input_dev);
+
+ return 0;
+}
+
+static const struct hv_vmbus_device_id id_table[] = {
+ /* Mouse guid */
+ { VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
+ 0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
+ { },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct hv_driver mousevsc_drv = {
+ .name = "mousevsc",
+ .id_table = id_table,
+ .probe = mousevsc_probe,
+ .remove = mousevsc_remove,
+};
+
+static int __init mousevsc_init(void)
+{
+ return vmbus_driver_register(&mousevsc_drv);
+}
+
+static void __exit mousevsc_exit(void)
+{
+ vmbus_driver_unregister(&mousevsc_drv);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_VERSION(HV_DRV_VERSION);
+module_init(mousevsc_init);
+module_exit(mousevsc_exit);
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
From: Joe Perches @ 2011-10-26 22:46 UTC (permalink / raw)
To: Greg KH
Cc: Haiyang Zhang, dmitry.torokhov, ohering, linux-kernel,
virtualization, devel
In-Reply-To: <20111026060742.GA19973@suse.de>
On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > Make some state that is boolean in nature, a boolean variable.
> > []
> > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > []
> > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > */
> > > struct mousevsc_dev {
> > > struct hv_device *device;
> > > - unsigned char init_complete;
> > > + bool init_complete;
> > > struct mousevsc_prt_msg protocol_req;
> > > struct mousevsc_prt_msg protocol_resp;
> > > /* Synchronize the request/response if needed */
> > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > unsigned char *report_desc;
> > > u32 report_desc_size;
> > > struct hv_input_dev_info hid_dev_info;
> > > - int connected;
> > > + bool connected;
> > > struct hid_device *hid_device;
> > > };
> >
> > One of the bools should be moved to be adjacent to the other.
> For a mouse driver, it really doesn't matter.
'course it doesn't matter, just like whitespace
doesn't matter. It's just done for form's sake.
^ permalink raw reply
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
From: Greg KH @ 2011-10-26 23:50 UTC (permalink / raw)
To: Joe Perches
Cc: Haiyang Zhang, dmitry.torokhov, ohering, linux-kernel,
virtualization, devel
In-Reply-To: <1319669211.21924.16.camel@Joe-Laptop>
On Wed, Oct 26, 2011 at 03:46:51PM -0700, Joe Perches wrote:
> On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> > On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > > Make some state that is boolean in nature, a boolean variable.
> > > []
> > > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > > []
> > > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > > */
> > > > struct mousevsc_dev {
> > > > struct hv_device *device;
> > > > - unsigned char init_complete;
> > > > + bool init_complete;
> > > > struct mousevsc_prt_msg protocol_req;
> > > > struct mousevsc_prt_msg protocol_resp;
> > > > /* Synchronize the request/response if needed */
> > > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > > unsigned char *report_desc;
> > > > u32 report_desc_size;
> > > > struct hv_input_dev_info hid_dev_info;
> > > > - int connected;
> > > > + bool connected;
> > > > struct hid_device *hid_device;
> > > > };
> > >
> > > One of the bools should be moved to be adjacent to the other.
> > For a mouse driver, it really doesn't matter.
>
> 'course it doesn't matter, just like whitespace
> doesn't matter. It's just done for form's sake.
Not at all.
Whitespace does matter, there is documented proof of that.
The fact that you want to move a bool to a different part of a structure
really is only your personal choice. It doesn't matter for size or
layout to make this code work better at all to do this.
This is your personal opinion only, please don't construe it as anything
else.
greg k-h
>
^ permalink raw reply
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
From: Joe Perches @ 2011-10-26 23:59 UTC (permalink / raw)
To: Greg KH
Cc: Haiyang Zhang, dmitry.torokhov, ohering, linux-kernel,
virtualization, devel
In-Reply-To: <20111026235054.GA31883@suse.de>
On Thu, 2011-10-27 at 01:50 +0200, Greg KH wrote:
> On Wed, Oct 26, 2011 at 03:46:51PM -0700, Joe Perches wrote:
> > On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> > > On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > > > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > > > Make some state that is boolean in nature, a boolean variable.
> > > > []
> > > > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > > > []
> > > > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > > > */
> > > > > struct mousevsc_dev {
> > > > > struct hv_device *device;
> > > > > - unsigned char init_complete;
> > > > > + bool init_complete;
> > > > > struct mousevsc_prt_msg protocol_req;
> > > > > struct mousevsc_prt_msg protocol_resp;
> > > > > /* Synchronize the request/response if needed */
> > > > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > > > unsigned char *report_desc;
> > > > > u32 report_desc_size;
> > > > > struct hv_input_dev_info hid_dev_info;
> > > > > - int connected;
> > > > > + bool connected;
> > > > > struct hid_device *hid_device;
> > > > > };
> > > > One of the bools should be moved to be adjacent to the other.
> > > For a mouse driver, it really doesn't matter.
> > 'course it doesn't matter, just like whitespace
> > doesn't matter. It's just done for form's sake.
> Not at all.
> Whitespace does matter, there is documented proof of that.
> The fact that you want to move a bool to a different part of a structure
> really is only your personal choice. It doesn't matter for size or
> layout to make this code work better at all to do this.
Just as what you suggest is your opinion.
Whether or not 4 or 8 extra/wasted bytes in
a struct is important or not is arguable.
I think disaggregating bools is akin to
disaggregating bitfields.
Not necessarily wrong, just odd.
cheers, Joe
^ permalink raw reply
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
From: Dmitry Torokhov @ 2011-10-27 0:03 UTC (permalink / raw)
To: Greg KH
Cc: Joe Perches, Haiyang Zhang, ohering, linux-kernel, virtualization,
devel
In-Reply-To: <20111026235054.GA31883@suse.de>
On Thu, Oct 27, 2011 at 01:50:54AM +0200, Greg KH wrote:
> On Wed, Oct 26, 2011 at 03:46:51PM -0700, Joe Perches wrote:
> > On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> > > On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > > > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > > > Make some state that is boolean in nature, a boolean variable.
> > > > []
> > > > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > > > []
> > > > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > > > */
> > > > > struct mousevsc_dev {
> > > > > struct hv_device *device;
> > > > > - unsigned char init_complete;
> > > > > + bool init_complete;
> > > > > struct mousevsc_prt_msg protocol_req;
> > > > > struct mousevsc_prt_msg protocol_resp;
> > > > > /* Synchronize the request/response if needed */
> > > > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > > > unsigned char *report_desc;
> > > > > u32 report_desc_size;
> > > > > struct hv_input_dev_info hid_dev_info;
> > > > > - int connected;
> > > > > + bool connected;
> > > > > struct hid_device *hid_device;
> > > > > };
> > > >
> > > > One of the bools should be moved to be adjacent to the other.
> > > For a mouse driver, it really doesn't matter.
> >
> > 'course it doesn't matter, just like whitespace
> > doesn't matter. It's just done for form's sake.
>
> Not at all.
>
> Whitespace does matter, there is documented proof of that.
>
> The fact that you want to move a bool to a different part of a structure
> really is only your personal choice. It doesn't matter for size or
> layout to make this code work better at all to do this.
It is a good practice to try minimizing footprint though. While it might
not matter much in this particular driver we should still try to produce
best code we can as it is often taken as source for new drivers.
>
> This is your personal opinion only, please don't construe it as anything
> else.
Same could be said about the email I am replying to.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: Dmitry Torokhov @ 2011-10-27 0:09 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
linux-input@vger.kernel.org, Haiyang Zhang, Jiri Kosina
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA3369B@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Sun, Oct 23, 2011 at 03:45:14PM +0000, KY Srinivasan wrote:
> > > +
> > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > > + if (t == 0) {
> > > + ret = -ETIMEDOUT;
> > > + goto cleanup;
> > > + }
> > > +
> > > + response = &input_dev->protocol_resp;
> > > +
> > > + if (!response->response.approved) {
> > > + pr_err("synthhid protocol request failed (version %d)",
> > > + SYNTHHID_INPUT_VERSION);
> > > + ret = -ENODEV;
> > > + goto cleanup;
> > > + }
> > > +
> > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> >
> > We just completed the wait for this completion, why are we waiting on
> > the same completion again?
>
> In response to our initial query, we expect the host to respond back with two
> distinct pieces of information; we wait for both these responses.
I think you misunderstand how completion works in Linux. IIRC about
Windows events they are different ;) You can not signal completion
several times and then expect to wait corrsponding number of times. Once
you signal completion is it, well, complete.
>
> >
> > > + if (t == 0) {
> > > + ret = -ETIMEDOUT;
> > > + goto cleanup;
> > > + }
> > > +
> > > + /*
> > > + * We should have gotten the device attr, hid desc and report
> > > + * desc at this point
> > > + */
> > > + if (input_dev->dev_info_status)
> > > + ret = -ENOMEM;
> >
> > -ENOMEM seems wrong.
> >
> There are many failures here and not being able to allocate memory is the
> primary one; and so I chose to capture that.
Any chance that these failures have their own exit paths?
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-27 1:19 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
linux-input@vger.kernel.org, Haiyang Zhang, Jiri Kosina
In-Reply-To: <20111027000926.GB6804@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Wednesday, October 26, 2011 8:09 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; linux-
> input@vger.kernel.org; Haiyang Zhang; Jiri Kosina
> Subject: Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of
> staging
>
> On Sun, Oct 23, 2011 at 03:45:14PM +0000, KY Srinivasan wrote:
> > > > +
> > > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > > > + if (t == 0) {
> > > > + ret = -ETIMEDOUT;
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + response = &input_dev->protocol_resp;
> > > > +
> > > > + if (!response->response.approved) {
> > > > + pr_err("synthhid protocol request failed (version %d)",
> > > > + SYNTHHID_INPUT_VERSION);
> > > > + ret = -ENODEV;
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > >
> > > We just completed the wait for this completion, why are we waiting on
> > > the same completion again?
> >
> > In response to our initial query, we expect the host to respond back with two
> > distinct pieces of information; we wait for both these responses.
>
> I think you misunderstand how completion works in Linux. IIRC about
> Windows events they are different ;) You can not signal completion
> several times and then expect to wait corrsponding number of times. Once
> you signal completion is it, well, complete.
Looking at the code for complete(), it looks like the "done" state is incremented
each time complete() is invoked and the code for do_wait_for_common() decrements the
done state each time it is invoked (if the completion is properly signaled and we are not dealing
with a timeout. So, what am I missing here.
>
> >
> > >
> > > > + if (t == 0) {
> > > > + ret = -ETIMEDOUT;
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + /*
> > > > + * We should have gotten the device attr, hid desc and report
> > > > + * desc at this point
> > > > + */
> > > > + if (input_dev->dev_info_status)
> > > > + ret = -ENOMEM;
> > >
> > > -ENOMEM seems wrong.
> > >
> > There are many failures here and not being able to allocate memory is the
> > primary one; and so I chose to capture that.
>
> Any chance that these failures have their own exit paths?
This is called from the probe functions and these errors are percolated up. On a different
note, I cleaned up this code based on the feedback I got from you. The patches have been
sent out.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: Dmitry Torokhov @ 2011-10-27 4:31 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
linux-input@vger.kernel.org, Haiyang Zhang, Jiri Kosina
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA51BAB@TK5EX14MBXC122.redmond.corp.microsoft.com>
On Thu, Oct 27, 2011 at 01:19:50AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Wednesday, October 26, 2011 8:09 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; linux-
> > input@vger.kernel.org; Haiyang Zhang; Jiri Kosina
> > Subject: Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of
> > staging
> >
> > On Sun, Oct 23, 2011 at 03:45:14PM +0000, KY Srinivasan wrote:
> > > > > +
> > > > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > > > > + if (t == 0) {
> > > > > + ret = -ETIMEDOUT;
> > > > > + goto cleanup;
> > > > > + }
> > > > > +
> > > > > + response = &input_dev->protocol_resp;
> > > > > +
> > > > > + if (!response->response.approved) {
> > > > > + pr_err("synthhid protocol request failed (version %d)",
> > > > > + SYNTHHID_INPUT_VERSION);
> > > > > + ret = -ENODEV;
> > > > > + goto cleanup;
> > > > > + }
> > > > > +
> > > > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > > >
> > > > We just completed the wait for this completion, why are we waiting on
> > > > the same completion again?
> > >
> > > In response to our initial query, we expect the host to respond back with two
> > > distinct pieces of information; we wait for both these responses.
> >
> > I think you misunderstand how completion works in Linux. IIRC about
> > Windows events they are different ;) You can not signal completion
> > several times and then expect to wait corrsponding number of times. Once
> > you signal completion is it, well, complete.
>
> Looking at the code for complete(), it looks like the "done" state is incremented
> each time complete() is invoked and the code for do_wait_for_common() decrements the
> done state each time it is invoked (if the completion is properly signaled and we are not dealing
> with a timeout. So, what am I missing here.
Hmm, you are right. I am not sure why I thought that completion has to
be re-initialized before it can be reused... I guess this is true only
if one uses complete_all().
--
Dmitry
^ permalink raw reply
* Re: [PATCH RFC V2 3/5] kvm hypervisor : Add two hypercalls to support pv-ticketlock
From: Avi Kivity @ 2011-10-27 10:17 UTC (permalink / raw)
To: Raghavendra K T
Cc: Raghavendra K T, KVM, Peter Zijlstra, Srivatsa Vaddagiri,
H. Peter Anvin, Jeremy Fitzhardinge, Dave Jiang, x86, Ingo Molnar,
Rik van Riel, Stefano Stabellini, Xen, Sedat Dilek,
Thomas Gleixner, Virtualization, Yinghai Lu,
Konrad Rzeszutek Wilk, Greg Kroah-Hartman, LKML, Suzuki Poulose
In-Reply-To: <4EA85A9D.5060203@linux.vnet.ibm.com>
On 10/26/2011 09:08 PM, Raghavendra K T wrote:
> On 10/26/2011 04:04 PM, Avi Kivity wrote:
>> On 10/25/2011 08:24 PM, Raghavendra K T wrote:
> CCing Ryan also
>>>
>>> So then do also you foresee the need for directed yield at some point,
>>> to address LHP? provided we have good improvements to prove.
>>
>> Doesn't this patchset completely eliminate lock holder preemption?
>>
> Basically I was curious whether we can do more better with your
> directed yield discussions in https://lkml.org/lkml/2010/8/2/106 .
>
> I felt we can get little more improvement with doing directed yield to
> lock-holder in case of LHP than sleeping. But I may be wrong.
>
> So wanted to get the feedback, on whether I am thinking in right
> direction.
i guess donating some time to the lock holder could help, but not by
much. The problem with non-pv spinlocks is that you can't just sleep,
since no one will wake you up, so you have to actively boost the lock
holder.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Olaf Hering @ 2011-10-27 10:37 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, jkosina
In-Reply-To: <1319645321-9770-1-git-send-email-kys@microsoft.com>
On Wed, Oct 26, K. Y. Srinivasan wrote:
> +static struct hv_driver mousevsc_drv = {
> + .name = "mousevsc",
This should be KBUILD_MODNAME.
Olaf
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-27 13:25 UTC (permalink / raw)
To: Olaf Hering
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, dmitry.torokhov@gmail.com, jkosina@suse.cz
In-Reply-To: <20111027103714.GA13732@aepfle.de>
> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Thursday, October 27, 2011 6:37 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> dmitry.torokhov@gmail.com; jkosina@suse.cz
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
>
> On Wed, Oct 26, K. Y. Srinivasan wrote:
>
> > +static struct hv_driver mousevsc_drv = {
> > + .name = "mousevsc",
>
> This should be KBUILD_MODNAME.
Thanks Olaf. Once I get some more comments (especially from Jiri), I will re-spin this patch
addressing all the comments I have gotten to date.
Regards,
K. Y
^ permalink raw reply
* [PATCH] VirtioConsole support.
From: Miche Baker-Harvey @ 2011-10-27 18:39 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: xen-devel, Benjamin Herrenschmidt, Greg Kroah-Hartman,
Miche Baker-Harvey, linux-kernel, virtualization, Anton Blanchard,
Amit Shah
In-Reply-To: <20111027173527.GA23839@phenom.dumpdata.com>
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.
+ */
+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);
+ /*
+ * xxx Use index 0 for now assuming there is no early HVC, since
+ * we don't support it.
+ */
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);
-
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);
}
static void control_work_handler(struct work_struct *work)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 7430bc3..03ff6ed 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -29,8 +29,9 @@
#include <linux/kernel.h>
#include <linux/kthread.h>
#include <linux/list.h>
-#include <linux/module.h>
#include <linux/major.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/sysrq.h>
#include <linux/tty.h>
#include <linux/tty_flip.h>
@@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
* list traversal.
*/
static DEFINE_SPINLOCK(hvc_structs_lock);
+/*
+ * only one task does allocation at a time.
+ */
+static DEFINE_MUTEX(hvc_ports_mutex);
/*
* This value is used to assign a tty->index value to a hvc_struct based
@@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
spin_unlock(&hvc_structs_lock);
+ kfree(hp->hvc_console);
kfree(hp);
}
@@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
int outbuf_size)
{
struct hvc_struct *hp;
+ struct console *cp;
int i;
/* We wait until a driver actually comes along */
+ mutex_lock(&hvc_ports_mutex);
if (!hvc_driver) {
int err = hvc_init();
- if (err)
+ if (err) {
+ mutex_unlock(&hvc_ports_mutex);
return ERR_PTR(err);
+ }
}
hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
GFP_KERNEL);
- if (!hp)
+ if (!hp) {
+ mutex_unlock(&hvc_ports_mutex);
return ERR_PTR(-ENOMEM);
+ }
hp->vtermno = vtermno;
hp->data = data;
@@ -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));
+ hp->hvc_console = cp;
+
spin_lock_init(&hp->lock);
spin_lock(&hvc_structs_lock);
@@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
i = ++last_hvc;
hp->index = i;
- hvc_console.index = i;
+ cp->index = i;
vtermnos[i] = vtermno;
cons_ops[i] = ops;
list_add_tail(&(hp->next), &hvc_structs);
spin_unlock(&hvc_structs_lock);
- register_console(&hvc_console);
+ register_console(cp);
+ mutex_unlock(&hvc_ports_mutex);
return hp;
}
@@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
unsigned long flags;
struct tty_struct *tty;
- unregister_console(&hvc_console);
+ BUG_ON(!hp->hvc_console);
+ unregister_console(hp->hvc_console);
spin_lock_irqsave(&hp->lock, flags);
tty = tty_kref_get(hp->tty);
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index c335a14..2d20ab7 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -58,6 +58,7 @@ struct hvc_struct {
const struct hv_ops *ops;
int irq_requested;
int data;
+ struct console *hvc_console;
struct winsize ws;
struct work_struct tty_resize;
struct list_head next;
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH] VirtioConsole support.
From: Konrad Rzeszutek Wilk @ 2011-10-27 21:15 UTC (permalink / raw)
To: Miche Baker-Harvey
Cc: xen-devel, Benjamin Herrenschmidt, Greg Kroah-Hartman,
linux-kernel, virtualization, Anton Blanchard, Amit Shah
In-Reply-To: <1319740793-2187-1-git-send-email-miche@google.com>
On Thu, Oct 27, 2011 at 11:39:53AM -0700, 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.
And you can stick
Reported-by-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 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.
> + */
> +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);
>
> + /*
> + * xxx Use index 0 for now assuming there is no early HVC, since
> + * we don't support it.
> + */
> 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);
> -
> 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);
> }
>
> static void control_work_handler(struct work_struct *work)
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 7430bc3..03ff6ed 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -29,8 +29,9 @@
> #include <linux/kernel.h>
> #include <linux/kthread.h>
> #include <linux/list.h>
> -#include <linux/module.h>
> #include <linux/major.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/sysrq.h>
> #include <linux/tty.h>
> #include <linux/tty_flip.h>
> @@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
> * list traversal.
> */
> static DEFINE_SPINLOCK(hvc_structs_lock);
> +/*
> + * only one task does allocation at a time.
> + */
> +static DEFINE_MUTEX(hvc_ports_mutex);
>
> /*
> * This value is used to assign a tty->index value to a hvc_struct based
> @@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
>
> spin_unlock(&hvc_structs_lock);
>
> + kfree(hp->hvc_console);
> kfree(hp);
> }
>
> @@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
> int outbuf_size)
> {
> struct hvc_struct *hp;
> + struct console *cp;
> int i;
>
> /* We wait until a driver actually comes along */
> + mutex_lock(&hvc_ports_mutex);
> if (!hvc_driver) {
> int err = hvc_init();
> - if (err)
> + if (err) {
> + mutex_unlock(&hvc_ports_mutex);
> return ERR_PTR(err);
> + }
> }
>
> hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
> GFP_KERNEL);
> - if (!hp)
> + if (!hp) {
> + mutex_unlock(&hvc_ports_mutex);
> return ERR_PTR(-ENOMEM);
> + }
>
> hp->vtermno = vtermno;
> hp->data = data;
> @@ -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));
> + hp->hvc_console = cp;
> +
> spin_lock_init(&hp->lock);
> spin_lock(&hvc_structs_lock);
>
> @@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
> i = ++last_hvc;
>
> hp->index = i;
> - hvc_console.index = i;
> + cp->index = i;
> vtermnos[i] = vtermno;
> cons_ops[i] = ops;
>
> list_add_tail(&(hp->next), &hvc_structs);
> spin_unlock(&hvc_structs_lock);
> - register_console(&hvc_console);
> + register_console(cp);
> + mutex_unlock(&hvc_ports_mutex);
>
> return hp;
> }
> @@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
> unsigned long flags;
> struct tty_struct *tty;
>
> - unregister_console(&hvc_console);
> + BUG_ON(!hp->hvc_console);
> + unregister_console(hp->hvc_console);
> spin_lock_irqsave(&hp->lock, flags);
> tty = tty_kref_get(hp->tty);
>
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index c335a14..2d20ab7 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -58,6 +58,7 @@ struct hvc_struct {
> const struct hv_ops *ops;
> int irq_requested;
> int data;
> + struct console *hvc_console;
> struct winsize ws;
> struct work_struct tty_resize;
> struct list_head next;
> --
> 1.7.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* [PATCH] VirtioConsole support.
From: Miche Baker-Harvey @ 2011-10-27 21:43 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: xen-devel, Benjamin Herrenschmidt, Greg Kroah-Hartman,
Miche Baker-Harvey, linux-kernel, virtualization, Anton Blanchard,
Amit Shah
In-Reply-To: <20111027173527.GA23839@phenom.dumpdata.com>
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.
Signed-off-by: Miche Baker-Harvey <miche@google.com>
Reported-by-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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.
+ */
+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);
+ /*
+ * xxx Use index 0 for now assuming there is no early HVC, since
+ * we don't support it.
+ */
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);
-
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);
}
static void control_work_handler(struct work_struct *work)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 7430bc3..03ff6ed 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -29,8 +29,9 @@
#include <linux/kernel.h>
#include <linux/kthread.h>
#include <linux/list.h>
-#include <linux/module.h>
#include <linux/major.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/sysrq.h>
#include <linux/tty.h>
#include <linux/tty_flip.h>
@@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
* list traversal.
*/
static DEFINE_SPINLOCK(hvc_structs_lock);
+/*
+ * only one task does allocation at a time.
+ */
+static DEFINE_MUTEX(hvc_ports_mutex);
/*
* This value is used to assign a tty->index value to a hvc_struct based
@@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
spin_unlock(&hvc_structs_lock);
+ kfree(hp->hvc_console);
kfree(hp);
}
@@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
int outbuf_size)
{
struct hvc_struct *hp;
+ struct console *cp;
int i;
/* We wait until a driver actually comes along */
+ mutex_lock(&hvc_ports_mutex);
if (!hvc_driver) {
int err = hvc_init();
- if (err)
+ if (err) {
+ mutex_unlock(&hvc_ports_mutex);
return ERR_PTR(err);
+ }
}
hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
GFP_KERNEL);
- if (!hp)
+ if (!hp) {
+ mutex_unlock(&hvc_ports_mutex);
return ERR_PTR(-ENOMEM);
+ }
hp->vtermno = vtermno;
hp->data = data;
@@ -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));
+ hp->hvc_console = cp;
+
spin_lock_init(&hp->lock);
spin_lock(&hvc_structs_lock);
@@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
i = ++last_hvc;
hp->index = i;
- hvc_console.index = i;
+ cp->index = i;
vtermnos[i] = vtermno;
cons_ops[i] = ops;
list_add_tail(&(hp->next), &hvc_structs);
spin_unlock(&hvc_structs_lock);
- register_console(&hvc_console);
+ register_console(cp);
+ mutex_unlock(&hvc_ports_mutex);
return hp;
}
@@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
unsigned long flags;
struct tty_struct *tty;
- unregister_console(&hvc_console);
+ BUG_ON(!hp->hvc_console);
+ unregister_console(hp->hvc_console);
spin_lock_irqsave(&hp->lock, flags);
tty = tty_kref_get(hp->tty);
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index c335a14..2d20ab7 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -58,6 +58,7 @@ struct hvc_struct {
const struct hv_ops *ops;
int irq_requested;
int data;
+ struct console *hvc_console;
struct winsize ws;
struct work_struct tty_resize;
struct list_head next;
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH] VirtioConsole support.
From: Konrad Rzeszutek Wilk @ 2011-10-27 21:55 UTC (permalink / raw)
To: Miche Baker-Harvey
Cc: xen-devel, Benjamin Herrenschmidt, Greg Kroah-Hartman,
linux-kernel, virtualization, Anton Blanchard, Amit Shah
In-Reply-To: <1319751802-27013-1-git-send-email-miche@google.com>
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.
Oh, and make sure to have the maintainers of the drivers/[tty|char]
in the 'To' field.
Cheers!
Konrad
>
> 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.
>
> Signed-off-by: Miche Baker-Harvey <miche@google.com>
> Reported-by-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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.
> + */
> +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);
>
> + /*
> + * xxx Use index 0 for now assuming there is no early HVC, since
> + * we don't support it.
> + */
> 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);
> -
> 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);
> }
>
> static void control_work_handler(struct work_struct *work)
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 7430bc3..03ff6ed 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -29,8 +29,9 @@
> #include <linux/kernel.h>
> #include <linux/kthread.h>
> #include <linux/list.h>
> -#include <linux/module.h>
> #include <linux/major.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/sysrq.h>
> #include <linux/tty.h>
> #include <linux/tty_flip.h>
> @@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
> * list traversal.
> */
> static DEFINE_SPINLOCK(hvc_structs_lock);
> +/*
> + * only one task does allocation at a time.
> + */
> +static DEFINE_MUTEX(hvc_ports_mutex);
>
> /*
> * This value is used to assign a tty->index value to a hvc_struct based
> @@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
>
> spin_unlock(&hvc_structs_lock);
>
> + kfree(hp->hvc_console);
> kfree(hp);
> }
>
> @@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
> int outbuf_size)
> {
> struct hvc_struct *hp;
> + struct console *cp;
> int i;
>
> /* We wait until a driver actually comes along */
> + mutex_lock(&hvc_ports_mutex);
> if (!hvc_driver) {
> int err = hvc_init();
> - if (err)
> + if (err) {
> + mutex_unlock(&hvc_ports_mutex);
> return ERR_PTR(err);
> + }
> }
>
> hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
> GFP_KERNEL);
> - if (!hp)
> + if (!hp) {
> + mutex_unlock(&hvc_ports_mutex);
> return ERR_PTR(-ENOMEM);
> + }
>
> hp->vtermno = vtermno;
> hp->data = data;
> @@ -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));
> + hp->hvc_console = cp;
> +
> spin_lock_init(&hp->lock);
> spin_lock(&hvc_structs_lock);
>
> @@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
> i = ++last_hvc;
>
> hp->index = i;
> - hvc_console.index = i;
> + cp->index = i;
> vtermnos[i] = vtermno;
> cons_ops[i] = ops;
>
> list_add_tail(&(hp->next), &hvc_structs);
> spin_unlock(&hvc_structs_lock);
> - register_console(&hvc_console);
> + register_console(cp);
> + mutex_unlock(&hvc_ports_mutex);
>
> return hp;
> }
> @@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
> unsigned long flags;
> struct tty_struct *tty;
>
> - unregister_console(&hvc_console);
> + BUG_ON(!hp->hvc_console);
> + unregister_console(hp->hvc_console);
> spin_lock_irqsave(&hp->lock, flags);
> tty = tty_kref_get(hp->tty);
>
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index c335a14..2d20ab7 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -58,6 +58,7 @@ struct hvc_struct {
> const struct hv_ops *ops;
> int irq_requested;
> int data;
> + struct console *hvc_console;
> struct winsize ws;
> struct work_struct tty_resize;
> struct list_head next;
> --
> 1.7.3.1
^ permalink raw reply
* Re: [PATCH] VirtioConsole support.
From: Joe Perches @ 2011-10-28 2:55 UTC (permalink / raw)
To: Miche Baker-Harvey
Cc: xen-devel, Konrad Rzeszutek Wilk, Benjamin Herrenschmidt,
Greg Kroah-Hartman, linux-kernel, virtualization, Anton Blanchard,
Amit Shah
In-Reply-To: <1319751802-27013-1-git-send-email-miche@google.com>
On Thu, 2011-10-27 at 14:43 -0700, Miche Baker-Harvey wrote:
> Multiple HVC console terminals enabled.
Just a note on allocation.
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
[]
> @@ -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.
^ permalink raw reply
* 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
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