diff mbox series

[v4,09/15] pci: Consolidate pci_iomap* and pci_iomap*wc

Message ID 20210805005218.2912076-10-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Add TDX Guest Support (shared-mm support) | expand

Commit Message

Kuppuswamy Sathyanarayanan Aug. 5, 2021, 12:52 a.m. UTC 
From: Andi Kleen <ak@linux.intel.com>

pci_iomap* and pci_iomap*wc are currently duplicated code, except
that the _wc variant does not support IO ports. Replace them
with a common helper and a callback for the mapping. I used
wrappers for the maps because some architectures implement ioremap
and friends with macros.

This will allow to add more variants without excessive code
duplications. This patch should have no behavior change.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 lib/pci_iomap.c | 81 +++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 37 deletions(-)

Comments

Bjorn Helgaas Aug. 12, 2021, 7:43 p.m. UTC | #1 
Is there a branch with all of this applied?  I was going to apply this
to help take a look at it, but it doesn't apply to v5.14-rc1.  I know
you listed some prereqs in the cover letter, but it's a fair amount of
work to sort all that out.

On Wed, Aug 04, 2021 at 05:52:12PM -0700, Kuppuswamy Sathyanarayanan wrote:
> From: Andi Kleen <ak@linux.intel.com>

If I were applying these, I would silently update the subject lines to
match previous commits.  Since these will probably be merged via a
different tree, you can update if there's a v5:

  PCI: Consolidate pci_iomap_range(), pci_iomap_wc_range()

Also applies to 11/15 and 12/15.

> pci_iomap* and pci_iomap*wc are currently duplicated code, except
> that the _wc variant does not support IO ports. Replace them
> with a common helper and a callback for the mapping. I used
> wrappers for the maps because some architectures implement ioremap
> and friends with macros.

Maybe spell some of this out:

  pci_iomap_range() and pci_iomap_wc_range() are currently duplicated
  code, ...  Implement them using a common helper,
  pci_iomap_range_map(), ...

Using "pci_iomap*" obscures the name and doesn't save any space.

Why is it safe to make pci_iomap_wc_range() support IO ports when it
didn't before?  That might be desirable, but I think it *is* a
functional change here.

IIUC, pci_iomap_wc_range() on an IO port range previously returned
NULL, and after this patch it will work the same as pci_iomap_range(),
i.e., it will return the result of __pci_ioport_map().

