Re: [RFC PATCH 06/13] driver core: firmware loader: always letfirmware_buf own the pages buffer
From: Borislav Petkov
Date: Wed Jul 25 2012 - 10:37:53 EST
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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/