* [PATCH 2/4] Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()
2014-08-26 19:05 ` [PATCH 1/4] Drivers: hv: vmbus: Cleanup vmbus_post_msg() K. Y. Srinivasan
@ 2014-08-26 19:05 ` K. Y. Srinivasan
2014-08-26 19:05 ` [PATCH 3/4] Drivers: hv: vmbus: Cleanup vmbus_close_internal() K. Y. Srinivasan
2014-08-26 19:05 ` [PATCH 4/4] Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl() K. Y. Srinivasan
2 siblings, 0 replies; 5+ messages in thread
From: K. Y. Srinivasan @ 2014-08-26 19:05 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sitsofe
Cc: K. Y. Srinivasan, stable
Eliminate calls to BUG_ON() by properly handling errors. In cases where
rollback is possible, we will return the appropriate error to have the
calling code decide how to rollback state. In the case where we are
transferring ownership of the guest physical pages to the host,
we will wait for the host to respond.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
drivers/hv/channel.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 531a593..0206314 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -435,7 +435,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
struct vmbus_channel_gpadl_teardown *msg;
struct vmbus_channel_msginfo *info;
unsigned long flags;
- int ret, t;
+ int ret;
info = kmalloc(sizeof(*info) +
sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
@@ -457,11 +457,12 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
ret = vmbus_post_msg(msg,
sizeof(struct vmbus_channel_gpadl_teardown));
- BUG_ON(ret != 0);
- t = wait_for_completion_timeout(&info->waitevent, 5*HZ);
- BUG_ON(t == 0);
+ if (ret)
+ goto post_msg_err;
+
+ wait_for_completion(&info->waitevent);
- /* Received a torndown response */
+post_msg_err:
spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
list_del(&info->msglistentry);
spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/4] Drivers: hv: vmbus: Cleanup vmbus_close_internal()
2014-08-26 19:05 ` [PATCH 1/4] Drivers: hv: vmbus: Cleanup vmbus_post_msg() K. Y. Srinivasan
2014-08-26 19:05 ` [PATCH 2/4] Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl() K. Y. Srinivasan
@ 2014-08-26 19:05 ` K. Y. Srinivasan
2014-08-27 10:04 ` Dan Carpenter
2014-08-26 19:05 ` [PATCH 4/4] Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl() K. Y. Srinivasan
2 siblings, 1 reply; 5+ messages in thread
From: K. Y. Srinivasan @ 2014-08-26 19:05 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sitsofe
Cc: K. Y. Srinivasan, stable
Eliminate calls to BUG_ON() in vmbus_close_internal().
We have chosen to potentially leak memory, than crash the guest
in case of failures.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
drivers/hv/channel.c | 32 +++++++++++++++++++++++++-------
1 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 0206314..d6c8ea2 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -479,10 +479,10 @@ static void reset_channel_cb(void *arg)
channel->onchannel_callback = NULL;
}
-static void vmbus_close_internal(struct vmbus_channel *channel)
+static int vmbus_close_internal(struct vmbus_channel *channel)
{
struct vmbus_channel_close_channel *msg;
- int ret;
+ int ret = 0;
channel->state = CHANNEL_OPEN_STATE;
channel->sc_creation_callback = NULL;
@@ -502,11 +502,28 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_close_channel));
- BUG_ON(ret != 0);
+ if (ret) {
+ pr_err("Close failed: close post msg return is %d\n", ret);
+ /*
+ * If we failed to post the close msg,
+ * it is perhaps better to leak memory.
+ */
+ goto close_err;
+ }
+
/* Tear down the gpadl for the channel's ring buffer */
- if (channel->ringbuffer_gpadlhandle)
- vmbus_teardown_gpadl(channel,
- channel->ringbuffer_gpadlhandle);
+ if (channel->ringbuffer_gpadlhandle) {
+ ret = vmbus_teardown_gpadl(channel,
+ channel->ringbuffer_gpadlhandle);
+ if (ret) {
+ pr_err("Close failed: teardown gpadl return %d\n", ret);
+ /*
+ * If we failed to teardown gpadl,
+ * it is perhaps better to leak memory.
+ */
+ goto close_err;
+ }
+ }
/* Cleanup the ring buffers for this channel */
hv_ringbuffer_cleanup(&channel->outbound);
@@ -515,7 +532,8 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
free_pages((unsigned long)channel->ringbuffer_pages,
get_order(channel->ringbuffer_pagecount * PAGE_SIZE));
-
+close_err:
+ return ret;
}
/*
--
1.7.4.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 3/4] Drivers: hv: vmbus: Cleanup vmbus_close_internal()
2014-08-26 19:05 ` [PATCH 3/4] Drivers: hv: vmbus: Cleanup vmbus_close_internal() K. Y. Srinivasan
@ 2014-08-27 10:04 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2014-08-27 10:04 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, olaf, apw, jasowang, sitsofe, stable
On Tue, Aug 26, 2014 at 12:05:51PM -0700, K. Y. Srinivasan wrote:
> -static void vmbus_close_internal(struct vmbus_channel *channel)
> +static int vmbus_close_internal(struct vmbus_channel *channel)
> {
> struct vmbus_channel_close_channel *msg;
> - int ret;
> + int ret = 0;
GCC has a feature which warns about uninitialized variables. Those
features are there to help prevent bugs. You are turning the feature
off here by initializing it with a bogus value. Don't do that.
>
> channel->state = CHANNEL_OPEN_STATE;
> channel->sc_creation_callback = NULL;
> @@ -502,11 +502,28 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
>
> ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_close_channel));
>
> - BUG_ON(ret != 0);
> + if (ret) {
> + pr_err("Close failed: close post msg return is %d\n", ret);
> + /*
> + * If we failed to post the close msg,
> + * it is perhaps better to leak memory.
> + */
> + goto close_err;
Just return directly. Don't introduce do-nothing gotos to lead the
reader through a series of pointless goto hops.
The goto label is poorly chosen. Label names should be based on the
thing which they do. "close_err" implies that something is closed but
that's not the case, the label doesn't do anything.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 4/4] Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()
2014-08-26 19:05 ` [PATCH 1/4] Drivers: hv: vmbus: Cleanup vmbus_post_msg() K. Y. Srinivasan
2014-08-26 19:05 ` [PATCH 2/4] Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl() K. Y. Srinivasan
2014-08-26 19:05 ` [PATCH 3/4] Drivers: hv: vmbus: Cleanup vmbus_close_internal() K. Y. Srinivasan
@ 2014-08-26 19:05 ` K. Y. Srinivasan
2 siblings, 0 replies; 5+ messages in thread
From: K. Y. Srinivasan @ 2014-08-26 19:05 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sitsofe
Cc: K. Y. Srinivasan, stable
Eliminate the call to BUG_ON() by waiting for the host to respond. We are
trying to reclaim the ownership of memory that was given to the host and so
we will have to wait until the host responds.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
drivers/hv/channel.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index d6c8ea2..c7ffd42 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -363,7 +363,6 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
u32 next_gpadl_handle;
unsigned long flags;
int ret = 0;
- int t;
next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle);
atomic_inc(&vmbus_connection.next_gpadl_handle);
@@ -410,9 +409,7 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
}
}
- t = wait_for_completion_timeout(&msginfo->waitevent, 5*HZ);
- BUG_ON(t == 0);
-
+ wait_for_completion(&msginfo->waitevent);
/* At this point, we received the gpadl created msg */
*gpadl_handle = gpadlmsg->gpadl;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 5+ messages in thread