On Thu, Jul 26, 2012 at 12:52 AM, Borislav Petkov <b...@amd64.org> wrote:
> On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
>> This patches introduces the three helpers below:
>>
>> void device_cache_firmwares(void)
>> void device_uncache_firmwares(void)
>> void device_uncache_firmwares_delay(unsigned long)
>
> I kinda don't like the plural of firmware: "firmwares". Can we call
> those
> device_cache_fw_images
> device_uncache_fw_images
Looks fine.
>
> or
> device_cache_fw_blobs
>
> or whatever?
>
>> so we can call device_cache_firmwares() to cache firmwares for
>> all devices which need firmwares to work, and the device driver
>> can get the firmware easily from kernel memory when system isn't
>> readly for completing their requests of loading firmwares.
>>
>> When system is ready for completing firmware loading, driver core
>> can call device_uncache_firmwares() or its delay version to free
>> the cached firmwares.
>>
>> The above helpers should be used to cache device firmwares during
>> system suspend/resume cycle in the following patches.
>>
>> Signed-off-by: Ming Lei <ming....@canonical.com>
>> ---
>> drivers/base/firmware_class.c | 236
>> +++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 230 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index c181e6d..7a96e75 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -22,6 +22,10 @@
>> #include <linux/slab.h>
>> #include <linux/sched.h>
>> #include <linux/list.h>
>> +#include <linux/async.h>
>> +#include <linux/pm.h>
>> +
>> +#include "base.h"
>>
>> MODULE_AUTHOR("Manuel Estrada Sainz");
>> MODULE_DESCRIPTION("Multi purpose firmware loading support");
>> @@ -91,6 +95,17 @@ struct firmware_cache {
>> /* firmware_buf instance will be added into the below list */
>> spinlock_t lock;
>> struct list_head head;
>> +
>> + /*
>> + * Name of firmware which has been cached successfully will be
>> + * added into the below list so that device uncache helper can
>> + * trace which firmware has been cached before.
>> + */
>
> Comment above the list_head and maybe call the list_head fw_names or so?
>
>> + spinlock_t name_lock;
>> + struct list_head name_head;
>> + wait_queue_head_t wait_queue;
>
> Stray \t
>
>> + int cnt;
>> + struct delayed_work work;
>> };
>>
>> struct firmware_buf {
>> @@ -107,6 +122,11 @@ struct firmware_buf {
>> char fw_id[];
>> };
>>
>> +struct fw_name_for_cache {
>> + struct list_head list;
>> + char name[];
>> +};
>
> Maybe fw_cache_entry?
Looks fine.
>
>> +
>> struct firmware_priv {
>> struct timer_list timeout;
>> bool nowait;
>> @@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
>> kref_put(&buf->ref, __fw_free_buf);
>> }
>>
>> -static void fw_cache_init(void)
>> -{
>> - spin_lock_init(&fw_cache.lock);
>> - INIT_LIST_HEAD(&fw_cache.head);
>> -}
>> -
>> static struct firmware_priv *to_firmware_priv(struct device *dev)
>> {
>> return container_of(dev, struct firmware_priv, dev);
>> @@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
>> return -EINVAL;
>> }
>>
>> +static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
>> +{
>> + struct fw_name_for_cache *nc;
>> +
>> + nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
>> + if (!nc)
>> + goto exit;
>> +
>> + strcpy(nc->name, name);
>> +exit:
>> + return nc;
>> +}
>> +
>> +static void free_fw_name_cache(struct fw_name_for_cache *nc)
>> +{
>> + kfree(nc);
>> +}
>> +
>> +static void __async_dev_cache_firmware(void *fw_name,
>> + async_cookie_t cookie)
>
> Arg alignment.
I am wondering why checkpatch.pl doesn't add the check...
>
>> +{
>> + struct fw_name_for_cache *nc;
>> + struct firmware_cache *fwc = &fw_cache;
>> + char *curr_name;
>> + int ret;
>> +
>> + /* 'fw_name' is stored in devres, and it may be released,
>> + * so allocate buffer to store the firmware name
>> + */
>> + curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
>> + if (!curr_name) {
>> + ret = -ENOMEM;
>> + goto drop_ref;
>> + }
>> +
>> + strcpy(curr_name, fw_name);
>
> AFAICT kstrdup already copies the existing string, why do you need to
> strcpy it again?
Good catch.
>
>> +
>> + ret = cache_firmware(curr_name);
>> +
>> + if (!ret) {
>
> Invert check logic to save an indentation level:
>
> if (ret)
> goto free;
>
> nc = alloc...
>
> ...
>
> free:
> kfree(curr_name);
>
>> + /*
>> + * allocate/all the instance of alloc_fw_name_cache
>> + * for uncaching later if cache_firmware returns
>> + * successfully
>> + */
>> + nc = alloc_fw_name_cache(curr_name);
>> +
>> + /*
>> + * have to uncache firmware in case of allocation
>> + * failure since we can't trace the firmware cache
>> + * any more without the firmware name.
>> + */
>> + if (!nc) {
>> + uncache_firmware(curr_name);
>> + } else {
>> + spin_lock(&fwc->name_lock);
>> + list_add(&nc->list, &fwc->name_head);
>> + spin_unlock(&fwc->name_lock);
>> + }
>> + }
>> + kfree(curr_name);
>> +
>> +drop_ref:
>> + spin_lock(&fwc->name_lock);
>> + fwc->cnt--;
>> + spin_unlock(&fwc->name_lock);
>> + wake_up(&fwc->wait_queue);
>> +}
>> +
>> +static void __dev_cache_firmware(struct device *dev, void *res)
>> +{
>> + struct fw_name_devm *fwn = res;
>> + const char *fw_name = fwn->name;
>> + struct firmware_cache *fwc = &fw_cache;
>> +
>> + dev_dbg(dev, "fw-%s %d\n", fw_name, fwc->cnt);
>> +
>> + spin_lock(&fwc->name_lock);
>> + fwc->cnt++;
>> + spin_unlock(&fwc->name_lock);
>> +
>> + async_schedule(__async_dev_cache_firmware, (void *)fw_name);
>> +}
>> +
>> +static int devm_name_match(struct device *dev, void *res,
>> + void *match_data)
>
> arg alignment
>
>> +{
>> + struct fw_name_devm *fwn = res;
>> + return (fwn->magic == (unsigned long)match_data);
>> +}
>> +
>> +static void dev_cache_firmware(struct device *dev)
>> +{
>> + devres_for_each_res(dev, fw_name_devm_release,
>> + devm_name_match, &fw_cache,
>> + __dev_cache_firmware);
>
> arg alignment
>
>> +}
>> +
>> +static void __device_uncache_firmwares(void)
>> +{
>> + struct firmware_cache *fwc = &fw_cache;
>> + struct fw_name_for_cache *nc;
>> +
>> + spin_lock(&fwc->name_lock);
>> + while (!list_empty(&fwc->name_head)) {
>> + nc = list_entry(fwc->name_head.next,
>> + struct fw_name_for_cache, list);
>> + list_del(&nc->list);
>> + spin_unlock(&fwc->name_lock);
>> +
>> + uncache_firmware(nc->name);
>> + free_fw_name_cache(nc);
>> +
>> + spin_lock(&fwc->name_lock);
>> + }
>> + spin_unlock(&fwc->name_lock);
>
> Same thing here: maybe check if on the last element on the list and
> don't grab the lock then?
>
>> +}
>> +
>> +extern struct list_head dpm_list;
>
> checkpatch says here:
>
> WARNING: externs should be avoided in .c files
> #181: FILE: drivers/base/firmware_class.c:1118:
> +extern struct list_head dpm_list;
>
> and I think it is correct. AFAICT, it should be included from
> <drivers/base/power/power.h>...
Good catch.
>
>> +/**
>> + * device_cache_firmwares - cache devices' firmwares
>> + *
>> + * For each devices, if they called request_firmware or
>> + * request_firmware_nowait successfully before, their firmware
>> + * name will be recored into these devices' devres link list, so
>> + * device_cache_firmwares can call cache_firmware() to cache these
>> + * firmwares for these devices, then these device drivers can load
>> + * their firmwares easily at any time even when system is not ready
>> + * to complete loading firmwares.
>
> This is a one looong sentence. Please simplify.
>
>> + *
>> + */
>> +static void device_cache_firmwares(void)
>> +{
>> + struct firmware_cache *fwc = &fw_cache;
>> + struct device *dev;
>> + DEFINE_WAIT(wait);
>> +
>> + pr_debug("%s\n", __func__);
>
> No real need for that debug printk since you have one below.
Suppose dev_cache_firmware hangs, you will see nothing without
the above line.
>
>> +
>> + device_pm_lock();
>> + list_for_each_entry(dev, &dpm_list, power.entry)
>> + dev_cache_firmware(dev);
>> + device_pm_unlock();
>> +
>> + pr_debug("%s firmwares %d\n", __func__, fwc->cnt);
>> +
>> + /* wait for completion of caching firmware for all devices */
>> + spin_lock(&fwc->name_lock);
>> + for (;;) {
>> + prepare_to_wait(&fwc->wait_queue, &wait,
>> + TASK_UNINTERRUPTIBLE);
>> + if (!fwc->cnt)
>> + break;
>> +
>> + spin_unlock(&fwc->name_lock);
>> +
>> + schedule();
>> +
>> + spin_lock(&fwc->name_lock);
>> + }
>> + spin_unlock(&fwc->name_lock);
>> + finish_wait(&fwc->wait_queue, &wait);
>> +}
>> +
>> +/**
>> + * device_uncache_firmwares - uncache devices' firmwares
>> + *
>> + * uncache all firmwares which have been cached successfully
>> + * by device_uncache_firmwares
>
> "by device_cache_firmwares earlier."
>
>> + *
>> + */
>> +static void device_uncache_firmwares(void)
>> +{
>> + pr_debug("%s\n", __func__);
>> + __device_uncache_firmwares();
>> +}
>> +
>
> Ok, the rest of the patches tomorrow.
Thank you for review.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
