[PATCH 1/2] pci: add pci_unmap_iospace function for PCI_IOBASE

[PATCH 1/2] pci: add pci_unmap_iospace function for PCI_IOBASE

[Sinan Kaya]

The PCI_IOBASE needs to be released after hotplug removal so that it can be
re-added back by the pci_remap_iospace function during insertion.

Adding unmap function to follow IO remap function.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c   | 25 +++++++++++++++++++++++++
 include/linux/pci.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3a516c0..f5faed2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -26,6 +26,7 @@
 #include <linux/device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pci_hotplug.h>
+#include <linux/vmalloc.h>
 #include <asm-generic/pci-bridge.h>
 #include <asm/setup.h>
 #include <linux/aer.h>
@@ -3169,6 +3170,30 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 #endif
 }
 
+/**
+ *	pci_unmap_iospace - Unmap the memory mapped I/O space
+ *	@virt_addr: virtual address to be unmapped
+ *	@size: size of the physical address to be unmapped
+ *
+ *	Unmap the CPU virtual address @virt_addr from virtual address space.
+ *	Only architectures that have memory mapped IO functions defined
+ *	(and the PCI_IOBASE value defined) should call this function.
+ */
+void  __weak pci_unmap_iospace(struct resource *res)
+{
+#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
+	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
+
+	unmap_kernel_range(vaddr, resource_size(res));
+#else
+	/*
+	 * This architecture does not have memory mapped I/O space,
+	 * so this function should never be called.
+	 */
+	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
+#endif
+}
+
 static void __pci_set_master(struct pci_dev *dev, bool enable)
 {
 	u16 old_cmd, cmd;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 398ae7e..c6e3f0e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1172,6 +1172,7 @@ int pci_register_io_range(phys_addr_t addr, resource_size_t size);
 unsigned long pci_address_to_pio(phys_addr_t addr);
 phys_addr_t pci_pio_to_address(unsigned long pio);
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
+void pci_unmap_iospace(struct resource *res);
 
 static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
 {
-- 
1.8.2.1

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

[Sinan Kaya]

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(+)

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

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

[Bjorn Helgaas]

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

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

[Sinan Kaya]

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

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

[Bjorn Helgaas]

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

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

[Sinan Kaya]

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

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

[Tomasz Nowicki]

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

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

[Sinan Kaya]

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

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

[Tomasz Nowicki]

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

Re: [PATCH 1/2] pci: add pci_unmap_iospace function for PCI_IOBASE

[Sinan Kaya]

Bjorn,
Any feedback here?

On 3/7/2016 5:21 PM, Sinan Kaya wrote:
> The PCI_IOBASE needs to be released after hotplug removal so that it can be
> re-added back by the pci_remap_iospace function during insertion.
> 
> Adding unmap function to follow IO remap function.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c   | 25 +++++++++++++++++++++++++
>  include/linux/pci.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3a516c0..f5faed2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -26,6 +26,7 @@
>  #include <linux/device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci_hotplug.h>
> +#include <linux/vmalloc.h>
>  #include <asm-generic/pci-bridge.h>
>  #include <asm/setup.h>
>  #include <linux/aer.h>
> @@ -3169,6 +3170,30 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  #endif
>  }
>  
> +/**
> + *	pci_unmap_iospace - Unmap the memory mapped I/O space
> + *	@virt_addr: virtual address to be unmapped
> + *	@size: size of the physical address to be unmapped
> + *
> + *	Unmap the CPU virtual address @virt_addr from virtual address space.
> + *	Only architectures that have memory mapped IO functions defined
> + *	(and the PCI_IOBASE value defined) should call this function.
> + */
> +void  __weak pci_unmap_iospace(struct resource *res)
> +{
> +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
> +	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
> +
> +	unmap_kernel_range(vaddr, resource_size(res));
> +#else
> +	/*
> +	 * This architecture does not have memory mapped I/O space,
> +	 * so this function should never be called.
> +	 */
> +	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> +#endif
> +}
> +
>  static void __pci_set_master(struct pci_dev *dev, bool enable)
>  {
>  	u16 old_cmd, cmd;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 398ae7e..c6e3f0e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1172,6 +1172,7 @@ int pci_register_io_range(phys_addr_t addr, resource_size_t size);
>  unsigned long pci_address_to_pio(phys_addr_t addr);
>  phys_addr_t pci_pio_to_address(unsigned long pio);
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
> +void pci_unmap_iospace(struct resource *res);
>  
>  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
>  {
> 

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

Re: [PATCH 1/2] pci: add pci_unmap_iospace function for PCI_IOBASE

[Bjorn Helgaas]

Hi Sinan,

On Mon, Mar 07, 2016 at 05:21:49PM -0500, Sinan Kaya wrote:
> The PCI_IOBASE needs to be released after hotplug removal so that it can be
> re-added back by the pci_remap_iospace function during insertion.
> 
> Adding unmap function to follow IO remap function.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c   | 25 +++++++++++++++++++++++++
>  include/linux/pci.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3a516c0..f5faed2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -26,6 +26,7 @@
>  #include <linux/device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci_hotplug.h>
> +#include <linux/vmalloc.h>
>  #include <asm-generic/pci-bridge.h>
>  #include <asm/setup.h>
>  #include <linux/aer.h>
> @@ -3169,6 +3170,30 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  #endif
>  }
>  
> +/**
> + *	pci_unmap_iospace - Unmap the memory mapped I/O space
> + *	@virt_addr: virtual address to be unmapped
> + *	@size: size of the physical address to be unmapped
> + *
> + *	Unmap the CPU virtual address @virt_addr from virtual address space.
> + *	Only architectures that have memory mapped IO functions defined
> + *	(and the PCI_IOBASE value defined) should call this function.
> + */
> +void  __weak pci_unmap_iospace(struct resource *res)

Why is this weak?  I assume probably because pci_remap_iospace() is
weak, but I don't see any reason why *that* needs to be weak.  There's
only one implementation.  I think neither one should be weak unless we
have an actual need for that.

> +{
> +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
> +	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
> +
> +	unmap_kernel_range(vaddr, resource_size(res));

There really aren't any other generic uses of unmap_kernel_range().
This isn't an unusual scenario, so I would expect this code to use a
pattern that's used elsewhere.

> +#else
> +	/*
> +	 * This architecture does not have memory mapped I/O space,
> +	 * so this function should never be called.
> +	 */
> +	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> +#endif
> +}
> +
>  static void __pci_set_master(struct pci_dev *dev, bool enable)
>  {
>  	u16 old_cmd, cmd;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 398ae7e..c6e3f0e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1172,6 +1172,7 @@ int pci_register_io_range(phys_addr_t addr, resource_size_t size);
>  unsigned long pci_address_to_pio(phys_addr_t addr);
>  phys_addr_t pci_pio_to_address(unsigned long pio);
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
> +void pci_unmap_iospace(struct resource *res);
>  
>  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
>  {
> -- 
> 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

Re: [PATCH 1/2] pci: add pci_unmap_iospace function for PCI_IOBASE

[Sinan Kaya]

On 4/7/2016 12:00 PM, Bjorn Helgaas wrote:
> Hi Sinan,
> 
> On Mon, Mar 07, 2016 at 05:21:49PM -0500, Sinan Kaya wrote:
>> The PCI_IOBASE needs to be released after hotplug removal so that it can be
>> re-added back by the pci_remap_iospace function during insertion.
>>
>> Adding unmap function to follow IO remap function.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/pci/pci.c   | 25 +++++++++++++++++++++++++
>>  include/linux/pci.h |  1 +
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 3a516c0..f5faed2 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/device.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/pci_hotplug.h>
>> +#include <linux/vmalloc.h>
>>  #include <asm-generic/pci-bridge.h>
>>  #include <asm/setup.h>
>>  #include <linux/aer.h>
>> @@ -3169,6 +3170,30 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>>  #endif
>>  }
>>  
>> +/**
>> + *	pci_unmap_iospace - Unmap the memory mapped I/O space
>> + *	@virt_addr: virtual address to be unmapped
>> + *	@size: size of the physical address to be unmapped
>> + *
>> + *	Unmap the CPU virtual address @virt_addr from virtual address space.
>> + *	Only architectures that have memory mapped IO functions defined
>> + *	(and the PCI_IOBASE value defined) should call this function.
>> + */
>> +void  __weak pci_unmap_iospace(struct resource *res)
> 
> Why is this weak?  I assume probably because pci_remap_iospace() is
> weak, but I don't see any reason why *that* needs to be weak.  There's
> only one implementation.  I think neither one should be weak unless we
> have an actual need for that.
> 

Right, copy paste mistake. Even the function parameter description above
is not right. 

I can get rid of the __weak from both on the next iteration.

>> +{
>> +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
>> +	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
>> +
>> +	unmap_kernel_range(vaddr, resource_size(res));
> 
> There really aren't any other generic uses of unmap_kernel_range().
> This isn't an unusual scenario, so I would expect this code to use a
> pattern that's used elsewhere.

OK, What's the best way to remove a mapping? I'm open for suggestions.
I copied this pattern from GHES driver.

> 
>> +#else
>> +	/*
>> +	 * This architecture does not have memory mapped I/O space,
>> +	 * so this function should never be called.
>> +	 */
>> +	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
>> +#endif
>> +}
>> +
>>  static void __pci_set_master(struct pci_dev *dev, bool enable)
>>  {
>>  	u16 old_cmd, cmd;
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 398ae7e..c6e3f0e 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1172,6 +1172,7 @@ int pci_register_io_range(phys_addr_t addr, resource_size_t size);
>>  unsigned long pci_address_to_pio(phys_addr_t addr);
>>  phys_addr_t pci_pio_to_address(unsigned long pio);
>>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
>> +void pci_unmap_iospace(struct resource *res);
>>  
>>  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
>>  {
>> -- 
>> 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
> 


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