| Message ID | 1457389310-3538-2-git-send-email-okaya@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Sinan, On Mon, Mar 07, 2016 at 05:21:50PM -0500, Sinan Kaya wrote: > The ACPI PCI driver is leaking out memory mappings > when a slot is removed. Upon insertion following a > removal, we are hitting a BUG statement. Second call > to the remap API hits a bug statement because the area is > already mapped. This patch releases additional virtual > memory mapped by pci_remap_iospace function as part of > __release_pci_root_info function if the region type is IO. I don't know what "removing a slot" means. You're changing pci_root.c, so I assume this is really an ACPI host bridge removal? The release should correspond to a mapping, and the changelog should point out where that mapping happens so we can see the symmetry. You say this is undoing the effect of pci_remap_iospace(), but that's only called by native drivers and the generic (OF) driver, not by pci_root.c. Please combine this with the previous patch so we have the new function and its use in the same patch. > BUG: failure at kernel/lib/ioremap.c:31/ioremap_pte_range()! > Kernel panic - not syncing: BUG! > CPU: 1 PID: 630 Comm: kworker/u48:3 Not tainted > Workqueue: kacpi_hotplug acpi_hotplug_work_fn Call trace: > dump_backtrace+0x0/0x10c [<ffff800000086e58>] > show_stack+0x10/0x1c [<ffff8000004d1f50>] > dump_stack+0x74/0xc4 > panic+0xe4/0x21c [<ffff8000002577cc>] > ioremap_page_range+0x290/0x30c [<ffff8000002861dc>] > pci_remap_iospace+0x88/0xa0 [<ffff8000000908f8>] > setup_resource+0x114/0x16c [<ffff8000002c0fc0>] > acpi_walk_resource_buffer+0x54/0xb0 > acpi_walk_resources+0x90/0xbc > pci_acpi_scan_root+0x184/0x2d0 > acpi_pci_root_add+0x368/0x434 > acpi_bus_attach+0x124/0x22c [<ffff8000002a5628>] > acpi_bus_scan+0x58/0x74 [<ffff8000002a57c0>] > acpi_device_hotplug+0xc4/0x3f0 [<ffff8000002a0468>] > acpi_hotplug_work_fn+0x1c/0x34 [<ffff8000000b9310>] > process_one_work+0x1e8/0x308 [<ffff8000000b9b04>] > worker_thread+0x294/0x3cc [<ffff8000000bd9 > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/acpi/pci_root.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 71bbfae..6d4bc2d 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -946,6 +946,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge) > > 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); > -- > 1.8.2.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 -- 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
On 4/7/2016 12:06 PM, Bjorn Helgaas wrote: >> __release_pci_root_info function if the region type is IO. > I don't know what "removing a slot" means. You're changing > pci_root.c, so I assume this is really an ACPI host bridge removal? > Correct, I'm removing the host bridge. > The release should correspond to a mapping, and the changelog should > point out where that mapping happens so we can see the symmetry. > I apologize. This is based on Tomasz's v5 patch here. https://github.com/semihalf-nowicki-tomasz/linux/blob/pci-acpi-v5/drivers/acpi/pci_root.c > You say this is undoing the effect of pci_remap_iospace(), but that's > only called by native drivers and the generic (OF) driver, not by > pci_root.c. See the ACPI root bridge driver above. > > Please combine this with the previous patch so we have the new > function and its use in the same patch. > I can do that. I was trying to keep the reviews as small as possible.
On Thu, Apr 07, 2016 at 01:45:19PM -0400, Sinan Kaya wrote: > On 4/7/2016 12:06 PM, Bjorn Helgaas wrote: > >> __release_pci_root_info function if the region type is IO. > > I don't know what "removing a slot" means. You're changing > > pci_root.c, so I assume this is really an ACPI host bridge removal? > > > > Correct, I'm removing the host bridge. > > > The release should correspond to a mapping, and the changelog should > > point out where that mapping happens so we can see the symmetry. > > > > I apologize. This is based on Tomasz's v5 patch here. > > https://github.com/semihalf-nowicki-tomasz/linux/blob/pci-acpi-v5/drivers/acpi/pci_root.c > > > > You say this is undoing the effect of pci_remap_iospace(), but that's > > only called by native drivers and the generic (OF) driver, not by > > pci_root.c. > > See the ACPI root bridge driver above. If this is a fix to patches that haven't been merged yet, we need to squash the fix into the patches. > > Please combine this with the previous patch so we have the new > > function and its use in the same patch. > > I can do that. I was trying to keep the reviews as small as possible. The object is not to make patches as small as possible. The object is to make them easy to review, merge, bisect, revert, and backport. If we're adding something new and it's called by many arches or many drivers, we might have to split it up for merging through several trees or so we can revert pieces independently. But here there's only one caller and I don't think we get any benefit from splitting it. But I guess this is all moot since it should be squashed into whatever added the pci_remap_iospace() in the first place. 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
Hi Tomasz, On 4/7/2016 5:41 PM, Bjorn Helgaas wrote: >>> You say this is undoing the effect of pci_remap_iospace(), but that's >>> > > only called by native drivers and the generic (OF) driver, not by >>> > > pci_root.c. >> > >> > See the ACPI root bridge driver above. > If this is a fix to patches that haven't been merged yet, we need to > squash the fix into the patches. > Can you merge these to two patches to your series for the next post? I need to remove weak on the first patch per direction from Bjorn and fix the function comments. You could as well do this while you are merging. Let me know what your preference is.
Hi Sinan, On 08.04.2016 05:40, Sinan Kaya wrote: > Hi Tomasz, > > On 4/7/2016 5:41 PM, Bjorn Helgaas wrote: >>>> You say this is undoing the effect of pci_remap_iospace(), but that's >>>>>> only called by native drivers and the generic (OF) driver, not by >>>>>> pci_root.c. >>>> >>>> See the ACPI root bridge driver above. >> If this is a fix to patches that haven't been merged yet, we need to >> squash the fix into the patches. >> > > Can you merge these to two patches to your series for the next post? > > I need to remove weak on the first patch per direction from Bjorn and > fix the function comments. You could as well do this while you are merging. > > Let me know what your preference is. > Please do necessary fixes for your patches and send me the repo reference link. I will merge these to my patch set. Thanks! Tomasz -- 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
Hi Tomasz, On 4/8/2016 2:51 AM, Tomasz Nowicki wrote: > Hi Sinan, > > On 08.04.2016 05:40, Sinan Kaya wrote: >> Hi Tomasz, >> >> On 4/7/2016 5:41 PM, Bjorn Helgaas wrote: >>>>> You say this is undoing the effect of pci_remap_iospace(), but that's >>>>>>> only called by native drivers and the generic (OF) driver, not by >>>>>>> pci_root.c. >>>>> >>>>> See the ACPI root bridge driver above. >>> If this is a fix to patches that haven't been merged yet, we need to >>> squash the fix into the patches. >>> >> >> Can you merge these to two patches to your series for the next post? >> >> I need to remove weak on the first patch per direction from Bjorn and >> fix the function comments. You could as well do this while you are merging. >> >> Let me know what your preference is. >> > > Please do necessary fixes for your patches and send me the repo reference link. I will merge these to my patch set. Thanks! > > Tomasz > -- > 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 I posted the updated patch here. Changes are: - I squashed these two patches together per Bjorn's request. - Removed the weak declarations from both remap and unmap calls. - Fixed the doxygen document to match the actual parameters. https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=okaya/pciunmapv2&id=6120a5f0c5e6d757f18a076917fa202e2c9823d0 Sinan
On 08.04.2016 19:46, Sinan Kaya wrote: > Hi Tomasz, > > On 4/8/2016 2:51 AM, Tomasz Nowicki wrote: >> Hi Sinan, >> >> On 08.04.2016 05:40, Sinan Kaya wrote: >>> Hi Tomasz, >>> >>> On 4/7/2016 5:41 PM, Bjorn Helgaas wrote: >>>>>> You say this is undoing the effect of pci_remap_iospace(), but that's >>>>>>>> only called by native drivers and the generic (OF) driver, not by >>>>>>>> pci_root.c. >>>>>> >>>>>> See the ACPI root bridge driver above. >>>> If this is a fix to patches that haven't been merged yet, we need to >>>> squash the fix into the patches. >>>> >>> >>> Can you merge these to two patches to your series for the next post? >>> >>> I need to remove weak on the first patch per direction from Bjorn and >>> fix the function comments. You could as well do this while you are merging. >>> >>> Let me know what your preference is. >>> >> >> Please do necessary fixes for your patches and send me the repo reference link. I will merge these to my patch set. Thanks! >> >> Tomasz >> -- >> 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 > > I posted the updated patch here. Changes are: > > - I squashed these two patches together per Bjorn's request. > - Removed the weak declarations from both remap and unmap calls. > - Fixed the doxygen document to match the actual parameters. > > https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=okaya/pciunmapv2&id=6120a5f0c5e6d757f18a076917fa202e2c9823d0 > Thanks Sinan! Tomasz -- 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
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 71bbfae..6d4bc2d 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -946,6 +946,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge) 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);
The ACPI PCI driver is leaking out memory mappings when a slot is removed. Upon insertion following a removal, we are hitting a BUG statement. Second call to the remap API hits a bug statement because the area is already mapped. This patch releases additional virtual memory mapped by pci_remap_iospace function as part of __release_pci_root_info function if the region type is IO. BUG: failure at kernel/lib/ioremap.c:31/ioremap_pte_range()! Kernel panic - not syncing: BUG! CPU: 1 PID: 630 Comm: kworker/u48:3 Not tainted Workqueue: kacpi_hotplug acpi_hotplug_work_fn Call trace: dump_backtrace+0x0/0x10c [<ffff800000086e58>] show_stack+0x10/0x1c [<ffff8000004d1f50>] dump_stack+0x74/0xc4 panic+0xe4/0x21c [<ffff8000002577cc>] ioremap_page_range+0x290/0x30c [<ffff8000002861dc>] pci_remap_iospace+0x88/0xa0 [<ffff8000000908f8>] setup_resource+0x114/0x16c [<ffff8000002c0fc0>] acpi_walk_resource_buffer+0x54/0xb0 acpi_walk_resources+0x90/0xbc pci_acpi_scan_root+0x184/0x2d0 acpi_pci_root_add+0x368/0x434 acpi_bus_attach+0x124/0x22c [<ffff8000002a5628>] acpi_bus_scan+0x58/0x74 [<ffff8000002a57c0>] acpi_device_hotplug+0xc4/0x3f0 [<ffff8000002a0468>] acpi_hotplug_work_fn+0x1c/0x34 [<ffff8000000b9310>] process_one_work+0x1e8/0x308 [<ffff8000000b9b04>] worker_thread+0x294/0x3cc [<ffff8000000bd9 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/acpi/pci_root.c | 2 ++ 1 file changed, 2 insertions(+)