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

[Luis R. Rodriguez]

On Wed, Sep 07, 2016 at 10:45:05AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
> 
> 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 <daniel.wagner@xxxxxxxxxxxx>
> Cc: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> Cc: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  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