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....@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
