Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares

[Ming Lei]

On Thu, Jul 26, 2012 at 12:52 AM, Borislav Petkov <bp@xxxxxxxxx> 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.lei@xxxxxxxxxxxxx>
>> ---
>>  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/