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

[Ming Lei <ming.lei@canonical.com>]

On Thu, Jul 26, 2012 at 12:25 AM, Borislav Petkov <bp@amd64.org> wrote:
> 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?

It is not necessary since the lock isn't held in hot path.

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

Because I don't want to put more constraint on the 'fn' about lock use, memory
allocation flag(gfp)..., from the view of API's user.

In fact, there is problem wrt. releasing lock since add_dr may add new node
during releasing lock.

So I plan to just hold the lock to cover calling 'fn' in -v1 instead
of using rcu list
helpers, which may introduce a lot change on devres.

Anyway the callers can copy the resources interested into a temporary list
in 'fn' and handle it later, which may simplify devres_for_each_res and other
devres helpers a lot.

Also I will introduce another parameter of 'void *data' to 'fn' in -v1.

Thanks,
--
Ming Lei