Messages in this thread Patch in this message | | | Date | Wed, 25 Jul 2012 16:37:50 +0200 | From | Borislav Petkov <> | Subject | Re: [RFC PATCH 06/13] driver core: firmware loader: always let firmware_buf own the pages buffer |
| |
On Wed, Jul 25, 2012 at 01:00:06AM +0800, Ming Lei wrote: > This patch always let firmware_buf own the pages buffer allocated > inside firmware_data_write, also add all instances of firmware_buf > into the firmware cache global list. Also introduce one private field > in 'struct firmware', so release_firmware will see the instance of > firmware_buf associated with one firmware instance, then just 'free' > the instance of firmware_buf. > > The firmware_buf instance represents one pages buffer for one > firmware image, so lots of firmware loading requests can share > the same firmware_buf instance if they request the same firmware > image file. > > This patch will make introducing cache_firmware/uncache_firmware > easily. > > In fact, the patch improves request_formware/release_firmware: > > - only request userspace to write firmware image once if > several devices share one same firmware image and its drivers > call request_firmware concurrently. > > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > drivers/base/firmware_class.c | 222 ++++++++++++++++++++++++++++------------- > include/linux/firmware.h | 3 + > 2 files changed, 157 insertions(+), 68 deletions(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 986d9df..225898e 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -21,6 +21,7 @@ > #include <linux/firmware.h> > #include <linux/slab.h> > #include <linux/sched.h> > +#include <linux/list.h> > > MODULE_AUTHOR("Manuel Estrada Sainz"); > MODULE_DESCRIPTION("Multi purpose firmware loading support"); > @@ -85,13 +86,18 @@ static inline long firmware_loading_timeout(void) > return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT; > } > > -/* fw_lock could be moved to 'struct firmware_priv' but since it is just > - * guarding for corner cases a global lock should be OK */ > -static DEFINE_MUTEX(fw_lock); > +struct firmware_cache { > + > + /* firmware_buf instance will be added into the below list */ > + spinlock_t lock; > + struct list_head head; > +}; > > struct firmware_buf { > + struct kref ref; > + struct list_head list; > struct completion completion; > - struct firmware *fw; > + struct firmware_cache *fwc; > unsigned long status; > void *data; > size_t size; > @@ -106,8 +112,93 @@ struct firmware_priv { > bool nowait; > struct device dev; > struct firmware_buf *buf; > + struct firmware *fw; > }; > > +#define to_fwbuf(d) container_of(d, struct firmware_buf, ref) > + > +/* fw_lock could be moved to 'struct firmware_priv' but since it is just > + * guarding for corner cases a global lock should be OK */ > +static DEFINE_MUTEX(fw_lock); > + > +static struct firmware_cache fw_cache; > + > +static struct firmware_buf *__allocate_fw_buf(const char *fw_name, > + struct firmware_cache *fwc)
Please indent like so:
static struct firmware_buf *__allocate_fw_buf(const char *fw_name, struct firmware_cache *fwc)
for better readability.
> +{ > + struct firmware_buf *buf; > + > + buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1 , GFP_ATOMIC); > + > + if (!buf) > + return buf; > + > + kref_init(&buf->ref); > + strcpy(buf->fw_id, fw_name); > + buf->fwc = fwc; > + init_completion(&buf->completion); > + > + pr_debug("%s: fw-%s buf=%p\n", __func__, fw_name, buf); > + > + return buf; > +} > + > +static int fw_lookup_and_alloate_buf(const char *fw_name, > + struct firmware_cache *fwc, > + struct firmware_buf **buf)
Ditto.
> +{ > + struct firmware_buf *tmp; > + > + spin_lock(&fwc->lock); > + list_for_each_entry(tmp, &fwc->head, list) > + if (!strcmp(tmp->fw_id, fw_name)) { > + kref_get(&tmp->ref); > + spin_unlock(&fwc->lock); > + *buf = tmp; > + return 1; > + } > + > + tmp = __allocate_fw_buf(fw_name, fwc); > + if (tmp) > + list_add(&tmp->list, &fwc->head); > + spin_unlock(&fwc->lock); > + > + *buf = tmp; > + > + return tmp ? 0 : -1; > +} > + > +static void __fw_free_buf(struct kref *ref) > +{ > + struct firmware_buf *buf = to_fwbuf(ref); > + struct firmware_cache *fwc = buf->fwc; > + int i; > + > + pr_debug("%s: fw-%s buf=%p nr_pages=%d\n", > + __func__, buf->fw_id, buf, buf->nr_pages);
Arg alignment:
pr_debug("%s: fw-%s buf=%p nr_pages=%d\n", __func__, buf->fw_id, buf, buf->nr_pages);
> + spin_lock(&fwc->lock); > + list_del(&buf->list); > + spin_unlock(&fwc->lock); > + > + vunmap(buf->data); > + for (i = 0; i < buf->nr_pages; i++) > + __free_page(buf->pages[i]); > + kfree(buf->pages); > + kfree(buf); > +} > + > +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); > @@ -118,7 +209,7 @@ static void fw_load_abort(struct firmware_priv *fw_priv) > struct firmware_buf *buf = fw_priv->buf; > > set_bit(FW_STATUS_ABORT, &buf->status); > - complete(&buf->completion); > + complete_all(&buf->completion); > } > > static ssize_t firmware_timeout_show(struct class *class, > @@ -158,23 +249,10 @@ static struct class_attribute firmware_class_attrs[] = { > __ATTR_NULL > }; > > -static void fw_free_buf(struct firmware_buf *buf) > -{ > - int i; > - > - if (!buf) > - return; > - > - for (i = 0; i < buf->nr_pages; i++) > - __free_page(buf->pages[i]); > - kfree(buf->pages); > -} > - > static void fw_dev_release(struct device *dev) > { > struct firmware_priv *fw_priv = to_firmware_priv(dev); > > - kfree(fw_priv->buf); > kfree(fw_priv); > > module_put(THIS_MODULE); > @@ -213,13 +291,8 @@ static ssize_t firmware_loading_show(struct device *dev, > /* firmware holds the ownership of pages */ > static void firmware_free_data(const struct firmware *fw) > { > - int i; > - vunmap(fw->data); > - if (fw->pages) { > - for (i = 0; i < PFN_UP(fw->size); i++) > - __free_page(fw->pages[i]); > - kfree(fw->pages); > - } > + WARN_ON(!fw->priv); > + fw_free_buf(fw->priv);
Why the WARN_ON?
Maybe rather:
if (fw->priv) fw_free_buf(fw->priv);
?
> /* Some architectures don't have PAGE_KERNEL_RO */ > @@ -270,7 +343,7 @@ static ssize_t firmware_loading_store(struct device *dev, > if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) { > set_bit(FW_STATUS_DONE, &fw_buf->status); > clear_bit(FW_STATUS_LOADING, &fw_buf->status); > - complete(&fw_buf->completion); > + complete_all(&fw_buf->completion); > break; > } > /* fallthrough */ > @@ -446,10 +519,10 @@ static void firmware_class_timeout(u_long data) > > static struct firmware_priv * > fw_create_instance(struct firmware *firmware, const char *fw_name, > - struct device *device, bool uevent, bool nowait) > + struct device *device, bool uevent, bool nowait) > +
Wrong alignment and superfluous newline.
> { > struct firmware_priv *fw_priv; > - struct firmware_buf *buf; > struct device *f_dev; > > fw_priv = kzalloc(sizeof(*fw_priv), GFP_KERNEL); > @@ -459,21 +532,10 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, > goto exit; > } > > - buf = kzalloc(sizeof(*buf) + strlen(fw_name) + 1, GFP_KERNEL); > - if (!buf) { > - dev_err(device, "%s: kmalloc failed\n", __func__); > - kfree(fw_priv); > - fw_priv = ERR_PTR(-ENOMEM); > - goto exit; > - } > - > - buf->fw = firmware; > - fw_priv->buf = buf; > fw_priv->nowait = nowait; > + fw_priv->fw = firmware; > setup_timer(&fw_priv->timeout, > firmware_class_timeout, (u_long) fw_priv); > - strcpy(buf->fw_id, fw_name); > - init_completion(&buf->completion); > > f_dev = &fw_priv->dev; > > @@ -485,12 +547,31 @@ exit: > return fw_priv; > } > > +/* store the pages buffer info firmware from buf */ > +static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw) > +{ > + buf->data = vmap(buf->pages, buf->nr_pages, > + 0, PAGE_KERNEL_RO); > + fw->data = buf->data; > + fw->pages = buf->pages; > + fw->size = buf->size; > + fw->priv = buf; > +} > + > +static void _request_firmware_cleanup(const struct firmware **firmware_p) > +{ > + release_firmware(*firmware_p); > + *firmware_p = NULL; > +} > + > static struct firmware_priv * > _request_firmware_prepare(const struct firmware **firmware_p, const char *name, > struct device *device, bool uevent, bool nowait) > { > struct firmware *firmware; > - struct firmware_priv *fw_priv; > + struct firmware_priv *fw_priv = NULL; > + struct firmware_buf *buf; > + int ret; > > if (!firmware_p) > return ERR_PTR(-EINVAL); > @@ -507,32 +588,40 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name, > return NULL; > } > > - fw_priv = fw_create_instance(firmware, name, device, uevent, nowait); > - if (IS_ERR(fw_priv)) { > - release_firmware(firmware); > - *firmware_p = NULL; > - } > - return fw_priv; > -} > + ret = fw_lookup_and_alloate_buf(name, &fw_cache, &buf); > > -static void _request_firmware_cleanup(const struct firmware **firmware_p) > -{ > - release_firmware(*firmware_p); > - *firmware_p = NULL; > -}
Superfluous newline here.
> + if (!ret) > + fw_priv = fw_create_instance(firmware, name, device, > + uevent, nowait); > > -/* transfer the ownership of pages to firmware */ > -static void fw_set_page_data(struct firmware_buf *buf) > -{ > - struct firmware *fw = buf->fw; > + if (IS_ERR(fw_priv) || ret == -1) { > + kfree(firmware); > + *firmware_p = NULL; > + return ERR_PTR(-ENOMEM); > + } else if (fw_priv) { > + fw_priv->buf = buf; > + return fw_priv; > + } > > - buf->data = vmap(buf->pages, buf->nr_pages, > - 0, PAGE_KERNEL_RO); > - fw->data = buf->data; > - fw->pages = buf->pages; > - fw->size = buf->size; > + /* share the cached buf, which is inprogessing or completed */
in progress
> +check_status:
One space in front of the label name.
> + mutex_lock(&fw_lock); > + if (test_bit(FW_STATUS_ABORT, &buf->status)) { > + fw_priv = ERR_PTR(-ENOENT); > + _request_firmware_cleanup(firmware_p); > + goto exit; > + } else if (test_bit(FW_STATUS_DONE, &buf->status)) { > + fw_priv = NULL; > + fw_set_page_data(buf, firmware); > + goto exit; > + } > + mutex_unlock(&fw_lock); > + wait_for_completion(&buf->completion); > + goto check_status;
Better yet, this is a label with a reverse-jump to it. This can probably be simplified with a while loop:
while (true) { if (test_bit(... ) { ...; break; } else if (test_bit(... ) { ...; break; } mutex_unlock(...); wait_for_completion(...); }
and this way you don't need the exit label either.
> > - WARN_ON(PFN_UP(fw->size) != buf->nr_pages); > +exit: > + mutex_unlock(&fw_lock); > + return fw_priv; > }
[ … ]
Thanks.
-- 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |