Messages in this thread | | | Date | Wed, 25 Jul 2012 18:52:24 +0200 | From | Borislav Petkov <> | Subject | Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares |
| |
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.lei@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
| |