[PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Haiyang Zhang]

[-- Attachment #1: Type: text/plain, Size: 5330 bytes --]

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: staging: hv: Fix race condition on IC channel initialization
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an atomic counter to ensure all channels are ready before
vmbus_init() returns. So another module won't have any uninitialized channel.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/channel_mgmt.c |   25 ++++++++++++++-----------
 drivers/staging/hv/hv_utils.c     |    7 ++++---
 drivers/staging/hv/utils.h        |    5 +++++
 drivers/staging/hv/vmbus_drv.c    |    5 +++++
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..68dfa0f 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -33,7 +33,6 @@ struct vmbus_channel_message_table_entry {
 	void (*messageHandler)(struct vmbus_channel_message_header *msg);
 };
 
-#define MAX_MSG_TYPES                    3
 #define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
 
 static const struct hv_guid
@@ -233,6 +232,10 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES] = {
 };
 EXPORT_SYMBOL(hv_cb_utils);
 
+/* Counter of IC channels initialized */
+atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
+
+
 /*
  * AllocVmbusChannel - Allocate and initialize a vmbus channel object
  */
@@ -373,22 +376,22 @@ static void VmbusChannelProcessOffer(void *context)
 		 * can cleanup properly
 		 */
 		newChannel->State = CHANNEL_OPEN_STATE;
-		cnt = 0;
 
-		while (cnt != MAX_MSG_TYPES) {
+		/* Open IC channels */
+		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
 			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
 				   &hv_cb_utils[cnt].data,
-				   sizeof(struct hv_guid)) == 0) {
+				   sizeof(struct hv_guid)) == 0 &&
+			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+					     2 * PAGE_SIZE, NULL, 0,
+					     hv_cb_utils[cnt].callback,
+					     newChannel) == 0) {
+				hv_cb_utils[cnt].channel = newChannel;
+				mb();
 				DPRINT_INFO(VMBUS, "%s",
 					    hv_cb_utils[cnt].log_msg);
-
-				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
-						    2 * PAGE_SIZE, NULL, 0,
-						    hv_cb_utils[cnt].callback,
-						    newChannel) == 0)
-					hv_cb_utils[cnt].channel = newChannel;
+				atomic_inc(&hv_utils_initcnt);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8a49aaf..22f6f4a 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -253,7 +253,7 @@ static void heartbeat_onchannelcallback(void *context)
 
 static int __init init_hyperv_utils(void)
 {
-	printk(KERN_INFO "Registering HyperV Utility Driver\n");
+	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
 
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&shutdown_onchannelcallback;
@@ -267,13 +267,12 @@ static int __init init_hyperv_utils(void)
 		&heartbeat_onchannelcallback;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
 
+	printk(KERN_INFO "Registered HyperV Utility Driver.\n");
 	return 0;
 }
 
 static void exit_hyperv_utils(void)
 {
-	printk(KERN_INFO "De-Registered HyperV Utility Driver\n");
-
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &chn_cb_negotiate;
@@ -285,6 +284,8 @@ static void exit_hyperv_utils(void)
 	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+	printk(KERN_INFO "De-Registered HyperV Utility Driver.\n");
 }
 
 module_init(init_hyperv_utils);
diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
index 7c07499..3291ab4 100644
--- a/drivers/staging/hv/utils.h
+++ b/drivers/staging/hv/utils.h
@@ -98,6 +98,10 @@ struct ictimesync_data{
 	u8 flags;
 } __attribute__((packed));
 
+
+/* Number of IC types supported */
+#define MAX_MSG_TYPES	3
+
 /* Index for each IC struct in array hv_cb_utils[] */
 #define HV_SHUTDOWN_MSG		0
 #define HV_TIMESYNC_MSG		1
@@ -115,5 +119,6 @@ extern void prep_negotiate_resp(struct icmsg_hdr *,
 				struct icmsg_negotiate *, u8 *);
 extern void chn_cb_negotiate(void *);
 extern struct hyperv_service_callback hv_cb_utils[];
+extern atomic_t hv_utils_initcnt;
 
 #endif /* __HV_UTILS_H_ */
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..160ee92 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -31,6 +31,7 @@
 #include "osd.h"
 #include "logging.h"
 #include "vmbus.h"
