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
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?
> +
> 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.
> +{
> + 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?
> +
> + 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>...
> +/**
> + * 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.
> +
> + 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.
--
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
--
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/
