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

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

[Haiyang Zhang]

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

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: [PATCH] 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 event waiting 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  |   23 +++++++++++++----------
 drivers/staging/hv/vmbus_drv.c     |   10 ++++++++++
 drivers/staging/hv/vmbus_private.h |    1 +
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..f99db1b 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
 	int ret;
 	int cnt;
 	unsigned long flags;
+	static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
 
 	DPRINT_ENTER(VMBUS);
 
@@ -373,22 +374,24 @@ 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;
+				if (atomic_inc_return(&ic_channel_initcnt) ==
+				   MAX_MSG_TYPES)
+					osd_WaitEventSet(ic_channel_ready);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..3ae8981 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -989,6 +989,8 @@ static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
 };
 MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
 
+struct osd_waitevent *ic_channel_ready;
+
 static int __init vmbus_init(void)
 {
 	int ret = 0;
@@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
 	if (!dmi_check_system(microsoft_hv_dmi_table))
 		return -ENODEV;
 
+	ic_channel_ready = osd_WaitEventCreate();
+	if (ic_channel_ready == NULL)
+		return -ENOMEM;
+
 	ret = vmbus_bus_init(VmbusInitialize);
 
+	/* Wait until all IC channels are initialized */
+	osd_WaitEventWait(ic_channel_ready);
+
+	kfree(ic_channel_ready);
 	DPRINT_EXIT(VMBUS_DRV);
 	return ret;
 }
diff --git a/drivers/staging/hv/vmbus_private.h b/drivers/staging/hv/vmbus_private.h
index 588c667..3fb8dad 100644
--- a/drivers/staging/hv/vmbus_private.h
+++ b/drivers/staging/hv/vmbus_private.h
@@ -130,5 +130,6 @@ int VmbusSetEvent(u32 childRelId);
 
 void VmbusOnEvents(void);
 
+extern struct osd_waitevent *ic_channel_ready;
 
 #endif /* _VMBUS_PRIVATE_H_ */
-- 
1.6.3.2


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

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: [PATCH] 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 event waiting 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  |   23 +++++++++++++----------
 drivers/staging/hv/vmbus_drv.c     |   10 ++++++++++
 drivers/staging/hv/vmbus_private.h |    1 +
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..f99db1b 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
 	int ret;
 	int cnt;
 	unsigned long flags;
+	static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
 
 	DPRINT_ENTER(VMBUS);
 
@@ -373,22 +374,24 @@ 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;
+				if (atomic_inc_return(&ic_channel_initcnt) ==
+				   MAX_MSG_TYPES)
+					osd_WaitEventSet(ic_channel_ready);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..3ae8981 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -989,6 +989,8 @@ static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
 };
 MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
 
+struct osd_waitevent *ic_channel_ready;
+
 static int __init vmbus_init(void)
 {
 	int ret = 0;
@@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
 	if (!dmi_check_system(microsoft_hv_dmi_table))
 		return -ENODEV;
 
+	ic_channel_ready = osd_WaitEventCreate();
+	if (ic_channel_ready == NULL)
+		return -ENOMEM;
+
 	ret = vmbus_bus_init(VmbusInitialize);
 
+	/* Wait until all IC channels are initialized */
+	osd_WaitEventWait(ic_channel_ready);
+
+	kfree(ic_channel_ready);
 	DPRINT_EXIT(VMBUS_DRV);
 	return ret;
 }
diff --git a/drivers/staging/hv/vmbus_private.h b/drivers/staging/hv/vmbus_private.h
index 588c667..3fb8dad 100644
--- a/drivers/staging/hv/vmbus_private.h
+++ b/drivers/staging/hv/vmbus_private.h
@@ -130,5 +130,6 @@ int VmbusSetEvent(u32 childRelId);
 
 void VmbusOnEvents(void);
 
+extern struct osd_waitevent *ic_channel_ready;
 
 #endif /* _VMBUS_PRIVATE_H_ */
-- 
1.6.3.2


[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
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 (modified)

[Greg KH]

On Wed, May 26, 2010 at 04:54:39PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Subject: [PATCH] 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 event waiting 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  |   23 +++++++++++++----------
>  drivers/staging/hv/vmbus_drv.c     |   10 ++++++++++
>  drivers/staging/hv/vmbus_private.h |    1 +
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
> index 3f53b4d..f99db1b 100644
> --- a/drivers/staging/hv/channel_mgmt.c
> +++ b/drivers/staging/hv/channel_mgmt.c
> @@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
>  	int ret;
>  	int cnt;
>  	unsigned long flags;
> +	static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);

