diff mbox

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

Message ID 1457389310-3538-1-git-send-email-okaya@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sinan Kaya March 7, 2016, 10:21 p.m. UTC
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(+)

Comments

Sinan Kaya March 16, 2016, 11:14 p.m. UTC | #1
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
Bjorn Helgaas April 7, 2016, 4 p.m. UTC | #2
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
Sinan Kaya April 7, 2016, 5:49 p.m. UTC | #3
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
>
diff mbox

Patch

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