Re: [PATCH 1/2] pci: remove pci_mmap_page_range wrapper

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jul 15, 2022 at 05:36:16PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The ARCH_GENERIC_PCI_MMAP_RESOURCE symbol came up in a recent discussion,
> and I noticed that this was left behind by an unfinished cleanup from
> 2017.
> 
> The only architecture that still relies on providing its
> own pci_mmap_page_range() helper instead of using the generic
> pci_mmap_resource_range() is sparc. Presumably the reasons for this have
> not changed, but at least this can be simplified by converting sparc to
> use the same interface as the others.
> 
> The only difference between the two is the device specific offset that
> gets added to or subtracted from vma->vm_pgoff.
> 
> Change the only caller of pci_mmap_page_range() in common code to subtract
> this offset and call the modern interface, while adding it back in the
> sparc implementation to preserve the existing behavior.
> 
> This removes the complexities of the dual interfaces from the common code,
> and keeps it all specific to the sparc architecture code. According to
> David Miller, the sparc code lets user space poke into the VGA I/O port
> registers by mmapping the I/O space of the parent bridge device, which is
> something that the generic pci_mmap_resource_range() code apparently
> does not.
> 
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Stafford Horne <shorne@gmail.com>
> Link: https://lore.kernel.org/lkml/1519887203.622.3.camel@infradead.org/t/
> Link: https://lore.kernel.org/lkml/20220714214657.2402250-3-shorne@gmail.com/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I applied both these patches on a pci/resource branch for v5.20,
thanks!

Sparc folks, let me know if you object or prefer to take these via
your tree.