+#include "utils.h"
 
 
 /* FIXME! We need to do this dynamically for PIC and APIC system */
@@ -1005,6 +1006,10 @@ static int __init vmbus_init(void)
 
 	ret = vmbus_bus_init(VmbusInitialize);
 
+	/* Wait until all IC channels are initialized */
+	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
+		msleep(100);
+
 	DPRINT_EXIT(VMBUS_DRV);
 	return ret;
 }
-- 
1.6.3.2


[-- Attachment #2: 0521-Fix-race-condition-on-IC-channel-initialization.patch --]
[-- Type: application/octet-stream, Size: 5166 bytes --]

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: [PATCH] Fix race condition on IC channel initialization
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an atomic counter to ensure all channels are ready before
vmbus_init() returns. So another module won't have any uninitialized channel.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/channel_mgmt.c |   25 ++++++++++++++-----------
 drivers/staging/hv/hv_utils.c     |    7 ++++---
 drivers/staging/hv/utils.h        |    5 +++++
 drivers/staging/hv/vmbus_drv.c    |    5 +++++
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..68dfa0f 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -33,7 +33,6 @@ struct vmbus_channel_message_table_entry {
 	void (*messageHandler)(struct vmbus_channel_message_header *msg);
 };
 
-#define MAX_MSG_TYPES                    3
 #define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
 
 static const struct hv_guid
@@ -233,6 +232,10 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES] = {
 };
 EXPORT_SYMBOL(hv_cb_utils);
 
+/* Counter of IC channels initialized */
+atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
+
+
 /*
  * AllocVmbusChannel - Allocate and initialize a vmbus channel object
  */
@@ -373,22 +376,22 @@ static void VmbusChannelProcessOffer(void *context)
 		 * can cleanup properly
 		 */
 		newChannel->State = CHANNEL_OPEN_STATE;
-		cnt = 0;
 
-		while (cnt != MAX_MSG_TYPES) {
+		/* Open IC channels */
+		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
 			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
 				   &hv_cb_utils[cnt].data,
-				   sizeof(struct hv_guid)) == 0) {
+				   sizeof(struct hv_guid)) == 0 &&
+			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+					     2 * PAGE_SIZE, NULL, 0,
+					     hv_cb_utils[cnt].callback,
+					     newChannel) == 0) {
+				hv_cb_utils[cnt].channel = newChannel;
+				mb();
 				DPRINT_INFO(VMBUS, "%s",
 					    hv_cb_utils[cnt].log_msg);
-
-				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
-						    2 * PAGE_SIZE, NULL, 0,
-						    hv_cb_utils[cnt].callback,
-						    newChannel) == 0)
-					hv_cb_utils[cnt].channel = newChannel;
+				atomic_inc(&hv_utils_initcnt);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8a49aaf..22f6f4a 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -253,7 +253,7 @@ static void heartbeat_onchannelcallback(void *context)
 
 static int __init init_hyperv_utils(void)
 {
-	printk(KERN_INFO "Registering HyperV Utility Driver\n");
+	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
 
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&shutdown_onchannelcallback;
@@ -267,13 +267,12 @@ static int __init init_hyperv_utils(void)
 		&heartbeat_onchannelcallback;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
 
+	printk(KERN_INFO "Registered HyperV Utility Driver.\n");
 	return 0;
 }
 
 static void exit_hyperv_utils(void)
 {
-	printk(KERN_INFO "De-Registered HyperV Utility Driver\n");
-
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &chn_cb_negotiate;
@@ -285,6 +284,8 @@ static void exit_hyperv_utils(void)
 	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+	printk(KERN_INFO "De-Registered HyperV Utility Driver.\n");
 }
 
 module_init(init_hyperv_utils);
diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
index 7c07499..3291ab4 100644
--- a/drivers/staging/hv/utils.h
+++ b/drivers/staging/hv/utils.h
@@ -98,6 +98,10 @@ struct ictimesync_data{
 	u8 flags;
 } __attribute__((packed));
 
+
+/* Number of IC types supported */
+#define MAX_MSG_TYPES	3
+
 /* Index for each IC struct in array hv_cb_utils[] */
 #define HV_SHUTDOWN_MSG		0
 #define HV_TIMESYNC_MSG		1
