[RFC 0/4] simplify remapping a resource for drivers

[RFC 0/4] simplify remapping a resource for drivers

[Wolfram Sang]

Mostly all platform drivers need to do something like this to get a ioremapped
pointer from a resource (and some have multiple resources to be remapped):

 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	if (!res) {
		dev_err(&pdev->dev, "can't get device resources\n");
		return -ENOENT;
	}

	res_size = resource_size(res);
	if (!devm_request_mem_region(&pdev->dev, res->start, res_size, res->name)) {
		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
				res_size, res->start);
		return -EBUSY;
	}

	priv->base = devm_ioremap_nocache(&pdev->dev, res->start, res_size);
	if (priv->base) {
		dev_err(&pdev->dev, "ioremap failed\n");
		return -ENOMEM;
	}

In case of not using managed devices, there is also code for cleaning up when
hitting an error. After this series, the same can be done with:

 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	err = devm_resource_to_mapped_ptr(&pdev->dev, res, &priv->base);
	if (err)
		return err;

The new function will perform all necessary checks and return with consistent
error codes and messages on failures. It also prevents common mistakes like
using a wrong type for res_size or not calling the _nocache-variant of ioremap
if the resource is not marked otherwise. Because the new functions works with
the struct device (plus a resource), it will probably be useful even if
platform drivers run out of fashion.

The first two patches are minor cleanups on devres found while working on the code.
The third patch adds the new function. The fourth patch is an example, so we have a
simple use-case for the proposed functionality.

The series can also be fetched from:

	git://git.pengutronix.de/git/wsa/linux-2.6.git resource_to_ptr

Looking forward to reviews and comments.

Thanks,

   Wolfram


Wolfram Sang (4):
  Documentation: devres: add allocation functions to list of supported calls
  lib: devres: add annotations for #endif
  lib: devres: add function which remaps a given resource
  watchdog: imx2: simplify resource allocation

 Documentation/driver-model/devres.txt |    5 +++
 drivers/watchdog/imx2_wdt.c           |   22 ++-----------
 include/linux/device.h                |    3 ++
 lib/devres.c                          |   56 +++++++++++++++++++++++++++++++-
 4 files changed, 65 insertions(+), 21 deletions(-)

-- 
1.7.6.3

[RFC 3/4] lib: devres: add convenience function to remap a resource

[Grant Likely]

