[PATCH 0/3] Drivers: hv: vmbus

[PATCH 0/3] Drivers: hv: vmbus

[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 <jasowang@redhat.com>
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(-)

-- 
1.7.4.1

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

[K. Y. Srinivasan]

Get rid of unnecessary forward declarations.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reported-by: Jason Wang <jasowang@redhat.com>
---
 drivers/hv/channel.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 4065374..1bb1a80 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -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.
-- 
1.7.4.1

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

[K. Y. Srinivasan]

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

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reported-by: Jason Wang <jasowang@redhat.com>
Cc: Stable <stable@vger.kernel.org>
---
 drivers/hv/channel.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 1bb1a80..773a2f2 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -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);
-- 
1.7.4.1

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

[K. Y. Srinivasan]

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

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reported-by: Jason Wang <jasowang@redhat.com>
---
 drivers/hv/channel.c      |    4 ----
 drivers/hv/hyperv_vmbus.h |    1 -
 drivers/hv/ring_buffer.c  |   11 -----------
 3 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 773a2f2..2edb440 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -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));
 
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index d8d1fad..d46f88e 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -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,
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 7233c88..25caf42 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -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
-- 
1.7.4.1

Re: [PATCH 0/3] Drivers: hv: vmbus

[Jason Wang]

On 10/13/2012 04:22 AM, K. Y. Srinivasan wrote:
> 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<jasowang@redhat.com>
> 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(-)
>
Thanks.

Acked-by: Jason Wang <jasowang@redhat.com>

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

[Dan Carpenter]

On Fri, Oct 12, 2012 at 01:22:42PM -0700, K. Y. Srinivasan wrote:
> -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

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

[Greg KH]

On Fri, Oct 12, 2012 at 01:22:43PM -0700, K. Y. Srinivasan wrote:
> hv_ringbuffer_cleanup() is an empty function; get rid of it.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>  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

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

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, October 24, 2012 6:45 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 3/3] Drivers: hv: Get rid of hv_ringbuffer_cleanup()
> 
> On Fri, Oct 12, 2012 at 01:22:43PM -0700, K. Y. Srinivasan wrote:
> > hv_ringbuffer_cleanup() is an empty function; get rid of it.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Reported-by: Jason Wang <jasowang@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  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...)

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