Patchwork [v2,08/16] PCI: Stop and remove devices in one pass

login
register
mail settings
Submitter Bjorn Helgaas
Date Aug. 17, 2012, 11:36 p.m.
Message ID <20120817233626.10973.57304.stgit@bhelgaas.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/178413/
State Superseded
Headers show

Comments

Bjorn Helgaas - Aug. 17, 2012, 11:36 p.m.
Previously, when we removed a PCI device, we made two passes over the
hierarchy rooted at the device.  In the first pass, we stopped all
the devices, and in the second, we removed them.

This patch combines the two passes into one so that we remove a device as
soon as it and all its children have been stopped.

Note that we previously stopped devices in reverse order and removed them
in forward order.  Now we stop and remove them in reverse order.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/remove.c |   42 +++++++-----------------------------------
 1 files changed, 7 insertions(+), 35 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - Aug. 18, 2012, 1:21 a.m.
On Fri, Aug 17, 2012 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Previously, when we removed a PCI device, we made two passes over the
> hierarchy rooted at the device.  In the first pass, we stopped all
> the devices, and in the second, we removed them.
>
> This patch combines the two passes into one so that we remove a device as
> soon as it and all its children have been stopped.
>
> Note that we previously stopped devices in reverse order and removed them
> in forward order.  Now we stop and remove them in reverse order.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/remove.c |   42 +++++++-----------------------------------
>  1 files changed, 7 insertions(+), 35 deletions(-)

looks like your cleanup will have some conflict with my pci root bus
hot plug branch.

http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=blob;f=drivers/pci/remove.c;h=f672731922f8db619fb36db05dbd9c27f3253c19;hb=refs/heads/for-pci-root-bus-hotplug

it add pci_stop_and_remove_bus and it depends on some functions that
you kill in this patch set.

 180 void pci_stop_and_remove_bus(struct pci_bus *bus)
181 {
182         struct pci_host_bridge *host_bridge = NULL;
183         struct pci_dev *pci_bridge = NULL;
184
185         pci_stop_bus_devices(bus);
186
187         if (pci_is_root_bus(bus)) {
188                 host_bridge = to_pci_host_bridge(bus->bridge);
189                 get_device(&host_bridge->dev);
190                 pci_stop_host_bridge(host_bridge);
191         } else
192                 pci_bridge = bus->self;
193
194         __pci_remove_bus_devices(bus);
195
196         pci_remove_bus(bus);
197
198         if (host_bridge) {
199                 host_bridge->bus = NULL;
200                 put_device(&host_bridge->dev);
201         }
202
203         if (pci_bridge)
204                 pci_bridge->subordinate = NULL;
205 }

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Aug. 20, 2012, 3:27 p.m.
On Fri, Aug 17, 2012 at 7:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Aug 17, 2012 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Previously, when we removed a PCI device, we made two passes over the
>> hierarchy rooted at the device.  In the first pass, we stopped all
>> the devices, and in the second, we removed them.
>>
>> This patch combines the two passes into one so that we remove a device as
>> soon as it and all its children have been stopped.
>>
>> Note that we previously stopped devices in reverse order and removed them
>> in forward order.  Now we stop and remove them in reverse order.
>>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>  drivers/pci/remove.c |   42 +++++++-----------------------------------
>>  1 files changed, 7 insertions(+), 35 deletions(-)
>
> looks like your cleanup will have some conflict with my pci root bus
> hot plug branch.

Yes, indeed, they have serious conflicts.

> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=blob;f=drivers/pci/remove.c;h=f672731922f8db619fb36db05dbd9c27f3253c19;hb=refs/heads/for-pci-root-bus-hotplug
>
> it add pci_stop_and_remove_bus and it depends on some functions that
> you kill in this patch set.

Do you have any suggestions on how to proceed?  In my opinion,
remove.c is a bit of a rat's nest right now.  It took me quite a long
time to sort out what's going on, but when you finally get down to the
bottom, it's actually very simple.  So I'd really like to clean it up
and expose that simple structure before we throw more stuff into it.

After my cleanup, pci_stop_and_remove_bus_device() and
pci_remove_bus() are the only exported interfaces left.  How hard
would it be to implement host bridge remove on top of that?

