hv: vmbus_free_channels(): remove the redundant free_channel()

PATCH In review Dexuan Cui 2 participants 5 messages Apr 9, 2015 Hyper-V/Azure core and drivers

Share & tools

Dexuan Cui

author maintainer

2015-04-09 05:03 UTC #0 lore raw

PATCH 1/1 hv: vmbus_free_channels(): remove the redundant free_channel()

free_channel() has been invoked in
vmbus_remove() -> hv_process_channel_removal(), or vmbus_remove() -> ... -> vmbus_close_internal() -> hv_process_channel_removal().

We also change to use list_for_each_entry_safe(), because the entry is removed in hv_process_channel_removal().

Thank Dan Carpenter for finding the issue!

Signed-off-by Dexuan Cui <[email protected]>

Reported-by Dan Carpenter <[email protected]>

Cc K. Y. Srinivasan <[email protected]>

Cc Vitaly Kuznetsov <[email protected]>

---
 drivers/hv/channel_mgmt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/hv/channel_mgmt.c +8 -3

index 0eeb1b3..865a3af 100644
@@ -212,11 +212,16 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
 
 void vmbus_free_channels(void)
 {
-	struct vmbus_channel *channel;
+	struct vmbus_channel *channel, *tmp;
+
+	list_for_each_entry_safe(channel, tmp, &vmbus_connection.chn_list,
+		listentry) {
+		/* if we don't set rescind to true, vmbus_close_internal()
+		 * won't invoke hv_process_channel_removal().
+		 */
+		channel->rescind = true;
 
-	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
 		vmbus_device_unregister(channel->device_obj);
-		free_channel(channel);
 	}
 }

Signature · 1 line

2.1.0

Greg KH

2015-04-09 09:08 UTC #1 lore raw

Quoted 22 lines · Dexuan Cui

free_channel() has been invoked in
vmbus_remove() -> hv_process_channel_removal(), or vmbus_remove() -> ... -> vmbus_close_internal() -> hv_process_channel_removal().
We also change to use list_for_each_entry_safe(), because the entry is removed in hv_process_channel_removal().
Thank Dan Carpenter for finding the issue!
Signed-off-by Dexuan Cui <[email protected]>
Reported-by Dan Carpenter <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
---
 drivers/hv/channel_mgmt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/hv/channel_mgmt.c +0 -0
index 0eeb1b3..865a3af 100644

drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 218

Quoted 8 lines

drivers/hv/channel_mgmt.c +4 -1

