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

[Ming Lei]

On Sun, Aug 30, 2015 at 4:25 PM, Arend van Spriel <arend@xxxxxxxxxxxx> wrote:
> On 08/29/2015 12:38 PM, Ming Lei wrote:
>>
>> On Sat, 29 Aug 2015 10:50:22 +0200
>> Arend van Spriel <arend@xxxxxxxxxxxx> 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@xxxxxxxx>
>>>>> 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@xxxxxxxxxxxxxxxxxxxx> wrote:
>>>>>>>>
>>>>>>>> On Wed, Aug 26, 2015 at 1:06 AM, Liam Girdwood
>>>>>>>> <liam.r.girdwood@xxxxxxxxxxxxxxx> 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.

Yes, strictly speaking, user mode request can't be handled with defer probe
during booting because we don't know how the user helper handles the
request, that said even checking if the firmware exists in current path doesn't
make sense for user mode request.

So the patch should have used defer proble for direct load only
during booting.

>
> 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,
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/