From dd336c554d8926c3348a2d5f2a5ef5597f6d1a06 Mon Sep 17 00:00:00 2001 From: David Woodhouse Date: Sun, 2 May 2010 11:21:21 +0300 Subject: firmware_class: fix memory leak - free allocated pages fix memory leak introduced by the patch 6e03a201bbe: firmware: speed up request_firmware() 1. vfree won't release pages there were allocated explicitly and mapped using vmap. The memory has to be vunmap-ed and the pages needs to be freed explicitly 2. page array is moved into the 'struct firmware' so that we can free it from release_firmware() and not only in fw_dev_release() The fix doesn't break the firmware load speed. Cc: Johannes Berg Cc: Ming Lei Cc: Catalin Marinas Singed-off-by: Kay Sievers Signed-off-by: David Woodhouse Signed-off-by: Tomas Winkler Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) (limited to 'drivers/base/firmware_class.c') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 985da11174e7..4c70b9148b28 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -130,6 +130,17 @@ static ssize_t firmware_loading_show(struct device *dev, return sprintf(buf, "%d\n", loading); } +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); + } +} + /* Some architectures don't have PAGE_KERNEL_RO */ #ifndef PAGE_KERNEL_RO #define PAGE_KERNEL_RO PAGE_KERNEL @@ -162,21 +173,21 @@ static ssize_t firmware_loading_store(struct device *dev, mutex_unlock(&fw_lock); break; } - vfree(fw_priv->fw->data); - fw_priv->fw->data = NULL; + firmware_free_data(fw_priv->fw); + memset(fw_priv->fw, 0, sizeof(struct firmware)); + /* If the pages are not owned by 'struct firmware' */ 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; - fw_priv->fw->size = 0; set_bit(FW_STATUS_LOADING, &fw_priv->status); mutex_unlock(&fw_lock); break; case 0: if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) { - vfree(fw_priv->fw->data); + vunmap(fw_priv->fw->data); fw_priv->fw->data = vmap(fw_priv->pages, fw_priv->nr_pages, 0, PAGE_KERNEL_RO); @@ -184,7 +195,10 @@ static ssize_t firmware_loading_store(struct device *dev, dev_err(dev, "%s: vmap() failed\n", __func__); goto err; } - /* Pages will be freed by vfree() */ + /* 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); @@ -578,7 +592,7 @@ release_firmware(const struct firmware *fw) if (fw->data == builtin->data) goto free_fw; } - vfree(fw->data); + firmware_free_data(fw); free_fw: kfree(fw); } -- cgit v1.2.3 From 6f18ff91d926ab411749a199b06a49f6bc72e9af Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Sat, 27 Feb 2010 21:43:22 +0100 Subject: Driver core: Reduce the level of request_firmware() messages The messages from _request_firmware() informing that firmware is being requested or built-in firmware is going to be used are printed at KERN_INFO, which produces lots of noise on systems with huge numbers of AMD CPUs. Reduce the level of these messages to KERN_DEBUG to get rid of that noise. Signed-off-by: Rafael J. Wysocki Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/base/firmware_class.c') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 4c70b9148b28..8e6c62b4f512 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -507,15 +507,14 @@ _request_firmware(const struct firmware **firmware_p, const char *name, builtin++) { if (strcmp(name, builtin->name)) continue; - dev_info(device, "firmware: using built-in firmware %s\n", - name); + dev_dbg(device, "firmware: using built-in firmware %s\n", name); firmware->size = builtin->size; firmware->data = builtin->data; return 0; } if (uevent) - dev_info(device, "firmware: requesting %s\n", name); + dev_dbg(device, "firmware: requesting %s\n", name); retval = fw_setup_device(firmware, &f_dev, name, device, uevent); if (retval) -- cgit v1.2.3 From e9045f9178f3e3445a3a5b85206f8681b3869562 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Mon, 29 Mar 2010 17:57:20 +0200 Subject: firmware class: export nowait to userspace When we use request_firmware_nowait(), userspace may not want to answer negatively right away when for example it is answering from an initrd only, but with request_firmware() it has to in order to not delay the kernel boot until the request times out. This allows userspace to differentiate between the two in order to be able to reply negatively to async requests only when all filesystems have been mounted and have been checked for the requested firmware file. Signed-off-by: Johannes Berg Cc: Kay Sievers Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'drivers/base/firmware_class.c') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 8e6c62b4f512..d45d1e1c40a4 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -50,6 +50,7 @@ struct firmware_priv { int page_array_size; const char *vdata; struct timer_list timeout; + bool nowait; }; #ifdef CONFIG_FW_LOADER @@ -112,6 +113,8 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env) return -ENOMEM; if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout)) return -ENOMEM; + if (add_uevent_var(env, "ASYNC=%d", fw_priv->nowait)) + return -ENOMEM; return 0; } @@ -441,7 +444,7 @@ error_kfree: static int fw_setup_device(struct firmware *fw, struct device **dev_p, const char *fw_name, struct device *device, - int uevent) + int uevent, bool nowait) { struct device *f_dev; struct firmware_priv *fw_priv; @@ -457,6 +460,8 @@ static int fw_setup_device(struct firmware *fw, struct device **dev_p, fw_priv = dev_get_drvdata(f_dev); + fw_priv->nowait = nowait; + fw_priv->fw = fw; sysfs_bin_attr_init(&fw_priv->attr_data); retval = sysfs_create_bin_file(&f_dev->kobj, &fw_priv->attr_data); @@ -484,7 +489,7 @@ out: static int _request_firmware(const struct firmware **firmware_p, const char *name, - struct device *device, int uevent) + struct device *device, int uevent, bool nowait) { struct device *f_dev; struct firmware_priv *fw_priv; @@ -516,7 +521,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (uevent) dev_dbg(device, "firmware: requesting %s\n", name); - retval = fw_setup_device(firmware, &f_dev, name, device, uevent); + retval = fw_setup_device(firmware, &f_dev, name, device, + uevent, nowait); if (retval) goto error_kfree_fw; @@ -573,7 +579,7 @@ request_firmware(const struct firmware **firmware_p, const char *name, struct device *device) { int uevent = 1; - return _request_firmware(firmware_p, name, device, uevent); + return _request_firmware(firmware_p, name, device, uevent, false); } /** @@ -619,7 +625,7 @@ request_firmware_work_func(void *arg) return 0; } ret = _request_firmware(&fw, fw_work->name, fw_work->device, - fw_work->uevent); + fw_work->uevent, true); fw_work->cont(fw, fw_work->context); -- cgit v1.2.3 From 673fae90d5ee4ae2b6403f9d45af7ff640f06a60 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Sat, 13 Mar 2010 23:49:13 -0800 Subject: firmware loader: rely on driver core to create class attribute Do not create 'timeout' attribute manually, let driver core do it for us. This also ensures that attribute is cleaned up properly. Signed-off-by: Dmitry Torokhov Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 59 ++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 35 deletions(-) (limited to 'drivers/base/firmware_class.c') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index d45d1e1c40a4..d351e773f439 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -101,9 +101,26 @@ firmware_timeout_store(struct class *class, return count; } -static CLASS_ATTR(timeout, 0644, firmware_timeout_show, firmware_timeout_store); +static struct class_attribute firmware_class_attrs[] = { + __ATTR(timeout, S_IWUSR | S_IRUGO, + firmware_timeout_show, firmware_timeout_store), + __ATTR_NULL +}; -static void fw_dev_release(struct device *dev); +static void fw_dev_release(struct device *dev) +{ + struct firmware_priv *fw_priv = dev_get_drvdata(dev); + int i; + + for (i = 0; i < fw_priv->nr_pages; i++) + __free_page(fw_priv->pages[i]); + kfree(fw_priv->pages); + kfree(fw_priv->fw_id); + kfree(fw_priv); + kfree(dev); + + module_put(THIS_MODULE); +} static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env) { @@ -121,6 +138,7 @@ static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env) static struct class firmware_class = { .name = "firmware", + .class_attrs = firmware_class_attrs, .dev_uevent = firmware_uevent, .dev_release = fw_dev_release, }; @@ -370,21 +388,6 @@ static struct bin_attribute firmware_attr_data_tmpl = { .write = firmware_data_write, }; -static void fw_dev_release(struct device *dev) -{ - struct firmware_priv *fw_priv = dev_get_drvdata(dev); - int i; - - for (i = 0; i < fw_priv->nr_pages; i++) - __free_page(fw_priv->pages[i]); - kfree(fw_priv->pages); - kfree(fw_priv->fw_id); - kfree(fw_priv); - kfree(dev); - - module_put(THIS_MODULE); -} - static void firmware_class_timeout(u_long data) { @@ -689,26 +692,12 @@ request_firmware_nowait( return 0; } -static int __init -firmware_class_init(void) +static int __init firmware_class_init(void) { - int error; - error = class_register(&firmware_class); - if (error) { - printk(KERN_ERR "%s: class_register failed\n", __func__); - return error; - } - error = class_create_file(&firmware_class, &class_attr_timeout); - if (error) { - printk(KERN_ERR "%s: class_create_file failed\n", - __func__); - class_unregister(&firmware_class); - } - return error; - + return class_register(&firmware_class); } -static void __exit -firmware_class_exit(void) + +static void __exit firmware_class_exit(void) { class_unregister(&firmware_class); } -- cgit v1.2.3 From bcb9bd18e397eabe14b45f4dc4283359ab148d79 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Sat, 13 Mar 2010 23:49:18 -0800 Subject: firmware loader: split out builtin firmware handling Split builtin firmware handling into separate functions to clean up the main body of code. Signed-off-by: Dmitry Torokhov Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 76 ++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 26 deletions(-) (limited to 'drivers/base/firmware_class.c') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index d351e773f439..c378a355bed4 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -27,6 +27,52 @@ MODULE_AUTHOR("Manuel Estrada Sainz"); MODULE_DESCRIPTION("Multi purpose firmware loading support"); MODULE_LICENSE("GPL"); +/* Builtin firmware support */ + +#ifdef CONFIG_FW_LOADER + +extern struct builtin_fw __start_builtin_fw[]; +extern struct builtin_fw __end_builtin_fw[]; + +static bool fw_get_builtin_firmware(struct firmware *fw, const char *name) +{ + struct builtin_fw *b_fw; + + for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) { + if (strcmp(name, b_fw->name) == 0) { + fw->size = b_fw->size; + fw->data = b_fw->data; + return true; + } + } + + return false; +} + +static bool fw_is_builtin_firmware(const struct firmware *fw) +{ + struct builtin_fw *b_fw; + + for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) + if (fw->data == b_fw->data) + return true; + + return false; +} + +#else /* Module case - no builtin firmware support */ + +static inline bool fw_get_builtin_firmware(struct firmware *fw, const char *name) +{ + return false; +} + +static inline bool fw_is_builtin_firmware(const struct firmware *fw) +{ + return false; +} +#endif + enum { FW_STATUS_LOADING, FW_STATUS_DONE, @@ -53,14 +99,6 @@ struct firmware_priv { bool nowait; }; -#ifdef CONFIG_FW_LOADER -extern struct builtin_fw __start_builtin_fw[]; -extern struct builtin_fw __end_builtin_fw[]; -#else /* Module case. Avoid ifdefs later; it'll all optimise out */ -static struct builtin_fw *__start_builtin_fw; -static struct builtin_fw *__end_builtin_fw; -#endif - static void fw_load_abort(struct firmware_priv *fw_priv) { @@ -497,7 +535,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name, struct device *f_dev; struct firmware_priv *fw_priv; struct firmware *firmware; - struct builtin_fw *builtin; int retval; if (!firmware_p) @@ -511,13 +548,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name, goto out; } - for (builtin = __start_builtin_fw; builtin != __end_builtin_fw; - builtin++) { - if (strcmp(name, builtin->name)) - continue; + if (fw_get_builtin_firmware(firmware, name)) { dev_dbg(device, "firmware: using built-in firmware %s\n", name); - firmware->size = builtin->size; - firmware->data = builtin->data; return 0; } @@ -589,19 +621,11 @@ request_firmware(const struct firmware **firmware_p, const char *name, * release_firmware: - release the resource associated with a firmware image * @fw: firmware resource to release **/ -void -release_firmware(const struct firmware *fw) +void release_firmware(const struct firmware *fw) { - struct builtin_fw *builtin; - if (fw) { - for (builtin = __start_builtin_fw; builtin != __end_builtin_fw; - builtin++) { - if (fw->data == builtin->data) - goto free_fw; - } - firmware_free_data(fw); - free_fw: + if (!fw_is_builtin_firmware(fw)) + firmware_free_data(fw); kfree(fw); } } -- cgit v1.2.3 From e177123f0c2d77ed2fde724ca352166ca9464619 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Sat, 13 Mar 2010 23:49:23 -0800 Subject: firmware loader: do not allocate firmare id separately fw_id has the same life time as firmware_priv so it makes sense to move it into firmware_priv structure instead of allocating separately. Signed-off-by: Dmitry Torokhov Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) (limited to 'drivers/base/firmware_class.c') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index c378a355bed4..d98e424675cf 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -86,7 +86,6 @@ static int loading_timeout = 60; /* In seconds */ static DEFINE_MUTEX(fw_lock); struct firmware_priv { - char *fw_id; struct completion completion; struct bin_attribute attr_data; struct firmware *fw; @@ -94,9 +93,9 @@ struct firmware_priv { struct page **pages; int nr_pages; int page_array_size; - const char *vdata; struct timer_list timeout; bool nowait; + char fw_id[]; }; static void @@ -153,7 +152,6 @@ static void fw_dev_release(struct device *dev) for (i = 0; i < fw_priv->nr_pages; i++) __free_page(fw_priv->pages[i]); kfree(fw_priv->pages); - kfree(fw_priv->fw_id); kfree(fw_priv); kfree(dev); @@ -437,8 +435,8 @@ static int fw_register_device(struct device **dev_p, const char *fw_name, struct device *device) { int retval; - struct firmware_priv *fw_priv = kzalloc(sizeof(*fw_priv), - GFP_KERNEL); + struct firmware_priv *fw_priv = + kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL); struct device *f_dev = kzalloc(sizeof(*f_dev), GFP_KERNEL); *dev_p = NULL; @@ -449,16 +447,9 @@ static int fw_register_device(struct device **dev_p, const char *fw_name, goto error_kfree; } + strcpy(fw_priv->fw_id, fw_name); init_completion(&fw_priv->completion); fw_priv->attr_data = firmware_attr_data_tmpl; - fw_priv->fw_id = kstrdup(fw_name, GFP_KERNEL); - if (!fw_priv->fw_id) { - dev_err(device, "%s: Firmware name allocation failed\n", - __func__); - retval = -ENOMEM; - goto error_kfree; - } - fw_priv->timeout.function = firmware_class_timeout; fw_priv->timeout.data = (u_long) fw_priv; init_timer(&fw_priv->timeout); -- cgit v1.2.3 From 2c3c8bea608866d8bd9dcf92657d57fdcac011c5 Mon Sep 17 00:00:00 2001 From: Chris Wright Date: Wed, 12 May 2010 18:28:57 -0700 Subject: sysfs: add struct file* to bin_attr callbacks This allows bin_attr->read,write,mmap callbacks to check file specific data (such as inode owner) as part of any privilege validation. Signed-off-by: Chris Wright Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'drivers/base/firmware_class.c') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index d98e424675cf..3f093b0dd217 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -278,8 +278,9 @@ static ssize_t firmware_loading_store(struct device *dev, static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store); static ssize_t -firmware_data_read(struct kobject *kobj, struct bin_attribute *bin_attr, - char *buffer, loff_t offset, size_t count) +firmware_data_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buffer, loff_t offset, + size_t count) { struct device *dev = to_dev(kobj); struct firmware_priv *fw_priv = dev_get_drvdata(dev); @@ -362,6 +363,7 @@ fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size) /** * firmware_data_write - write method for firmware + * @filp: open sysfs file * @kobj: kobject for the device * @bin_attr: bin_attr structure * @buffer: buffer being written @@ -372,8 +374,9 @@ fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size) * the driver as a firmware image. **/ static ssize_t -firmware_data_write(struct kobject *kobj, struct bin_attribute *bin_attr, - char *buffer, loff_t offset, size_t count) +firmware_data_write(struct file* filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buffer, + loff_t offset, size_t count) { struct device *dev = to_dev(kobj); struct firmware_priv *fw_priv = dev_get_drvdata(dev); -- cgit v1.2.3