On Tue, 08 Sep 2015 10:25:31 +0200,
Kim, Milo wrote:
>
> > The fallback to user helper mode is bad for the recent udev, since
> > udev already dropped the f/w support code completely. Thus every
> > non-existing f/w load will result in 60 seconds stall.
>
> However, timeout is changed to MAX_JIFFY_OFFSET when FW_OPT_UEVENT flag
> is not set.
>
> static int _request_firmware_load(struct firmware_priv *fw_priv,
> unsigned int opt_flags, long timeout)
> {
> (snip)
>
> if (opt_flags & FW_OPT_UEVENT) {
> buf->need_uevent = true;
> dev_set_uevent_suppress(f_dev, false);
> dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
> kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
> } else {
> timeout = MAX_JIFFY_OFFSET;
> }
>
> retval = wait_for_completion_interruptible_timeout(&buf->completion,
> timeout);
> }
>
> It will take too long to get the result.
Why it takes too long? It's the timeout, so it happens only when the
input isn't completed.
> I don't know the reason why
> timeout was modified in the commit [68ff2a00dbf5: firmware_loader:
> handle timeout via wait_for_completion_interruptible_timeout()].
My guess about the rationale behind the change is that, if it's no
udev event, the (more-or-less) manual interaction is expected. If
it's done by human, we can't expect that it's typed always so quickly
in time.
> Moreover, this time value is not identical to the result of
> timeout_show().
That's bad, indeed.
> Is it OK to remove the line as follows?
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 171841a..8187404 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -909,8 +909,6 @@ static int _request_firmware_load(struct
> firmware_priv *fw_priv,
> dev_set_uevent_suppress(f_dev, false);
> dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
> kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
> - } else {
> - timeout = MAX_JIFFY_OFFSET;
> }
>
> retval = wait_for_completion_interruptible_timeout(&buf->completion,
>
> If the driver requires longer loading time, then it can be done by
> updating '/sys/class/firmware/timeout'.
I guess this would be harmless for most cases. But it's better to
have a clarification why the shorter timeout is mandatory...
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
