Re: Problems loading firmware using built-in drivers with kernels that use initramfs.

[Arend van Spriel <arend@broadcom.com>]

On 08/29/2015 12:38 PM, Ming Lei wrote:
> On Sat, 29 Aug 2015 10:50:22 +0200
> Arend van Spriel <arend@broadcom.com> wrote:
>
>> On 08/29/2015 09:11 AM, Takashi Iwai wrote:
>>> On Sat, 29 Aug 2015 06:09:01 +0200,
>>> Ming Lei wrote:
>>>>
>>>> On Sat, Aug 29, 2015 at 9:11 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>>>> On Thu, Aug 27, 2015 at 08:55:13AM +0800, Ming Lei wrote:
>>>>>> On Thu, Aug 27, 2015 at 2:07 AM, Linus Torvalds
>>>>>> <torvalds@linux-foundation.org> wrote:
>>>>>>> On Wed, Aug 26, 2015 at 1:06 AM, Liam Girdwood
>>>>>>> <liam.r.girdwood@linux.intel.com> wrote:
>>>>>>>>
>>>>>>>> I think the options are to either :-
>>>>>>>>
>>>>>>>> 1) Don not support audio DSP drivers using topology data as built-in
>>>>>>>> drivers. Audio is not really a critical system required for booting
>>>>>>>> anyway.
>>>>>>>
>>>>>>> Yes, forcing it to be a module and not letting people compile it in by
>>>>>>> mistake (and then not have it work) is an option.
>>>>>>>
>>>>>>> That said, there are situations where people don't want to use
>>>>>>> modules. I used to eschew them for security reasons, for example - now
>>>>>>> I instead just do a one-time temporary key. But others may have other
>>>>>>> reasons to try to avoid modules.
>>>>>>>
>>>>>>>> 2) Create a default PCM for every driver that has topology data on the
>>>>>>>> assumption that every sound card will at least 1 PCM. This PCM can then
>>>>>>>> be re-configured when the FW is loaded.
>>>>>>>
>>>>>>> That would seem to be the better option if it is reasonably implementable.
>>>>>>>
>>>>>>> Of course, some kind of timer-based retry (limited *somehow*) of the
>>>>>>> fw loading could work too, but smells really really hacky.
>>>>>>
>>>>>> Yeah, years ago, we discussed to use -EPROBE_DEFER for the situation,
>>>>>> which should be one kind of fix, but looks there were objections at that time.
>>>>>
>>>>> That would still be a hack. I'll note there is also asynchronous probe support
>>>>> now but to use that would also be a hack for this issue. We don't want to
>>>>
>>>> If we think firmware as one kind of resources like regulators, gpio and others,
>>>> PROBE_DEFER is one good match for firmware loading case, and
>>>> it has been used by lots of drivers, so why can't it be used for
>>>> firmware loading?
>>>>
>>>> One problem is that we need to convert drivers into returning -EPROBE_DEFER
>>>> in case of request failure, and that may involve some work, but which
>>>> should be mechanical.
>>>
>>> I find such a delaying mechanism not so bad, too.  It's very
>>> straightforward, at least, no big pain in the transition in the driver
>>> side.
>>
>> Not sure how this is going to work with request_firmware_nowait(). We
>> use that in our drivers to get rid of ~60 sec. delay in probe and
>> consequently boot time when built-in. So basically we return 0 on probe
>> lacking better knowledge. Guess we can always move back to
>> request_firmware calls when defer_probe support is available.
>
> How about the following untested draft patch?
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb46..f66912f 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -171,6 +171,12 @@ static void driver_deferred_probe_trigger(void)
>   	queue_work(deferred_wq, &deferred_probe_work);
>   }
>
> +void driver_trigger_fw_load()
> +{
> +	driver_deferred_probe_trigger();
> +}
> +EXPORT_SYMBOL_GPL(driver_trigger_fw_load);
> +
>   /**
>    * deferred_probe_initcall() - Enable probing of deferred devices
>    *
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8524450..f879a07 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1132,6 +1132,11 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>   	if (ret <= 0) /* error or already assigned */
>   		goto out;
>
> +	if (system_state == SYSTEM_BOOTING) {
> +		ret = -EPROBE_DEFER;
> +		goto out;
> +	}
> +
>   	ret = 0;
>   	timeout = firmware_loading_timeout();
>   	if (opt_flags & FW_OPT_NOWAIT) {
> @@ -1311,6 +1316,9 @@ request_firmware_nowait(
>   {
>   	struct firmware_work *fw_work;
>
> +	if (system_state == SYSTEM_BOOTING)
> +		return -EPROBE_DEFER;
> +

Does this mean a built-in driver can not get firmware from initramfs or 
built in the kernel early. Seems a bit too aggressive. The problem 
stated in this thread is when the firmware is not on initramfs but only 
on the rootfs.

Regards,
Arend

>   	fw_work = kzalloc(sizeof(struct firmware_work), gfp);
>   	if (!fw_work)
>   		return -ENOMEM;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 5d7bc63..1c189fe 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -289,6 +289,7 @@ extern struct device_driver *driver_find(const char *name,
>   					 struct bus_type *bus);
>   extern int driver_probe_done(void);
>   extern void wait_for_device_probe(void);
> +extern void driver_trigger_fw_load(void);
>
>
>   /* sysfs interface for exporting driver attributes */
> diff --git a/init/main.c b/init/main.c
> index 9e64d70..be8411b 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -943,6 +943,9 @@ static int __ref kernel_init(void *unused)
>
>   	flush_delayed_fput();
>
> +	/* trigger probe for request_firmware and its no_wait pair */
> +	driver_trigger_fw_load();
> +
>   	if (ramdisk_execute_command) {
>   		ret = run_init_process(ramdisk_execute_command);
>   		if (!ret)
>
>
>
> Thanks,
>