From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
This is not safe for spinlock. Fix this by moving the children to be
deleted to a list on the stack, and removing them after spinlock is
released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
@@ -3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs)return0;if(!keep_devs){-/* Delete any children which might still exist. */+INIT_LIST_HEAD(&removed);++/* Move all present children to the list on stack */spin_lock_irqsave(&hbus->device_list_lock,flags);-list_for_each_entry_safe(hpdev,tmp,&hbus->children,list_entry){+list_for_each_entry_safe(hpdev,tmp,&hbus->children,list_entry)+list_move_tail(&hpdev->list_entry,&removed);+spin_unlock_irqrestore(&hbus->device_list_lock,flags);++/* Remove all children in the list */+while(!list_empty(&removed)){+hpdev=list_first_entry(&removed,structhv_pci_dev,+list_entry);list_del(&hpdev->list_entry);if(hpdev->pci_slot)pci_destroy_slot(hpdev->pci_slot);
From: Wei Liu <wei.liu@kernel.org> Date: 2021-08-24 11:02:16
On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
quoted hunk
From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
This is not safe for spinlock. Fix this by moving the children to be
deleted to a list on the stack, and removing them after spinlock is
released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
This can be moved to where it is needed -- the if(!keep_dev) branch --
to limit its scope.
quoted hunk
/*
* After the host sends the RESCIND_CHANNEL message, it doesn't
@@ -3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) return 0; if (!keep_devs) {- /* Delete any children which might still exist. */+ INIT_LIST_HEAD(&removed);++ /* Move all present children to the list on stack */ spin_lock_irqsave(&hbus->device_list_lock, flags);- list_for_each_entry_safe(hpdev, tmp, &hbus->children, list_entry) {+ list_for_each_entry_safe(hpdev, tmp, &hbus->children, list_entry)+ list_move_tail(&hpdev->list_entry, &removed);+ spin_unlock_irqrestore(&hbus->device_list_lock, flags);++ /* Remove all children in the list */+ while (!list_empty(&removed)) {+ hpdev = list_first_entry(&removed, struct hv_pci_dev,+ list_entry);
list_for_each_entry_safe can also be used here, right?
Wei.
quoted hunk
list_del(&hpdev->list_entry);
if (hpdev->pci_slot)
pci_destroy_slot(hpdev->pci_slot);
@@ -3239,7 +3249,6 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) put_pcichild(hpdev); put_pcichild(hpdev); }- spin_unlock_irqrestore(&hbus->device_list_lock, flags); } ret = hv_send_resources_released(hdev);
"Fix a bug ..." is not a very useful subject line. It doesn't say
anything about what the patch *does*. It doesn't hint at a locking
change.
On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
It's unfortunate that slots are not better integrated into the PCI
core. I'm sorry your driver even has to worry about this.
This is not safe for spinlock. Fix this by moving the children to be
deleted to a list on the stack, and removing them after spinlock is
released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
A lore link to Dan's report would be useful here.
quoted hunk
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
@@ -3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs)return0;if(!keep_devs){-/* Delete any children which might still exist. */+INIT_LIST_HEAD(&removed);++/* Move all present children to the list on stack */spin_lock_irqsave(&hbus->device_list_lock,flags);-list_for_each_entry_safe(hpdev,tmp,&hbus->children,list_entry){+list_for_each_entry_safe(hpdev,tmp,&hbus->children,list_entry)+list_move_tail(&hpdev->list_entry,&removed);+spin_unlock_irqrestore(&hbus->device_list_lock,flags);++/* Remove all children in the list */+while(!list_empty(&removed)){+hpdev=list_first_entry(&removed,structhv_pci_dev,+list_entry);list_del(&hpdev->list_entry);if(hpdev->pci_slot)pci_destroy_slot(hpdev->pci_slot);
From: Long Li <longli@microsoft.com> Date: 2021-08-24 17:30:55
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
quoted
From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
This is not safe for spinlock. Fix this by moving the children to be
deleted to a list on the stack, and removing them after spinlock is
released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the
device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index a53bd8728d0d..d4f3cce18957 100644
@@ -3220,6 +3220,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev,
bool keep_devs)
quoted
struct hv_pci_dev *hpdev, *tmp;
unsigned long flags;
int ret;
+ struct list_head removed;
This can be moved to where it is needed -- the if(!keep_dev) branch -- to limit its
scope.
quoted
/*
* After the host sends the RESCIND_CHANNEL message, it doesn't @@
-3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool
keep_devs)
quoted
return 0;
if (!keep_devs) {
- /* Delete any children which might still exist. */
+ INIT_LIST_HEAD(&removed);
+
+ /* Move all present children to the list on stack */
spin_lock_irqsave(&hbus->device_list_lock, flags);
- list_for_each_entry_safe(hpdev, tmp, &hbus->children,
+ list_move_tail(&hpdev->list_entry, &removed);
+ spin_unlock_irqrestore(&hbus->device_list_lock, flags);
+
+ /* Remove all children in the list */
+ while (!list_empty(&removed)) {
+ hpdev = list_first_entry(&removed, struct hv_pci_dev,
+ list_entry);
list_for_each_entry_safe can also be used here, right?
Wei.
I will address your comments.
Long
quoted
list_del(&hpdev->list_entry);
if (hpdev->pci_slot)
pci_destroy_slot(hpdev->pci_slot);
@@ -3239,7 +3249,6 @@ static int hv_pci_bus_exit(struct hv_device *hdev,
From: Long Li <longli@microsoft.com> Date: 2021-08-24 17:35:06
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
"Fix a bug ..." is not a very useful subject line. It doesn't say anything about what
the patch *does*. It doesn't hint at a locking change.
On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
quoted
From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
It's unfortunate that slots are not better integrated into the PCI core. I'm sorry
your driver even has to worry about this.
quoted
This is not safe for spinlock. Fix this by moving the children to be
deleted to a list on the stack, and removing them after spinlock is
released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the
device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
A lore link to Dan's report would be useful here.
I will address your comments and send v2.
Long
quoted
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index a53bd8728d0d..d4f3cce18957 100644
@@ -3220,6 +3220,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev,
bool keep_devs)
quoted
struct hv_pci_dev *hpdev, *tmp;
unsigned long flags;
int ret;
+ struct list_head removed;
/*
* After the host sends the RESCIND_CHANNEL message, it doesn't @@
-3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool
keep_devs)
quoted
return 0;
if (!keep_devs) {
- /* Delete any children which might still exist. */
+ INIT_LIST_HEAD(&removed);
+
+ /* Move all present children to the list on stack */
spin_lock_irqsave(&hbus->device_list_lock, flags);
- list_for_each_entry_safe(hpdev, tmp, &hbus->children,
+ list_move_tail(&hpdev->list_entry, &removed);
+ spin_unlock_irqrestore(&hbus->device_list_lock, flags);
+
+ /* Remove all children in the list */
+ while (!list_empty(&removed)) {
+ hpdev = list_first_entry(&removed, struct hv_pci_dev,
+ list_entry);
list_del(&hpdev->list_entry);
if (hpdev->pci_slot)
pci_destroy_slot(hpdev->pci_slot);
@@ -3239,7 +3249,6 @@ static int hv_pci_bus_exit(struct hv_device *hdev,
From: Michael Kelley <hidden> Date: 2021-08-25 19:11:28
From: Long Li <longli@microsoft.com> Sent: Tuesday, August 24, 2021 10:28 AM
quoted
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
quoted
From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
This is not safe for spinlock. Fix this by moving the children to be
deleted to a list on the stack, and removing them after spinlock is
released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the
device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index a53bd8728d0d..d4f3cce18957 100644
@@ -3220,6 +3220,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev,
bool keep_devs)
quoted
struct hv_pci_dev *hpdev, *tmp;
unsigned long flags;
int ret;
+ struct list_head removed;
This can be moved to where it is needed -- the if(!keep_dev) branch -- to limit its
scope.
quoted
/*
* After the host sends the RESCIND_CHANNEL message, it doesn't @@
-3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool
keep_devs)
quoted
return 0;
if (!keep_devs) {
- /* Delete any children which might still exist. */
+ INIT_LIST_HEAD(&removed);
+
+ /* Move all present children to the list on stack */
spin_lock_irqsave(&hbus->device_list_lock, flags);
- list_for_each_entry_safe(hpdev, tmp, &hbus->children,
+ list_move_tail(&hpdev->list_entry, &removed);
+ spin_unlock_irqrestore(&hbus->device_list_lock, flags);
+
+ /* Remove all children in the list */
+ while (!list_empty(&removed)) {
+ hpdev = list_first_entry(&removed, struct hv_pci_dev,
+ list_entry);
list_for_each_entry_safe can also be used here, right?
Wei.
I will address your comments.
Long
I thought list_for_each_entry_safe() is for use when list manipulation
is *not* protected by a lock and you want to safely walk the list
even if an entry gets removed. If the list is protected by a lock or
not subject to contention (as is the case here), then
list_for_each_entry() is the simpler implementation. The original
implementation didn't need to use the _safe version because of
the spin lock.
Or do I have it backwards?
Michael
quoted
quoted
list_del(&hpdev->list_entry);
if (hpdev->pci_slot)
pci_destroy_slot(hpdev->pci_slot);
@@ -3239,7 +3249,6 @@ static int hv_pci_bus_exit(struct hv_device *hdev,
From: Long Li <longli@microsoft.com> Date: 2021-08-25 20:25:24
Subject: RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
From: Long Li <longli@microsoft.com> Sent: Tuesday, August 24, 2021 10:28
AM
quoted
quoted
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on
the bus
On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com
wrote:
quoted
quoted
quoted
From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
This is not safe for spinlock. Fix this by moving the children to
be deleted to a list on the stack, and removing them after
spinlock is released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing
the
device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index a53bd8728d0d..d4f3cce18957 100644
@@ -3220,6 +3220,7 @@ static int hv_pci_bus_exit(struct hv_device
*hdev,
bool keep_devs)
quoted
struct hv_pci_dev *hpdev, *tmp;
unsigned long flags;
int ret;
+ struct list_head removed;
This can be moved to where it is needed -- the if(!keep_dev) branch
-- to limit its scope.
quoted
/*
* After the host sends the RESCIND_CHANNEL message, it doesn't
@@
-3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device
*hdev, bool
keep_devs)
quoted
return 0;
if (!keep_devs) {
- /* Delete any children which might still exist. */
+ INIT_LIST_HEAD(&removed);
+
+ /* Move all present children to the list on stack */
spin_lock_irqsave(&hbus->device_list_lock, flags);
- list_for_each_entry_safe(hpdev, tmp, &hbus->children,
+ list_move_tail(&hpdev->list_entry, &removed);
+ spin_unlock_irqrestore(&hbus->device_list_lock, flags);
+
+ /* Remove all children in the list */
+ while (!list_empty(&removed)) {
+ hpdev = list_first_entry(&removed, struct hv_pci_dev,
+ list_entry);
list_for_each_entry_safe can also be used here, right?
Wei.
I will address your comments.
Long
I thought list_for_each_entry_safe() is for use when list manipulation is *not*
protected by a lock and you want to safely walk the list even if an entry gets
removed. If the list is protected by a lock or not subject to contention (as is the
case here), then
list_for_each_entry() is the simpler implementation. The original
implementation didn't need to use the _safe version because of the spin lock.
Or do I have it backwards?
Michael
I think we need list_for_each_entry_safe() because we delete the list elements while going through them:
Here is the comment on list_for_each_entry_safe():
/**
* Loop through the list, keeping a backup pointer to the element. This
* macro allows for the deletion of a list element while looping through the
* list.
*
* See list_for_each_entry for more details.
*/
quoted
quoted
quoted
list_del(&hpdev->list_entry);
if (hpdev->pci_slot)
pci_destroy_slot(hpdev->pci_slot);
@@ -3239,7 +3249,6 @@ static int hv_pci_bus_exit(struct hv_device
From: Rob Herring <robh@kernel.org> Date: 2021-08-25 20:32:49
On Wed, Aug 25, 2021 at 2:11 PM Michael Kelley [off-list ref] wrote:
From: Long Li <longli@microsoft.com> Sent: Tuesday, August 24, 2021 10:28 AM
quoted
quoted
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
quoted
From: Long Li <longli@microsoft.com>
In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.
This is not safe for spinlock. Fix this by moving the children to be
deleted to a list on the stack, and removing them after spinlock is
released.
Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the
device")
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <redacted>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <redacted>
Cc: Dan Carpenter <redacted>
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index a53bd8728d0d..d4f3cce18957 100644
@@ -3220,6 +3220,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev,
bool keep_devs)
quoted
struct hv_pci_dev *hpdev, *tmp;
unsigned long flags;
int ret;
+ struct list_head removed;
This can be moved to where it is needed -- the if(!keep_dev) branch -- to limit its
scope.
quoted
/*
* After the host sends the RESCIND_CHANNEL message, it doesn't @@
-3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool
keep_devs)
quoted
return 0;
if (!keep_devs) {
- /* Delete any children which might still exist. */
+ INIT_LIST_HEAD(&removed);
+
+ /* Move all present children to the list on stack */
spin_lock_irqsave(&hbus->device_list_lock, flags);
- list_for_each_entry_safe(hpdev, tmp, &hbus->children,
+ list_move_tail(&hpdev->list_entry, &removed);
+ spin_unlock_irqrestore(&hbus->device_list_lock, flags);
+
+ /* Remove all children in the list */
+ while (!list_empty(&removed)) {
+ hpdev = list_first_entry(&removed, struct hv_pci_dev,
+ list_entry);
list_for_each_entry_safe can also be used here, right?
Wei.
I will address your comments.
Long
I thought list_for_each_entry_safe() is for use when list manipulation
is *not* protected by a lock and you want to safely walk the list
even if an entry gets removed. If the list is protected by a lock or
not subject to contention (as is the case here), then
list_for_each_entry() is the simpler implementation. The original
implementation didn't need to use the _safe version because of
the spin lock.
Or do I have it backwards?
"_safe" only means "safe against removal of list entry" as the
kerneldoc says. But that means removal within the loop iteration, not
any writer. A lock is needed in either case if there's another writer.
Don't ask me about the RCU variant though...
Rob
From: Michael Kelley <hidden> Date: 2021-08-26 16:50:51
From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021 1:25 PM
quoted
I thought list_for_each_entry_safe() is for use when list manipulation is *not*
protected by a lock and you want to safely walk the list even if an entry gets
removed. If the list is protected by a lock or not subject to contention (as is the
case here), then
list_for_each_entry() is the simpler implementation. The original
implementation didn't need to use the _safe version because of the spin lock.
Or do I have it backwards?
Michael
I think we need list_for_each_entry_safe() because we delete the list elements while going through them:
Here is the comment on list_for_each_entry_safe():
/**
* Loop through the list, keeping a backup pointer to the element. This
* macro allows for the deletion of a list element while looping through the
* list.
*
* See list_for_each_entry for more details.
*/
Got it. Thanks (and to Rob Herring). I read that comment but
with the wrong assumptions and didn't understand it correctly.
Interestingly, pci-hyperv.c has another case of looping through
this list and removing items where the _safe version is not used.
See pci_devices_present_work() where the missing children are
moved to a list on the stack.
Michael
From: Wei Liu <wei.liu@kernel.org> Date: 2021-08-26 19:41:26
On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021 1:25 PM
quoted
quoted
I thought list_for_each_entry_safe() is for use when list manipulation is *not*
protected by a lock and you want to safely walk the list even if an entry gets
removed. If the list is protected by a lock or not subject to contention (as is the
case here), then
list_for_each_entry() is the simpler implementation. The original
implementation didn't need to use the _safe version because of the spin lock.
Or do I have it backwards?
Michael
I think we need list_for_each_entry_safe() because we delete the list elements while going through them:
Here is the comment on list_for_each_entry_safe():
/**
* Loop through the list, keeping a backup pointer to the element. This
* macro allows for the deletion of a list element while looping through the
* list.
*
* See list_for_each_entry for more details.
*/
Got it. Thanks (and to Rob Herring). I read that comment but
with the wrong assumptions and didn't understand it correctly.
Interestingly, pci-hyperv.c has another case of looping through
this list and removing items where the _safe version is not used.
See pci_devices_present_work() where the missing children are
moved to a list on the stack.
That can be converted too, I think.
The original code is not wrong per-se. It is just not as concise as
using list_for_each_entry_safe.
Wei.
From: Long Li <longli@microsoft.com> Date: 2021-08-26 20:09:25
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
quoted
From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021
1:25 PM
quoted
quoted
I thought list_for_each_entry_safe() is for use when list
manipulation is *not* protected by a lock and you want to safely
walk the list even if an entry gets removed. If the list is
protected by a lock or not subject to contention (as is the case
here), then
list_for_each_entry() is the simpler implementation. The original
implementation didn't need to use the _safe version because of the spin
lock.
quoted
quoted
quoted
Or do I have it backwards?
Michael
I think we need list_for_each_entry_safe() because we delete the list
elements while going through them:
quoted
quoted
Here is the comment on list_for_each_entry_safe():
/**
* Loop through the list, keeping a backup pointer to the element.
This
* macro allows for the deletion of a list element while looping
through the
* list.
*
* See list_for_each_entry for more details.
*/
Got it. Thanks (and to Rob Herring). I read that comment but
with the wrong assumptions and didn't understand it correctly.
Interestingly, pci-hyperv.c has another case of looping through this
list and removing items where the _safe version is not used.
See pci_devices_present_work() where the missing children are moved to
a list on the stack.
That can be converted too, I think.
The original code is not wrong per-se. It is just not as concise as using
list_for_each_entry_safe.
Wei.
I assume we are talking about the following code in pci_devices_present_work():
list_for_each_entry(hpdev, &hbus->children, list_entry) {
if (hpdev->reported_missing) {
found = true;
put_pcichild(hpdev);
list_move_tail(&hpdev->list_entry, &removed);
break;
}
}
This code is correct as there is a "break" after a list entry is removed from the list. So there is no need to use the _safe version. This code can be converted to use the _safe version.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-08-26 20:15:10
On Thu, Aug 26, 2021 at 08:09:19PM +0000, Long Li wrote:
quoted
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
quoted
From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021
1:25 PM
quoted
quoted
I thought list_for_each_entry_safe() is for use when list
manipulation is *not* protected by a lock and you want to safely
walk the list even if an entry gets removed. If the list is
protected by a lock or not subject to contention (as is the case
here), then
list_for_each_entry() is the simpler implementation. The original
implementation didn't need to use the _safe version because of the spin
lock.
quoted
quoted
quoted
Or do I have it backwards?
Michael
I think we need list_for_each_entry_safe() because we delete the list
elements while going through them:
quoted
quoted
Here is the comment on list_for_each_entry_safe():
/**
* Loop through the list, keeping a backup pointer to the element.
This
* macro allows for the deletion of a list element while looping
through the
* list.
*
* See list_for_each_entry for more details.
*/
Got it. Thanks (and to Rob Herring). I read that comment but
with the wrong assumptions and didn't understand it correctly.
Interestingly, pci-hyperv.c has another case of looping through this
list and removing items where the _safe version is not used.
See pci_devices_present_work() where the missing children are moved to
a list on the stack.
That can be converted too, I think.
The original code is not wrong per-se. It is just not as concise as using
list_for_each_entry_safe.
Wei.
I assume we are talking about the following code in pci_devices_present_work():
list_for_each_entry(hpdev, &hbus->children, list_entry) {
if (hpdev->reported_missing) {
found = true;
put_pcichild(hpdev);
list_move_tail(&hpdev->list_entry, &removed);
break;
}
}
This code is correct as there is a "break" after a list entry is
removed from the list. So there is no need to use the _safe version.
This code can be converted to use the _safe version.
After this block there is another block like
while (!list_empty(removed)) {
...
list_del(...)
}
I assumed Michael was referring to that block. :-)
Wei.
From: Long Li <longli@microsoft.com> Date: 2021-08-26 20:21:03
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
On Thu, Aug 26, 2021 at 08:09:19PM +0000, Long Li wrote:
quoted
quoted
Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on
the bus
On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
quoted
From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25,
2021
1:25 PM
quoted
quoted
I thought list_for_each_entry_safe() is for use when list
manipulation is *not* protected by a lock and you want to
safely walk the list even if an entry gets removed. If the
list is protected by a lock or not subject to contention (as
is the case here), then
list_for_each_entry() is the simpler implementation. The
original implementation didn't need to use the _safe version
because of the spin
lock.
quoted
quoted
quoted
Or do I have it backwards?
Michael
I think we need list_for_each_entry_safe() because we delete the
list
elements while going through them:
quoted
quoted
Here is the comment on list_for_each_entry_safe():
/**
* Loop through the list, keeping a backup pointer to the element.
This
* macro allows for the deletion of a list element while looping
through the
* list.
*
* See list_for_each_entry for more details.
*/
Got it. Thanks (and to Rob Herring). I read that comment but
with the wrong assumptions and didn't understand it correctly.
Interestingly, pci-hyperv.c has another case of looping through
this list and removing items where the _safe version is not used.
See pci_devices_present_work() where the missing children are
moved to a list on the stack.
That can be converted too, I think.
The original code is not wrong per-se. It is just not as concise as
using list_for_each_entry_safe.
Wei.
I assume we are talking about the following code in
pci_devices_present_work():
quoted
list_for_each_entry(hpdev, &hbus->children, list_entry) {
if (hpdev->reported_missing) {
found = true;
put_pcichild(hpdev);
list_move_tail(&hpdev->list_entry, &removed);
break;
}
}
This code is correct as there is a "break" after a list entry is
removed from the list. So there is no need to use the _safe version.
This code can be converted to use the _safe version.
After this block there is another block like
while (!list_empty(removed)) {
...
list_del(...)
}
I assumed Michael was referring to that block. :-)
Wei.
This block is also correct. We don't have a bug here but there is a better way to code it.
Long