On Mon, Oct 24, 2011 at 10:42:08AM +0200, Wolfram Sang wrote:
> Almost every platform_driver does the three steps get_resource,
> request_mem_region, ioremap. This does not only lead to a lot of code
> duplication, but also a number of similar error strings and inconsistent
> error codes on failure. So, introduce a helper function which simplifies
> remapping a resource and make it hard to do something wrong.
> 
> A few notes on design choices:
> 
> * not 100% sure on ENOENT when the resource is invalid. Seems to be frequently
>   used currently and fits "somehow"
> 
> * the remapped pointer is a function argument and not a return value. This is to
>   save users from forgetting to use IS_ERR/PTR_ERR.
> 
> * the usage of IORESOURCE_CACHEABLE enforces calling the correct ioremap-variant.
>   A number of drivers miss to call the _nocache-version, although they should do.
> 
> * the new function is not named 'devm_ioremap_resource' because it does more than
>   just ioremap, namely request_mem_region. I didn't want to create implicit
>   knowledge here.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  Documentation/driver-model/devres.txt |    1 +
>  include/linux/device.h                |    3 ++
>  lib/devres.c                          |   52 +++++++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index badd964..3aeb4f4 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -266,6 +266,7 @@ IOMAP
>    devm_ioremap()
>    devm_ioremap_nocache()
>    devm_iounmap()
> +  devm_resource_to_mapped_ptr() : checks resource, requests region, ioremaps
>    pcim_iomap()
>    pcim_iounmap()
>    pcim_iomap_table()	: array of mapped addresses indexed by BAR
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c20dfbf..82a621c 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -481,6 +481,9 @@ extern int devres_release_group(struct device *dev, void *id);
>  extern void *devm_kzalloc(struct device *dev, size_t size, gfp_t gfp);
>  extern void devm_kfree(struct device *dev, void *p);
>  
> +int __must_check devm_resource_to_mapped_ptr(struct device *dev,
> +			struct resource *res, void __iomem **dest);
> +
>  struct device_dma_parameters {
>  	/*
>  	 * a low level driver may set these to teach IOMMU code about
> diff --git a/lib/devres.c b/lib/devres.c
> index 78777ae..970588b 100644
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -85,6 +85,58 @@ void devm_iounmap(struct device *dev, void __iomem *addr)
>  }
>  EXPORT_SYMBOL(devm_iounmap);
>  
> +/**
> + * devm_resource_to_mapped_ptr - Check, request region, and ioremap resource

Kernel doc format should have '()' for functions is:
/*
 * devm_resource_to_mapped_ptr() - Check, request region, and ioremap resource

> + * @dev: Generic device to handle the resource for
> + * @res: resource to be handled
> + * @dest: Pointer to pointer which carries the remapped address
> + *
> + * Takes all necessary steps to ioremap a mem resource. Uses managed device, so
> + * everything is undone on driver detach. Checks arguments, so you can feed
> + * it the result from e.g. platform_get_resource() directly:
> + *
> + *	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + *	err = devm_resource_to_mapped_ptr(&pdev->dev, res, &priv->regs);
> + *	if (err)
> + *		return err;
> + */
> +int __must_check devm_resource_to_mapped_ptr(struct device *dev,
> +			struct resource *res, void __iomem **dest)

I'm not happy with the API.  I understand trying to avoid losing the
error code, but passing back the value in a passed in pointer is
rather horrible to use (as I discovered with the
of_property_read_u32() patches) and it becomes too easy to improperly
use it.

I'd argue that the users of this probably really won't care about the
actual error code.  If this function fails, then things are rather
horribly wrong, and it would be entirely appropriate to do rely on the
dev_err() to inform the user of the actual error code.

So, just return the mapped (void __iomem *) value after requesting and
mapping the region, and make exceptional situations exceptional.  :-)

Otherwise, I'm quite happy to see this helper.  Good work!

g.

