Messages in this thread Patch in this message | | | Date | Wed, 25 Jul 2012 15:59:48 +0200 | From | Borislav Petkov <> | Subject | Re: [RFC PATCH 05/13] driver core: firmware loader: introduce firmware_buf |
| |
On Wed, Jul 25, 2012 at 01:00:05AM +0800, Ming Lei wrote: > This patch introduces struct firmware_buf to describe the buffer > which holds the firmware data, which will make the following > cache_firmware/uncache_firmware implemented easily. > > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > drivers/base/firmware_class.c | 176 +++++++++++++++++++++++------------------ > 1 file changed, 101 insertions(+), 75 deletions(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 0b343b8..986d9df 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -89,7 +89,7 @@ static inline long firmware_loading_timeout(void) > * guarding for corner cases a global lock should be OK */ > static DEFINE_MUTEX(fw_lock); > > -struct firmware_priv { > +struct firmware_buf { > struct completion completion; > struct firmware *fw; > unsigned long status; > @@ -98,10 +98,14 @@ struct firmware_priv { > struct page **pages; > int nr_pages; > int page_array_size; > + char fw_id[]; > +}; > + > +struct firmware_priv { > struct timer_list timeout; > - struct device dev; > bool nowait; > - char fw_id[]; > + struct device dev; > + struct firmware_buf *buf; > }; > > static struct firmware_priv *to_firmware_priv(struct device *dev) > @@ -111,8 +115,10 @@ static struct firmware_priv *to_firmware_priv(struct device *dev) > > static void fw_load_abort(struct firmware_priv *fw_priv) > { > - set_bit(FW_STATUS_ABORT, &fw_priv->status); > - complete(&fw_priv->completion); > + struct firmware_buf *buf = fw_priv->buf; > + > + set_bit(FW_STATUS_ABORT, &buf->status); > + complete(&buf->completion); > } > > static ssize_t firmware_timeout_show(struct class *class, > @@ -152,16 +158,23 @@ static struct class_attribute firmware_class_attrs[] = { > __ATTR_NULL > }; > > -static void fw_dev_release(struct device *dev) > +static void fw_free_buf(struct firmware_buf *buf) > { > - struct firmware_priv *fw_priv = to_firmware_priv(dev); > int i; > > - /* free untransfered pages buffer */ > - for (i = 0; i < fw_priv->nr_pages; i++) > - __free_page(fw_priv->pages[i]); > - kfree(fw_priv->pages); > + if (!buf) > + return;
This is subtle: the caller of fw_free_buf might forget to assign NULL to the buf ptr.
Why not pass struct firmware_priv *fw_priv to the function instead and ...
> + > + for (i = 0; i < buf->nr_pages; i++) > + __free_page(buf->pages[i]); > + kfree(buf->pages);
assign NULL to the ptr as a last step, when all is done:
fw_priv->buf = NULL;
This way you're making sure ->buf is NULL after all pages are freed and your check above is always correct.
> +} > + > +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); > @@ -171,7 +184,7 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env) > { > struct firmware_priv *fw_priv = to_firmware_priv(dev); > > - if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->fw_id)) > + if (add_uevent_var(env, "FIRMWARE=%s", fw_priv->buf->fw_id)) > return -ENOMEM; > if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout)) > return -ENOMEM; > @@ -192,7 +205,7 @@ static ssize_t firmware_loading_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct firmware_priv *fw_priv = to_firmware_priv(dev); > - int loading = test_bit(FW_STATUS_LOADING, &fw_priv->status); > + int loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status); > > return sprintf(buf, "%d\n", loading); > } > @@ -231,32 +244,33 @@ static ssize_t firmware_loading_store(struct device *dev, > const char *buf, size_t count) > { > struct firmware_priv *fw_priv = to_firmware_priv(dev); > + struct firmware_buf *fw_buf = fw_priv->buf; > int loading = simple_strtol(buf, NULL, 10); > int i; > > mutex_lock(&fw_lock); > > - if (!fw_priv->fw) > + if (!fw_buf) > goto out; > > switch (loading) { > case 1: > /* discarding any previous partial load */ > - if (!test_bit(FW_STATUS_DONE, &fw_priv->status)) { > - for (i = 0; i < fw_priv->nr_pages; i++) > - __free_page(fw_priv->pages[i]); > - kfree(fw_priv->pages); > - fw_priv->pages = NULL; > - fw_priv->page_array_size = 0; > - fw_priv->nr_pages = 0; > - set_bit(FW_STATUS_LOADING, &fw_priv->status); > + if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) { > + for (i = 0; i < fw_buf->nr_pages; i++) > + __free_page(fw_buf->pages[i]); > + kfree(fw_buf->pages); > + fw_buf->pages = NULL; > + fw_buf->page_array_size = 0; > + fw_buf->nr_pages = 0; > + set_bit(FW_STATUS_LOADING, &fw_buf->status); > } > break; > case 0: > - if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) { > - set_bit(FW_STATUS_DONE, &fw_priv->status); > - clear_bit(FW_STATUS_LOADING, &fw_priv->status); > - complete(&fw_priv->completion); > + 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); > break; > } > /* fallthrough */ > @@ -280,21 +294,21 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj, > { > struct device *dev = kobj_to_dev(kobj); > struct firmware_priv *fw_priv = to_firmware_priv(dev); > - struct firmware *fw; > + struct firmware_buf *buf; > ssize_t ret_count; > > mutex_lock(&fw_lock); > - fw = fw_priv->fw; > - if (!fw || test_bit(FW_STATUS_DONE, &fw_priv->status)) { > + buf = fw_priv->buf; > + if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) { > ret_count = -ENODEV; > goto out; > } > - if (offset > fw_priv->size) { > + if (offset > buf->size) { > ret_count = 0; > goto out; > } > - if (count > fw_priv->size - offset) > - count = fw_priv->size - offset; > + if (count > buf->size - offset) > + count = buf->size - offset; > > ret_count = count; > > @@ -304,11 +318,11 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj, > int page_ofs = offset & (PAGE_SIZE-1); > int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count); > > - page_data = kmap(fw_priv->pages[page_nr]); > + page_data = kmap(buf->pages[page_nr]); > > memcpy(buffer, page_data + page_ofs, page_cnt); > > - kunmap(fw_priv->pages[page_nr]); > + kunmap(buf->pages[page_nr]); > buffer += page_cnt; > offset += page_cnt; > count -= page_cnt; > @@ -320,12 +334,13 @@ out:
While you're at it, you can indent this "out:" label one space to the right so that the diff can pick up the function name in the hunk tag above instead of the label.
@@ -329,7 +329,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj, offset += page_cnt; count -= page_cnt; } -out: + out: mutex_unlock(&fw_lock); return ret_count; }
> static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size) > { > + struct firmware_buf *buf = fw_priv->buf; > int pages_needed = ALIGN(min_size, PAGE_SIZE) >> PAGE_SHIFT; > > /* If the array of pages is too small, grow it... */ > - if (fw_priv->page_array_size < pages_needed) { > + if (buf->page_array_size < pages_needed) { > int new_array_size = max(pages_needed, > - fw_priv->page_array_size * 2); > + buf->page_array_size * 2); > struct page **new_pages; > > new_pages = kmalloc(new_array_size * sizeof(void *), > @@ -334,24 +349,24 @@ static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size) > fw_load_abort(fw_priv); > return -ENOMEM; > } > - memcpy(new_pages, fw_priv->pages, > - fw_priv->page_array_size * sizeof(void *)); > - memset(&new_pages[fw_priv->page_array_size], 0, sizeof(void *) * > - (new_array_size - fw_priv->page_array_size)); > - kfree(fw_priv->pages); > - fw_priv->pages = new_pages; > - fw_priv->page_array_size = new_array_size; > + memcpy(new_pages, buf->pages, > + buf->page_array_size * sizeof(void *)); > + memset(&new_pages[buf->page_array_size], 0, sizeof(void *) * > + (new_array_size - buf->page_array_size)); > + kfree(buf->pages); > + buf->pages = new_pages; > + buf->page_array_size = new_array_size; > } > > - while (fw_priv->nr_pages < pages_needed) { > - fw_priv->pages[fw_priv->nr_pages] = > + while (buf->nr_pages < pages_needed) { > + buf->pages[buf->nr_pages] = > alloc_page(GFP_KERNEL | __GFP_HIGHMEM); > > - if (!fw_priv->pages[fw_priv->nr_pages]) { > + if (!buf->pages[buf->nr_pages]) { > fw_load_abort(fw_priv); > return -ENOMEM; > } > - fw_priv->nr_pages++; > + buf->nr_pages++; > } > return 0; > } > @@ -374,15 +389,15 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj, > { > struct device *dev = kobj_to_dev(kobj); > struct firmware_priv *fw_priv = to_firmware_priv(dev); > - struct firmware *fw; > + struct firmware_buf *buf; > ssize_t retval; > > if (!capable(CAP_SYS_RAWIO)) > return -EPERM; > > mutex_lock(&fw_lock); > - fw = fw_priv->fw; > - if (!fw || test_bit(FW_STATUS_DONE, &fw_priv->status)) { > + buf = fw_priv->buf; > + if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) { > retval = -ENODEV; > goto out; > } > @@ -399,17 +414,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj, > int page_ofs = offset & (PAGE_SIZE - 1); > int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count); > > - page_data = kmap(fw_priv->pages[page_nr]); > + page_data = kmap(buf->pages[page_nr]); > > memcpy(page_data + page_ofs, buffer, page_cnt); > > - kunmap(fw_priv->pages[page_nr]); > + kunmap(buf->pages[page_nr]); > buffer += page_cnt; > offset += page_cnt; > count -= page_cnt; > } > > - fw_priv->size = max_t(size_t, offset, fw_priv->size); > + buf->size = max_t(size_t, offset, buf->size); > out: > mutex_unlock(&fw_lock); > return retval; > @@ -434,20 +449,31 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, > struct device *device, bool uevent, bool nowait) > { > struct firmware_priv *fw_priv; > + struct firmware_buf *buf; > struct device *f_dev; > > - fw_priv = kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL); > + fw_priv = kzalloc(sizeof(*fw_priv), GFP_KERNEL); > if (!fw_priv) { > dev_err(device, "%s: kmalloc failed\n", __func__); > - return ERR_PTR(-ENOMEM); > + fw_priv = ERR_PTR(-ENOMEM); > + goto exit; > } > > - fw_priv->fw = firmware; > + 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; > - strcpy(fw_priv->fw_id, fw_name); > - init_completion(&fw_priv->completion); > 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; > > @@ -455,7 +481,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, > dev_set_name(f_dev, "%s", fw_name); > f_dev->parent = device; > f_dev->class = &firmware_class; > - > +exit:
Ditto: please indent label names one position to the right.
[ … ]
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/
| |