Why is this an atomic_t?

>  	DPRINT_ENTER(VMBUS);
>  
> @@ -373,22 +374,24 @@ 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();

What is the mb() call for?  Why is it necessary?  (hint, if you need it,
something else is really wrong...)

Something wierd happened with your indentation here, it doesn't line up
properly.  That call to VmbusChannelOpen() needs to go in a full tab,
not 4 spaces.

Please always run your patch through the checkpatch.pl script before
sending it to me.


>  				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;
> +				if (atomic_inc_return(&ic_channel_initcnt) ==
> +				   MAX_MSG_TYPES)
> +					osd_WaitEventSet(ic_channel_ready);
>  			}
> -			cnt++;
>  		}
>  	}
>  	DPRINT_EXIT(VMBUS);
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index c21731a..3ae8981 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -989,6 +989,8 @@ static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
>  };
>  MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
>  
> +struct osd_waitevent *ic_channel_ready;

What's with the "ic_" naming scheme here?  It should be "hv_" right?

> +
>  static int __init vmbus_init(void)
>  {
>  	int ret = 0;
> @@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
>  	if (!dmi_check_system(microsoft_hv_dmi_table))
>  		return -ENODEV;
>  
> +	ic_channel_ready = osd_WaitEventCreate();
> +	if (ic_channel_ready == NULL)
> +		return -ENOMEM;
> +
>  	ret = vmbus_bus_init(VmbusInitialize);

As you are using this "ic_channel_ready variable only within the
vmbus_bus_init() call, why not just make it local to there?  Then
there's no need to do the create/init/wait/free sequence outside the
init call.

The init call should just do all of this for us, right?

thanks,

greg k-h

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

[Haiyang Zhang]

> From: Greg KH [mailto:gregkh@suse.de]
> > +	static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
> Why is this an atomic_t?

As discussed previously, I used atomic_t to handle more general case 
if vmbus interrupts happen on every cpu.

> > +			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> > +					     2 * PAGE_SIZE, NULL, 0,
> > +					     hv_cb_utils[cnt].callback,
> > +					     newChannel) == 0) {
> > +				hv_cb_utils[cnt].channel = newChannel;
> > +				mb();
> 
> What is the mb() call for?  Why is it necessary?  (hint, if you need it,
> something else is really wrong...)

It ensures the channel assignment happens before the wakeup call: 
osd_WaitEventSet(ic_channel_ready), if the compiler optimization re-arrange 
the execution order. 

> Something wierd happened with your indentation here, it doesn't line up
> properly.  That call to VmbusChannelOpen() needs to go in a full tab,
> not 4 spaces.
> 
> Please always run your patch through the checkpatch.pl script before
> sending it to me.

Sure, I will replace it with TAB. I already ran checkpatch.pl on 
this patch -- no error:
staging-next-2.6> scripts/checkpatch.pl 0525-Fix-race-condition-on-IC-channel-initialization.patch
total: 0 errors, 0 warnings, 71 lines checked

0525-Fix-race-condition-on-IC-channel-initialization.patch has no obvious style problems and is ready for submission.

> > +struct osd_waitevent *ic_channel_ready;
> 
> What's with the "ic_" naming scheme here?  It should be "hv_" right?

IC stands for "integration components", such as Shutdown, Timesync, 
Heartbeat, etc.

> As you are using this "ic_channel_ready variable only within the
> vmbus_bus_init() call, why not just make it local to there?  Then
> there's no need to do the create/init/wait/free sequence outside the
> init call.
> 
> The init call should just do all of this for us, right?

The ic_channel_ready variable is called by VmbusChannelProcessOffer /  osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's 
not a local variable.

Thanks,

- Haiyang

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

[Greg KH]

On Wed, May 26, 2010 at 09:25:31PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:gregkh@suse.de]
> > > +	static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
> > Why is this an atomic_t?
> 
> As discussed previously, I used atomic_t to handle more general case 
> if vmbus interrupts happen on every cpu.

Ok, but then you should use a lock to protect the variable, not an atomic_t.

