Messages in this thread | | | Date | Tue, 24 Jul 2012 20:10:45 +0200 | From | Borislav Petkov <> | Subject | Re: [RFC PATCH 01/13] driver core: firmware loader: simplify pages ownership transfer |
| |
On Wed, Jul 25, 2012 at 01:00:01AM +0800, Ming Lei wrote: > This patch doesn't transfer ownership of pages' buffer to the > instance of firmware until the firmware loading is completed, > which will simplify firmware_loading_store a lot, so help > to introduce the following cache_firmware and uncache_firmware > mechanism during system suspend-resume cycle. > > In fact, this patch fixes one bug: if writing data into > firmware loader device is bypassed between writting 1 and 0 to > 'loading', OOPS will be triggered without the patch. > > Also add some comments to make code more readable. > > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > drivers/base/firmware_class.c | 57 ++++++++++++++++++++++++----------------- > 1 file changed, 34 insertions(+), 23 deletions(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 803cfc1..f789bbd 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -93,6 +93,8 @@ struct firmware_priv { > struct completion completion; > struct firmware *fw; > unsigned long status; > + void *data; > + size_t size; > struct page **pages; > int nr_pages; > int page_array_size; > @@ -156,9 +158,11 @@ static void fw_dev_release(struct device *dev) > 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); > + > kfree(fw_priv); > > module_put(THIS_MODULE); > @@ -194,6 +198,7 @@ static ssize_t firmware_loading_show(struct device *dev, > return sprintf(buf, "%d\n", loading); > } > > +/* firmware holds the ownership of pages */ > static void firmware_free_data(const struct firmware *fw) > { > int i; > @@ -237,9 +242,7 @@ static ssize_t firmware_loading_store(struct device *dev, > > switch (loading) { > case 1: > - firmware_free_data(fw_priv->fw); > - memset(fw_priv->fw, 0, sizeof(struct firmware)); > - /* If the pages are not owned by 'struct firmware' */ > + /* discarding any previous partial load */ > for (i = 0; i < fw_priv->nr_pages; i++) > __free_page(fw_priv->pages[i]); > kfree(fw_priv->pages); > @@ -250,20 +253,6 @@ static ssize_t firmware_loading_store(struct device *dev, > break; > case 0: > if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) { > - vunmap(fw_priv->fw->data); > - fw_priv->fw->data = vmap(fw_priv->pages, > - fw_priv->nr_pages, > - 0, PAGE_KERNEL_RO); > - if (!fw_priv->fw->data) { > - dev_err(dev, "%s: vmap() failed\n", __func__); > - goto err; > - } > - /* Pages are now owned by 'struct firmware' */ > - fw_priv->fw->pages = fw_priv->pages; > - fw_priv->pages = NULL; > - > - fw_priv->page_array_size = 0; > - fw_priv->nr_pages = 0; > complete(&fw_priv->completion); > clear_bit(FW_STATUS_LOADING, &fw_priv->status); > break; > @@ -273,7 +262,6 @@ static ssize_t firmware_loading_store(struct device *dev, > dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading); > /* fallthrough */ > case -1: > - err: > fw_load_abort(fw_priv); > break; > } > @@ -299,12 +287,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj, > ret_count = -ENODEV; > goto out; > } > - if (offset > fw->size) { > + if (offset > fw_priv->size) { > ret_count = 0; > goto out; > } > - if (count > fw->size - offset) > - count = fw->size - offset; > + if (count > fw_priv->size - offset) > + count = fw_priv->size - offset; > > ret_count = count; > > @@ -396,6 +384,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj, > retval = -ENODEV; > goto out; > } > + > retval = fw_realloc_buffer(fw_priv, offset + count); > if (retval) > goto out; > @@ -418,7 +407,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj, > count -= page_cnt; > } > > - fw->size = max_t(size_t, offset, fw->size); > + fw_priv->size = max_t(size_t, offset, fw_priv->size); > out: > mutex_unlock(&fw_lock); > return retval; > @@ -504,6 +493,24 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p) > *firmware_p = NULL; > } > > +/* transfer the ownership of pages to firmware */ > +static void fw_set_page_data(struct firmware_priv *fw_priv) > +{ > + struct firmware *fw = fw_priv->fw; > + > + fw_priv->data = vmap(fw_priv->pages, fw_priv->nr_pages, > + 0, PAGE_KERNEL_RO);
We don't need to check the return value of vmap() here like we do above?
> + fw->data = fw_priv->data; > + fw->pages = fw_priv->pages; > + fw->size = fw_priv->size;
-- 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
| |