@@ -115,5 +119,6 @@ extern void prep_negotiate_resp(struct icmsg_hdr *,
 				struct icmsg_negotiate *, u8 *);
 extern void chn_cb_negotiate(void *);
 extern struct hyperv_service_callback hv_cb_utils[];
+extern atomic_t hv_utils_initcnt;
 
 #endif /* __HV_UTILS_H_ */
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..160ee92 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -31,6 +31,7 @@
 #include "osd.h"
 #include "logging.h"
 #include "vmbus.h"
+#include "utils.h"
 
 
 /* FIXME! We need to do this dynamically for PIC and APIC system */
@@ -1005,6 +1006,10 @@ static int __init vmbus_init(void)
 
 	ret = vmbus_bus_init(VmbusInitialize);
 
+	/* Wait until all IC channels are initialized */
+	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
+		msleep(100);
+
 	DPRINT_EXIT(VMBUS_DRV);
 	return ret;
 }
-- 
1.6.3.2

Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Greg KH]

On Fri, May 21, 2010 at 07:58:26PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Subject: staging: hv: Fix race condition on IC channel initialization
> There is a possible race condition when hv_utils starts to load immediately
> after hv_vmbus is loading - null pointer error could happen.
> This patch added an atomic counter to ensure all channels are ready before
> vmbus_init() returns. So another module won't have any uninitialized channel.

Better, but not quite ready...

> +/* Counter of IC channels initialized */
> +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);

This doesn't need to be an atomic variable, does it really?

Why not have a simple bool variable "vmbus_initialized" or something.
It starts out as false, and then turns true when you are up and ready.
Then provide a function that tests it:
	bool hv_vmbus_ready(void)
	{
		return vmbus_initialized
	}
	EXPORT_SYMBOL_GPL(hv_vmbus_ready);

>  /*
>   * AllocVmbusChannel - Allocate and initialize a vmbus channel object
>   */
> @@ -373,22 +376,22 @@ static void VmbusChannelProcessOffer(void *context)
>  		 * can cleanup properly
>  		 */
>  		newChannel->State = CHANNEL_OPEN_STATE;
> -		cnt = 0;
>  
> -		while (cnt != MAX_MSG_TYPES) {
> +		/* Open IC channels */
> +		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
>  			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
>  				   &hv_cb_utils[cnt].data,
> -				   sizeof(struct hv_guid)) == 0) {
> +				   sizeof(struct hv_guid)) == 0 &&
> +			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> +					     2 * PAGE_SIZE, NULL, 0,
> +					     hv_cb_utils[cnt].callback,
> +					     newChannel) == 0) {
> +				hv_cb_utils[cnt].channel = newChannel;
> +				mb();
>  				DPRINT_INFO(VMBUS, "%s",
>  					    hv_cb_utils[cnt].log_msg);
> -
> -				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> -						    2 * PAGE_SIZE, NULL, 0,
> -						    hv_cb_utils[cnt].callback,
> -						    newChannel) == 0)
> -					hv_cb_utils[cnt].channel = newChannel;
> +				atomic_inc(&hv_utils_initcnt);
>  			}

Then set the vmbus_initialized to be true right here.

This way, no one needs to know about what the internal number of
messages are, or any other internal mess that would require the bus to
be up and running properly.

> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -253,7 +253,7 @@ static void heartbeat_onchannelcallback(void *context)
>  
>  static int __init init_hyperv_utils(void)
>  {
> -	printk(KERN_INFO "Registering HyperV Utility Driver\n");
> +	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
>  
>  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
>  		&shutdown_onchannelcallback;
> @@ -267,13 +267,12 @@ static int __init init_hyperv_utils(void)
>  		&heartbeat_onchannelcallback;
>  	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
>  
> +	printk(KERN_INFO "Registered HyperV Utility Driver.\n");

Just do one printk, if any at all here.  You really don't need it, it
just clutters up the syslog.  Especially given your code here, it's not
going to ever be a long time between those two messages :)

