Re: [PATCH v4 1/4] firmware: Move umh locking code into fw_load_from_user_helper()

[Daniel Wagner <daniel.wagner@bmw-carit.de>]

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