Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code

[Takashi Iwai <tiwai@suse.de>]

At Thu, 31 Jan 2013 17:48:34 +0800,
Ming Lei wrote:
> 
(snip)
> > +/* called from request_firmware() and request_firmware_work_func() */
> > +static int
> > +_request_firmware(const struct firmware **firmware_p, const char *name,
> > +                 struct device *device, bool uevent, bool nowait)
> > +{
> > +       struct firmware *fw;
> > +       long timeout;
> > +       int ret;
> > +
> > +       if (!firmware_p)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(&fw, name, device);
> > +       if (ret <= 0) /* error or already assigned */
> > +               goto out;
> > +
> > +       ret = 0;
> > +       timeout = firmware_loading_timeout();
> 
> The above line may be moved below the line of 'if (nowait) '.

I thought of that, too, but later found that the timeout is used in
the call of fw_load_from_user_helper() below.

> > +       if (nowait) {
> > +               timeout = usermodehelper_read_lock_wait(timeout);
> > +               if (!timeout) {
> > +                       dev_dbg(device, "firmware: %s loading timed out\n",
> > +                               name);
> > +                       ret = -EBUSY;
> > +               }
> > +       } else {
> > +               ret = usermodehelper_read_trylock();
> > +               if (WARN_ON(ret)) {
> > +                       dev_err(device, "firmware: %s will not be loaded\n",
> > +                               name);
> > +               }
> > +       }
> > +
> > +       if (!ret) {
> > +               if (!fw_get_filesystem_firmware(device, fw->priv))
> > +                       ret = fw_load_from_user_helper(fw, name, device,
> > +                                                      uevent, nowait,
> > +                                                      timeout);
> > +               if (!ret)
> > +                       ret = assign_firmware_buf(fw, device);
> > +       }
> > +
> > +       usermodehelper_read_unlock();
> 
> The above should be move inside the above 'if (!ret) {...}'

Yep, but it would make the code less understandable.
I'll make rather the error path of usermodehelper lock goto's.

> 
> Except for the above two comments, looks the patch v4 is fine.
> 
> Please CC greg-kh when you send v5, and you may add my ack
> for the patchset.
> 
>             Acked-by: Ming Lei <ming.lei@canonical.com>

OK, thanks!


Takashi