>  	return 0;
>  }
>  
>  static void exit_hyperv_utils(void)
>  {
> -	printk(KERN_INFO "De-Registered HyperV Utility Driver\n");
> -
>  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
>  		&chn_cb_negotiate;
>  	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &chn_cb_negotiate;
> @@ -285,6 +284,8 @@ static void exit_hyperv_utils(void)
>  	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
>  		&chn_cb_negotiate;
>  	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
> +
> +	printk(KERN_INFO "De-Registered HyperV Utility Driver.\n");

Again, just drop the thing entirely.

>  }
>  
>  module_init(init_hyperv_utils);
> diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
> index 7c07499..3291ab4 100644
> --- a/drivers/staging/hv/utils.h
> +++ b/drivers/staging/hv/utils.h
> @@ -98,6 +98,10 @@ struct ictimesync_data{
>  	u8 flags;
>  } __attribute__((packed));
>  
> +
> +/* Number of IC types supported */
> +#define MAX_MSG_TYPES	3

Now you can keep this #define private.

> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -31,6 +31,7 @@
>  #include "osd.h"
>  #include "logging.h"
>  #include "vmbus.h"
> +#include "utils.h"
>  
>  
>  /* FIXME! We need to do this dynamically for PIC and APIC system */
> @@ -1005,6 +1006,10 @@ static int __init vmbus_init(void)
>  
>  	ret = vmbus_bus_init(VmbusInitialize);
>  
> +	/* Wait until all IC channels are initialized */
> +	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
> +		msleep(100);

this turns into a simple function call, again, never needing to know
about message types or any other mess.

Sound good?

thanks,

greg k-h

Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Ky Srinivasan]

>>> On 5/21/2010 at  4:12 PM, in message <20100521201228.GA6712@suse.de>, Greg KH
<gregkh@suse.de> wrote: 
> On Fri, May 21, 2010 at 07:58:26PM +0000, Haiyang Zhang wrote:
>> From: Haiyang Zhang <haiyangz@microsoft.com>
>> 
>> Subject: staging: hv: Fix race condition on IC channel initialization
>> There is a possible race condition when hv_utils starts to load immediately
>> after hv_vmbus is loading - null pointer error could happen.
>> This patch added an atomic counter to ensure all channels are ready before
>> vmbus_init() returns. So another module won't have any uninitialized 
> channel.
> 
> Better, but not quite ready...
> 
>> +/* Counter of IC channels initialized */
>> +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
> 
> This doesn't need to be an atomic variable, does it really?
> 
> Why not have a simple bool variable "vmbus_initialized" or something.
> It starts out as false, and then turns true when you are up and ready.
> Then provide a function that tests it:
> 	bool hv_vmbus_ready(void)
> 	{
> 		return vmbus_initialized
> 	}
> 	EXPORT_SYMBOL_GPL(hv_vmbus_ready);
I agree with Greg; I would go a step further and deal with this issue as part of loading the bus driver. After all, we already have dependencies established for various LIC drivers  on the bus driver. The fact that even after the bus driver is loaded we cannot reliably load other drivers implies that there is an additional dependency that is not currently being handled. Why can't we ensure that the bus driver is fully initialized before we are done with loading the bus driver.

Regards,