I think it's OK (and even preferable) if the host bridge-related code
can be segregated by itself rather than being sprinkled around through
lots of PCI code.  So maybe the pci_is_root_bus()  and
pci_stop_host_bridge() stuff below could be in some sort of host
bridge interface that just calls the code in remove.c.

>  180 void pci_stop_and_remove_bus(struct pci_bus *bus)
> 181 {
> 182         struct pci_host_bridge *host_bridge = NULL;
> 183         struct pci_dev *pci_bridge = NULL;
> 184
> 185         pci_stop_bus_devices(bus);
> 186
> 187         if (pci_is_root_bus(bus)) {
> 188                 host_bridge = to_pci_host_bridge(bus->bridge);
> 189                 get_device(&host_bridge->dev);
> 190                 pci_stop_host_bridge(host_bridge);
> 191         } else
> 192                 pci_bridge = bus->self;
> 193
> 194         __pci_remove_bus_devices(bus);
> 195
> 196         pci_remove_bus(bus);
> 197
> 198         if (host_bridge) {
> 199                 host_bridge->bus = NULL;
> 200                 put_device(&host_bridge->dev);
> 201         }
> 202
> 203         if (pci_bridge)
> 204                 pci_bridge->subordinate = NULL;
> 205 }
>
> Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - Aug. 21, 2012, 5:39 a.m.
On Mon, Aug 20, 2012 at 8:27 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Aug 17, 2012 at 7:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Fri, Aug 17, 2012 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> Previously, when we removed a PCI device, we made two passes over the
>>> hierarchy rooted at the device.  In the first pass, we stopped all
>>> the devices, and in the second, we removed them.
>>>
>>> This patch combines the two passes into one so that we remove a device as
>>> soon as it and all its children have been stopped.
>>>
>>> Note that we previously stopped devices in reverse order and removed them
>>> in forward order.  Now we stop and remove them in reverse order.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>  drivers/pci/remove.c |   42 +++++++-----------------------------------
>>>  1 files changed, 7 insertions(+), 35 deletions(-)
>>
>> looks like your cleanup will have some conflict with my pci root bus
>> hot plug branch.
>
> Yes, indeed, they have serious conflicts.
>
>> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=blob;f=drivers/pci/remove.c;h=f672731922f8db619fb36db05dbd9c27f3253c19;hb=refs/heads/for-pci-root-bus-hotplug
>>
>> it add pci_stop_and_remove_bus and it depends on some functions that
>> you kill in this patch set.
>
> Do you have any suggestions on how to proceed?  In my opinion,
> remove.c is a bit of a rat's nest right now.  It took me quite a long
> time to sort out what's going on, but when you finally get down to the
> bottom, it's actually very simple.  So I'd really like to clean it up
> and expose that simple structure before we throw more stuff into it.
>
> After my cleanup, pci_stop_and_remove_bus_device() and
> pci_remove_bus() are the only exported interfaces left.  How hard
> would it be to implement host bridge remove on top of that?
>
> I think it's OK (and even preferable) if the host bridge-related code
> can be segregated by itself rather than being sprinkled around through
> lots of PCI code.  So maybe the pci_is_root_bus()  and
> pci_stop_host_bridge() stuff below could be in some sort of host
> bridge interface that just calls the code in remove.c.

Yes, your change is quite clean.
Please go ahead to push your changes for 3.7 and my Acked-by.

