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

[Ming Lei]

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;
+
 	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/