> 
> > > +			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> > > +					     2 * PAGE_SIZE, NULL, 0,
> > > +					     hv_cb_utils[cnt].callback,
> > > +					     newChannel) == 0) {
> > > +				hv_cb_utils[cnt].channel = newChannel;
> > > +				mb();
> > 
> > What is the mb() call for?  Why is it necessary?  (hint, if you need it,
> > something else is really wrong...)
> 
> It ensures the channel assignment happens before the wakeup call: 
> osd_WaitEventSet(ic_channel_ready), if the compiler optimization re-arrange 
> the execution order. 

If you care about this, because some other thread is looking at it, then
you really need to protect it with a lock.  Don't rely on a mb() to get
it all correct for you (hint, I doubt it will...)

> > Something wierd happened with your indentation here, it doesn't line up
> > properly.  That call to VmbusChannelOpen() needs to go in a full tab,
> > not 4 spaces.
> > 
> > Please always run your patch through the checkpatch.pl script before
> > sending it to me.
> 
> Sure, I will replace it with TAB. I already ran checkpatch.pl on 
> this patch -- no error:
> staging-next-2.6> scripts/checkpatch.pl 0525-Fix-race-condition-on-IC-channel-initialization.patch
> total: 0 errors, 0 warnings, 71 lines checked
> 
> 0525-Fix-race-condition-on-IC-channel-initialization.patch has no obvious style problems and is ready for submission.

Looks like a bug in checkpatch.pl, go poke Andy about that please.

> > > +struct osd_waitevent *ic_channel_ready;
> > 
> > What's with the "ic_" naming scheme here?  It should be "hv_" right?
> 
> IC stands for "integration components", such as Shutdown, Timesync, 
> Heartbeat, etc.

Yes, I know what it stands for, but the rest of the world doesn't :)

> > As you are using this "ic_channel_ready variable only within the
> > vmbus_bus_init() call, why not just make it local to there?  Then
> > there's no need to do the create/init/wait/free sequence outside the
> > init call.
> > 
> > The init call should just do all of this for us, right?
> 
> The ic_channel_ready variable is called by VmbusChannelProcessOffer /
> osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's
> not a local variable.

But again, this logic should be within the init call, as it's part of
the proper init sequence.  So just put it in that call please.

thanks,

greg k-h

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

[Hank Janssen]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Wednesday, May 26, 2010 2:49 PM
> To: Haiyang Zhang
> Cc: 'linux-kernel@vger.kernel.org'; 'devel@driverdev.osuosl.org'; 
> 'virtualization@lists.osdl.org'; Hank Janssen
> Subject: Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel 
> initialization (modified)
> On Wed, May 26, 2010 at 09:25:31PM +0000, Haiyang Zhang wrote:
>>> From: Greg KH [mailto:gregkh@suse.de]
>>> What's with the "ic_" naming scheme here?  It should be "hv_"
> right?
>> 
>> IC stands for "integration components", such as Shutdown, Timesync,
>> Heartbeat, etc.
> 
> Yes, I know what it stands for, but the rest of the world doesn't :)

Marketing in the their infinite wisdom decided that going forward they 
should be called Linux Integration Service so IS. 

Sigh..............

We will change it to hv_  :)

Hank.

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

[Haiyang Zhang]

> From: Greg KH [mailto:gregkh@suse.de]
> > As discussed previously, I used atomic_t to handle more general case
> > if vmbus interrupts happen on every cpu.
> 
> Ok, but then you should use a lock to protect the variable, not an
> atomic_t.
> If you care about this, because some other thread is looking at it,
> then
> you really need to protect it with a lock.  Don't rely on a mb() to get
> it all correct for you (hint, I doubt it will...)

Actually, since the interrupts only happen on cpu0, this is not a concern. How about use a simple int variable here? Also, remove the mb().

> > The ic_channel_ready variable is called by VmbusChannelProcessOffer /
> > osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's
> > not a local variable.
> 
> But again, this logic should be within the init call, as it's part of
> the proper init sequence.  So just put it in that call please.

The VmbusChannelProcessOffer() is called from interrupt context, and initialize the channels, wake up vmbus_init when all channels are ready. If using local variable only, how to pass the channel ready info to vmbus_init() which is in a different context?

Thanks,

