Re: [RFC PATCH 10/13] driver core: devres: introduce devres_for_each_res

[Borislav Petkov <bp@amd64.org>]

On Wed, Jul 25, 2012 at 01:00:10AM +0800, Ming Lei wrote:
> This patch introduces one devres API of devres_for_each_res
> so that the device's driver can iterate each resource it has
> interest in.
> 
> The firmware loader will use the API to get each firmware name
> from the device instance.
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/devres.c  |   42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |    3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 2360adb..8273ba5 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -144,6 +144,48 @@ EXPORT_SYMBOL_GPL(devres_alloc);
>  #endif
>  
>  /**
> + * devres_for_each_res - Resource iterator
> + * @dev: Device to iterate resource from
> + * @release: Look for resources associated with this release function
> + * @match: Match function (optional)
> + * @match_data: Data for the match function
> + * @fn: function to be called for each matched resource.
> + *
> + * Call @fn for each devres of @dev which is associated with @release
> + * and for which @match returns 1.
> + *
> + * RETURNS:
> + * 	void
> + */
> +void devres_for_each_res(struct device *dev, dr_release_t release,
> +			dr_match_t match, void *match_data,
> +			void (*fn)(struct device *, void *))
> +{
> +	struct devres_node *node;
> +	struct devres_node *tmp;
> +	unsigned long flags;
> +
> +	if (!fn)
> +		return;
> +
> +	spin_lock_irqsave(&dev->devres_lock, flags);
> +	list_for_each_entry_safe_reverse(node, tmp,
> +			&dev->devres_head, entry) {

Why break this line?

	list_for_each_entry_safe_reverse(node, tmp, &dev->devres_head, entry) {

is perfectly fine.

> +		struct devres *dr = container_of(node, struct devres, node);
> +
> +		if (node->release != release)
> +			continue;
> +		if (match && !match(dev, dr->data, match_data))
> +			continue;
> +		spin_unlock_irqrestore(&dev->devres_lock, flags);
> +		fn(dev, dr->data);
> +		spin_lock_irqsave(&dev->devres_lock, flags);
> +	}
> +	spin_unlock_irqrestore(&dev->devres_lock, flags);

This looks strange. For the last node on the list, we're grabbing the
lock and releasing it right after.

Probably check whether node is the last element and only re-grab the
lock if it isn't?

And btw, don't we need to hold the ->devres_lock during the whole search
like callers of find_dr do, for example?

> +}
> +EXPORT_SYMBOL_GPL(devres_for_each_res);
> +
> +/**
>   * devres_free - Free device resource data
>   * @res: Pointer to devres data to free
>   *
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 52a5f15..34dc1f8 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -536,6 +536,9 @@ extern void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp,
>  #else
>  extern void *devres_alloc(dr_release_t release, size_t size, gfp_t gfp);
>  #endif
> +extern void devres_for_each_res(struct device *dev, dr_release_t release,
> +			dr_match_t match, void *match_data,
> +			void (*fn)(struct device *, void *));

Args alignment.

>  extern void devres_free(void *res);
>  extern void devres_add(struct device *dev, void *res);
>  extern void *devres_find(struct device *dev, dr_release_t release,
> -- 
> 1.7.9.5
> 
> 

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551