| Message ID | 1466682138-25281-1-git-send-email-wangyijing@huawei.com |
|---|---|
| State | New |
| Headers | show |
ping.. 在 2016/6/23 19:42, Yijing Wang 写道: > pci_host_bridge holds the top resources(IO port/Mem/bus), > now we release pci_host_bridge resources in > acpi_pci_root_release_info() which would be called when > pci_host_bridge device refcount reach 0. In some cases, > pci_host_bridge refcount cannot reach 0 after we remove > pci root bus in pci_remove_root_bus(). Then if we want to > hot add pci root bus, we cannot use pci_host_bridge > resources because of conflicts with old resources which are > still in system. I think this is not reasonable. > > 1. For pci devices, we would release their resources in > pci_destroy_dev() regardless of pci device refcount. > 2. When we try to remove pci root bus, there is no devices > need to use the pci_host_bridge resources again, release > pci_host_bridge resources is safe. > 3. In some cases, users woule make mistake, for example, > user get a pci device(increase refcount), but forget to > put this device, then if we do hotplug pci root bus, > it would make all pci devices cannot work after hot add. > > I found this issue in the following case: > 1. I have a raid pci device in my system; > 2. I mount a disk which connect to this raid. > 3. hot remove the pci root bus. > 4. hot add the pci root bus. > 5. found the resource conflicts for the children pci devices under this root bus. > > pci_root_bus increase a refcount at pci_host_bridge. > pci_root_bus decrease a refcount at pci_host_bridge in > release_pcibus_dev() when pci_root_bus device refcount reach 0. > > pci_dev increase a refcount at pci_bus in pci_alloc_dev(). > pci_dev decrease a refcount at pci_bus in pci_release_dev() > when pci_dev refcount reach 0. > > If any pci device refcount cannot reach 0, then its pci_bus > refcount cannot reach 0 too, the result is pci_host_bridge > refcount cannot reach 0. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > --- > drivers/acpi/pci_root.c | 11 ----------- > drivers/pci/host-bridge.c | 15 +++++++++++++++ > drivers/pci/pci.h | 2 +- > drivers/pci/remove.c | 1 + > 4 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index d144168..c5142d0 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -839,17 +839,6 @@ static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info) > > static void acpi_pci_root_release_info(struct pci_host_bridge *bridge) > { > - struct resource *res; > - struct resource_entry *entry; > - > - resource_list_for_each_entry(entry, &bridge->windows) { > - res = entry->res; > - if (res->flags & IORESOURCE_IO) > - pci_unmap_iospace(res); > - if (res->parent && > - (res->flags & (IORESOURCE_MEM | IORESOURCE_IO))) > - release_resource(res); > - } > __acpi_pci_root_release_info(bridge->release_data); > } > > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c > index 5f4a2e0..3594967 100644 > --- a/drivers/pci/host-bridge.c > +++ b/drivers/pci/host-bridge.c > @@ -8,6 +8,21 @@ > > #include "pci.h" > > +void release_host_bridge_resources(struct pci_host_bridge *bridge) > +{ > + struct resource *res; > + struct resource_entry *entry; > + > + resource_list_for_each_entry(entry, &bridge->windows) { > + res = entry->res; > + if (res->flags & IORESOURCE_IO) > + pci_unmap_iospace(res); > + if (res->parent && > + (res->flags & (IORESOURCE_MEM | IORESOURCE_IO))) > + release_resource(res); > + } > +} > + > static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) > { > while (bus->parent) > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 9730c47..46725d9 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -11,7 +11,7 @@ extern const unsigned char pcie_link_speed[]; > bool pcie_cap_has_lnkctl(const struct pci_dev *dev); > > /* Functions internal to the PCI core code */ > - > +void release_host_bridge_resources(struct pci_host_bridge *bridge); > int pci_create_sysfs_dev_files(struct pci_dev *pdev); > void pci_remove_sysfs_dev_files(struct pci_dev *pdev); > #if !defined(CONFIG_DMI) && !defined(CONFIG_ACPI) > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c > index d1ef7ac..a42b396 100644 > --- a/drivers/pci/remove.c > +++ b/drivers/pci/remove.c > @@ -161,6 +161,7 @@ void pci_remove_root_bus(struct pci_bus *bus) > pci_remove_bus(bus); > host_bridge->bus = NULL; > > + release_host_bridge_resources(host_bridge); > /* remove the host bridge */ > device_unregister(&host_bridge->dev); > } > -- 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
在 2016/8/23 1:28, Bjorn Helgaas 写道: > On Thu, Jun 23, 2016 at 07:42:18PM +0800, Yijing Wang wrote: >> pci_host_bridge holds the top resources(IO port/Mem/bus), >> now we release pci_host_bridge resources in >> acpi_pci_root_release_info() which would be called when >> pci_host_bridge device refcount reach 0. In some cases, >> pci_host_bridge refcount cannot reach 0 after we remove >> pci root bus in pci_remove_root_bus(). > > Did you figure out *why* the host bridge refcount is non-zero? > That seems like it could be part of the problem. 1. pci_create_root_bus() //root bus get a refcount of hostbridge， put the refcount when root bus release(bus dev refcount == 0); 2. pci_alloc_dev() //pci dev get a refcount of pci_bus, put the refcount when pci_dev release(pci_dev refcount == 0) 3. some upper driver could get the pci dev refcount(e.g. we found if we mount a fs in mptsas disk, the mptsas pci dev refcount would be added) 4. if we start remove the root bus before umount, in this case, the mptsas pci dev refcount won't reach 0, so as the step 1 and 2 show, the root bus and host bridge refcount won't reach 0 too. > > You're moving some release_resource() calls from pci_root.c to > host-bridge.c. Where are the corresponding insert or request resource > calls? It's more maintainable if we keep the insert and remove paths > close in the code. > >> Then if we want to >> hot add pci root bus, we cannot use pci_host_bridge >> resources because of conflicts with old resources which are >> still in system. I think this is not reasonable. >> >> 1. For pci devices, we would release their resources in >> pci_destroy_dev() regardless of pci device refcount. >> 2. When we try to remove pci root bus, there is no devices >> need to use the pci_host_bridge resources again, release >> pci_host_bridge resources is safe. >> 3. In some cases, users woule make mistake, for example, >> user get a pci device(increase refcount), but forget to >> put this device, then if we do hotplug pci root bus, >> it would make all pci devices cannot work after hot add. > > Can you explain this a little more? Are you talking about a *driver* > that forgets to put the device? Yes, may some pci drivers make a mistake, the refcount control the device object release is fine, but I think move the mem resource release out is better. > >> I found this issue in the following case: >> 1. I have a raid pci device in my system; >> 2. I mount a disk which connect to this raid. >> 3. hot remove the pci root bus. >> 4. hot add the pci root bus. >> 5. found the resource conflicts for the children pci devices under this root bus. >> >> pci_root_bus increase a refcount at pci_host_bridge. >> pci_root_bus decrease a refcount at pci_host_bridge in >> release_pcibus_dev() when pci_root_bus device refcount reach 0. >> >> pci_dev increase a refcount at pci_bus in pci_alloc_dev(). >> pci_dev decrease a refcount at pci_bus in pci_release_dev() >> when pci_dev refcount reach 0. >> >> If any pci device refcount cannot reach 0, then its pci_bus >> refcount cannot reach 0 too, the result is pci_host_bridge >> refcount cannot reach 0. > > . > -- 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
>>>> 1. For pci devices, we would release their resources in >>>> pci_destroy_dev() regardless of pci device refcount. >>>> 2. When we try to remove pci root bus, there is no devices >>>> need to use the pci_host_bridge resources again, release >>>> pci_host_bridge resources is safe. >>>> 3. In some cases, users woule make mistake, for example, >>>> user get a pci device(increase refcount), but forget to >>>> put this device, then if we do hotplug pci root bus, >>>> it would make all pci devices cannot work after hot add. >>> >>> Can you explain this a little more? Are you talking about a *driver* >>> that forgets to put the device? >> >> Yes, may some pci drivers make a mistake, the refcount control the device object >> release is fine, but I think move the mem resource release out is better. > > If this is caused by driver bugs, I think we need to fix the driver > bugs. Agree, but it's better if pci root bus hotplug could work well regardless of the drviers' bug. After we stop all pci devices, I think no one could uses the resources again, it's no need to free host bridge resource until its refcount reach 0. But this is just my personal opinion :) > > So far all I see here is "it works when I do this." What we need is > an argument for "it's correct to do this." It's certainly possible > that you're already making that argument and I'm just not > understanding it. > >>>> I found this issue in the following case: >>>> 1. I have a raid pci device in my system; >>>> 2. I mount a disk which connect to this raid. >>>> 3. hot remove the pci root bus. >>>> 4. hot add the pci root bus. >>>> 5. found the resource conflicts for the children pci devices under this root bus. >>>> >>>> pci_root_bus increase a refcount at pci_host_bridge. >>>> pci_root_bus decrease a refcount at pci_host_bridge in >>>> release_pcibus_dev() when pci_root_bus device refcount reach 0. >>>> >>>> pci_dev increase a refcount at pci_bus in pci_alloc_dev(). >>>> pci_dev decrease a refcount at pci_bus in pci_release_dev() >>>> when pci_dev refcount reach 0. >>>> >>>> If any pci device refcount cannot reach 0, then its pci_bus >>>> refcount cannot reach 0 too, the result is pci_host_bridge >>>> refcount cannot reach 0. >>> >>> . >>> >> > > . > -- 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
在 2016/8/25 5:48, Sinan Kaya 写道: > On 8/24/2016 5:39 PM, Bjorn Helgaas wrote: >>> Yes, may some pci drivers make a mistake, the refcount control the device object >>>> release is fine, but I think move the mem resource release out is better. >> If this is caused by driver bugs, I think we need to fix the driver >> bugs. >> >> So far all I see here is "it works when I do this." What we need is >> an argument for "it's correct to do this." It's certainly possible >> that you're already making that argument and I'm just not >> understanding it. >> > > I also want to point out that the sysfs is not the only device removal > path. As Bjorn pointed out, you were moving resource free code into the sysfs > path and breaking other path (ACPI hotplug). No, this changes is also in ACPI hotplug path, but I found antoher problem, now host bridge resource allocated pci_acpi_root_add_resources(), if we move the free into pci_remove_root_bus(), it's not symmetrical as Bjorn pointed out. This may introudce some problem, E.g. if we created host bridge, but create root bus fail, in this case ,we have no way to free host bridge resource again. :( > -- 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
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index d144168..c5142d0 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -839,17 +839,6 @@ static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info) static void acpi_pci_root_release_info(struct pci_host_bridge *bridge) { - struct resource *res; - struct resource_entry *entry; - - resource_list_for_each_entry(entry, &bridge->windows) { - res = entry->res; - if (res->flags & IORESOURCE_IO) - pci_unmap_iospace(res); - if (res->parent && - (res->flags & (IORESOURCE_MEM | IORESOURCE_IO))) - release_resource(res); - } __acpi_pci_root_release_info(bridge->release_data); } diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c index 5f4a2e0..3594967 100644 --- a/drivers/pci/host-bridge.c +++ b/drivers/pci/host-bridge.c @@ -8,6 +8,21 @@ #include "pci.h" +void release_host_bridge_resources(struct pci_host_bridge *bridge) +{ + struct resource *res; + struct resource_entry *entry; + + resource_list_for_each_entry(entry, &bridge->windows) { + res = entry->res; + if (res->flags & IORESOURCE_IO) + pci_unmap_iospace(res); + if (res->parent && + (res->flags & (IORESOURCE_MEM | IORESOURCE_IO))) + release_resource(res); + } +} + static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) { while (bus->parent) diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 9730c47..46725d9 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -11,7 +11,7 @@ extern const unsigned char pcie_link_speed[]; bool pcie_cap_has_lnkctl(const struct pci_dev *dev); /* Functions internal to the PCI core code */ - +void release_host_bridge_resources(struct pci_host_bridge *bridge); int pci_create_sysfs_dev_files(struct pci_dev *pdev); void pci_remove_sysfs_dev_files(struct pci_dev *pdev); #if !defined(CONFIG_DMI) && !defined(CONFIG_ACPI) diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index d1ef7ac..a42b396 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -161,6 +161,7 @@ void pci_remove_root_bus(struct pci_bus *bus) pci_remove_bus(bus); host_bridge->bus = NULL; + release_host_bridge_resources(host_bridge); /* remove the host bridge */ device_unregister(&host_bridge->dev); }
pci_host_bridge holds the top resources(IO port/Mem/bus), now we release pci_host_bridge resources in acpi_pci_root_release_info() which would be called when pci_host_bridge device refcount reach 0. In some cases, pci_host_bridge refcount cannot reach 0 after we remove pci root bus in pci_remove_root_bus(). Then if we want to hot add pci root bus, we cannot use pci_host_bridge resources because of conflicts with old resources which are still in system. I think this is not reasonable. 1. For pci devices, we would release their resources in pci_destroy_dev() regardless of pci device refcount. 2. When we try to remove pci root bus, there is no devices need to use the pci_host_bridge resources again, release pci_host_bridge resources is safe. 3. In some cases, users woule make mistake, for example, user get a pci device(increase refcount), but forget to put this device, then if we do hotplug pci root bus, it would make all pci devices cannot work after hot add. I found this issue in the following case: 1. I have a raid pci device in my system; 2. I mount a disk which connect to this raid. 3. hot remove the pci root bus. 4. hot add the pci root bus. 5. found the resource conflicts for the children pci devices under this root bus. pci_root_bus increase a refcount at pci_host_bridge. pci_root_bus decrease a refcount at pci_host_bridge in release_pcibus_dev() when pci_root_bus device refcount reach 0. pci_dev increase a refcount at pci_bus in pci_alloc_dev(). pci_dev decrease a refcount at pci_bus in pci_release_dev() when pci_dev refcount reach 0. If any pci device refcount cannot reach 0, then its pci_bus refcount cannot reach 0 too, the result is pci_host_bridge refcount cannot reach 0. Signed-off-by: Yijing Wang <wangyijing@huawei.com> --- drivers/acpi/pci_root.c | 11 ----------- drivers/pci/host-bridge.c | 15 +++++++++++++++ drivers/pci/pci.h | 2 +- drivers/pci/remove.c | 1 + 4 files changed, 17 insertions(+), 12 deletions(-) -- 1.7.1 -- 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