From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941074AbcIHMlk (ORCPT ); Thu, 8 Sep 2016 08:41:40 -0400 Received: from cit-hm8-mail01.bmw-carit.de ([212.118.206.84]:58490 "EHLO cit-hm8-gw01.bmw-carit.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932570AbcIHMli (ORCPT ); Thu, 8 Sep 2016 08:41:38 -0400 X-CTCH-RefID: str=0001.0A0C0208.57D15C7E.0002,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0 Subject: Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper() To: "Luis R. Rodriguez" , Daniel Wagner References: <1473237908-20989-1-git-send-email-wagi@monom.org> <1473237908-20989-2-git-send-email-wagi@monom.org> <20160907233306.GZ3296@wotan.suse.de> CC: , Ming Lei , Greg Kroah-Hartman , "Srivatsa S. Bhat" , "Rafael J. Wysocki" From: Daniel Wagner Message-ID: <509a727e-e784-8e6f-c112-760a0f8493b6@bmw-carit.de> Date: Thu, 8 Sep 2016 14:41:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160907233306.GZ3296@wotan.suse.de> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.10.50.50] X-ClientProxiedBy: CIT-HM8-EX01.bmw-carit.intra (10.40.100.13) To CIT-HM8-EX01.bmw-carit.intra (10.40.100.13) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/08/2016 01:33 AM, Luis R. Rodriguez wrote: >> 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. Okay, so let's finish this round of refactoring first. I prefer going in smaller steps and see if there are any regressions with those changes. >> 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. Updated the patch accordingly. > 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. So far I have it tested with kvm. I'll give it a spin on real hardware. Good point. > 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. Will do. cheers, daniel