[PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver

[PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver

[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

[PATCH 1/3] Staging: hv: mousevsc: Address some style issues

[K. Y. Srinivasan]

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

[PATCH 2/3] Staging: hv: mousevsc: Add a check to prevent memory corruption

[K. Y. Srinivasan]

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

[PATCH 3/3] Staging: hv: mousevsc: Use the KBUILD_MODNAME macro

[K. Y. Srinivasan]

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

RE: [PATCH 3/3] Staging: hv: mousevsc: Use the KBUILD_MODNAME macro

[H Hartley Sweeten]

On Friday, October 28, 2011 3:11 PM, K. Y. Srinivasan wrote:
> 
> 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,

It's just my opinion, but I'm not sure this is better.

By changing the name to KBUILD_MODNAME you can no longer grep the source to
find the driver based on it's name:

$ grep -r name * | grep mousevsc
drivers/staging/hv/hv_mouse.c:  .name = "mousevsc",

Also, doesn't this change the name of the driver from "mousevsc" to "hv_mouse"?
Does anything else need modified to handle the name change or is the matching
handled strictly by the id_table?

Regards,
Hartley

Re: [PATCH 3/3] Staging: hv: mousevsc: Use the KBUILD_MODNAME macro

[Greg KH]

On Fri, Oct 28, 2011 at 06:02:22PM -0500, H Hartley Sweeten wrote:
> On Friday, October 28, 2011 3:11 PM, K. Y. Srinivasan wrote:
> > 
> > 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,
> 
> It's just my opinion, but I'm not sure this is better.
> 
> By changing the name to KBUILD_MODNAME you can no longer grep the source to
> find the driver based on it's name:

That's fine.

> $ grep -r name * | grep mousevsc
> drivers/staging/hv/hv_mouse.c:  .name = "mousevsc",
> 
> Also, doesn't this change the name of the driver from "mousevsc" to "hv_mouse"?

Yes.

> Does anything else need modified to handle the name change or is the matching
> handled strictly by the id_table?

No, this only shows up in sysfs, no other changes needed.

thanks,

greg k-h

Re: [PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver

[Greg KH]

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

RE: [PATCH 0/3] Staging: hv: mousevsc: cleanup the mouse driver

[KY Srinivasan]

> -----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