- Haiyang

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

[Greg KH]

On Wed, May 26, 2010 at 10:23:08PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:gregkh@suse.de]
> > > As discussed previously, I used atomic_t to handle more general case
> > > if vmbus interrupts happen on every cpu.
> > 
> > Ok, but then you should use a lock to protect the variable, not an
> > atomic_t.
> > If you care about this, because some other thread is looking at it,
> > then
> > you really need to protect it with a lock.  Don't rely on a mb() to get
> > it all correct for you (hint, I doubt it will...)
> 
> Actually, since the interrupts only happen on cpu0, this is not a
> concern. How about use a simple int variable here? Also, remove the
> mb().

How about a lock!

What's so scary about a pretty little semaphore?  They are all cute and
cuddley and don't bite anyone.  You should not be afraid to use them,
they are here to do your bidding.

> > > The ic_channel_ready variable is called by VmbusChannelProcessOffer /
> > > osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's
> > > not a local variable.
> > 
> > But again, this logic should be within the init call, as it's part of
> > the proper init sequence.  So just put it in that call please.
> 
> The VmbusChannelProcessOffer() is called from interrupt context, and
> initialize the channels, wake up vmbus_init when all channels are
> ready. If using local variable only, how to pass the channel ready
> info to vmbus_init() which is in a different context?

No, I mean move the logic you added here, into the vmbus_init() call.

thanks,

greg k-h

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

[Haiyang Zhang]

> From: Greg KH [mailto:gregkh@suse.de]
> How about a lock!
> 
> What's so scary about a pretty little semaphore?  They are all cute and
> cuddley and don't bite anyone.  You should not be afraid to use them,
> they are here to do your bidding.

No problem, we will add a lock here.

> > The VmbusChannelProcessOffer() is called from interrupt context, and
> > initialize the channels, wake up vmbus_init when all channels are
> > ready. If using local variable only, how to pass the channel ready
> > info to vmbus_init() which is in a different context?
> 
> No, I mean move the logic you added here, into the vmbus_init() call.

Do you mean:
Move the event creat/wait/free, which is currently in vmbus_init(), into vmbus_bus_init() function.  hv_channle_ready will still be a global variable. And, the wakeup call -- osd_WaitEventSet() --remains in VmbusChannelProcessOffer() ?

Thanks,

- Haiyang

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

[Jiri Slaby]

On 05/26/2010 11:25 PM, Haiyang Zhang wrote:
>> From: Greg KH [mailto:gregkh@suse.de]
>>> +			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
>>> +					     2 * PAGE_SIZE, NULL, 0,
>>> +					     hv_cb_utils[cnt].callback,
>>> +					     newChannel) == 0) {
>>> +				hv_cb_utils[cnt].channel = newChannel;
>>> +				mb();
>>
>> What is the mb() call for?  Why is it necessary?  (hint, if you need it,
>> something else is really wrong...)
> 
> It ensures the channel assignment happens before the wakeup call: 
> osd_WaitEventSet(ic_channel_ready), if the compiler optimization re-arrange 
> the execution order. 

wake_up() is a barrier, you don't need the mb() there.

BTW osd_WaitEventSet et al. can be easily converted to completion.

-- 
js

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

[Greg KH]

On Wed, May 26, 2010 at 10:52:36PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:gregkh@suse.de]
> > How about a lock!
> > 
> > What's so scary about a pretty little semaphore?  They are all cute and
> > cuddley and don't bite anyone.  You should not be afraid to use them,
> > they are here to do your bidding.
> 
> No problem, we will add a lock here.
> 
> > > The VmbusChannelProcessOffer() is called from interrupt context, and
> > > initialize the channels, wake up vmbus_init when all channels are
> > > ready. If using local variable only, how to pass the channel ready
> > > info to vmbus_init() which is in a different context?
> > 
> > No, I mean move the logic you added here, into the vmbus_init() call.
> 
> Do you mean:
> Move the event creat/wait/free, which is currently in vmbus_init(),
> into vmbus_bus_init() function.

Yes.

> hv_channle_ready will still be a global variable.

That's fine.

> And, the wakeup call -- osd_WaitEventSet() --remains in
> VmbusChannelProcessOffer() ?

Yes.

And as Jiri pointed out, this should be a simple completion.

thanks,

greg k-h