Date:	Fri, 14 Feb 2014 14:00:57 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	linux-pci@...r.kernel.org
Cc:	e1000-devel@...ts.sourceforge.net,
	Stephen Hemminger <stephen@...workplumber.org>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	linux-kernel@...r.kernel.org,
	Aaron F Brown <aaron.f.brown@...el.com>
Subject: Re: [PATCH 1/2] PCI: Remove unused MMIO exclusivity support

[+cc Aaron]

On Thu, Jan 30, 2014 at 12:20:38PM -0700, Bjorn Helgaas wrote:
> This reverts commit e8de1481fd71 ("resource: allow MMIO exclusivity for
> device drivers"), removing these exported interfaces:
> 
>   pci_request_region_exclusive()
>   pci_request_regions_exclusive()
>   pci_request_selected_regions_exclusive()
> 
> There's nothing wrong with the MMIO exclusivity code, but it is used only
> by the e1000e driver, and the only reason it's there is because it was
> added during a bug hunt.  The bug has been fixed, and apparently no other
> drivers have found it useful in the five years since then.
> 
> This is based on Stephen Hemminger's patch (see link below), but goes a bit
> further by removing the use in e1000e.
> 
> Link: http://lkml.kernel.org/r/20131227132710.7190647c@nehalam.linuxnetplumber.net
> Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
> CC: Arjan van de Ven <arjan@...ux.intel.com>
> CC: Stephen Hemminger <stephen@...workplumber.org>

I'm dropping this one for now because it's used by:

  - e1000e
  - alpha pci_mmap_resource()
  - sp5100_tco_setupdevice()

That's still sort of a marginal number of users, but in any event, it's
clear that a little more work would be required to remove it.

> ---
>  Documentation/kernel-parameters.txt        |    4 -
>  arch/x86/mm/init.c                         |    2 -
>  drivers/net/ethernet/intel/e1000e/netdev.c |    3 -
>  drivers/pci/pci-sysfs.c                    |    3 -
>  drivers/pci/pci.c                          |  112 +++-------------------------
>  include/linux/ioport.h                     |    5 -
>  include/linux/pci.h                        |    3 -
>  kernel/resource.c                          |   54 --------------
>  8 files changed, 14 insertions(+), 172 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 8f441dab0396..4943bddeacc1 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1323,10 +1323,6 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			no_x2apic_optout
>  				BIOS x2APIC opt-out request will be ignored
>  
> -	iomem=		Disable strict checking of access to MMIO memory
> -		strict	regions from userspace.
> -		relaxed
> -
>  	iommu=		[x86]
>  		off
>  		force
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index f97130618113..575cbfd89238 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -583,8 +583,6 @@ int devmem_is_allowed(unsigned long pagenr)
>  {
>  	if (pagenr < 256)
>  		return 1;
> -	if (iomem_is_exclusive(pagenr << PAGE_SHIFT))
> -		return 0;
>  	if (!page_is_ram(pagenr))
>  		return 1;
>  	return 0;
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 6d91933c4cdd..97a11b19e46f 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6574,8 +6574,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	}
>  
>  	bars = pci_select_bars(pdev, IORESOURCE_MEM);
> -	err = pci_request_selected_regions_exclusive(pdev, bars,
> -						     e1000e_driver_name);
> +	err = pci_request_selected_regions(pdev, bars, e1000e_driver_name);
>  	if (err)
>  		goto err_pci_reg;
>  
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 276ef9c18802..4580fa859f38 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -993,9 +993,6 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
>  	vma->vm_pgoff += start >> PAGE_SHIFT;
>  	mmap_type = res->flags & IORESOURCE_MEM ? pci_mmap_mem : pci_mmap_io;
>  
> -	if (res->flags & IORESOURCE_MEM && iomem_is_exclusive(start))
> -		return -EINVAL;
> -
>  	return pci_mmap_page_range(pdev, vma, mmap_type, write_combine);
>  }
>  
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1febe90831b4..1011c0b281ca 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2432,26 +2432,20 @@ void pci_release_region(struct pci_dev *pdev, int bar)
>  }
>  
>  /**
> - *	__pci_request_region - Reserved PCI I/O and memory resource
> + *	pci_request_region - Reserved PCI I/O and memory resource
>   *	@pdev: PCI device whose resources are to be reserved
>   *	@bar: BAR to be reserved
>   *	@res_name: Name to be associated with resource.
> - *	@exclusive: whether the region access is exclusive or not
>   *
>   *	Mark the PCI region associated with PCI device @pdev BR @bar as
>   *	being reserved by owner @res_name.  Do not access any
>   *	address inside the PCI regions unless this call returns
>   *	successfully.
>   *
> - *	If @exclusive is set, then the region is marked so that userspace
> - *	is explicitly not allowed to map the resource via /dev/mem or
> - *	sysfs MMIO access.
> - *
>   *	Returns 0 on success, or %EBUSY on error.  A warning
>   *	message is also printed on failure.
>   */
> -static int __pci_request_region(struct pci_dev *pdev, int bar, const char *res_name,
> -									int exclusive)
> +int pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
>  {
>  	struct pci_devres *dr;
>  
> @@ -2464,9 +2458,8 @@ static int __pci_request_region(struct pci_dev *pdev, int bar, const char *res_n
>  			goto err_out;
>  	}
>  	else if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> -		if (!__request_mem_region(pci_resource_start(pdev, bar),
> -					pci_resource_len(pdev, bar), res_name,
> -					exclusive))
> +		if (!request_mem_region(pci_resource_start(pdev, bar),
> +					pci_resource_len(pdev, bar), res_name))
>  			goto err_out;
>  	}
>  
> @@ -2483,47 +2476,6 @@ err_out:
>  }
>  
>  /**
> - *	pci_request_region - Reserve PCI I/O and memory resource
> - *	@pdev: PCI device whose resources are to be reserved
> - *	@bar: BAR to be reserved
> - *	@res_name: Name to be associated with resource
> - *
> - *	Mark the PCI region associated with PCI device @pdev BAR @bar as
> - *	being reserved by owner @res_name.  Do not access any
> - *	address inside the PCI regions unless this call returns
> - *	successfully.
> - *
> - *	Returns 0 on success, or %EBUSY on error.  A warning
> - *	message is also printed on failure.
> - */
> -int pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
> -{
> -	return __pci_request_region(pdev, bar, res_name, 0);
> -}
> -
> -/**
> - *	pci_request_region_exclusive - Reserved PCI I/O and memory resource
> - *	@pdev: PCI device whose resources are to be reserved
> - *	@bar: BAR to be reserved
> - *	@res_name: Name to be associated with resource.
> - *
> - *	Mark the PCI region associated with PCI device @pdev BR @bar as
> - *	being reserved by owner @res_name.  Do not access any
> - *	address inside the PCI regions unless this call returns
> - *	successfully.
> - *
> - *	Returns 0 on success, or %EBUSY on error.  A warning
> - *	message is also printed on failure.
> - *
> - *	The key difference that _exclusive makes it that userspace is
> - *	explicitly not allowed to map the resource via /dev/mem or
> - *	sysfs.
> - */
> -int pci_request_region_exclusive(struct pci_dev *pdev, int bar, const char *res_name)
> -{
> -	return __pci_request_region(pdev, bar, res_name, IORESOURCE_EXCLUSIVE);
> -}
> -/**
>   * pci_release_selected_regions - Release selected PCI I/O and memory resources
>   * @pdev: PCI device whose resources were previously reserved
>   * @bars: Bitmask of BARs to be released
> @@ -2540,14 +2492,20 @@ void pci_release_selected_regions(struct pci_dev *pdev, int bars)
>  			pci_release_region(pdev, i);
>  }
>  
> -static int __pci_request_selected_regions(struct pci_dev *pdev, int bars,
> -				 const char *res_name, int excl)
> +/**
> + * pci_request_selected_regions - Reserve selected PCI I/O and memory resources
> + * @pdev: PCI device whose resources are to be reserved
> + * @bars: Bitmask of BARs to be requested
> + * @res_name: Name to be associated with resource
> + */
> +int pci_request_selected_regions(struct pci_dev *pdev, int bars,
> +				 const char *res_name)
>  {
>  	int i;
>  
>  	for (i = 0; i < 6; i++)
>  		if (bars & (1 << i))
> -			if (__pci_request_region(pdev, i, res_name, excl))
> +			if (pci_request_region(pdev, i, res_name))
>  				goto err_out;
>  	return 0;
>  
> @@ -2561,25 +2519,6 @@ err_out:
>  
>  
>  /**
> - * pci_request_selected_regions - Reserve selected PCI I/O and memory resources
> - * @pdev: PCI device whose resources are to be reserved
> - * @bars: Bitmask of BARs to be requested
> - * @res_name: Name to be associated with resource
> - */
> -int pci_request_selected_regions(struct pci_dev *pdev, int bars,
> -				 const char *res_name)
> -{
> -	return __pci_request_selected_regions(pdev, bars, res_name, 0);
> -}
> -
> -int pci_request_selected_regions_exclusive(struct pci_dev *pdev,
> -				 int bars, const char *res_name)
> -{
> -	return __pci_request_selected_regions(pdev, bars, res_name,
> -			IORESOURCE_EXCLUSIVE);
> -}
> -
> -/**
>   *	pci_release_regions - Release reserved PCI I/O and memory resources
>   *	@pdev: PCI device whose resources were previously reserved by pci_request_regions
>   *
> @@ -2611,28 +2550,6 @@ int pci_request_regions(struct pci_dev *pdev, const char *res_name)
>  	return pci_request_selected_regions(pdev, ((1 << 6) - 1), res_name);
>  }
>  
> -/**
> - *	pci_request_regions_exclusive - Reserved PCI I/O and memory resources
> - *	@pdev: PCI device whose resources are to be reserved
> - *	@res_name: Name to be associated with resource.
> - *
> - *	Mark all PCI regions associated with PCI device @pdev as
> - *	being reserved by owner @res_name.  Do not access any
> - *	address inside the PCI regions unless this call returns
> - *	successfully.
> - *
> - *	pci_request_regions_exclusive() will mark the region so that
> - *	/dev/mem and the sysfs MMIO access will not be allowed.
> - *
> - *	Returns 0 on success, or %EBUSY on error.  A warning
> - *	message is also printed on failure.
> - */
> -int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
> -{
> -	return pci_request_selected_regions_exclusive(pdev,
> -					((1 << 6) - 1), res_name);
> -}
> -
>  static void __pci_set_master(struct pci_dev *dev, bool enable)
>  {
>  	u16 old_cmd, cmd;
> @@ -4387,13 +4304,10 @@ EXPORT_SYMBOL(pci_find_capability);
>  EXPORT_SYMBOL(pci_bus_find_capability);
>  EXPORT_SYMBOL(pci_release_regions);
>  EXPORT_SYMBOL(pci_request_regions);
> -EXPORT_SYMBOL(pci_request_regions_exclusive);
>  EXPORT_SYMBOL(pci_release_region);
>  EXPORT_SYMBOL(pci_request_region);
> -EXPORT_SYMBOL(pci_request_region_exclusive);
>  EXPORT_SYMBOL(pci_release_selected_regions);
>  EXPORT_SYMBOL(pci_request_selected_regions);
> -EXPORT_SYMBOL(pci_request_selected_regions_exclusive);
>  EXPORT_SYMBOL(pci_set_master);
>  EXPORT_SYMBOL(pci_clear_master);
>  EXPORT_SYMBOL(pci_set_mwi);
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 89b7c24a36e9..f2d45ea3ee53 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -49,7 +49,6 @@ struct resource {
>  #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
>  #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
>  
> -#define IORESOURCE_EXCLUSIVE	0x08000000	/* Userland may not map this resource */
>  #define IORESOURCE_DISABLED	0x10000000
>  #define IORESOURCE_UNSET	0x20000000
>  #define IORESOURCE_AUTO		0x40000000
> @@ -173,10 +172,7 @@ static inline unsigned long resource_type(const struct resource *res)
>  /* Convenience shorthand with allocation */
>  #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
>  #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
> -#define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
>  #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
> -#define request_mem_region_exclusive(start,n,name) \
> -	__request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)
>  #define rename_region(region, newname) do { (region)->name = (newname); } while (0)
>  
>  extern struct resource * __request_region(struct resource *,
> @@ -222,7 +218,6 @@ extern struct resource * __devm_request_region(struct device *dev,
>  extern void __devm_release_region(struct device *dev, struct resource *parent,
>  				  resource_size_t start, resource_size_t n);
>  extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
> -extern int iomem_is_exclusive(u64 addr);
>  
>  extern int
>  walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index fb57c892b214..b3cd9d58f5a9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1038,13 +1038,10 @@ void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
>  		    int (*)(const struct pci_dev *, u8, u8));
>  #define HAVE_PCI_REQ_REGIONS	2
>  int __must_check pci_request_regions(struct pci_dev *, const char *);
> -int __must_check pci_request_regions_exclusive(struct pci_dev *, const char *);
>  void pci_release_regions(struct pci_dev *);
>  int __must_check pci_request_region(struct pci_dev *, int, const char *);
> -int __must_check pci_request_region_exclusive(struct pci_dev *, int, const char *);
>  void pci_release_region(struct pci_dev *, int);
>  int pci_request_selected_regions(struct pci_dev *, int, const char *);
> -int pci_request_selected_regions_exclusive(struct pci_dev *, int, const char *);
>  void pci_release_selected_regions(struct pci_dev *, int);
>  
>  /* drivers/pci/bus.c */
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 3f285dce9347..9a29d989aa5e 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1306,57 +1306,3 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
>  
>  	return err;
>  }
> -
> -#ifdef CONFIG_STRICT_DEVMEM
> -static int strict_iomem_checks = 1;
> -#else
> -static int strict_iomem_checks;
> -#endif
> -
> -/*
> - * check if an address is reserved in the iomem resource tree
> - * returns 1 if reserved, 0 if not reserved.
> - */
> -int iomem_is_exclusive(u64 addr)
> -{
> -	struct resource *p = &iomem_resource;
> -	int err = 0;
> -	loff_t l;
> -	int size = PAGE_SIZE;
> -
> -	if (!strict_iomem_checks)
> -		return 0;
> -
> -	addr = addr & PAGE_MASK;
> -
> -	read_lock(&resource_lock);
> -	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
> -		/*
> -		 * We can probably skip the resources without
> -		 * IORESOURCE_IO attribute?
> -		 */
> -		if (p->start >= addr + size)
> -			break;
> -		if (p->end < addr)
> -			continue;
> -		if (p->flags & IORESOURCE_BUSY &&
> -		     p->flags & IORESOURCE_EXCLUSIVE) {
> -			err = 1;
> -			break;
> -		}
> -	}
> -	read_unlock(&resource_lock);
> -
> -	return err;
> -}
> -
> -static int __init strict_iomem(char *str)
> -{
> -	if (strstr(str, "relaxed"))
> -		strict_iomem_checks = 0;
> -	if (strstr(str, "strict"))
> -		strict_iomem_checks = 1;
> -	return 1;
> -}
> -
> -__setup("iomem=", strict_iomem);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists