Drivers: hv: vmbus

PATCH Superseded K. Y. Srinivasan 4 participants 8 messages Oct 12, 2012 – Oct 25, 2012 Hyper-V/Azure core and drivers

Share & tools

K. Y. Srinivasan

author

2012-10-12 20:02 UTC #0 lore raw

cover Drivers: hv: vmbus

Fix a bug in the error path of vmbus_open(). As part of this also get rid of some unnecessary forward declarations as well as empty functions. I would like to thank Jason Wang <[email protected]> for reporting the issues.

K. Y. Srinivasan (3):
Drivers: hv: Get rid of unnecessary forward declarations
Drivers: hv: Cleanup error handling in vmbus_open()
Drivers: hv: Get rid of hv_ringbuffer_cleanup()

 drivers/hv/channel.c      |   36 +++++++++++++-----------------------
 drivers/hv/hyperv_vmbus.h |    1 -
 drivers/hv/ring_buffer.c  |   11 -----------
 3 files changed, 13 insertions(+), 35 deletions(-)

Signature · 1 line

1.7.4.1

K. Y. Srinivasan

author

2012-10-12 20:02 UTC #1 lore raw

PATCH 1/3 Drivers: hv: Get rid of unnecessary forward declarations

Get rid of unnecessary forward declarations.

Signed-off-by K. Y. Srinivasan <[email protected]>

Reviewed-by Haiyang Zhang <[email protected]>

Reported-by Jason Wang <[email protected]>

---
 drivers/hv/channel.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

drivers/hv/channel.c +0 -8

index 4065374..1bb1a80 100644
@@ -33,14 +33,6 @@
 #define NUM_PAGES_SPANNED(addr, len) \
 ((PAGE_ALIGN(addr + len) >> PAGE_SHIFT) - (addr >> PAGE_SHIFT))
 
-/* Internal routines */
-static int create_gpadl_header(
-	void *kbuffer,	/* must be phys and virt contiguous */
-	u32 size,	/* page-size multiple */
-	struct vmbus_channel_msginfo **msginfo,
-	u32 *messagecount);
-static void vmbus_setevent(struct vmbus_channel *channel);
-
 /*
  * vmbus_setevent- Trigger an event notification on the specified
  * channel.

Signature · 1 line

1.7.4.1

K. Y. Srinivasan

author

2012-10-12 20:02 UTC #2 lore raw

PATCH 2/3 Drivers: hv: Cleanup error handling in vmbus_open()

Fix a memory leak in the error handling path in the function vmbus_open().

Signed-off-by K. Y. Srinivasan <[email protected]>

Reviewed-by Haiyang Zhang <[email protected]>

Reported-by Jason Wang <[email protected]>

Cc Stable <[email protected]>

---
 drivers/hv/channel.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

drivers/hv/channel.c +13 ~~-11~~

index 1bb1a80..773a2f2 100644
@@ -138,14 +138,14 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 
 	if (ret != 0) {
 		err = ret;
-		goto errorout;
+		goto error0;
 	}
 
 	ret = hv_ringbuffer_init(
 		&newchannel->inbound, in, recv_ringbuffer_size);
 	if (ret != 0) {
 		err = ret;
-		goto errorout;
+		goto error0;
 	}

@@ -160,7 +160,7 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 
 	if (ret != 0) {
 		err = ret;
-		goto errorout;
+		goto error0;
 	}
 
 	/* Create and init the channel open message */

@@ -169,7 +169,7 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 			   GFP_KERNEL);
 	if (!open_info) {
 		err = -ENOMEM;
-		goto errorout;
+		goto error0;
 	}
 
 	init_completion(&open_info->waitevent);

@@ -185,7 +185,7 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 
 	if (userdatalen > MAX_USER_DEFINED_BYTES) {
 		err = -EINVAL;
-		goto errorout;
+		goto error0;
 	}
 
 	if (userdatalen)

@@ -200,19 +200,18 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 			       sizeof(struct vmbus_channel_open_channel));
 
 	if (ret != 0)
-		goto cleanup;
+		goto error1;
 
 	t = wait_for_completion_timeout(&open_info->waitevent, 5*HZ);
 	if (t == 0) {
 		err = -ETIMEDOUT;
-		goto errorout;
+		goto error1;
 	}
 
 
 	if (open_info->response.open_result.status)
 		err = open_info->response.open_result.status;
 
-cleanup:
 	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
 	list_del(&open_info->msglistentry);
 	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);

@@ -220,9 +219,12 @@ cleanup:
 	kfree(open_info);
 	return err;
 
-errorout:
-	hv_ringbuffer_cleanup(&newchannel->outbound);
-	hv_ringbuffer_cleanup(&newchannel->inbound);
+error1:
+	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
+	list_del(&open_info->msglistentry);
+	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
+
+error0:
 	free_pages((unsigned long)out,
 		get_order(send_ringbuffer_size + recv_ringbuffer_size));
 	kfree(open_info);

Signature · 1 line

1.7.4.1

Dan Carpenter

2012-10-18 09:46 UTC #3 lore raw

Re: [PATCH 2/3] Drivers: hv: Cleanup error handling in vmbus_open()

drivers/hv/channel.c cleanup: · line 227

Quoted 9 lines · K. Y. Srinivasan

drivers/hv/channel.c +6 -3

-errorout:
-	hv_ringbuffer_cleanup(&newchannel->outbound);
-	hv_ringbuffer_cleanup(&newchannel->inbound);
+error1:
+	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
+	list_del(&open_info->msglistentry);
+	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
+
+error0:

