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

[Ming Lei]

On Wed, Sep 7, 2016 at 4:45 PM, Daniel Wagner <wagi@xxxxxxxxx> wrote:
> From: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
>
> When we load the firmware directly we don't need to take the umh
> lock.

I am wondering if it can be wrong.

Actually in case of firmware loading, the usermode helper lock doesn't
only mean the user helper is usable, and it also may serve to mark the
filesystem/block device is ready for firmware loading, and of couse direct
loading need fs/block to be ready too.

> 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.
>
> The usermodehelper locking code was added by b298d289c792 ("PM / Sleep:
> Fix freezer failures due to racy usermodehelper_is_disabled()").
>
> 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_priv = fw_create_instance(firmware, name, device, opt_flags);
> -       if (IS_ERR(fw_priv))
> -               return PTR_ERR(fw_priv);
> +       if (IS_ERR(fw_priv)) {
> +               ret = PTR_ERR(fw_priv);
> +               goto release_lock;
> +       }
>
>         fw_priv->buf = firmware->priv;
> -       return _request_firmware_load(fw_priv, opt_flags, timeout);
> +       ret = _request_firmware_load(fw_priv, opt_flags, timeout);
> +
> +release_lock:
> +       usermodehelper_read_unlock();
> +
> +       return ret;
>  }
>
>  #ifdef CONFIG_PM_SLEEP
> @@ -1150,25 +1175,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>         if (ret <= 0) /* error or already assigned */
>                 goto out;
>
> -       ret = 0;
> -       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);
> -                       ret = -EBUSY;
> -                       goto out;
> -               }
> -       } else {
> -               ret = usermodehelper_read_trylock();
> -               if (WARN_ON(ret)) {
> -                       dev_err(device, "firmware: %s will not be loaded\n",
> -                               name);
> -                       goto out;
> -               }
> -       }
> -
>         ret = fw_get_filesystem_firmware(device, fw->priv);
>         if (ret) {
>                 if (!(opt_flags & FW_OPT_NO_WARN))
> @@ -1185,8 +1191,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>         if (!ret)
>                 ret = assign_firmware_buf(fw, device, opt_flags);
>
> -       usermodehelper_read_unlock();
> -
>   out:
>         if (ret < 0) {
>                 release_firmware(fw);
> --
> 2.7.4