> ---
>  Documentation/PCI/sysfs-pci.rst |  2 +-
>  arch/sparc/kernel/pci.c         | 13 +++++++---
>  drivers/pci/mmap.c              | 44 ---------------------------------
>  drivers/pci/proc.c              |  7 +++++-
>  include/linux/pci.h             | 12 +--------
>  5 files changed, 17 insertions(+), 61 deletions(-)
> 
> diff --git a/Documentation/PCI/sysfs-pci.rst b/Documentation/PCI/sysfs-pci.rst
> index 742fbd21dc1f..f495185aa88a 100644
> --- a/Documentation/PCI/sysfs-pci.rst
> +++ b/Documentation/PCI/sysfs-pci.rst
> @@ -125,7 +125,7 @@ implementation of that functionality. To support the historical interface of
>  mmap() through files in /proc/bus/pci, platforms may also set HAVE_PCI_MMAP.
>  
>  Alternatively, platforms which set HAVE_PCI_MMAP may provide their own
> -implementation of pci_mmap_page_range() instead of defining
> +implementation of pci_mmap_resource_range() instead of defining
>  ARCH_GENERIC_PCI_MMAP_RESOURCE.
>  
>  Platforms which support write-combining maps of PCI resources must define
> diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
> index 31b0c1983286..f580db840bf7 100644
> --- a/arch/sparc/kernel/pci.c
> +++ b/arch/sparc/kernel/pci.c
> @@ -876,17 +876,22 @@ static void __pci_mmap_set_pgprot(struct pci_dev *dev, struct vm_area_struct *vm
>  
>  /* Perform the actual remap of the pages for a PCI device mapping, as appropriate
>   * for this architecture.  The region in the process to map is described by vm_start
> - * and vm_end members of VMA, the base physical address is found in vm_pgoff.
> + * and vm_end members of VMA, the BAR relative address is found in vm_pgoff.
>   * The pci device structure is provided so that architectures may make mapping
>   * decisions on a per-device or per-bus basis.
>   *
>   * Returns a negative error code on failure, zero on success.
>   */
> -int pci_mmap_page_range(struct pci_dev *dev, int bar,
> -			struct vm_area_struct *vma,
> -			enum pci_mmap_state mmap_state, int write_combine)
> +int pci_mmap_resource_range(struct pci_dev *dev, int bar,
> +			    struct vm_area_struct *vma,
> +			    enum pci_mmap_state mmap_state, int write_combine)
>  {
>  	int ret;
> +	resource_size_t start, end;
> +
> +	/* convert per-BAR address to PCI bus address */
> +	pci_resource_to_user(dev, bar, &dev->resource[bar], &start, &end);
> +	vma->vm_pgoff += start >> PAGE_SHIFT;
>  
>  	ret = __pci_mmap_make_offset(dev, vma, mmap_state);
>  	if (ret < 0)
> diff --git a/drivers/pci/mmap.c b/drivers/pci/mmap.c
> index b8c9011987f4..4504039056d1 100644
> --- a/drivers/pci/mmap.c
> +++ b/drivers/pci/mmap.c
> @@ -13,27 +13,6 @@
>  
>  #ifdef ARCH_GENERIC_PCI_MMAP_RESOURCE
>  
> -/*
> - * Modern setup: generic pci_mmap_resource_range(), and implement the legacy
> - * pci_mmap_page_range() (if needed) as a wrapper round it.
> - */
> -
> -#ifdef HAVE_PCI_MMAP
> -int pci_mmap_page_range(struct pci_dev *pdev, int bar,
> -			struct vm_area_struct *vma,
> -			enum pci_mmap_state mmap_state, int write_combine)
> -{
> -	resource_size_t start, end;
> -
> -	pci_resource_to_user(pdev, bar, &pdev->resource[bar], &start, &end);
> -
> -	/* Adjust vm_pgoff to be the offset within the resource */
> -	vma->vm_pgoff -= start >> PAGE_SHIFT;
> -	return pci_mmap_resource_range(pdev, bar, vma, mmap_state,
> -				       write_combine);
> -}
> -#endif
> -
>  static const struct vm_operations_struct pci_phys_vm_ops = {
>  #ifdef CONFIG_HAVE_IOREMAP_PROT
>  	.access = generic_access_phys,
> @@ -70,27 +49,4 @@ int pci_mmap_resource_range(struct pci_dev *pdev, int bar,
>  				  vma->vm_page_prot);
>  }
>  
> -#elif defined(HAVE_PCI_MMAP) /* && !ARCH_GENERIC_PCI_MMAP_RESOURCE */
> -
> -/*
> - * Legacy setup: Implement pci_mmap_resource_range() as a wrapper around
> - * the architecture's pci_mmap_page_range(), converting to "user visible"
> - * addresses as necessary.
> - */
> -
> -int pci_mmap_resource_range(struct pci_dev *pdev, int bar,
> -			    struct vm_area_struct *vma,
> -			    enum pci_mmap_state mmap_state, int write_combine)
> -{
> -	resource_size_t start, end;
> -
> -	/*
> -	 * pci_mmap_page_range() expects the same kind of entry as coming
> -	 * from /proc/bus/pci/ which is a "user visible" value. If this is
> -	 * different from the resource itself, arch will do necessary fixup.
> -	 */
> -	pci_resource_to_user(pdev, bar, &pdev->resource[bar], &start, &end);
> -	vma->vm_pgoff += start >> PAGE_SHIFT;
> -	return pci_mmap_page_range(pdev, bar, vma, mmap_state, write_combine);
> -}
>  #endif
> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index 31b26d8ea6cc..f967709082d6 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -244,6 +244,7 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	struct pci_dev *dev = pde_data(file_inode(file));
>  	struct pci_filp_private *fpriv = file->private_data;
> +	resource_size_t start, end;
>  	int i, ret, write_combine = 0, res_bit = IORESOURCE_MEM;
>  
>  	if (!capable(CAP_SYS_RAWIO) ||
> @@ -278,7 +279,11 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
>  	    iomem_is_exclusive(dev->resource[i].start))
>  		return -EINVAL;
>  
> -	ret = pci_mmap_page_range(dev, i, vma,
> +	pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
> +
> +	/* Adjust vm_pgoff to be the offset within the resource */
> +	vma->vm_pgoff -= start >> PAGE_SHIFT;
> +	ret = pci_mmap_resource_range(dev, i, vma,
>  				  fpriv->mmap_state, write_combine);
>  	if (ret < 0)
>  		return ret;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 81a57b498f22..060af91bafcd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1909,24 +1909,14 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>  
>  #include <asm/pci.h>
>  
> -/* These two functions provide almost identical functionality. Depending
> - * on the architecture, one will be implemented as a wrapper around the
> - * other (in drivers/pci/mmap.c).
> - *
> +/*
>   * pci_mmap_resource_range() maps a specific BAR, and vm->vm_pgoff
>   * is expected to be an offset within that region.
>   *
> - * pci_mmap_page_range() is the legacy architecture-specific interface,
> - * which accepts a "user visible" resource address converted by
> - * pci_resource_to_user(), as used in the legacy mmap() interface in
> - * /proc/bus/pci/.
>   */
>  int pci_mmap_resource_range(struct pci_dev *dev, int bar,
>  			    struct vm_area_struct *vma,
>  			    enum pci_mmap_state mmap_state, int write_combine);
> -int pci_mmap_page_range(struct pci_dev *pdev, int bar,
> -			struct vm_area_struct *vma,
> -			enum pci_mmap_state mmap_state, int write_combine);
>  
>  #ifndef arch_can_pci_mmap_wc
>  #define arch_can_pci_mmap_wc()		0
> -- 
> 2.29.2
>