It's better to give the labels meaningful names like error_del and error_pages instead of GW-BASIC numbers.

regards,
dan carpenter

K. Y. Srinivasan

author

2012-10-12 20:02 UTC #4 lore raw

PATCH 3/3 Drivers: hv: Get rid of hv_ringbuffer_cleanup()

In reply to K. Y. Srinivasan

hv_ringbuffer_cleanup() is an empty function; get rid of it.

Signed-off-by K. Y. Srinivasan <[email protected]>

Reviewed-by Haiyang Zhang <[email protected]>

Reported-by Jason Wang <[email protected]>

---
 drivers/hv/channel.c      |    4 ----
 drivers/hv/hyperv_vmbus.h |    1 -
 drivers/hv/ring_buffer.c  |   11 -----------
 3 files changed, 0 insertions(+), 16 deletions(-)

drivers/hv/channel.c +0 -4

index 773a2f2..2edb440 100644
@@ -529,10 +529,6 @@ void vmbus_close(struct vmbus_channel *channel)
 		vmbus_teardown_gpadl(channel,
 					  channel->ringbuffer_gpadlhandle);
 
-	/* Cleanup the ring buffers for this channel */
-	hv_ringbuffer_cleanup(&channel->outbound);
-	hv_ringbuffer_cleanup(&channel->inbound);
-
 	free_pages((unsigned long)channel->ringbuffer_pages,
 		get_order(channel->ringbuffer_pagecount * PAGE_SIZE));

drivers/hv/hyperv_vmbus.h +0 -1

index d8d1fad..d46f88e 100644
@@ -551,7 +551,6 @@ extern void hv_synic_cleanup(void *arg);
 int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info, void *buffer,
 		   u32 buflen);
 
-void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info);
 
 int hv_ringbuffer_write(struct hv_ring_buffer_info *ring_info,
 		    struct scatterlist *sglist,

drivers/hv/ring_buffer.c +0 ~~-11~~

index 7233c88..25caf42 100644
@@ -281,17 +281,6 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
 
 /*
  *
- * hv_ringbuffer_cleanup()
- *
- * Cleanup the ring buffer
- *
- */
-void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info)
-{
-}
-
-/*
- *
  * hv_ringbuffer_write()
  *
  * Write to the ring buffer

Signature · 1 line

1.7.4.1

Greg KH

2012-10-24 22:45 UTC #5 lore raw

Re: [PATCH 3/3] Drivers: hv: Get rid of hv_ringbuffer_cleanup()

Quoted 11 lines · K. Y. Srinivasan

hv_ringbuffer_cleanup() is an empty function; get rid of it.
Signed-off-by K. Y. Srinivasan <[email protected]>
Reviewed-by Haiyang Zhang <[email protected]>
Reported-by Jason Wang <[email protected]>
Acked-by Jason Wang <[email protected]>
---
 drivers/hv/channel.c      |    4 ----
 drivers/hv/hyperv_vmbus.h |    1 -
 drivers/hv/ring_buffer.c  |   11 -----------
 3 files changed, 0 insertions(+), 16 deletions(-)

That's nice, but this patch causes build failures, so I can't accept it.

Please test your patches better (hint, a simple 'git grep' would have worked here...)

greg k-h

KY Srinivasan

author

2012-10-25 14:25 UTC #6 lore raw

RE: [PATCH 3/3] Drivers: hv: Get rid of hv_ringbuffer_cleanup()

Quoted 23 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Wednesday, October 24, 2012 6:45 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 3/3] Drivers: hv: Get rid of hv_ringbuffer_cleanup()
Quoted 11 lines · K. Y. Srinivasan
hv_ringbuffer_cleanup() is an empty function; get rid of it.
Signed-off-by K. Y. Srinivasan <[email protected]>
Reviewed-by Haiyang Zhang <[email protected]>
Reported-by Jason Wang <[email protected]>
Acked-by Jason Wang <[email protected]>
---
 drivers/hv/channel.c      |    4 ----
 drivers/hv/hyperv_vmbus.h |    1 -
 drivers/hv/ring_buffer.c  |   11 -----------
 3 files changed, 0 insertions(+), 16 deletions(-)

Quoted 4 lines

That's nice, but this patch causes build failures, so I can't accept it.
Please test your patches better (hint, a simple 'git grep' would have worked here...)

Sorry about that. I am pretty sure I built (with this patch) both vmbus and the util drivers. This function is a private function for the vmbus driver and in fact I went back and rebuilt the drivers in the hv directory without any problem (with this patch applied) in the same tree where I developed these patches (linux-next tree dated October 2, 2012). Specifically, what was the build problem you ran into.

Regards,

K. Y

Jason Wang

Acked-by on the v1 series

2012-10-17 06:06 UTC #7 lore raw

In reply to K. Y. Srinivasan

Quoted 10 lines · K. Y. Srinivasan

Fix a bug in the error path of vmbus_open(). As part of this also get rid of some unnecessary forward declarations as well as empty functions. I would like to thank Jason Wang<[email protected]> for reporting the issues.
K. Y. Srinivasan (3):
Drivers: hv: Get rid of unnecessary forward declarations
Drivers: hv: Cleanup error handling in vmbus_open()
Drivers: hv: Get rid of hv_ringbuffer_cleanup()

Quoted 5 lines

  drivers/hv/channel.c      |   36 +++++++++++++-----------------------
  drivers/hv/hyperv_vmbus.h |    1 -
  drivers/hv/ring_buffer.c  |   11 -----------
  3 files changed, 13 insertions(+), 35 deletions(-)

Thanks.

Acked-by Jason Wang <[email protected]>