From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755283Ab3A3LJf (ORCPT ); Wed, 30 Jan 2013 06:09:35 -0500 Received: from cantor2.suse.de ([195.135.220.15]:43212 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754677Ab3A3LI6 (ORCPT ); Wed, 30 Jan 2013 06:08:58 -0500 Date: Wed, 30 Jan 2013 12:08:57 +0100 Message-ID: From: Takashi Iwai To: Ming Lei Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code In-Reply-To: References: <1359470814-7993-1-git-send-email-tiwai@suse.de> <1359470814-7993-2-git-send-email-tiwai@suse.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 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 --- 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