linux-kernel - Re: [RFC PATCH 10/13] driver core: devres: introduce devres_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACVXFVNgMQGOULphipVi5L5wWFuEyuiZgzRCdh9pOSW=RA2dYA@mail.gmail.com>
Date:	Fri, 27 Jul 2012 00:51:26 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Borislav Petkov <bp@...64.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 10/13] driver core: devres: introduce devres_for_each_res

On Thu, Jul 26, 2012 at 12:25 AM, Borislav Petkov <bp@...64.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@...onical.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
--
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/