Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755016AbdC3DZR (ORCPT ); Wed, 29 Mar 2017 23:25:17 -0400 Received: from mail.kernel.org ([198.145.29.136]:51516 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754471AbdC3DZO (ORCPT ); Wed, 29 Mar 2017 23:25:14 -0400 From: "Luis R. Rodriguez" To: gregkh@linuxfoundation.org Cc: wagi@monom.org, dwmw2@infradead.org, rafal@milecki.pl, arend.vanspriel@broadcom.com, rjw@rjwysocki.net, yi1.li@linux.intel.com, atull@opensource.altera.com, moritz.fischer@ettus.com, pmladek@suse.com, johannes.berg@intel.com, emmanuel.grumbach@intel.com, luciano.coelho@intel.com, kvalo@codeaurora.org, luto@kernel.org, takahiro.akashi@linaro.org, dhowells@redhat.com, pjones@redhat.com, linux-kernel@vger.kernel.org, "Luis R. Rodriguez" Subject: [PATCH 5/5] firmware: move umh try locks into the umh code Date: Wed, 29 Mar 2017 20:24:50 -0700 Message-Id: <20170330032450.17121-6-mcgrof@kernel.org> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20170330032450.17121-1-mcgrof@kernel.org> References: <20170330032450.17121-1-mcgrof@kernel.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5533 Lines: 160 This moves the usermode helper locks into only code paths that use the usermode helper API from the kernel. The usermode helper locks were originally added to prevent stalling suspend, later the firmware cache was added to help with this, and further later direct filesystem lookup was added by Linus to completely bypass udev due to the amount of issues the umh approach had. The usermode helper locks were kept even when the direct filesystem lookup mechanism is used though. A lot has changed since the original usermode helper locks were added but the recent commit which added the code for firmware_enabled() are intended to address any possible races cured only as collateral by using the locks as though side consequence of code evolution and this not being addressed any time sooner. With the firmware_enabled() code in place we are a bit more sure to move the usermode helper locks to UMH only code. There is a bit of history here so let's recap a bit of it to ensure nothing is lost and things are clear. The direct filesystem approach to loading firmware is rather new, it was added via commit abb139e75c2cdb ("firmware: teach the kernel to load firmware files directly from the filesystem") by Linus merged on the v3.7 release, to enable to bypass udev. usermodehelper_read_lock_wait() was added earlier via commit 9b78c1da60b3c ("firmware_class: Do not warn that system is not ready from async loads") merged on v3.4, after Rafael noted that the async firmware API call request_firmware_nowait() should not be penalized to fail if userspace is not available yet or frozen, it'd allow for a timeout grace period before giving up. The WARN_ON() was kept for the sync firmware API call though on request_firmware(). At this time there was no direct filesystem lookup for firmware though. The original usermode helper lock came from commit a144c6a6c924a ("PM: Print a warning if firmware is requested when tasks are frozen") merged on the v3.0 kernel by Rafael to print a warning back when firmware requests were used on resume(), thaw() or restore() callbacks and there was no direct fs lookups or the firmware cache. Signed-off-by: Luis R. Rodriguez --- drivers/base/firmware_class.c | 68 +++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index f28fbfab9c94..54fc4c42f126 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -1089,16 +1089,45 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, static int fw_load_from_user_helper(struct firmware *firmware, const char *name, struct device *device, - unsigned int opt_flags, long timeout) + unsigned int opt_flags) { struct firmware_priv *fw_priv; + long timeout; + int ret; + + timeout = firmware_loading_timeout(); + if (opt_flags & FW_OPT_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, opt_flags); - if (IS_ERR(fw_priv)) - return PTR_ERR(fw_priv); + if (IS_ERR(fw_priv)) { + ret = PTR_ERR(fw_priv); + goto out_unlock; + } fw_priv->buf = firmware->priv; - return _request_firmware_load(fw_priv, opt_flags, timeout); + ret = _request_firmware_load(fw_priv, opt_flags, timeout); + + if (!ret) + ret = assign_firmware_buf(firmware, device, opt_flags); + +out_unlock: + usermodehelper_read_unlock(); + + return ret; } static void kill_pending_fw_fallback_reqs(bool only_kill_custom) @@ -1119,8 +1148,7 @@ static void kill_pending_fw_fallback_reqs(bool only_kill_custom) #else /* CONFIG_FW_LOADER_USER_HELPER */ static inline int fw_load_from_user_helper(struct firmware *firmware, const char *name, - struct device *device, unsigned int opt_flags, - long timeout) + struct device *device, unsigned int opt_flags) { return -ENOENT; } @@ -1181,7 +1209,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name, unsigned int opt_flags) { struct firmware *fw = NULL; - long timeout; int ret; if (!firmware_p) @@ -1202,25 +1229,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name, goto out; } - ret = 0; - timeout = firmware_loading_timeout(); - if (opt_flags & FW_OPT_NOWAIT) { - timeout = usermodehelper_read_lock_wait(timeout); - if (!timeout) { - dev_dbg(device, "firmware: %s loading timed out\n", - name); - ret = -EBUSY; - goto out; - } - } else { - ret = usermodehelper_read_trylock(); - if (WARN_ON(ret)) { - dev_err(device, "firmware: %s will not be loaded\n", - name); - goto out; - } - } - ret = fw_get_filesystem_firmware(device, fw->priv); if (ret) { if (!(opt_flags & FW_OPT_NO_WARN)) @@ -1230,15 +1238,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (opt_flags & FW_OPT_USERHELPER) { dev_warn(device, "Falling back to user helper\n"); ret = fw_load_from_user_helper(fw, name, device, - opt_flags, timeout); + opt_flags); } - } - - if (!ret) + } else ret = assign_firmware_buf(fw, device, opt_flags); - usermodehelper_read_unlock(); - out: if (ret < 0) { release_firmware(fw); -- 2.11.0