Messages in this thread | | | Subject | Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper() | From | Daniel Wagner <> | Date | Thu, 8 Sep 2016 14:41:33 +0200 |
| |
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 <daniel.wagner@bmw-carit.de> >> Cc: Ming Lei <ming.lei@canonical.com> >> Cc: Luis R. Rodriguez <mcgrof@kernel.org> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> 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
| |