K. Y
> 
>>  /*
>>   * AllocVmbusChannel - Allocate and initialize a vmbus channel object
>>   */
>> @@ -373,22 +376,22 @@ static void VmbusChannelProcessOffer(void *context)
>>  		 * can cleanup properly
>>  		 */
>>  		newChannel->State = CHANNEL_OPEN_STATE;
>> -		cnt = 0;
>>  
>> -		while (cnt != MAX_MSG_TYPES) {
>> +		/* Open IC channels */
>> +		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
>>  			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
>>  				   &hv_cb_utils[cnt].data,
>> -				   sizeof(struct hv_guid)) == 0) {
>> +				   sizeof(struct hv_guid)) == 0 &&
>> +			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
>> +					     2 * PAGE_SIZE, NULL, 0,
>> +					     hv_cb_utils[cnt].callback,
>> +					     newChannel) == 0) {
>> +				hv_cb_utils[cnt].channel = newChannel;
>> +				mb();
>>  				DPRINT_INFO(VMBUS, "%s",
>>  					    hv_cb_utils[cnt].log_msg);
>> -
>> -				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
>> -						    2 * PAGE_SIZE, NULL, 0,
>> -						    hv_cb_utils[cnt].callback,
>> -						    newChannel) == 0)
>> -					hv_cb_utils[cnt].channel = newChannel;
>> +				atomic_inc(&hv_utils_initcnt);
>>  			}
> 
> Then set the vmbus_initialized to be true right here.
> 
> This way, no one needs to know about what the internal number of
> messages are, or any other internal mess that would require the bus to
> be up and running properly.
> 
>> --- a/drivers/staging/hv/hv_utils.c
>> +++ b/drivers/staging/hv/hv_utils.c
>> @@ -253,7 +253,7 @@ static void heartbeat_onchannelcallback(void *context)
>>  
>>  static int __init init_hyperv_utils(void)
>>  {
>> -	printk(KERN_INFO "Registering HyperV Utility Driver\n");
>> +	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
>>  
>>  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
>>  		&shutdown_onchannelcallback;
>> @@ -267,13 +267,12 @@ static int __init init_hyperv_utils(void)
>>  		&heartbeat_onchannelcallback;
>>  	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
>>  
>> +	printk(KERN_INFO "Registered HyperV Utility Driver.\n");
> 
> Just do one printk, if any at all here.  You really don't need it, it
> just clutters up the syslog.  Especially given your code here, it's not
> going to ever be a long time between those two messages :)
> 
>>  	return 0;
>>  }
>>  
>>  static void exit_hyperv_utils(void)
>>  {
>> -	printk(KERN_INFO "De-Registered HyperV Utility Driver\n");
>> -
>>  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
>>  		&chn_cb_negotiate;
>>  	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &chn_cb_negotiate;
>> @@ -285,6 +284,8 @@ static void exit_hyperv_utils(void)
>>  	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
>>  		&chn_cb_negotiate;
>>  	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
>> +
>> +	printk(KERN_INFO "De-Registered HyperV Utility Driver.\n");
> 
> Again, just drop the thing entirely.
> 
>>  }
>>  
>>  module_init(init_hyperv_utils);
>> diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
>> index 7c07499..3291ab4 100644
>> --- a/drivers/staging/hv/utils.h
>> +++ b/drivers/staging/hv/utils.h
>> @@ -98,6 +98,10 @@ struct ictimesync_data{
>>  	u8 flags;
>>  } __attribute__((packed));
>>  
>> +
>> +/* Number of IC types supported */
>> +#define MAX_MSG_TYPES	3
> 
> Now you can keep this #define private.
> 
>> --- a/drivers/staging/hv/vmbus_drv.c
>> +++ b/drivers/staging/hv/vmbus_drv.c
>> @@ -31,6 +31,7 @@
>>  #include "osd.h"
>>  #include "logging.h"
>>  #include "vmbus.h"
>> +#include "utils.h"
>>  
>>  
>>  /* FIXME! We need to do this dynamically for PIC and APIC system */
>> @@ -1005,6 +1006,10 @@ static int __init vmbus_init(void)
>>  
>>  	ret = vmbus_bus_init(VmbusInitialize);
>>  
>> +	/* Wait until all IC channels are initialized */
>> +	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
>> +		msleep(100);
> 
> this turns into a simple function call, again, never needing to know
> about message types or any other mess.
> 
> Sound good?
> 
> thanks,
> 
> greg k-h
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Greg KH]

On Fri, May 21, 2010 at 02:21:46PM -0600, Ky Srinivasan wrote:
> 
> 
> >>> On 5/21/2010 at  4:12 PM, in message <20100521201228.GA6712@suse.de>, Greg KH
> <gregkh@suse.de> wrote: 
> > On Fri, May 21, 2010 at 07:58:26PM +0000, Haiyang Zhang wrote:
> >> From: Haiyang Zhang <haiyangz@microsoft.com>
> >> 
> >> Subject: staging: hv: Fix race condition on IC channel initialization
> >> There is a possible race condition when hv_utils starts to load immediately
> >> after hv_vmbus is loading - null pointer error could happen.
> >> This patch added an atomic counter to ensure all channels are ready before
> >> vmbus_init() returns. So another module won't have any uninitialized 
> > channel.
> > 
> > Better, but not quite ready...
> > 
> >> +/* Counter of IC channels initialized */
> >> +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
> > 
> > This doesn't need to be an atomic variable, does it really?
> > 
> > Why not have a simple bool variable "vmbus_initialized" or something.
> > It starts out as false, and then turns true when you are up and ready.
> > Then provide a function that tests it:
> > 	bool hv_vmbus_ready(void)
> > 	{
> > 		return vmbus_initialized
> > 	}
> > 	EXPORT_SYMBOL_GPL(hv_vmbus_ready);
> I agree with Greg; I would go a step further and deal with this issue
> as part of loading the bus driver. After all, we already have
> dependencies established for various LIC drivers  on the bus driver.
> The fact that even after the bus driver is loaded we cannot reliably
> load other drivers implies that there is an additional dependency that
> is not currently being handled. Why can't we ensure that the bus
> driver is fully initialized before we are done with loading the bus
> driver.