> This will allow to add more variants without excessive code
> duplications. This patch should have no behavior change.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  lib/pci_iomap.c | 81 +++++++++++++++++++++++++++----------------------
>  1 file changed, 44 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
> index 2d3eb1cb73b8..6251c3f651c6 100644
> --- a/lib/pci_iomap.c
> +++ b/lib/pci_iomap.c
> @@ -10,6 +10,46 @@
>  #include <linux/export.h>
>  
>  #ifdef CONFIG_PCI
> +
> +/*
> + * Callback wrappers because some architectures define ioremap et.al.
> + * as macros.
> + */
> +static void __iomem *map_ioremap(phys_addr_t addr, size_t size)
> +{
> +	return ioremap(addr, size);
> +}
> +
> +static void __iomem *map_ioremap_wc(phys_addr_t addr, size_t size)
> +{
> +	return ioremap_wc(addr, size);
> +}
> +
> +static void __iomem *pci_iomap_range_map(struct pci_dev *dev,
> +					 int bar,
> +					 unsigned long offset,
> +					 unsigned long maxlen,
> +					 void __iomem *(*mapm)(phys_addr_t,
> +							       size_t))
> +{
> +	resource_size_t start = pci_resource_start(dev, bar);
> +	resource_size_t len = pci_resource_len(dev, bar);
> +	unsigned long flags = pci_resource_flags(dev, bar);
> +
> +	if (len <= offset || !start)
> +		return NULL;
> +	len -= offset;
> +	start += offset;
> +	if (maxlen && len > maxlen)
> +		len = maxlen;
> +	if (flags & IORESOURCE_IO)
> +		return __pci_ioport_map(dev, start, len);
> +	if (flags & IORESOURCE_MEM)
> +		return mapm(start, len);
> +	/* What? */
> +	return NULL;
> +}
> +
>  /**
>   * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
>   * @dev: PCI device that owns the BAR
> @@ -30,22 +70,8 @@ void __iomem *pci_iomap_range(struct pci_dev *dev,
>  			      unsigned long offset,
>  			      unsigned long maxlen)
>  {
> -	resource_size_t start = pci_resource_start(dev, bar);
> -	resource_size_t len = pci_resource_len(dev, bar);
> -	unsigned long flags = pci_resource_flags(dev, bar);
> -
> -	if (len <= offset || !start)
> -		return NULL;
> -	len -= offset;
> -	start += offset;
> -	if (maxlen && len > maxlen)
> -		len = maxlen;
> -	if (flags & IORESOURCE_IO)
> -		return __pci_ioport_map(dev, start, len);
> -	if (flags & IORESOURCE_MEM)
> -		return ioremap(start, len);
> -	/* What? */
> -	return NULL;
> +	return pci_iomap_range_map(dev, bar, offset, maxlen,
> +				   map_ioremap);
>  }
>  EXPORT_SYMBOL(pci_iomap_range);
>  
> @@ -70,27 +96,8 @@ void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
>  				 unsigned long offset,
>  				 unsigned long maxlen)
>  {
> -	resource_size_t start = pci_resource_start(dev, bar);
> -	resource_size_t len = pci_resource_len(dev, bar);
> -	unsigned long flags = pci_resource_flags(dev, bar);
> -
> -
> -	if (flags & IORESOURCE_IO)
> -		return NULL;
> -
> -	if (len <= offset || !start)
> -		return NULL;
> -
> -	len -= offset;
> -	start += offset;
> -	if (maxlen && len > maxlen)
> -		len = maxlen;
> -
> -	if (flags & IORESOURCE_MEM)
> -		return ioremap_wc(start, len);
> -
> -	/* What? */
> -	return NULL;
> +	return pci_iomap_range_map(dev, bar, offset, maxlen,
> +				   map_ioremap_wc);
>  }
>  EXPORT_SYMBOL_GPL(pci_iomap_wc_range);
>  
> -- 
> 2.25.1
>
Andi Kleen Aug. 12, 2021, 10:11 p.m. UTC | #2 
> Why is it safe to make pci_iomap_wc_range() support IO ports when it
> didn't before?  That might be desirable, but I think it *is* a
> functional change here.

None of the callers use it to map IO ports, so it will be a no-op for 
them. But you're right, it's a (minor) functional change.

-Andi
Kuppuswamy Sathyanarayanan Aug. 12, 2021, 10:29 p.m. UTC | #3 
On 8/12/21 12:43 PM, Bjorn Helgaas wrote:
> Is there a branch with all of this applied?  I was going to apply this

Its is maintained in following tree.

https://github.com/intel/tdx/commit/93fd5b655172ba9e3350487995102a8b2c41de27

> to help take a look at it, but it doesn't apply to v5.14-rc1.  I know

This patch can be applied independently. I have just applied it on top
of v5.14-rc5, and it seems to apply clean. Can you try -rc5?

> you listed some prereqs in the cover letter, but it's a fair amount of
> work to sort all that out.
> 
> On Wed, Aug 04, 2021 at 05:52:12PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> From: Andi Kleen <ak@linux.intel.com>
> 
> If I were applying these, I would silently update the subject lines to
> match previous commits.  Since these will probably be merged via a
> different tree, you can update if there's a v5:
> 
>    PCI: Consolidate pci_iomap_range(), pci_iomap_wc_range()

Yes. I will fix this in next version.

> 
> Also applies to 11/15 and 12/15.

Will do the same.

> 
>> pci_iomap* and pci_iomap*wc are currently duplicated code, except
>> that the _wc variant does not support IO ports. Replace them
>> with a common helper and a callback for the mapping. I used
>> wrappers for the maps because some architectures implement ioremap
>> and friends with macros.
> 
> Maybe spell some of this out:
> 
>    pci_iomap_range() and pci_iomap_wc_range() are currently duplicated
>    code, ...  Implement them using a common helper,
>    pci_iomap_range_map(), ...
> 
> Using "pci_iomap*" obscures the name and doesn't save any space.
> 
> Why is it safe to make pci_iomap_wc_range() support IO ports when it
> didn't before?  That might be desirable, but I think it *is* a
> functional change here.

