[PATCH 1/2] resource: Add device-managed request/release_resource()

[PATCH 1/2] resource: Add device-managed request/release_resource()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Provide device-managed implementations of the request_resource() and
release_resource() functions. Upon failure to request a resource, the
new devm_request_resource() function will output an error message for
consistent error reporting.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 Documentation/driver-model/devres.txt |  2 +
 include/linux/ioport.h                |  5 +++
 kernel/resource.c                     | 73 +++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 034d32b00846..b466f3d19bcb 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -252,6 +252,8 @@ IIO
   devm_iio_device_unregister()
 
 IO region
+  devm_request_resource()
+  devm_release_resource()
   devm_request_region()
   devm_request_mem_region()
   devm_release_region()
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 142ec544167c..2c5250222278 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -215,6 +215,11 @@ static inline int __deprecated check_region(resource_size_t s,
 
 /* Wrappers for managed devices */
 struct device;
+
+extern int devm_request_resource(struct device *dev, struct resource *root,
+				 struct resource *new);
+extern void devm_release_resource(struct device *dev, struct resource *new);
+
 #define devm_request_region(dev,start,n,name) \
 	__devm_request_region(dev, &ioport_resource, (start), (n), (name))
 #define devm_request_mem_region(dev,start,n,name) \
diff --git a/kernel/resource.c b/kernel/resource.c
index da14b8d09296..39d99aa401a6 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1248,6 +1248,79 @@ int release_mem_region_adjustable(struct resource *parent,
 /*
  * Managed region resource
  */
+static void devm_resource_release(struct device *dev, void *ptr)
+{
+	struct resource **r = ptr;
+
+	release_resource(*r);
+}
+
+/**
+ * devm_request_resource() - request and reserve an I/O or memory resource
+ * @dev: device for which to request the resource
+ * @root: root of the resource tree from which to request the resource
+ * @new: descriptor of the resource to request
+ *
+ * This is a device-managed version of request_resource(). There is usually
+ * no need to release resources requested by this function explicitly since
+ * that will be taken care of when the device is unbound from its driver.
+ * If for some reason the resource needs to be released explicitly, because
+ * of ordering issues for example, drivers must call devm_release_resource()
+ * rather than the regular release_resource().
+ *
+ * When a conflict is detected between any existing resources and the newly
+ * requested resource, an error message will be printed.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int devm_request_resource(struct device *dev, struct resource *root,
+			  struct resource *new)
+{
+	struct resource *conflict, **ptr;
+
+	ptr = devres_alloc(devm_resource_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	*ptr = new;
+
+	conflict = request_resource_conflict(root, new);
+	if (!conflict) {
+		devres_add(dev, ptr);
+		return 0;
+	}
+
+	dev_err(dev, "resource collision: %pR conflicts with %s %pR\n", new,
+		conflict->name, conflict);
+	devres_free(ptr);
+	return -EBUSY;
+}
+EXPORT_SYMBOL(devm_request_resource);
+
+static int devm_resource_match(struct device *dev, void *res, void *data)
+{
+	struct resource **ptr = res;
+
+	if (WARN_ON(!ptr || !*ptr))
+		return 0;
+
+	return *ptr == data;
+}
+
+/**
+ * devm_release_resource() - release a previously requested resource
+ * @dev: device for which to release the resource
+ * @new: descriptor of the resource to release
+ *
+ * Releases a resource previously requested using devm_request_resource().
+ */
+void devm_release_resource(struct device *dev, struct resource *new)
+{
+	WARN_ON(devres_release(dev, devm_resource_release, devm_resource_match,
+			       new));
+}
+EXPORT_SYMBOL(devm_release_resource);
+
 struct region_devres {
 	struct resource *parent;
 	resource_size_t start;
-- 
2.0.1

[PATCH 2/2] PCI: tegra: Implement a proper resource hierarchy

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Currently the resource hierarchy generated from the PCIe host bridge is
completely flat:

	$ cat /proc/iomem
	00000000-00000fff : /pcie-controller@00003000/pci@1,0
	00003000-000037ff : pads
	00003800-000039ff : afi
	10000000-1fffffff : cs
	28000000-28003fff : r8169
	28004000-28004fff : r8169
	...

The host bridge driver doesn't request all the resources that are used.
Windows allocated to each of the root ports aren't tracked, so there is
no way for resources allocated to individual devices to be matched up
with the correct parent resource by the PCI core.

This patch addresses this in two steps. It first takes the union of all
regions associated with the PCIe host bridge (control registers, root
port registers, configuration space, I/O and prefetchable as well as
non-prefetchable memory regions) and uses it as the new root of the
resource hierarchy.

Subsequently, regions are allocated from within this new root resource
so that the resource tree looks much more like what's expected:

	# cat /proc/iomem
	00000000-3fffffff : /pcie-controller@00003000
	  00000000-00000fff : /pcie-controller@00003000/pci@1,0
	  00003000-000037ff : pads
	  00003800-000039ff : afi
	  10000000-1fffffff : cs
	  20000000-27ffffff : non-prefetchable
	  28000000-3fffffff : prefetchable
	    28000000-280fffff : PCI Bus 0000:01
	      28000000-28003fff : 0000:01:00.0
	        28000000-28003fff : r8169
	      28004000-28004fff : 0000:01:00.0
	        28004000-28004fff : r8169
	...

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/host/pci-tegra.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 49b3bd5f5926..b0b337ebaf48 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -252,6 +252,7 @@ struct tegra_pcie {
 	struct list_head buses;
 	struct resource *cs;
 
+	struct resource all;
 	struct resource io;
 	struct resource mem;
 	struct resource prefetch;
@@ -625,6 +626,15 @@ DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
 static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
 {
 	struct tegra_pcie *pcie = sys_to_pcie(sys);
+	int err;
+
+	err = devm_request_resource(pcie->dev, &pcie->all, &pcie->mem);
+	if (err < 0)
+		return err;
+
+	err = devm_request_resource(pcie->dev, &pcie->all, &pcie->prefetch);
+	if (err)
+		return err;
 
 	pci_add_resource_offset(&sys->resources, &pcie->mem, sys->mem_offset);
 	pci_add_resource_offset(&sys->resources, &pcie->prefetch,
@@ -1439,6 +1449,12 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 	struct resource res;
 	int err;
 
+	memset(&pcie->all, 0, sizeof(pcie->all));
+	pcie->all.flags = IORESOURCE_MEM;
+	pcie->all.name = np->full_name;
+	pcie->all.start = ~0;
+	pcie->all.end = 0;
+
 	if (of_pci_range_parser_init(&parser, np)) {
 		dev_err(pcie->dev, "missing \"ranges\" property\n");
 		return -EINVAL;
@@ -1450,21 +1466,31 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		switch (res.flags & IORESOURCE_TYPE_BITS) {
 		case IORESOURCE_IO:
 			memcpy(&pcie->io, &res, sizeof(res));
-			pcie->io.name = "I/O";
+			pcie->io.name = np->full_name;
 			break;
 
 		case IORESOURCE_MEM:
 			if (res.flags & IORESOURCE_PREFETCH) {
 				memcpy(&pcie->prefetch, &res, sizeof(res));
-				pcie->prefetch.name = "PREFETCH";
+				pcie->prefetch.name = "prefetchable";
 			} else {
 				memcpy(&pcie->mem, &res, sizeof(res));
-				pcie->mem.name = "MEM";
+				pcie->mem.name = "non-prefetchable";
 			}
 			break;
 		}
+
+		if (res.start <= pcie->all.start)
+			pcie->all.start = res.start;
+
+		if (res.end >= pcie->all.end)
+			pcie->all.end = res.end;
 	}
 
+	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->all);
+	if (err < 0)
+		return err;
+
 	err = of_pci_parse_bus_range(np, &pcie->busn);
 	if (err < 0) {
 		dev_err(pcie->dev, "failed to parse ranges property: %d\n",
-- 
2.0.1

Re: [PATCH 2/2] PCI: tegra: Implement a proper resource hierarchy

[Stephen Warren]

On 07/11/2014 01:08 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Currently the resource hierarchy generated from the PCIe host bridge is
> completely flat:
...
> The host bridge driver doesn't request all the resources that are used.
> Windows allocated to each of the root ports aren't tracked, so there is
> no way for resources allocated to individual devices to be matched up
> with the correct parent resource by the PCI core.
> 
> This patch addresses this in two steps. It first takes the union of all
> regions associated with the PCIe host bridge (control registers, root
> port registers, configuration space, I/O and prefetchable as well as
> non-prefetchable memory regions) and uses it as the new root of the
> resource hierarchy.

This patch briefly,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

Re: [PATCH 1/2] resource: Add device-managed request/release_resource()

[Bjorn Helgaas]

[+cc Tejun, LKML]

On Fri, Jul 11, 2014 at 09:08:24AM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Provide device-managed implementations of the request_resource() and
> release_resource() functions. Upon failure to request a resource, the
> new devm_request_resource() function will output an error message for
> consistent error reporting.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

This seems OK to me, but I don't consider myself a devres maintainer.  I
added Tejun and LKML for any comment.  Minor nit below.

> ---
>  Documentation/driver-model/devres.txt |  2 +
>  include/linux/ioport.h                |  5 +++
>  kernel/resource.c                     | 73 +++++++++++++++++++++++++++++++++++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index 034d32b00846..b466f3d19bcb 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -252,6 +252,8 @@ IIO
>    devm_iio_device_unregister()
>  
>  IO region
> +  devm_request_resource()
> +  devm_release_resource()
>    devm_request_region()
>    devm_request_mem_region()
>    devm_release_region()
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 142ec544167c..2c5250222278 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -215,6 +215,11 @@ static inline int __deprecated check_region(resource_size_t s,
>  
>  /* Wrappers for managed devices */
>  struct device;
> +
> +extern int devm_request_resource(struct device *dev, struct resource *root,
> +				 struct resource *new);
> +extern void devm_release_resource(struct device *dev, struct resource *new);
> +
>  #define devm_request_region(dev,start,n,name) \
>  	__devm_request_region(dev, &ioport_resource, (start), (n), (name))
>  #define devm_request_mem_region(dev,start,n,name) \
> diff --git a/kernel/resource.c b/kernel/resource.c
> index da14b8d09296..39d99aa401a6 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1248,6 +1248,79 @@ int release_mem_region_adjustable(struct resource *parent,
>  /*
>   * Managed region resource
>   */
> +static void devm_resource_release(struct device *dev, void *ptr)
> +{
> +	struct resource **r = ptr;
> +
> +	release_resource(*r);
> +}
> +
> +/**
> + * devm_request_resource() - request and reserve an I/O or memory resource
> + * @dev: device for which to request the resource
> + * @root: root of the resource tree from which to request the resource
> + * @new: descriptor of the resource to request
> + *
> + * This is a device-managed version of request_resource(). There is usually
> + * no need to release resources requested by this function explicitly since
> + * that will be taken care of when the device is unbound from its driver.
> + * If for some reason the resource needs to be released explicitly, because
> + * of ordering issues for example, drivers must call devm_release_resource()
> + * rather than the regular release_resource().
> + *
> + * When a conflict is detected between any existing resources and the newly
> + * requested resource, an error message will be printed.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int devm_request_resource(struct device *dev, struct resource *root,
> +			  struct resource *new)
> +{
> +	struct resource *conflict, **ptr;
> +
> +	ptr = devres_alloc(devm_resource_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	*ptr = new;
> +
> +	conflict = request_resource_conflict(root, new);
> +	if (!conflict) {
> +		devres_add(dev, ptr);
> +		return 0;
> +	}
> +
> +	dev_err(dev, "resource collision: %pR conflicts with %s %pR\n", new,
> +		conflict->name, conflict);
> +	devres_free(ptr);
> +	return -EBUSY;

Personally I would write this as:

  conflict = request_resource_conflict(...);
  if (conflict) {
    dev_err(...);
    devres_free(...);
    return -EBUSY;
  }

  devres_add(...);
  return 0;

so the straight-line path is the normal, non-error path and errors are
detected and dealt with in the "if" bodies.  Right now the "if" bodies
are a mix of error handling and normal path.  But that's just my personal
preference.

> +}
> +EXPORT_SYMBOL(devm_request_resource);
> +
> +static int devm_resource_match(struct device *dev, void *res, void *data)
> +{
> +	struct resource **ptr = res;
> +
> +	if (WARN_ON(!ptr || !*ptr))
> +		return 0;
> +
> +	return *ptr == data;
> +}
> +
> +/**
> + * devm_release_resource() - release a previously requested resource
> + * @dev: device for which to release the resource
> + * @new: descriptor of the resource to release
> + *
> + * Releases a resource previously requested using devm_request_resource().
> + */
> +void devm_release_resource(struct device *dev, struct resource *new)
> +{
> +	WARN_ON(devres_release(dev, devm_resource_release, devm_resource_match,
> +			       new));
> +}
> +EXPORT_SYMBOL(devm_release_resource);
> +
>  struct region_devres {
>  	struct resource *parent;
>  	resource_size_t start;
> -- 
> 2.0.1
> 

Re: [PATCH 1/2] resource: Add device-managed request/release_resource()

[Tejun Heo]

Hello,

On Tue, Jul 22, 2014 at 12:50:02PM -0600, Bjorn Helgaas wrote:
> [+cc Tejun, LKML]
> 
> On Fri, Jul 11, 2014 at 09:08:24AM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Provide device-managed implementations of the request_resource() and
> > release_resource() functions. Upon failure to request a resource, the
> > new devm_request_resource() function will output an error message for
> > consistent error reporting.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> This seems OK to me, but I don't consider myself a devres maintainer.  I
> added Tejun and LKML for any comment.  Minor nit below.

If there are gonna be users of the interface, sure.

> > +int devm_request_resource(struct device *dev, struct resource *root,
> > +			  struct resource *new)
> > +{
> > +	struct resource *conflict, **ptr;
> > +
> > +	ptr = devres_alloc(devm_resource_release, sizeof(*ptr), GFP_KERNEL);
> > +	if (!ptr)
> > +		return -ENOMEM;
> > +
> > +	*ptr = new;
> > +
> > +	conflict = request_resource_conflict(root, new);
> > +	if (!conflict) {
> > +		devres_add(dev, ptr);
> > +		return 0;
> > +	}
> > +
> > +	dev_err(dev, "resource collision: %pR conflicts with %s %pR\n", new,
> > +		conflict->name, conflict);
> > +	devres_free(ptr);
> > +	return -EBUSY;
> 
> Personally I would write this as:
> 
>   conflict = request_resource_conflict(...);
>   if (conflict) {
>     dev_err(...);
>     devres_free(...);
>     return -EBUSY;
>   }
> 
>   devres_add(...);
>   return 0;
> 
> so the straight-line path is the normal, non-error path and errors are
> detected and dealt with in the "if" bodies.  Right now the "if" bodies
> are a mix of error handling and normal path.  But that's just my personal
> preference.

Agreed.

> > +static int devm_resource_match(struct device *dev, void *res, void *data)
> > +{
> > +	struct resource **ptr = res;
> > +
> > +	if (WARN_ON(!ptr || !*ptr))
> > +		return 0;

How would !ptr or !*ptr possibly happen?  Wouldn't that be a bug
already?

Thanks.

-- 
tejun

Re: [PATCH 1/2] resource: Add device-managed request/release_resource()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

On Tue, Jul 22, 2014 at 03:01:20PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Tue, Jul 22, 2014 at 12:50:02PM -0600, Bjorn Helgaas wrote:
> > [+cc Tejun, LKML]
> > 
> > On Fri, Jul 11, 2014 at 09:08:24AM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Provide device-managed implementations of the request_resource() and
> > > release_resource() functions. Upon failure to request a resource, the
> > > new devm_request_resource() function will output an error message for
> > > consistent error reporting.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > This seems OK to me, but I don't consider myself a devres maintainer.  I
> > added Tejun and LKML for any comment.  Minor nit below.
> 
> If there are gonna be users of the interface, sure.
> 
> > > +int devm_request_resource(struct device *dev, struct resource *root,
> > > +			  struct resource *new)
> > > +{
> > > +	struct resource *conflict, **ptr;
> > > +
> > > +	ptr = devres_alloc(devm_resource_release, sizeof(*ptr), GFP_KERNEL);
> > > +	if (!ptr)
> > > +		return -ENOMEM;
> > > +
> > > +	*ptr = new;
> > > +
> > > +	conflict = request_resource_conflict(root, new);
> > > +	if (!conflict) {
> > > +		devres_add(dev, ptr);
> > > +		return 0;
> > > +	}
> > > +
> > > +	dev_err(dev, "resource collision: %pR conflicts with %s %pR\n", new,
> > > +		conflict->name, conflict);
> > > +	devres_free(ptr);
> > > +	return -EBUSY;
> > 
> > Personally I would write this as:
> > 
> >   conflict = request_resource_conflict(...);
> >   if (conflict) {
> >     dev_err(...);
> >     devres_free(...);
> >     return -EBUSY;
> >   }
> > 
> >   devres_add(...);
> >   return 0;
> > 
> > so the straight-line path is the normal, non-error path and errors are
> > detected and dealt with in the "if" bodies.  Right now the "if" bodies
> > are a mix of error handling and normal path.  But that's just my personal
> > preference.
> 
> Agreed.
> 
> > > +static int devm_resource_match(struct device *dev, void *res, void *data)
> > > +{
> > > +	struct resource **ptr = res;
> > > +
> > > +	if (WARN_ON(!ptr || !*ptr))
> > > +		return 0;
> 
> How would !ptr or !*ptr possibly happen?  Wouldn't that be a bug
> already?

Honestly, I copied that from similar implementations. But checking the
code again, I don't think they can actually happen. The value returned
by devres_alloc() is a struct devres * with an added offset so that it
points at the payload immediately following the struct devres. So at
least !ptr can never happen.

!*ptr could happen since the devres code calls the match function on
every resource managed for the device and someone could've inserted a
NULL (or 0) value. But since we're not dereferencing *ptr this should
not be an issue. That said, having either res or data above be NULL
isn't something that devres was meant to deal with anyway, since it
relies on the pointers being unique for the matching.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]