Messages in this thread | | | Date | Thu, 31 Jan 2013 10:55:54 +0100 | From | Takashi Iwai <> | Subject | Re: [PATCH 1/4] firmware: Refactoring for splitting user-mode helper code |
| |
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
| |