Um, I think that is what this patch fixes :)

It just doesn't do it in a way that I think is very good...

thanks,

greg k-h

Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Ky Srinivasan]

>>> On 5/21/2010 at  4:55 PM, in message <20100521205527.GB9594@suse.de>, Greg KH
<gregkh@suse.de> wrote: 
> On Fri, May 21, 2010 at 02:21:46PM -0600, Ky Srinivasan wrote:
>> 
>> 
>> >>> On 5/21/2010 at  4:12 PM, in message <20100521201228.GA6712@suse.de>, Greg KH
>> <gregkh@suse.de> wrote: 
>> > On Fri, May 21, 2010 at 07:58:26PM +0000, Haiyang Zhang wrote:
>> >> From: Haiyang Zhang <haiyangz@microsoft.com>
>> >> 
>> >> Subject: staging: hv: Fix race condition on IC channel initialization
>> >> There is a possible race condition when hv_utils starts to load immediately
>> >> after hv_vmbus is loading - null pointer error could happen.
>> >> This patch added an atomic counter to ensure all channels are ready before
>> >> vmbus_init() returns. So another module won't have any uninitialized 
>> > channel.
>> > 
>> > Better, but not quite ready...
>> > 
>> >> +/* Counter of IC channels initialized */
>> >> +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
>> > 
>> > This doesn't need to be an atomic variable, does it really?
>> > 
>> > Why not have a simple bool variable "vmbus_initialized" or something.
>> > It starts out as false, and then turns true when you are up and ready.
>> > Then provide a function that tests it:
>> > 	bool hv_vmbus_ready(void)
>> > 	{
>> > 		return vmbus_initialized
>> > 	}
>> > 	EXPORT_SYMBOL_GPL(hv_vmbus_ready);
>> I agree with Greg; I would go a step further and deal with this issue
>> as part of loading the bus driver. After all, we already have
>> dependencies established for various LIC drivers  on the bus driver.
>> The fact that even after the bus driver is loaded we cannot reliably
>> load other drivers implies that there is an additional dependency that
>> is not currently being handled. Why can't we ensure that the bus
>> driver is fully initialized before we are done with loading the bus
>> driver.
> 
> Um, I think that is what this patch fixes :)
> 
> It just doesn't do it in a way that I think is very good...
Ok, my mistake. When I saw hv_vmbus_ready function being exported, I was under the impression each of the drivers that depend on the bus driver to check if the bus driver was properly initialized.

Regards,

K. Y
> 
> thanks,
> 
> greg k-h

RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Haiyang Zhang]

> From: Greg KH [mailto:gregkh@suse.de]
> > +/* Counter of IC channels initialized */
> > +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
> 
> This doesn't need to be an atomic variable, does it really?
> 
> Why not have a simple bool variable "vmbus_initialized" or something.
> It starts out as false, and then turns true when you are up and ready.
> Then provide a function that tests it:
> 	bool hv_vmbus_ready(void)
> 	{
> 		return vmbus_initialized
> 	}
> 	EXPORT_SYMBOL_GPL(hv_vmbus_ready);
> 
> 
> this turns into a simple function call, again, never needing to know
> about message types or any other mess.

This looks good. I will add the hv_vmbus_ready() function. It doesn't even 
have to be exported symbol, because it's only used in vmbus module to ensure 
all channels are ready before vmbus_init() returns. Other modules won't get a 
chance to see uninitialized channels after hv_vmbus is loaded.

Also, I'll cleanup the printk in hv_utils load/unload.

Regarding the atomic variable -- the channel offer processing function is 
triggered by interrupts from host -- should we be concerned about "counter++" 
racing with each other in two interrupts happening around the same time?

Thanks,

- Haiyang

Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Greg KH]