I would try to rebase pci-root-bus hot plug branch according to the
output of discussion of pci mini-summit.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Aug. 22, 2012, 5:40 p.m.
On Mon, Aug 20, 2012 at 10:39 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Aug 20, 2012 at 8:27 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Fri, Aug 17, 2012 at 7:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Fri, Aug 17, 2012 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>> Previously, when we removed a PCI device, we made two passes over the
>>>> hierarchy rooted at the device.  In the first pass, we stopped all
>>>> the devices, and in the second, we removed them.
>>>>
>>>> This patch combines the two passes into one so that we remove a device as
>>>> soon as it and all its children have been stopped.
>>>>
>>>> Note that we previously stopped devices in reverse order and removed them
>>>> in forward order.  Now we stop and remove them in reverse order.
>>>>
>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> ---
>>>>  drivers/pci/remove.c |   42 +++++++-----------------------------------
>>>>  1 files changed, 7 insertions(+), 35 deletions(-)
>>>
>>> looks like your cleanup will have some conflict with my pci root bus
>>> hot plug branch.
>>
>> Yes, indeed, they have serious conflicts.
>>
>>> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=blob;f=drivers/pci/remove.c;h=f672731922f8db619fb36db05dbd9c27f3253c19;hb=refs/heads/for-pci-root-bus-hotplug
>>>
>>> it add pci_stop_and_remove_bus and it depends on some functions that
>>> you kill in this patch set.
>>
>> Do you have any suggestions on how to proceed?  In my opinion,
>> remove.c is a bit of a rat's nest right now.  It took me quite a long
>> time to sort out what's going on, but when you finally get down to the
>> bottom, it's actually very simple.  So I'd really like to clean it up
>> and expose that simple structure before we throw more stuff into it.
>>
>> After my cleanup, pci_stop_and_remove_bus_device() and
>> pci_remove_bus() are the only exported interfaces left.  How hard
>> would it be to implement host bridge remove on top of that?
>>
>> I think it's OK (and even preferable) if the host bridge-related code
>> can be segregated by itself rather than being sprinkled around through
>> lots of PCI code.  So maybe the pci_is_root_bus()  and
>> pci_stop_host_bridge() stuff below could be in some sort of host
>> bridge interface that just calls the code in remove.c.
>
> Yes, your change is quite clean.
> Please go ahead to push your changes for 3.7 and my Acked-by.
>
> I would try to rebase pci-root-bus hot plug branch according to the
> output of discussion of pci mini-summit.

I fixed the pcmcia bridge check you noted, added your acks and
Yijing's tested-by, and pushed the updated branch to
http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=shortlog;h=refs/heads/pci/bjorn-cleanup-remove

I plan to put it in my next branch tomorrow or Friday.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 30d002e..3828104 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -78,8 +78,6 @@  void pci_remove_bus(struct pci_bus *pci_bus)
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
-static void pci_stop_bus_device(struct pci_dev *dev);
-
 /**
  * pci_stop_and_remove_bus_device - remove a PCI device and any children
  * @dev: the device to remove
@@ -92,38 +90,8 @@  static void pci_stop_bus_device(struct pci_dev *dev);
  * device lists, remove the /proc entry, and notify userspace
  * (/sbin/hotplug).
  */
-static void __pci_remove_bus_device(struct pci_dev *dev)
-{
-	struct pci_bus *bus = dev->subordinate;
-	struct pci_dev *child, *tmp;
-
-	if (bus) {
-		list_for_each_entry_safe(child, tmp, &bus->devices, bus_list)
-			__pci_remove_bus_device(child);
-
-		pci_remove_bus(bus);
-		dev->subordinate = NULL;
-	}
-
-	pci_destroy_dev(dev);
-}
-
 void pci_stop_and_remove_bus_device(struct pci_dev *dev)
 {
-	pci_stop_bus_device(dev);
-	__pci_remove_bus_device(dev);
-}
-
-/**
- * pci_stop_bus_device - stop a PCI device and any children
- * @dev: the device to stop
- *
- * Stop a PCI device (detach the driver, remove from the global list
- * and so on). This also stop any subordinate buses and children in a
- * depth-first manner.
- */
-static void pci_stop_bus_device(struct pci_dev *dev)
-{
 	struct pci_bus *bus = dev->subordinate;
 	struct pci_dev *child, *tmp;
 
@@ -133,12 +101,16 @@  static void pci_stop_bus_device(struct pci_dev *dev)
 	 * iterator.  Therefore, iterate in reverse so we remove the VFs
 	 * first, then the PF.
 	 */
-	if (bus)
+	if (bus) {
 		list_for_each_entry_safe_reverse(child, tmp,
 						 &bus->devices, bus_list)
-			pci_stop_bus_device(child);
+			pci_stop_and_remove_bus_device(child);
+
+		pci_remove_bus(bus);
+		dev->subordinate = NULL;
+	}
 
 	pci_stop_dev(dev);
+	pci_destroy_dev(dev);
 }
-
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);