From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751373AbcIGXdS (ORCPT ); Wed, 7 Sep 2016 19:33:18 -0400 Received: from mx2.suse.de ([195.135.220.15]:48149 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750991AbcIGXdJ (ORCPT ); Wed, 7 Sep 2016 19:33:09 -0400 Date: Thu, 8 Sep 2016 01:33:06 +0200 From: "Luis R. Rodriguez" To: Daniel Wagner Cc: linux-kernel@vger.kernel.org, Daniel Wagner , Ming Lei , "Luis R . Rodriguez" , Greg Kroah-Hartman , "Srivatsa S. Bhat" , "Rafael J. Wysocki" Subject: Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper() Message-ID: <20160907233306.GZ3296@wotan.suse.de> References: <1473237908-20989-1-git-send-email-wagi@monom.org> <1473237908-20989-2-git-send-email-wagi@monom.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473237908-20989-2-git-send-email-wagi@monom.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 07, 2016 at 10:45:05AM +0200, Daniel Wagner wrote: > From: Daniel Wagner > > When we load the firmware directly we don't need to take the umh > lock. 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_UER_HELPER. Great work! Just one issue found, noted below. > The usermodehelper locking code was added by b298d289c792 ("PM / Sleep: > Fix freezer failures due to racy usermodehelper_is_disabled()"). Thanks, this helps to give some perspective, I'll note that commit also refers to commit a144c6a (PM: Print a warning if firmware is requested when tasks are frozen) by Srivatsa a long time ago which added a warning print if a driver requested firmware when tasks are frozen. That commit log further clarifies that the issues is that some drivers erroneously use request_firmware() in their driver's ->resume() (or ->thaw(), or ->restore()) callbacks, it further clarifies that is not going to work unless the firmware has been built in. It did not explain *why* it wouldn't work though. But note it also mentioned how drivers that do have request_firmware() calls on resume stall resume -- the reason for the stalling is the stupid usermode helper. The kernel now "fixed" these by returning an error in such cases, it does this by checking kernel user mode helper is disabled, this is why it would not work. But note that we should be disabling the usermode helper on suspend too, and likely the reason we never ran into an issue with the cache stuff is we would fail if the usermode helper was disabled anyway. This is a long winded way of saying that these commits further confirm removal of using the usermode helper from the firmware cache work for suspend/resume. > 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 960f8f7..d4fee06 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_load_from_user_helper() no longer needs the timeout parameter then. Given this fact I'll chime in with some other, IMHO cosmetic things for this series. This however is the just the biggest issue for this series that I've found. That and testing this at run time didn't boot on my system, it could be an issue with linux-next next-20160907 booting on my system, I hadn't tried that yet. I did put your series through 0-day though and it went through fine though. Since you will need a respin I'd appreciate if you can Cc Takashi, Bjorn, Daniel Vetter, and Arend van Spriel on these series as some of them have expressed interest in the umh stuff, so best to get wider review as well. While at it please Cc Rafael and Srivatsa. Luis