On Fri, May 21, 2010 at 10:07:17PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:gregkh@suse.de]
> > > +/* Counter of IC channels initialized */
> > > +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
> > 
> > This doesn't need to be an atomic variable, does it really?
> > 
> > Why not have a simple bool variable "vmbus_initialized" or something.
> > It starts out as false, and then turns true when you are up and ready.
> > Then provide a function that tests it:
> > 	bool hv_vmbus_ready(void)
> > 	{
> > 		return vmbus_initialized
> > 	}
> > 	EXPORT_SYMBOL_GPL(hv_vmbus_ready);
> > 
> > 
> > this turns into a simple function call, again, never needing to know
> > about message types or any other mess.
> 
> This looks good. I will add the hv_vmbus_ready() function. It doesn't even 
> have to be exported symbol, because it's only used in vmbus module to ensure 
> all channels are ready before vmbus_init() returns. Other modules won't get a 
> chance to see uninitialized channels after hv_vmbus is loaded.
> 
> Also, I'll cleanup the printk in hv_utils load/unload.
> 
> Regarding the atomic variable -- the channel offer processing function is 
> triggered by interrupts from host -- should we be concerned about "counter++" 
> racing with each other in two interrupts happening around the same time?

If you are, having races like this, then you should be using a lock to
protect lots of things, not just one single atomic variable, right?

thanks,

greg k-h

RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Ky Srinivasan]

>>> On 5/21/2010 at  6:07 PM, in message
<1FB5E1D5CA062146B38059374562DF7266B8B5F2@TK5EX14MBXC128.redmond.corp.microsoft.
om>, Haiyang Zhang <haiyangz@microsoft.com> wrote: 
>>  From: Greg KH [mailto:gregkh@suse.de]
>> > +/* Counter of IC channels initialized */
>> > +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
>> 
>> This doesn't need to be an atomic variable, does it really?
>> 
>> Why not have a simple bool variable "vmbus_initialized" or something.
>> It starts out as false, and then turns true when you are up and ready.
>> Then provide a function that tests it:
>> 	bool hv_vmbus_ready(void)
>> 	{
>> 		return vmbus_initialized
>> 	}
>> 	EXPORT_SYMBOL_GPL(hv_vmbus_ready);
>> 
>> 
>> this turns into a simple function call, again, never needing to know
>> about message types or any other mess.
> 
> This looks good. I will add the hv_vmbus_ready() function. It doesn't even 
> have to be exported symbol, because it's only used in vmbus module to ensure 
> 
> all channels are ready before vmbus_init() returns. Other modules won't get 
> a 
> chance to see uninitialized channels after hv_vmbus is loaded.
> 
> Also, I'll cleanup the printk in hv_utils load/unload.
> 
> Regarding the atomic variable -- the channel offer processing function is 
> triggered by interrupts from host -- should we be concerned about "counter++" 
> racing with each other in two interrupts happening around the same time?
You would need to protect the increment, if interrupts are going to come in on any cpu and update the counter. While in your current implementation interrupts are only delivered on cpu0, it is still  probably good to deal with the more general case and protect the counter.

On a slightly different note, why don't you make the synchronization more explicit than what you currently have: Rather than polling the variable in a loop, why don't you put that context to sleep and the interrupt context that updates the count would be responsible for issuing the wakeup when the conditions are appropriate - when all channels are initialized.

Regards,

K. Y 
> 
> Thanks,
> 
> - Haiyang
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/virtualization

RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization

[Haiyang Zhang]

> From: Ky Srinivasan [mailto:ksrinivasan@novell.com]
> You would need to protect the increment, if interrupts are going to
> come in on any cpu and update the counter. While in your current
> implementation interrupts are only delivered on cpu0, it is still
> probably good to deal with the more general case and protect the
> counter.
> 
> On a slightly different note, why don't you make the synchronization
> more explicit than what you currently have: Rather than polling the
> variable in a loop, why don't you put that context to sleep and the
> interrupt context that updates the count would be responsible for
> issuing the wakeup when the conditions are appropriate - when all
> channels are initialized.

Thank you for the suggestion. I will keep the counter atomic to handle 
more general case potentially. To ensure channels are ready before 
vmbus_init() returns, I used an event waiting mechanism instead of 
polling the variable periodically. A modified patch will be submitted 
soon.

Thanks,

- Haiyang