Patchwork [2/7,RESEND] PCI: Correctly clean up pci root buses in function pci_remove_bus()

login
register
mail settings
Submitter Taku Izumi
Date Aug. 10, 2012, 6:12 a.m.
Message ID <20120810151227.cc02c783.izumi.taku@jp.fujitsu.com>
Download mbox | patch
Permalink /patch/176363/
State Superseded
Headers show

Comments

Taku Izumi - Aug. 10, 2012, 6:12 a.m.
From: Jiang Liu <jiang.liu@huawei.com>

    Correctly clean up pci root buses in function pci_remove_bus()
    
    The function pci_create_root_bus() allocates the pci bus structure,
    registers the bus device and creates the legacy files for a pci root
    bus, but returns without setting the is_added flag. The is_added flag
    for a pci root bus will be set by function pci_scan_child_bus().
    If a pci root bus is destroyed before calling pci_scan_child_bus(),
    the is_added flag will not be set.  So teach function pci_remove_bus()
    to detect such a case and correctly clean up pci root buses.
    
    Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
    Signed-off-by: Yinghai Lu <yinghai@kernel.org>
    Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/pci/remove.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 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
Bjorn Helgaas - Aug. 16, 2012, 5:11 p.m.
On Thu, Aug 9, 2012 at 11:12 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
>     Correctly clean up pci root buses in function pci_remove_bus()
>
>     The function pci_create_root_bus() allocates the pci bus structure,
>     registers the bus device and creates the legacy files for a pci root
>     bus, but returns without setting the is_added flag. The is_added flag
>     for a pci root bus will be set by function pci_scan_child_bus().
>     If a pci root bus is destroyed before calling pci_scan_child_bus(),
>     the is_added flag will not be set.

What's the scenario where this happens?  Most of the
pci_scan_child_bus() calls look like this:

    bus = pci_create_root_bus();
    pci_scan_child_bus(bus);

In those cases, there's no opportunity to destroy the root bus before
calling pci_scan_child_bus().

I don't like to add additional tests as you're doing here because it's
telling us that we have more special cases to think about.  Apparently
bus->is_added is telling us that we've created legacy files and
registered the bus device.  EXCEPT for root buses.  Why is it
different for root buses?  Why don't we set root_bus->is_added at the
same time we create the legacy files and register the root bus's
device?

>     So teach function pci_remove_bus()
>     to detect such a case and correctly clean up pci root buses.
>
>     Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>     Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>     Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/pci/remove.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> Index: Bjorn-next-0808/drivers/pci/remove.c
> ===================================================================
> --- Bjorn-next-0808.orig/drivers/pci/remove.c
> +++ Bjorn-next-0808/drivers/pci/remove.c
> @@ -70,11 +70,10 @@ void pci_remove_bus(struct pci_bus *pci_
>         list_del(&pci_bus->node);
>         pci_bus_release_busn_res(pci_bus);
>         up_write(&pci_bus_sem);
> -       if (!pci_bus->is_added)
> -               return;
> -
> -       pci_remove_legacy_files(pci_bus);
> -       device_unregister(&pci_bus->dev);
> +       if (pci_bus->is_added || pci_is_root_bus(pci_bus)) {
> +               pci_remove_legacy_files(pci_bus);
> +               device_unregister(&pci_bus->dev);
> +       }
>  }
>  EXPORT_SYMBOL(pci_remove_bus);
>
>
--
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
Jiang Liu - Aug. 30, 2012, 3:43 p.m.
On 08/17/2012 01:11 AM, Bjorn Helgaas wrote:
> On Thu, Aug 9, 2012 at 11:12 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> From: Jiang Liu <jiang.liu@huawei.com>
>>
>>     Correctly clean up pci root buses in function pci_remove_bus()
>>
>>     The function pci_create_root_bus() allocates the pci bus structure,
>>     registers the bus device and creates the legacy files for a pci root
>>     bus, but returns without setting the is_added flag. The is_added flag
>>     for a pci root bus will be set by function pci_scan_child_bus().
>>     If a pci root bus is destroyed before calling pci_scan_child_bus(),
>>     the is_added flag will not be set.
> 
> What's the scenario where this happens?  Most of the
> pci_scan_child_bus() calls look like this:
> 
>     bus = pci_create_root_bus();
>     pci_scan_child_bus(bus);
> 
> In those cases, there's no opportunity to destroy the root bus before
> calling pci_scan_child_bus().
> 
> I don't like to add additional tests as you're doing here because it's
> telling us that we have more special cases to think about.  Apparently
> bus->is_added is telling us that we've created legacy files and
> registered the bus device.  EXCEPT for root buses.  Why is it
> different for root buses?  Why don't we set root_bus->is_added at the
> same time we create the legacy files and register the root bus's
> device?
Hi Bjorn,
	Previously this patch was used to cover error recovery path in
our new ACPI based hotplug framework. Now thing has changed a lot and
it can't be cleanly applied to pci-next branch, so please ignore it:)
I will resubmit it if needed in future.
	--Gerry

> 
>>     So teach function pci_remove_bus()
>>     to detect such a case and correctly clean up pci root buses.
>>
>>     Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>     Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>     Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
>> ---
>>  drivers/pci/remove.c |    9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> Index: Bjorn-next-0808/drivers/pci/remove.c
>> ===================================================================
>> --- Bjorn-next-0808.orig/drivers/pci/remove.c
>> +++ Bjorn-next-0808/drivers/pci/remove.c
>> @@ -70,11 +70,10 @@ void pci_remove_bus(struct pci_bus *pci_
>>         list_del(&pci_bus->node);
>>         pci_bus_release_busn_res(pci_bus);
>>         up_write(&pci_bus_sem);
>> -       if (!pci_bus->is_added)
>> -               return;
>> -
>> -       pci_remove_legacy_files(pci_bus);
>> -       device_unregister(&pci_bus->dev);
>> +       if (pci_bus->is_added || pci_is_root_bus(pci_bus)) {
>> +               pci_remove_legacy_files(pci_bus);
>> +               device_unregister(&pci_bus->dev);
>> +       }
>>  }
>>  EXPORT_SYMBOL(pci_remove_bus);
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
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

Index: Bjorn-next-0808/drivers/pci/remove.c
===================================================================
--- Bjorn-next-0808.orig/drivers/pci/remove.c
+++ Bjorn-next-0808/drivers/pci/remove.c
@@ -70,11 +70,10 @@  void pci_remove_bus(struct pci_bus *pci_
 	list_del(&pci_bus->node);
 	pci_bus_release_busn_res(pci_bus);
 	up_write(&pci_bus_sem);
-	if (!pci_bus->is_added)
-		return;
-
-	pci_remove_legacy_files(pci_bus);
-	device_unregister(&pci_bus->dev);
+	if (pci_bus->is_added || pci_is_root_bus(pci_bus)) {
+		pci_remove_legacy_files(pci_bus);
+		device_unregister(&pci_bus->dev);
+	}
 }
 EXPORT_SYMBOL(pci_remove_bus);