Re: [PATCH] leds:lp55xx: Correct Kconfig dependency for f/w user helper

[Takashi Iwai <tiwai@suse.de>]

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