@@ -212,11 +212,16 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
 
 void vmbus_free_channels(void)
 {
-	struct vmbus_channel *channel;
+	struct vmbus_channel *channel, *tmp;
+
+	list_for_each_entry_safe(channel, tmp, &vmbus_connection.chn_list,
+		listentry) {

Horrid indentation, never put continuations on a 1 tab stop location, you can't read this properly, right?

drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 222

Quoted 4 lines

drivers/hv/channel_mgmt.c +4 -0

+		/* if we don't set rescind to true, vmbus_close_internal()
+		 * won't invoke hv_process_channel_removal().
+		 */
+		channel->rescind = true;

You are changing the logic here, right? Why do you now need the _safe() call if you aren't freeing anything?

confused,

greg k-h

Dexuan Cui

author maintainer

2015-04-09 09:15 UTC #2 lore raw

Quoted 54 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Thursday, April 9, 2015 17:07
To: Dexuan Cui
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; [email protected]; Haiyang Zhang
Subject: Re: [PATCH] hv: vmbus_free_channels(): remove the redundant free_channel()
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct
Quoted 22 lines · Dexuan Cui
free_channel() has been invoked in
vmbus_remove() -> hv_process_channel_removal(), or vmbus_remove() -> ... -> vmbus_close_internal() -> hv_process_channel_removal().
We also change to use list_for_each_entry_safe(), because the entry is removed in hv_process_channel_removal().
Thank Dan Carpenter for finding the issue!
Signed-off-by Dexuan Cui <[email protected]>
Reported-by Dan Carpenter <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
---
 drivers/hv/channel_mgmt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/hv/channel_mgmt.c +0 -0
index 0eeb1b3..865a3af 100644
@@ -212,11 +212,16 @@ void hv_process_channel_removal(struct
vmbus_channel *channel, u32 relid)
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 218
Quoted 8 lines
drivers/hv/channel_mgmt.c +4 -1
 void vmbus_free_channels(void)
 {
-	struct vmbus_channel *channel;
+	struct vmbus_channel *channel, *tmp;
+
+	list_for_each_entry_safe(channel, tmp, &vmbus_connection.chn_list,
+		listentry) {
Horrid indentation, never put continuations on a 1 tab stop location, you can't read this properly, right?
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 222
Quoted 4 lines
drivers/hv/channel_mgmt.c +4 -0
+		/* if we don't set rescind to true, vmbus_close_internal()
+		 * won't invoke hv_process_channel_removal().
+		 */
+		channel->rescind = true;
You are changing the logic here, right? Why do you now need the _safe() call if you aren't freeing anything?

Quoted 3 lines

confused,
greg k-h

Hi Greg,
vmbus_device_unregister() invokes device_unregister() -> vmbus_remove() -> hv_process_channel_removal() -- in this function, we remove the entry from the list:
list_del(&channel->listentry)
and we run free_channel(channel).

Thanks,
-- Dexuan

Greg KH

2015-04-09 09:22 UTC #3 lore raw

Quoted 56 lines · Dexuan Cui

Quoted 56 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Thursday, April 9, 2015 17:07
To: Dexuan Cui
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; [email protected]; Haiyang Zhang
Subject: Re: [PATCH] hv: vmbus_free_channels(): remove the redundant free_channel()
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct
Quoted 22 lines · Dexuan Cui
free_channel() has been invoked in
vmbus_remove() -> hv_process_channel_removal(), or vmbus_remove() -> ... -> vmbus_close_internal() -> hv_process_channel_removal().
We also change to use list_for_each_entry_safe(), because the entry is removed in hv_process_channel_removal().
Thank Dan Carpenter for finding the issue!
Signed-off-by Dexuan Cui <[email protected]>
Reported-by Dan Carpenter <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
---
 drivers/hv/channel_mgmt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/hv/channel_mgmt.c +0 -0
index 0eeb1b3..865a3af 100644
@@ -212,11 +212,16 @@ void hv_process_channel_removal(struct
vmbus_channel *channel, u32 relid)
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 218
Quoted 8 lines
drivers/hv/channel_mgmt.c +4 -1
 void vmbus_free_channels(void)
 {
-	struct vmbus_channel *channel;
+	struct vmbus_channel *channel, *tmp;
+
+	list_for_each_entry_safe(channel, tmp, &vmbus_connection.chn_list,
+		listentry) {
Horrid indentation, never put continuations on a 1 tab stop location, you can't read this properly, right?
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 222
Quoted 4 lines
drivers/hv/channel_mgmt.c +4 -0
+		/* if we don't set rescind to true, vmbus_close_internal()
+		 * won't invoke hv_process_channel_removal().
+		 */
+		channel->rescind = true;
You are changing the logic here, right? Why do you now need the _safe() call if you aren't freeing anything?
confused,

Quoted 8 lines

Quoted 1 line
greg k-h
Hi Greg,
vmbus_device_unregister() invokes device_unregister() -> vmbus_remove() -> hv_process_channel_removal() -- in this function, we remove the entry from the list:
list_del(&channel->listentry)
and we run free_channel(channel).

What is "this function" referring to here? vmbus_free_channels()?

still confused.

greg k-h

Dexuan Cui

author maintainer

2015-04-09 09:27 UTC #4 lore raw

Thanks,
-- Dexuan

Quoted 76 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Thursday, April 9, 2015 17:22
To: Dexuan Cui
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; [email protected]; Haiyang Zhang
Subject: Re: [PATCH] hv: vmbus_free_channels(): remove the redundant free_channel()
Quoted 56 lines · Dexuan Cui
Quoted 56 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Thursday, April 9, 2015 17:07
To: Dexuan Cui
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan; [email protected]; [email protected]; Haiyang Zhang
Subject: Re: [PATCH] hv: vmbus_free_channels(): remove the redundant free_channel()
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct
Quoted 22 lines · Dexuan Cui
free_channel() has been invoked in
vmbus_remove() -> hv_process_channel_removal(), or vmbus_remove() -> ... -> vmbus_close_internal() -> hv_process_channel_removal().
We also change to use list_for_each_entry_safe(), because the entry is removed in hv_process_channel_removal().
Thank Dan Carpenter for finding the issue!
Signed-off-by Dexuan Cui <[email protected]>
Reported-by Dan Carpenter <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Vitaly Kuznetsov <[email protected]>
---
 drivers/hv/channel_mgmt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/hv/channel_mgmt.c +0 -0
index 0eeb1b3..865a3af 100644
@@ -212,11 +212,16 @@ void hv_process_channel_removal(struct
vmbus_channel *channel, u32 relid)
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 218
Quoted 8 lines
drivers/hv/channel_mgmt.c +4 -1
 void vmbus_free_channels(void)
 {
-	struct vmbus_channel *channel;
+	struct vmbus_channel *channel, *tmp;
+
+	list_for_each_entry_safe(channel, tmp, &vmbus_connection.chn_list,
+		listentry) {
Horrid indentation, never put continuations on a 1 tab stop location, you can't read this properly, right?
drivers/hv/channel_mgmt.c void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) · line 222
Quoted 4 lines
drivers/hv/channel_mgmt.c +4 -0
+		/* if we don't set rescind to true, vmbus_close_internal()
+		 * won't invoke hv_process_channel_removal().
+		 */
+		channel->rescind = true;
You are changing the logic here, right? Why do you now need the _safe() call if you aren't freeing anything?
confused,
Quoted 8 lines
Quoted 1 line
greg k-h
Hi Greg,
vmbus_device_unregister() invokes device_unregister() -> vmbus_remove() -> hv_process_channel_removal() -- in this function, we remove the entry from the list:
list_del(&channel->listentry)
and we run free_channel(channel).

Quoted 5 lines

What is "this function" referring to here? vmbus_free_channels()?
still confused.
greg k-h

"this function" means hv_process_channel_removal().

-- Dexuan