lkml.org 
[lkml]   [2016]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()
From
Date
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

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.091 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site