Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759246AbcJTJxm (ORCPT ); Thu, 20 Oct 2016 05:53:42 -0400 Received: from atlantic540.startdedicated.de ([188.138.9.77]:46955 "EHLO atlantic540.startdedicated.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754597AbcJTJw1 (ORCPT ); Thu, 20 Oct 2016 05:52:27 -0400 From: Daniel Wagner To: linux-kernel@vger.kernel.org Cc: Ming Lei , "Luis R . Rodriguez" , Greg Kroah-Hartman , "Srivatsa S . Bhat" , "Rafael J . Wysocki" , Daniel Vetter , Takashi Iwai , Bjorn Andersson , Arend van Spriel , Daniel Wagner Subject: [PATCH v6 1/6] firmware: move umh locking code into fw_load_from_user_helper() Date: Thu, 20 Oct 2016 11:52:07 +0200 Message-Id: <1476957132-27818-2-git-send-email-wagi@monom.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1476957132-27818-1-git-send-email-wagi@monom.org> References: <1476957132-27818-1-git-send-email-wagi@monom.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5268 Lines: 145 From: Daniel Wagner When we load the firmware directly we don't need to take the umh lock(1). So move this part inside fw_load_from_user_helper which is only available when CONFIG_FW_LOADER_USER_HELPER is set. This avoids a dependency on firmware_loading_timeout() even when !CONFIG_FW_LOADER_USER_HELPER. firmware_loading_timeout() will be moved into the CONFIG_FW_LOADER_USER_HELPER section later. The usermodehelper locking code was added by b298d289c792 ("PM / Sleep: Fix freezer failures due to racy usermodehelper_is_disabled()"). (1) Luis analized this: The original goal of the usermode helper lock came can be traced back in time via a144c6a ("PM: Print a warning if firmware is requested when task are frozen) when Rafael noticed drivers were calling to request firmware on *resume* calls! Why would that be an issue? It was because of the stupid delays incurred on resume when firmware *was not found* __due__ to the stupid user mode helper timeout delay and people believing this often meant users confusing resume *stalling* for resume was *broken! Later b298d289c7 ("PM / Sleep: Fix freezer failures due to racy usermodehelper_is_disabled()") addressed races. That code in turn was later massaged into shape to better accept direct FS loading via commit 4e0c92d015235 ("firmware: Refactoring for splitting user-mode helper code"). So for a while we've been nagging driver developers to not add request_firmware() calls on resume calls. In fact the drivers/base/firmware_class.c code *kills* firmware UMH calls when we go to suspend for the firmware cache, as such even on suspend its a stupid idea to use the UMH, not only because it can stall suspend but *also* because we kill these UMH calls. [...] So, as I see it the user mode helper lock should have *nothing* to do with the race between reading files from the kernel and having a path ready. If it was *used* for that, that was papering over the real issue. Its no different of a hack for instance as if a driver using request_firmware() tried to use async probe to avoid the same race. Yes it will help, but no, it does not mean it is deterministically safe. Reviewing commit 247bc03742545 ("PM / Sleep: Mitigate race between the freezer and request_firmware()") which originally extended umh state machine from just being enabled/disabled, with the concepts of UMH_ENABLED, UMH_FREEZING, UMH_DISABLED -- its goal was to prevent UMH uses during suspend. So -- the "UMH lock" on firmware was actually added to help avoid races between freezing and request_firmware(). We should not re-use UMH status notifiers when the firmware UMH is disabled for the same concepts -- if we needed such a concept then we should take this out from UMH code and generalize it. Signed-off-by: Daniel Wagner Cc: Ming Lei Cc: Luis R. Rodriguez Cc: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 52 +++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 960f8f7c7aa2..d4fee06b67f3 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -981,13 +981,38 @@ static int fw_load_from_user_helper(struct firmware *firmware, unsigned int opt_flags, long timeout) { struct firmware_priv *fw_priv; + 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 release_lock; + } fw_priv->buf = firmware->priv; - return _request_firmware_load(fw_priv, opt_flags, timeout); + ret = _request_firmware_load(fw_priv, opt_flags, timeout); + +release_lock: + usermodehelper_read_unlock(); + + return ret; } #ifdef CONFIG_PM_SLEEP @@ -1150,25 +1175,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (ret <= 0) /* error or already assigned */ 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)) @@ -1185,8 +1191,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!ret) ret = assign_firmware_buf(fw, device, opt_flags); - usermodehelper_read_unlock(); - out: if (ret < 0) { release_firmware(fw); -- 2.7.4