Agree. Commit log had to be updated. I will include these details
in next submission.

> 
> IIUC, pci_iomap_wc_range() on an IO port range previously returned
> NULL, and after this patch it will work the same as pci_iomap_range(),
> i.e., it will return the result of __pci_ioport_map().
> 
>> This will allow to add more variants without excessive code
>> duplications. This patch should have no behavior change.
>>
>> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   lib/pci_iomap.c | 81 +++++++++++++++++++++++++++----------------------
>>   1 file changed, 44 insertions(+), 37 deletions(-)
>>
diff mbox series

Patch

diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index 2d3eb1cb73b8..6251c3f651c6 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -10,6 +10,46 @@ 
 #include <linux/export.h>
 
 #ifdef CONFIG_PCI
+
+/*
+ * Callback wrappers because some architectures define ioremap et.al.
+ * as macros.
+ */
+static void __iomem *map_ioremap(phys_addr_t addr, size_t size)
+{
+	return ioremap(addr, size);
+}
+
+static void __iomem *map_ioremap_wc(phys_addr_t addr, size_t size)
+{
+	return ioremap_wc(addr, size);
+}
+
+static void __iomem *pci_iomap_range_map(struct pci_dev *dev,
+					 int bar,
+					 unsigned long offset,
+					 unsigned long maxlen,
+					 void __iomem *(*mapm)(phys_addr_t,
+							       size_t))
+{
+	resource_size_t start = pci_resource_start(dev, bar);
+	resource_size_t len = pci_resource_len(dev, bar);
+	unsigned long flags = pci_resource_flags(dev, bar);
+
+	if (len <= offset || !start)
+		return NULL;
+	len -= offset;
+	start += offset;
+	if (maxlen && len > maxlen)
+		len = maxlen;
+	if (flags & IORESOURCE_IO)
+		return __pci_ioport_map(dev, start, len);
+	if (flags & IORESOURCE_MEM)
+		return mapm(start, len);
+	/* What? */
+	return NULL;
+}
+
 /**
  * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
  * @dev: PCI device that owns the BAR
@@ -30,22 +70,8 @@  void __iomem *pci_iomap_range(struct pci_dev *dev,
 			      unsigned long offset,
 			      unsigned long maxlen)
 {
-	resource_size_t start = pci_resource_start(dev, bar);
-	resource_size_t len = pci_resource_len(dev, bar);
-	unsigned long flags = pci_resource_flags(dev, bar);
-
-	if (len <= offset || !start)
-		return NULL;
-	len -= offset;
-	start += offset;
-	if (maxlen && len > maxlen)
-		len = maxlen;
-	if (flags & IORESOURCE_IO)
-		return __pci_ioport_map(dev, start, len);
-	if (flags & IORESOURCE_MEM)
-		return ioremap(start, len);
-	/* What? */
-	return NULL;
+	return pci_iomap_range_map(dev, bar, offset, maxlen,
+				   map_ioremap);
 }
 EXPORT_SYMBOL(pci_iomap_range);
 
@@ -70,27 +96,8 @@  void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
 				 unsigned long offset,
 				 unsigned long maxlen)
 {
-	resource_size_t start = pci_resource_start(dev, bar);
-	resource_size_t len = pci_resource_len(dev, bar);
-	unsigned long flags = pci_resource_flags(dev, bar);
-
-
-	if (flags & IORESOURCE_IO)
-		return NULL;
-
-	if (len <= offset || !start)
-		return NULL;
-
-	len -= offset;
-	start += offset;
-	if (maxlen && len > maxlen)
-		len = maxlen;
-
-	if (flags & IORESOURCE_MEM)
-		return ioremap_wc(start, len);
-
-	/* What? */
-	return NULL;
+	return pci_iomap_range_map(dev, bar, offset, maxlen,
+				   map_ioremap_wc);
 }
 EXPORT_SYMBOL_GPL(pci_iomap_wc_range);