Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code
From: Takashi Iwai
Date: Wed Jan 30 2013 - 06:09:41 EST
At Wed, 30 Jan 2013 11:53:14 +0100,
Takashi Iwai wrote:
>
> At Wed, 30 Jan 2013 18:50:05 +0800,
> Ming Lei wrote:
> >
> > On Wed, Jan 30, 2013 at 6:31 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > >
> > > But it's supposed to be cached, no?
> >
> > Generally it will be cached, but some crazy devices might come as new
> > device during resume, so we still need to handle the situation.
>
> In that case, shouldn't we simply return an error instead of
> usermodehelper lock (at least for direct loading)?
The patch below is what I have in my mind...
Takashi
---
From: Takashi Iwai <tiwai@xxxxxxx>
Subject: [PATCH] firmware: Skip usermodehelper_lock for direct loading
We don't need a user mode helper lock for the direct fw loading.
This also reduces the use of timeout when user mode helper is
disabled, thus we can move the code into ifdef block, too.
For avoiding the possible call of request_firmware() without caching,
add a check of suspend state for the direct loading case, and returns
immediately if it's called during suspend/resume. Then it proceeds to
the user mode helper if enabled, or returns the error.
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
drivers/base/firmware_class.c | 97 ++++++++++++++++++++++++-------------------
1 file changed, 54 insertions(+), 43 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index f1caad7..c973bb0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -88,13 +88,6 @@ enum {
FW_STATUS_ABORT,
};
-static int loading_timeout = 60; /* In seconds */
-
-static inline long firmware_loading_timeout(void)
-{
- return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
-}
-
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
spinlock_t lock;
@@ -315,6 +308,9 @@ static bool fw_get_filesystem_firmware(struct device *device,
bool success = false;
char *path = __getname();
+ if (device->power.is_suspended)
+ return false;
+
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
struct file *file;
@@ -457,6 +453,20 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
#define is_fw_load_aborted(buf) \
test_bit(FW_STATUS_ABORT, &(buf)->status)
+static int loading_timeout = 60; /* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+ return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
+}
+
+static inline long set_loading_timeout(long val)
+{
+ long old_timeout = loading_timeout;
+ loading_timeout = val;
+ return old_timeout;
+}
+
static ssize_t firmware_timeout_show(struct class *class,
struct class_attribute *attr,
char *buf)
@@ -875,22 +885,44 @@ err_put_dev:
static int fw_load_from_user_helper(struct firmware *firmware,
const char *name, struct device *device,
- bool uevent, bool nowait, long timeout)
+ bool uevent, bool nowait)
{
struct firmware_priv *fw_priv;
+ long timeout;
+ int ret;
+
+ timeout = firmware_loading_timeout();
+ if (nowait) {
+ timeout = usermodehelper_read_lock_wait(timeout);
+ if (!timeout) {
+ dev_dbg(device, "firmware: %s loading timed out\n",
+ name);
+ return -EBUSY;
+ }
+ } else {
+ ret = usermodehelper_read_trylock();
+ if (WARN_ON(ret)) {
+ dev_err(device, "firmware: %s will not be loaded\n",
+ name);
+ return ret;
+ }
+ }
fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
if (IS_ERR(fw_priv))
- return PTR_ERR(fw_priv);
+ ret = PTR_ERR(fw_priv);
+ else {
+ fw_priv->buf = firmware->priv;
+ ret = _request_firmware_load(fw_priv, uevent, timeout);
+ }
- fw_priv->buf = firmware->priv;
- return _request_firmware_load(fw_priv, uevent, timeout);
+ usermodehelper_read_unlock();
+ return ret;
}
#else /* CONFIG_FW_LOADER_USER_HELPER */
static inline int
fw_load_from_user_helper(struct firmware *firmware, const char *name,
- struct device *device, bool uevent, bool nowait,
- long timeout)
+ struct device *device, bool uevent, bool nowait)
{
return -ENOENT;
}
@@ -898,6 +930,8 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
/* No abort during direct loading */
#define is_fw_load_aborted(buf) false
+static inline long set_loading_timeout(long val) { return 0; }
+
#endif /* CONFIG_FW_LOADER_USER_HELPER */
@@ -1006,7 +1040,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
struct device *device, bool uevent, bool nowait)
{
struct firmware *fw;
- long timeout;
int ret;
if (!firmware_p)
@@ -1017,32 +1050,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
goto out;
ret = 0;
- timeout = firmware_loading_timeout();
- if (nowait) {
- timeout = usermodehelper_read_lock_wait(timeout);
- if (!timeout) {
- dev_dbg(device, "firmware: %s loading timed out\n",
- name);
- ret = -EBUSY;
- }
- } else {
- ret = usermodehelper_read_trylock();
- if (WARN_ON(ret)) {
- dev_err(device, "firmware: %s will not be loaded\n",
- name);
- }
- }
-
- if (!ret) {
- if (!fw_get_filesystem_firmware(device, fw->priv))
- ret = fw_load_from_user_helper(fw, name, device,
- uevent, nowait,
- timeout);
- if (!ret)
- ret = assign_firmware_buf(fw, device);
- }
-
- usermodehelper_read_unlock();
+ if (!fw_get_filesystem_firmware(device, fw->priv))
+ ret = fw_load_from_user_helper(fw, name, device,
+ uevent, nowait);
+ if (!ret)
+ ret = assign_firmware_buf(fw, device);
out:
if (ret < 0) {
@@ -1406,8 +1418,7 @@ static void device_cache_fw_images(void)
* firmware in current distributions is about 2M bytes,
* so 10 secs should be enough.
*/
- old_timeout = loading_timeout;
- loading_timeout = 10;
+ old_timeout = set_loading_timeout(10);
mutex_lock(&fw_lock);
fwc->state = FW_LOADER_START_CACHE;
@@ -1417,7 +1428,7 @@ static void device_cache_fw_images(void)
/* wait for completion of caching firmware for all devices */
async_synchronize_full_domain(&fw_cache_domain);
- loading_timeout = old_timeout;
+ set_loading_timeout(old_timeout);
}
/**
--
1.8.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/