From f2791d733a2f06997b573d1a3cfde21e6f529826 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Mon, 26 Mar 2012 22:46:52 +0200 Subject: PM / Runtime: don't forget to wake up waitqueue on failure This patch (as1535) fixes a bug in the runtime PM core. When a runtime suspend attempt completes, whether successfully or not, the device's power.wait_queue is supposed to be signalled. But this doesn't happen in the failure pathway of rpm_suspend() when another autosuspend attempt is rescheduled. As a result, a task can get stuck indefinitely on the wait queue (I have seen this happen in testing). The patch fixes the problem by moving the wake_up_all() call up near the start of the failure code. Signed-off-by: Alan Stern CC: Signed-off-by: Rafael J. Wysocki --- drivers/base/power/runtime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/base') diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 541f821..bd0f394 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -532,6 +532,8 @@ static int rpm_suspend(struct device *dev, int rpmflags) dev->power.suspend_time = ktime_set(0, 0); dev->power.max_time_suspended_ns = -1; dev->power.deferred_resume = false; + wake_up_all(&dev->power.wait_queue); + if (retval == -EAGAIN || retval == -EBUSY) { dev->power.runtime_error = 0; @@ -547,7 +549,6 @@ static int rpm_suspend(struct device *dev, int rpmflags) } else { pm_runtime_cancel_pending(dev); } - wake_up_all(&dev->power.wait_queue); goto out; } -- cgit v1.1 From fe2e39d8782d885755139304d8dba0b3e5bfa878 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Wed, 28 Mar 2012 23:29:45 +0200 Subject: firmware_class: Rework usermodehelper check Instead of two functions, read_lock_usermodehelper() and usermodehelper_is_disabled(), used in combination, introduce usermodehelper_read_trylock() that will only return with umhelper_sem held if usermodehelper_disabled is unset (and will return -EAGAIN otherwise) and make _request_firmware() use it. Rename read_unlock_usermodehelper() to usermodehelper_read_unlock() to follow the naming convention of the new function. Signed-off-by: Rafael J. Wysocki Acked-by: Greg Kroah-Hartman Cc: stable@vger.kernel.org --- drivers/base/firmware_class.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 6c9387d..deee871 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -533,12 +533,10 @@ static int _request_firmware(const struct firmware **firmware_p, return 0; } - read_lock_usermodehelper(); - - if (WARN_ON(usermodehelper_is_disabled())) { + retval = usermodehelper_read_trylock(); + if (WARN_ON(retval)) { dev_err(device, "firmware: %s will not be loaded\n", name); - retval = -EBUSY; - goto out; + goto out_nolock; } if (uevent) @@ -573,8 +571,9 @@ static int _request_firmware(const struct firmware **firmware_p, fw_destroy_instance(fw_priv); out: - read_unlock_usermodehelper(); + usermodehelper_read_unlock(); +out_nolock: if (retval) { release_firmware(firmware); *firmware_p = NULL; -- cgit v1.1 From 811fa4004485dec8977176bf605a5b0508ee206c Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Wed, 28 Mar 2012 23:29:55 +0200 Subject: firmware_class: Split _request_firmware() into three functions, v2 Split _request_firmware() into three functions, _request_firmware_prepare() doing preparatory work that need not be done under umhelper_sem, _request_firmware_cleanup() doing the post-error cleanup and _request_firmware() carrying out the remaining operations. This change is requisite for moving the acquisition of umhelper_sem from _request_firmware() to the callers, which is going to be done subsequently. Signed-off-by: Rafael J. Wysocki Acked-by: Greg Kroah-Hartman Reviewed-by: Stephen Boyd Cc: stable@vger.kernel.org --- drivers/base/firmware_class.c | 58 ++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 17 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index deee871..6029067 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -435,7 +435,7 @@ static void firmware_class_timeout(u_long data) } static struct firmware_priv * -fw_create_instance(struct firmware *firmware, const char *fw_name, +fw_create_instance(const struct firmware *firmware, const char *fw_name, struct device *device, bool uevent, bool nowait) { struct firmware_priv *fw_priv; @@ -449,7 +449,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, goto err_out; } - fw_priv->fw = firmware; + fw_priv->fw = (struct firmware *)firmware; fw_priv->nowait = nowait; strcpy(fw_priv->fw_id, fw_name); init_completion(&fw_priv->completion); @@ -510,13 +510,10 @@ static void fw_destroy_instance(struct firmware_priv *fw_priv) device_unregister(f_dev); } -static int _request_firmware(const struct firmware **firmware_p, - const char *name, struct device *device, - bool uevent, bool nowait) +static int _request_firmware_prepare(const struct firmware **firmware_p, + const char *name, struct device *device) { - struct firmware_priv *fw_priv; struct firmware *firmware; - int retval = 0; if (!firmware_p) return -EINVAL; @@ -533,10 +530,26 @@ static int _request_firmware(const struct firmware **firmware_p, return 0; } + return 1; +} + +static void _request_firmware_cleanup(const struct firmware **firmware_p) +{ + release_firmware(*firmware_p); + *firmware_p = NULL; +} + +static int _request_firmware(const struct firmware *firmware, + const char *name, struct device *device, + bool uevent, bool nowait) +{ + struct firmware_priv *fw_priv; + int retval; + retval = usermodehelper_read_trylock(); if (WARN_ON(retval)) { dev_err(device, "firmware: %s will not be loaded\n", name); - goto out_nolock; + return retval; } if (uevent) @@ -572,13 +585,6 @@ static int _request_firmware(const struct firmware **firmware_p, out: usermodehelper_read_unlock(); - -out_nolock: - if (retval) { - release_firmware(firmware); - *firmware_p = NULL; - } - return retval; } @@ -601,7 +607,17 @@ int request_firmware(const struct firmware **firmware_p, const char *name, struct device *device) { - return _request_firmware(firmware_p, name, device, true, false); + int ret; + + ret = _request_firmware_prepare(firmware_p, name, device); + if (ret <= 0) + return ret; + + ret = _request_firmware(*firmware_p, name, device, true, false); + if (ret) + _request_firmware_cleanup(firmware_p); + + return ret; } /** @@ -639,8 +655,16 @@ static int request_firmware_work_func(void *arg) return 0; } - ret = _request_firmware(&fw, fw_work->name, fw_work->device, + ret = _request_firmware_prepare(&fw, fw_work->name, fw_work->device); + if (ret <= 0) + goto out; + + ret = _request_firmware(fw, fw_work->name, fw_work->device, fw_work->uevent, true); + if (ret) + _request_firmware_cleanup(&fw); + + out: fw_work->cont(fw, fw_work->context); module_put(fw_work->module); -- cgit v1.1 From 9b78c1da60b3c62ccdd1509f0902ad19ceaf776b Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Wed, 28 Mar 2012 23:30:02 +0200 Subject: firmware_class: Do not warn that system is not ready from async loads If firmware is requested asynchronously, by calling request_firmware_nowait(), there is no reason to fail the request (and warn the user) when the system is (presumably temporarily) unready to handle it (because user space is not available yet or frozen). For this reason, introduce an alternative routine for read-locking umhelper_sem, usermodehelper_read_lock_wait(), that will wait for usermodehelper_disabled to be unset (possibly with a timeout) and make request_firmware_work_func() use it instead of usermodehelper_read_trylock(). Accordingly, modify request_firmware() so that it uses usermodehelper_read_trylock() to acquire umhelper_sem and remove the code related to that lock from _request_firmware(). Signed-off-by: Rafael J. Wysocki Acked-by: Greg Kroah-Hartman Cc: stable@vger.kernel.org --- drivers/base/firmware_class.c | 51 +++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 21 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 6029067..72c644b 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -81,6 +81,11 @@ enum { static int loading_timeout = 60; /* In seconds */ +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); @@ -541,31 +546,22 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p) static int _request_firmware(const struct firmware *firmware, const char *name, struct device *device, - bool uevent, bool nowait) + bool uevent, bool nowait, long timeout) { struct firmware_priv *fw_priv; - int retval; - - retval = usermodehelper_read_trylock(); - if (WARN_ON(retval)) { - dev_err(device, "firmware: %s will not be loaded\n", name); - return retval; - } + int retval = 0; if (uevent) dev_dbg(device, "firmware: requesting %s\n", name); fw_priv = fw_create_instance(firmware, name, device, uevent, nowait); - if (IS_ERR(fw_priv)) { - retval = PTR_ERR(fw_priv); - goto out; - } + if (IS_ERR(fw_priv)) + return PTR_ERR(fw_priv); if (uevent) { - if (loading_timeout > 0) + if (timeout != MAX_SCHEDULE_TIMEOUT) mod_timer(&fw_priv->timeout, - round_jiffies_up(jiffies + - loading_timeout * HZ)); + round_jiffies_up(jiffies + timeout)); kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD); } @@ -582,9 +578,6 @@ static int _request_firmware(const struct firmware *firmware, mutex_unlock(&fw_lock); fw_destroy_instance(fw_priv); - -out: - usermodehelper_read_unlock(); return retval; } @@ -613,7 +606,14 @@ request_firmware(const struct firmware **firmware_p, const char *name, if (ret <= 0) return ret; - ret = _request_firmware(*firmware_p, name, device, true, false); + ret = usermodehelper_read_trylock(); + if (WARN_ON(ret)) { + dev_err(device, "firmware: %s will not be loaded\n", name); + } else { + ret = _request_firmware(*firmware_p, name, device, true, false, + firmware_loading_timeout()); + usermodehelper_read_unlock(); + } if (ret) _request_firmware_cleanup(firmware_p); @@ -648,6 +648,7 @@ static int request_firmware_work_func(void *arg) { struct firmware_work *fw_work = arg; const struct firmware *fw; + long timeout; int ret; if (!arg) { @@ -659,8 +660,16 @@ static int request_firmware_work_func(void *arg) if (ret <= 0) goto out; - ret = _request_firmware(fw, fw_work->name, fw_work->device, - fw_work->uevent, true); + timeout = usermodehelper_read_lock_wait(firmware_loading_timeout()); + if (timeout) { + ret = _request_firmware(fw, fw_work->name, fw_work->device, + fw_work->uevent, true, timeout); + usermodehelper_read_unlock(); + } else { + dev_dbg(fw_work->device, "firmware: %s loading timed out\n", + fw_work->name); + ret = -EAGAIN; + } if (ret) _request_firmware_cleanup(&fw); -- cgit v1.1 From dddb5549da6b15ea8b9ce9ee0859c8d1fa268b5b Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 28 Mar 2012 23:30:43 +0200 Subject: firmware_class: Reorganize fw_create_instance() Recent patches to split up the three phases of request_firmware() lead to a casting away of const in fw_create_instance(). We can avoid this cast by splitting up fw_create_instance() a bit. Make _request_firmware_setup() return a struct fw_priv and use that struct instead of passing struct firmware to _request_firmware(). Move the uevent and device file creation bits to the loading phase and rename the function to _request_firmware_load() to better reflect its purpose. Signed-off-by: Stephen Boyd Acked-by: Greg Kroah-Hartman Signed-off-by: Rafael J. Wysocki --- drivers/base/firmware_class.c | 135 +++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 73 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 72c644b..ae00a2f 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -440,21 +440,19 @@ static void firmware_class_timeout(u_long data) } static struct firmware_priv * -fw_create_instance(const struct firmware *firmware, const char *fw_name, +fw_create_instance(struct firmware *firmware, const char *fw_name, struct device *device, bool uevent, bool nowait) { struct firmware_priv *fw_priv; struct device *f_dev; - int error; fw_priv = kzalloc(sizeof(*fw_priv) + strlen(fw_name) + 1 , GFP_KERNEL); if (!fw_priv) { dev_err(device, "%s: kmalloc failed\n", __func__); - error = -ENOMEM; - goto err_out; + return ERR_PTR(-ENOMEM); } - fw_priv->fw = (struct firmware *)firmware; + fw_priv->fw = firmware; fw_priv->nowait = nowait; strcpy(fw_priv->fw_id, fw_name); init_completion(&fw_priv->completion); @@ -468,74 +466,37 @@ fw_create_instance(const struct firmware *firmware, const char *fw_name, f_dev->parent = device; f_dev->class = &firmware_class; - dev_set_uevent_suppress(f_dev, true); - - /* Need to pin this module until class device is destroyed */ - __module_get(THIS_MODULE); - - error = device_add(f_dev); - if (error) { - dev_err(device, "%s: device_register failed\n", __func__); - goto err_put_dev; - } - - error = device_create_bin_file(f_dev, &firmware_attr_data); - if (error) { - dev_err(device, "%s: sysfs_create_bin_file failed\n", __func__); - goto err_del_dev; - } - - error = device_create_file(f_dev, &dev_attr_loading); - if (error) { - dev_err(device, "%s: device_create_file failed\n", __func__); - goto err_del_bin_attr; - } - - if (uevent) - dev_set_uevent_suppress(f_dev, false); - return fw_priv; - -err_del_bin_attr: - device_remove_bin_file(f_dev, &firmware_attr_data); -err_del_dev: - device_del(f_dev); -err_put_dev: - put_device(f_dev); -err_out: - return ERR_PTR(error); -} - -static void fw_destroy_instance(struct firmware_priv *fw_priv) -{ - struct device *f_dev = &fw_priv->dev; - - device_remove_file(f_dev, &dev_attr_loading); - device_remove_bin_file(f_dev, &firmware_attr_data); - device_unregister(f_dev); } -static int _request_firmware_prepare(const struct firmware **firmware_p, - const char *name, struct device *device) +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; if (!firmware_p) - return -EINVAL; + return ERR_PTR(-EINVAL); *firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL); if (!firmware) { dev_err(device, "%s: kmalloc(struct firmware) failed\n", __func__); - return -ENOMEM; + return ERR_PTR(-ENOMEM); } if (fw_get_builtin_firmware(firmware, name)) { dev_dbg(device, "firmware: using built-in firmware %s\n", name); - return 0; + return NULL; } - return 1; + fw_priv = fw_create_instance(firmware, name, device, uevent, nowait); + if (IS_ERR(fw_priv)) { + release_firmware(firmware); + *firmware_p = NULL; + } + return fw_priv; } static void _request_firmware_cleanup(const struct firmware **firmware_p) @@ -544,21 +505,38 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p) *firmware_p = NULL; } -static int _request_firmware(const struct firmware *firmware, - const char *name, struct device *device, - bool uevent, bool nowait, long timeout) +static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent, + long timeout) { - struct firmware_priv *fw_priv; int retval = 0; + struct device *f_dev = &fw_priv->dev; + + dev_set_uevent_suppress(f_dev, true); - if (uevent) - dev_dbg(device, "firmware: requesting %s\n", name); + /* Need to pin this module until class device is destroyed */ + __module_get(THIS_MODULE); - fw_priv = fw_create_instance(firmware, name, device, uevent, nowait); - if (IS_ERR(fw_priv)) - return PTR_ERR(fw_priv); + retval = device_add(f_dev); + if (retval) { + dev_err(f_dev, "%s: device_register failed\n", __func__); + goto err_put_dev; + } + + retval = device_create_bin_file(f_dev, &firmware_attr_data); + if (retval) { + dev_err(f_dev, "%s: sysfs_create_bin_file failed\n", __func__); + goto err_del_dev; + } + + retval = device_create_file(f_dev, &dev_attr_loading); + if (retval) { + dev_err(f_dev, "%s: device_create_file failed\n", __func__); + goto err_del_bin_attr; + } if (uevent) { + dev_set_uevent_suppress(f_dev, false); + dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_id); if (timeout != MAX_SCHEDULE_TIMEOUT) mod_timer(&fw_priv->timeout, round_jiffies_up(jiffies + timeout)); @@ -577,7 +555,13 @@ static int _request_firmware(const struct firmware *firmware, fw_priv->fw = NULL; mutex_unlock(&fw_lock); - fw_destroy_instance(fw_priv); + device_remove_file(f_dev, &dev_attr_loading); +err_del_bin_attr: + device_remove_bin_file(f_dev, &firmware_attr_data); +err_del_dev: + device_del(f_dev); +err_put_dev: + put_device(f_dev); return retval; } @@ -600,17 +584,19 @@ int request_firmware(const struct firmware **firmware_p, const char *name, struct device *device) { + struct firmware_priv *fw_priv; int ret; - ret = _request_firmware_prepare(firmware_p, name, device); - if (ret <= 0) - return ret; + fw_priv = _request_firmware_prepare(firmware_p, name, device, true, + false); + if (IS_ERR_OR_NULL(fw_priv)) + return PTR_RET(fw_priv); ret = usermodehelper_read_trylock(); if (WARN_ON(ret)) { dev_err(device, "firmware: %s will not be loaded\n", name); } else { - ret = _request_firmware(*firmware_p, name, device, true, false, + ret = _request_firmware_load(fw_priv, true, firmware_loading_timeout()); usermodehelper_read_unlock(); } @@ -648,6 +634,7 @@ static int request_firmware_work_func(void *arg) { struct firmware_work *fw_work = arg; const struct firmware *fw; + struct firmware_priv *fw_priv; long timeout; int ret; @@ -656,14 +643,16 @@ static int request_firmware_work_func(void *arg) return 0; } - ret = _request_firmware_prepare(&fw, fw_work->name, fw_work->device); - if (ret <= 0) + fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device, + fw_work->uevent, true); + if (IS_ERR_OR_NULL(fw_priv)) { + ret = PTR_RET(fw_priv); goto out; + } timeout = usermodehelper_read_lock_wait(firmware_loading_timeout()); if (timeout) { - ret = _request_firmware(fw, fw_work->name, fw_work->device, - fw_work->uevent, true, timeout); + ret = _request_firmware_load(fw_priv, fw_work->uevent, timeout); usermodehelper_read_unlock(); } else { dev_dbg(fw_work->device, "firmware: %s loading timed out\n", -- cgit v1.1 From a36cf844c543c6193445a7b1492d16e5a8cf376e Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 28 Mar 2012 23:31:00 +0200 Subject: firmware_class: Move request_firmware_nowait() to workqueues Oddly enough a work_struct was already part of the firmware_work structure but nobody was using it. Instead of creating a new kthread for each request_firmware_nowait() call just schedule the work on the system workqueue. This should avoid some overhead in forking new threads when they're not strictly necessary. Signed-off-by: Stephen Boyd Acked-by: Greg Kroah-Hartman Signed-off-by: Rafael J. Wysocki --- drivers/base/firmware_class.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) (limited to 'drivers/base') diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index ae00a2f..5401814 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -16,10 +16,11 @@ #include #include #include -#include +#include #include #include #include +#include #define to_dev(obj) container_of(obj, struct device, kobj) @@ -630,19 +631,15 @@ struct firmware_work { bool uevent; }; -static int request_firmware_work_func(void *arg) +static void request_firmware_work_func(struct work_struct *work) { - struct firmware_work *fw_work = arg; + struct firmware_work *fw_work; const struct firmware *fw; struct firmware_priv *fw_priv; long timeout; int ret; - if (!arg) { - WARN_ON(1); - return 0; - } - + fw_work = container_of(work, struct firmware_work, work); fw_priv = _request_firmware_prepare(&fw, fw_work->name, fw_work->device, fw_work->uevent, true); if (IS_ERR_OR_NULL(fw_priv)) { @@ -667,8 +664,6 @@ static int request_firmware_work_func(void *arg) module_put(fw_work->module); kfree(fw_work); - - return ret; } /** @@ -694,7 +689,6 @@ request_firmware_nowait( const char *name, struct device *device, gfp_t gfp, void *context, void (*cont)(const struct firmware *fw, void *context)) { - struct task_struct *task; struct firmware_work *fw_work; fw_work = kzalloc(sizeof (struct firmware_work), gfp); @@ -713,15 +707,8 @@ request_firmware_nowait( return -EFAULT; } - task = kthread_run(request_firmware_work_func, fw_work, - "firmware/%s", name); - if (IS_ERR(task)) { - fw_work->cont(NULL, fw_work->context); - module_put(fw_work->module); - kfree(fw_work); - return PTR_ERR(task); - } - + INIT_WORK(&fw_work->work, request_firmware_work_func); + schedule_work(&fw_work->work); return 0; } -- cgit v1.1