Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1373509 > unrolled thread

Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown Bjorn Helgaas <helgaas@kernel.org> - 2016-04-07 18:10 +0200
    Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown Sinan Kaya <okaya@codeaurora.org> - 2016-04-07 19:50 +0200
      Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown Bjorn Helgaas <helgaas@kernel.org> - 2016-04-07 23:50 +0200
        Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown Sinan Kaya <okaya@codeaurora.org> - 2016-04-08 05:50 +0200
          Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown Tomasz Nowicki <tn@semihalf.com> - 2016-04-08 09:00 +0200
            Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown Sinan Kaya <okaya@codeaurora.org> - 2016-04-08 19:50 +0200
              Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown Tomasz Nowicki <tn@semihalf.com> - 2016-04-09 10:50 +0200

#1373509 — Re: [PATCH 2/2] pci, acpi: free IO resource during shutdown

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

[toc] | [next] | [standalone]

#1373598

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.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1373799

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

[toc] | [prev] | [next] | [standalone]

#1373948

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.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1374019

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

[toc] | [prev] | [next] | [standalone]

#1374397

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

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1374588

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web