> +{
> +	resource_size_t size;
> +	const char *name;
> +
> +	BUG_ON(!dev || !dest);
> +
> +	*dest = NULL;
> +
> +	if (!res || resource_type(res) != IORESOURCE_MEM) {
> +		dev_err(dev, "invalid resource\n");
> +		return -ENOENT;
> +	}
> +
> +	size = resource_size(res);
> +	name = res->name ?: dev_name(dev);
> +
> +	if (!devm_request_mem_region(dev, res->start, size, name)) {
> +		dev_err(dev, "can't request region for resource %pR\n", res);
> +		return -EBUSY;
> +	}
> +
> +	if (res->flags & IORESOURCE_CACHEABLE)
> +		*dest = devm_ioremap(dev, res->start, size);
> +	else
> +		*dest = devm_ioremap_nocache(dev, res->start, size);
> +
> +	if (!*dest) {
> +		dev_err(dev, "ioremap failed for resource %pR\n", res);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(devm_resource_to_mapped_ptr);
> +
>  #ifdef CONFIG_HAS_IOPORT
>  /*
>   * Generic iomap devres
> -- 
> 1.7.6.3
> 

[RFC 4/4] watchdog: imx2: simplify resource allocation

[Grant Likely]

On Mon, Oct 24, 2011 at 10:42:09AM +0200, Wolfram Sang wrote:
> Use the new devm_resource_to_mapped_ptr function to simplify the code
> and standardize the error-handling. Silently fixes a wrong type for the
> resource_size variable as well.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/watchdog/imx2_wdt.c |   22 +++-------------------
>  1 files changed, 3 insertions(+), 19 deletions(-)

w00t!  I can see a lot of drivers simplified by using this.

g.

> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index b8ef2c6..1fc6714 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -247,28 +247,12 @@ static struct miscdevice imx2_wdt_miscdev = {
>  static int __init imx2_wdt_probe(struct platform_device *pdev)
>  {
>  	int ret;
> -	int res_size;
>  	struct resource *res;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "can't get device resources\n");
> -		return -ENODEV;
> -	}
> -
> -	res_size = resource_size(res);
> -	if (!devm_request_mem_region(&pdev->dev, res->start, res_size,
> -		res->name)) {
> -		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
> -			res_size, res->start);
> -		return -ENOMEM;
> -	}
> -
> -	imx2_wdt.base = devm_ioremap_nocache(&pdev->dev, res->start, res_size);
> -	if (!imx2_wdt.base) {
> -		dev_err(&pdev->dev, "ioremap failed\n");
> -		return -ENOMEM;
> -	}
> +	ret = devm_resource_to_mapped_ptr(&pdev->dev, res, &imx2_wdt.base);
> +	if (ret)
> +		return ret;
>  
>  	imx2_wdt.clk = clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(imx2_wdt.clk)) {
> -- 
> 1.7.6.3
> 

[RFC 1/4] Documentation: devres: add allocation functions to list of supported calls

[Tejun Heo]

On Mon, Oct 24, 2011 at 10:42:06AM +0200, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[RFC 3/4] lib: devres: add convenience function to remap a resource

[Tejun Heo]

Hello,

On Mon, Oct 24, 2011 at 10:42:08AM +0200, Wolfram Sang wrote:
> * the new function is not named 'devm_ioremap_resource' because it does more than
>   just ioremap, namely request_mem_region. I didn't want to create implicit
>   knowledge here.

Maybe devm_request_and_ioremap_resource()?  Maybe it's a bit long but
A_to_B names are usually used for trivial conversions.

> +int __must_check devm_resource_to_mapped_ptr(struct device *dev,
> +			struct resource *res, void __iomem **dest)

If we're already printing out dev_err(), I think we can live with
%NULL return on error.

> +{
> +	resource_size_t size;
> +	const char *name;
> +
> +	BUG_ON(!dev || !dest);
> +
> +	*dest = NULL;
> +
> +	if (!res || resource_type(res) != IORESOURCE_MEM) {
> +		dev_err(dev, "invalid resource\n");
> +		return -ENOENT;
> +	}
> +
> +	size = resource_size(res);
> +	name = res->name ?: dev_name(dev);
> +
> +	if (!devm_request_mem_region(dev, res->start, size, name)) {
> +		dev_err(dev, "can't request region for resource %pR\n", res);
> +		return -EBUSY;
> +	}
> +
> +	if (res->flags & IORESOURCE_CACHEABLE)
> +		*dest = devm_ioremap(dev, res->start, size);
> +	else
> +		*dest = devm_ioremap_nocache(dev, res->start, size);
> +
> +	if (!*dest) {
> +		dev_err(dev, "ioremap failed for resource %pR\n", res);
> +		return -ENOMEM;
> +	}

So, for library functions, it's nice if the function doesn't hold any
extra resource on error return.  devres has grouping to support this.
You just need to wrap allocations inside a group and remove it on
success and release on error.  ie.

	if (!devres_open_group(dev, NULL, GFP_KERNEL))
		return NULL;
	if (alloc_resource(0) < 0)
		goto fail;
	if (alloc_resource(1) < 0)
		goto fail;
	devres_remove_group(dev, NULL);
	return 0;
fail:
	devres_release_group(dev, NULL);
	return 0;

Thanks.

-- 
tejun

[RFC 3/4] lib: devres: add convenience function to remap a resource

[Tejun Heo]

Hello, Grant.

On Mon, Oct 24, 2011 at 01:56:12PM +0200, Grant Likely wrote:
> > +/**
> > + * devm_resource_to_mapped_ptr - Check, request region, and ioremap resource
> 
> Kernel doc format should have '()' for functions is:

Simple Grepping gives me an order of magnitude more on the ones w/o
'()'.  Maybe it's better to change the rule at this point?

Thanks.

-- 
tejun

[RFC 3/4] lib: devres: add convenience function to remap a resource

[Wolfram Sang]

Hi,

> On Mon, Oct 24, 2011 at 10:42:08AM +0200, Wolfram Sang wrote:
> > * the new function is not named 'devm_ioremap_resource' because it does more than
> >   just ioremap, namely request_mem_region. I didn't want to create implicit
> >   knowledge here.
> 
> Maybe devm_request_and_ioremap_resource()?  Maybe it's a bit long but
> A_to_B names are usually used for trivial conversions.

I had this name in mind, too. Yet, it implies that the function requests a
resource but in fact it requests a mem_region. Requesting a resource (e.g.
platform_get_resource) is THE thing the function does not do. So I scrapped
this name. But will do some more brainstorming for a better name in V2.

> 
> > +int __must_check devm_resource_to_mapped_ptr(struct device *dev,
> > +			struct resource *res, void __iomem **dest)
> 
> If we're already printing out dev_err(), I think we can live with
> %NULL return on error.

Yes, already fixed this way.

> > +{
> > +	resource_size_t size;
> > +	const char *name;
> > +
> > +	BUG_ON(!dev || !dest);
> > +
> > +	*dest = NULL;
> > +
> > +	if (!res || resource_type(res) != IORESOURCE_MEM) {
> > +		dev_err(dev, "invalid resource\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	size = resource_size(res);
> > +	name = res->name ?: dev_name(dev);
> > +
> > +	if (!devm_request_mem_region(dev, res->start, size, name)) {
> > +		dev_err(dev, "can't request region for resource %pR\n", res);
> > +		return -EBUSY;
> > +	}
> > +
> > +	if (res->flags & IORESOURCE_CACHEABLE)
> > +		*dest = devm_ioremap(dev, res->start, size);
> > +	else
> > +		*dest = devm_ioremap_nocache(dev, res->start, size);
> > +
> > +	if (!*dest) {
> > +		dev_err(dev, "ioremap failed for resource %pR\n", res);
> > +		return -ENOMEM;
> > +	}
> 
> So, for library functions, it's nice if the function doesn't hold any
> extra resource on error return.  devres has grouping to support this.
> You just need to wrap allocations inside a group and remove it on
> success and release on error.  ie.
> 
> 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> 		return NULL;
> 	if (alloc_resource(0) < 0)
> 		goto fail;
> 	if (alloc_resource(1) < 0)
> 		goto fail;
> 	devres_remove_group(dev, NULL);
> 	return 0;
> fail:
> 	devres_release_group(dev, NULL);
> 	return 0;

I am not sure I got what you mean here, but wouldn't it be easier to simply
call devm_release_mem_region() when devm_ioremap comes back with a NULL
pointer?

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111024/01ff8aba/attachment-0001.sig>

[RFC 3/4] lib: devres: add convenience function to remap a resource

[Tejun Heo]

On Mon, Oct 24, 2011 at 10:09:33PM +0200, Wolfram Sang wrote:
> > So, for library functions, it's nice if the function doesn't hold any
> > extra resource on error return.  devres has grouping to support this.
> > You just need to wrap allocations inside a group and remove it on
> > success and release on error.  ie.
> > 
> > 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> > 		return NULL;
> > 	if (alloc_resource(0) < 0)
> > 		goto fail;
> > 	if (alloc_resource(1) < 0)
> > 		goto fail;
> > 	devres_remove_group(dev, NULL);
> > 	return 0;
> > fail:
> > 	devres_release_group(dev, NULL);
> > 	return 0;
> 
> I am not sure I got what you mean here, but wouldn't it be easier to simply
> call devm_release_mem_region() when devm_ioremap comes back with a NULL
> pointer?

Oh, yeah, that would work too. :)

Thanks.

-- 
tejun