linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* A workaround for request_firmware() stuck in module_init
@ 2012-09-04 13:06 Takashi Iwai
  2012-09-04 15:52 ` Ming Lei
  2012-09-05 16:51 ` Lucas De Marchi
  0 siblings, 2 replies; 21+ messages in thread
From: Takashi Iwai @ 2012-09-04 13:06 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Ming Lei, Rafael J. Wysocki, Kay Sievers, linux-kernel

Hi,

as I've got recently a few bug reports regarding the stuck with
request_firmware() in module_init of some sound drivers, I started
looking at the issue.  Strangely, the problem doesn't happen on
openSUSE 12.2 although it has the same udev version with libkmod as
Fedora.  So I installed Fedora 17, and indeed I could see a problem
there.

Obviously a solution would be to rewrite the driver code to use
request_firmware_nowait() instead.  But it'd need a lot of code
shuffling, and most of such drivers are old stuff I don't want to do a
serious surgery.

So I tried an easier workaround by using the deferred probing.
An experimental patch is below.  As you can see, from the driver side,
it's simple: just add two lines at the head of each probe function.

Do you think this kind of hack is OK?  If not, any better (IOW easier)
solution?


thanks,

Takashi

===

Subject: [PATCH] driver-core: Add a helper to work around the stuck with request_firmware()

Since the recent udev loads the module with libkmod, the module
loading works no longer properly when a driver calls
request_firmware() in module_init.  Certainly we can fix all these
with request_firmware_nowait(), but it'd need fairly lots of code
changes, so it's no preferred for a lazy person like me.

This patch adds an easier workaround: use the deferred probing.

The driver that may call request_firmware() in module_init should call
the new helper function dev_defer_for_fw_load(), and returns
immediately with -EPROBE_DEFER if it's true.  If it's false, simply
continues the rest.  That's all.

In the driver core side, a new bit flag field is added to the device
private data for bookkeeping and triggering the deferred probe
explicitly.  (Otherwise the trigger won't happen unless any new driver
binding occurs.)

As an example implementation, the patch contains the fix for
sound/pci/rme9652/hdsp.c, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/base/base.h      |  6 ++++++
 drivers/base/dd.c        | 25 +++++++++++++++++++++++++
 include/linux/firmware.h |  6 ++++++
 sound/pci/rme9652/hdsp.c |  3 +++
 4 files changed, 40 insertions(+)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 6ee17bb..de5a7ca 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -67,6 +67,7 @@ struct driver_private {
  * list soon.
  * @device - pointer back to the struct class that this structure is
  * associated with.
+ * @flags - extra bit flags, currently used for deferred probing for fw loader
  *
  * Nothing outside of the driver core should ever touch these fields.
  */
@@ -78,6 +79,7 @@ struct device_private {
 	struct list_head deferred_probe;
 	void *driver_data;
 	struct device *device;
+	unsigned int flags;
 };
 #define to_device_private_parent(obj)	\
 	container_of(obj, struct device_private, knode_parent)
@@ -86,6 +88,10 @@ struct device_private {
 #define to_device_private_bus(obj)	\
 	container_of(obj, struct device_private, knode_bus)
 
+/* bit flags */
+#define DEV_FLAG_IN_DEFERRED_PROBE	(1 << 0)
+#define DEV_FLAG_NEED_TRIGGER		(1 << 1)
+
 extern int device_private_init(struct device *dev);
 
 /* initialisation functions */
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e3bbed8..aaefa7e 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -97,7 +97,9 @@ static void deferred_probe_work_func(struct work_struct *work)
 		device_pm_unlock();
 
 		dev_dbg(dev, "Retrying from deferred list\n");
+		dev->p->flags |= DEV_FLAG_IN_DEFERRED_PROBE;
 		bus_probe_device(dev);
+		dev->p->flags &= ~DEV_FLAG_IN_DEFERRED_PROBE;
 
 		mutex_lock(&deferred_probe_mutex);
 
@@ -301,6 +303,10 @@ probe_failed:
 		/* Driver requested deferred probing */
 		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
 		driver_deferred_probe_add(dev);
+		if (dev->p->flags & DEV_FLAG_NEED_TRIGGER) {
+			driver_deferred_probe_trigger();
+			dev->p->flags &= ~DEV_FLAG_NEED_TRIGGER;
+		}
 	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
@@ -587,3 +593,22 @@ int dev_set_drvdata(struct device *dev, void *data)
 	return 0;
 }
 EXPORT_SYMBOL(dev_set_drvdata);
+
+#ifdef CONFIG_FW_LOADER
+/**
+ * dev_defer_for_fw_load - check if deferred probe for fw loader is needed
+ * @dev: device
+ *
+ * When a driver may invoke request_firmware() in its module init, call this
+ * function at the beginning of the probe function.  When it's true, the probe
+ * should return immediately with -EPROBE_DEFER.
+ */
+bool dev_defer_for_fw_load(struct device *dev)
+{
+	if (dev->p->flags & DEV_FLAG_IN_DEFERRED_PROBE)
+		return false;
+	dev->p->flags |= DEV_FLAG_NEED_TRIGGER;
+	return true;
+}
+EXPORT_SYMBOL_GPL(dev_defer_for_fw_load);
+#endif
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 1e7c011..7caa96c 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -44,6 +44,7 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+bool dev_defer_for_fw_load(struct device *dev);
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -62,6 +63,11 @@ static inline int request_firmware_nowait(
 static inline void release_firmware(const struct firmware *fw)
 {
 }
+
+static inline bool dev_defer_for_fw_load(struct device *dev)
+{
+	return false;
+}
 #endif
 
 #endif
diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c
index 0d6930c..427ac24 100644
--- a/sound/pci/rme9652/hdsp.c
+++ b/sound/pci/rme9652/hdsp.c
@@ -5594,6 +5594,9 @@ static int __devinit snd_hdsp_probe(struct pci_dev *pci,
 	struct snd_card *card;
 	int err;
 
+	if (dev_defer_for_fw_load(&pci->dev))
+		return -EPROBE_DEFER;
+
 	if (dev >= SNDRV_CARDS)
 		return -ENODEV;
 	if (!enable[dev]) {
-- 
1.7.11.5


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-04 13:06 A workaround for request_firmware() stuck in module_init Takashi Iwai
@ 2012-09-04 15:52 ` Ming Lei
  2012-09-04 16:10   ` Takashi Iwai
  2012-09-05 16:51 ` Lucas De Marchi
  1 sibling, 1 reply; 21+ messages in thread
From: Ming Lei @ 2012-09-04 15:52 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel

On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> Hi,
>
> as I've got recently a few bug reports regarding the stuck with
> request_firmware() in module_init of some sound drivers, I started
> looking at the issue.  Strangely, the problem doesn't happen on
> openSUSE 12.2 although it has the same udev version with libkmod as
> Fedora.  So I installed Fedora 17, and indeed I could see a problem
> there.

It should be a bug in udev, as discussed in the below link:

          http://marc.info/?t=134552745100002&r=1&w=2

>
> Obviously a solution would be to rewrite the driver code to use
> request_firmware_nowait() instead.  But it'd need a lot of code
> shuffling, and most of such drivers are old stuff I don't want to do a
> serious surgery.
>
> So I tried an easier workaround by using the deferred probing.
> An experimental patch is below.  As you can see, from the driver side,
> it's simple: just add two lines at the head of each probe function.

In fact, the defer probe should only be applied in situations which
driver is built in kernel and request_firmware is called in .probe().

>
> Do you think this kind of hack is OK?  If not, any better (IOW easier)
> solution?

Looks your solution is a bit complicated, and I have wrote a similar
patch to address the problem, but Linus thought that it may hide the
problem of drivers.

        http://marc.info/?t=134278790800004&r=1&w=2

IMO, driver core needn't to be changed, and the defer probe can be
supported just by changes in request_firmware() and its caller.

Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-04 15:52 ` Ming Lei
@ 2012-09-04 16:10   ` Takashi Iwai
  2012-09-05  1:15     ` Ming Lei
  0 siblings, 1 reply; 21+ messages in thread
From: Takashi Iwai @ 2012-09-04 16:10 UTC (permalink / raw)
  To: Ming Lei; +Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel

At Tue, 4 Sep 2012 23:52:15 +0800,
Ming Lei wrote:
> 
> On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > Hi,
> >
> > as I've got recently a few bug reports regarding the stuck with
> > request_firmware() in module_init of some sound drivers, I started
> > looking at the issue.  Strangely, the problem doesn't happen on
> > openSUSE 12.2 although it has the same udev version with libkmod as
> > Fedora.  So I installed Fedora 17, and indeed I could see a problem
> > there.
> 
> It should be a bug in udev, as discussed in the below link:
> 
>           http://marc.info/?t=134552745100002&r=1&w=2

Yeah, if udev has a fix, I'm fine.  I'll proactively ignore these bug
reports.  But did it really happen...?

> > Obviously a solution would be to rewrite the driver code to use
> > request_firmware_nowait() instead.  But it'd need a lot of code
> > shuffling, and most of such drivers are old stuff I don't want to do a
> > serious surgery.
> >
> > So I tried an easier workaround by using the deferred probing.
> > An experimental patch is below.  As you can see, from the driver side,
> > it's simple: just add two lines at the head of each probe function.
> 
> In fact, the defer probe should only be applied in situations which
> driver is built in kernel and request_firmware is called in .probe().

Is it?  I thought the deferred probe is basically not for this problem
but rather for the dependency problem with other modules.  That's the
reason it's triggered only upon the successful binding.

And IMO, the deferred probe for the built-in kernel is also silly.
Did anyone really make it working for built-in kernel driver and
external firmware files?  (For the resume, it's of course a different
issue.  And I guess it's been solved by your fw cache patch, right?)

> > Do you think this kind of hack is OK?  If not, any better (IOW easier)
> > solution?
> 
> Looks your solution is a bit complicated, and I have wrote a similar
> patch to address the problem, but Linus thought that it may hide the
> problem of drivers.
> 
>         http://marc.info/?t=134278790800004&r=1&w=2
> 
> IMO, driver core needn't to be changed, and the defer probe can be
> supported just by changes in request_firmware() and its caller.

Ideally, yes.  But it won't work in some drivers like sound drivers,
that have its own device number counting changed at each probe call.
For such drivers, the deferred probe check must be done before
entering the normal probe procedure, and returning -EPROBE_DEFER would
be too late (or need more complex fallbacks).


thanks,

Takashi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-04 16:10   ` Takashi Iwai
@ 2012-09-05  1:15     ` Ming Lei
  2012-09-05  5:53       ` Takashi Iwai
  0 siblings, 1 reply; 21+ messages in thread
From: Ming Lei @ 2012-09-05  1:15 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel,
	Linus Torvalds

On Wed, Sep 5, 2012 at 12:10 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Tue, 4 Sep 2012 23:52:15 +0800,
> Ming Lei wrote:
>>
>> On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > Hi,
>> >
>> > as I've got recently a few bug reports regarding the stuck with
>> > request_firmware() in module_init of some sound drivers, I started
>> > looking at the issue.  Strangely, the problem doesn't happen on
>> > openSUSE 12.2 although it has the same udev version with libkmod as
>> > Fedora.  So I installed Fedora 17, and indeed I could see a problem
>> > there.
>>
>> It should be a bug in udev, as discussed in the below link:
>>
>>           http://marc.info/?t=134552745100002&r=1&w=2
>
> Yeah, if udev has a fix, I'm fine.  I'll proactively ignore these bug
> reports.  But did it really happen...?

Linus has expressed that it should be fixed by udev, maybe we can
wait some time to see if it will happen, :-)

There are more than 300 request_firmware called in probe(), even
adding 2 line code in these drivers will involve much workload, since
you need to find the matched probe()  for one request_firmware and
sometimes it is not easy.

>
>> > Obviously a solution would be to rewrite the driver code to use
>> > request_firmware_nowait() instead.  But it'd need a lot of code
>> > shuffling, and most of such drivers are old stuff I don't want to do a
>> > serious surgery.
>> >
>> > So I tried an easier workaround by using the deferred probing.
>> > An experimental patch is below.  As you can see, from the driver side,
>> > it's simple: just add two lines at the head of each probe function.
>>
>> In fact, the defer probe should only be applied in situations which
>> driver is built in kernel and request_firmware is called in .probe().
>
> Is it?  I thought the deferred probe is basically not for this problem
> but rather for the dependency problem with other modules.  That's the
> reason it's triggered only upon the successful binding.

Sorry, could you explain the dependency in a bit detail?

>From your patch, I understand you just convert the caller of
request_firmware from module_init into deferred_probe_work_func(),
so the udev problem is avoided, isn't it?

Looks making all .probe() run non-module_init context is still a solution, :-)

>
> And IMO, the deferred probe for the built-in kernel is also silly.
> Did anyone really make it working for built-in kernel driver and
> external firmware files?  (For the resume, it's of course a different

Yes, my original patch does work for the built-in situations.

> issue.  And I guess it's been solved by your fw cache patch, right?)
>
>> > Do you think this kind of hack is OK?  If not, any better (IOW easier)
>> > solution?
>>
>> Looks your solution is a bit complicated, and I have wrote a similar
>> patch to address the problem, but Linus thought that it may hide the
>> problem of drivers.
>>
>>         http://marc.info/?t=134278790800004&r=1&w=2
>>
>> IMO, driver core needn't to be changed, and the defer probe can be
>> supported just by changes in request_firmware() and its caller.
>
> Ideally, yes.  But it won't work in some drivers like sound drivers,
> that have its own device number counting changed at each probe call.
> For such drivers, the deferred probe check must be done before
> entering the normal probe procedure, and returning -EPROBE_DEFER would
> be too late (or need more complex fallbacks).

Simply, you can put the firmware loading at the start of probe to
avoid the specific
sound problem, :-)


Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05  1:15     ` Ming Lei
@ 2012-09-05  5:53       ` Takashi Iwai
  2012-09-05 11:32         ` Ming Lei
  0 siblings, 1 reply; 21+ messages in thread
From: Takashi Iwai @ 2012-09-05  5:53 UTC (permalink / raw)
  To: Ming Lei
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel,
	Linus Torvalds

At Wed, 5 Sep 2012 09:15:34 +0800,
Ming Lei wrote:
> 
> On Wed, Sep 5, 2012 at 12:10 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Tue, 4 Sep 2012 23:52:15 +0800,
> > Ming Lei wrote:
> >>
> >> On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > Hi,
> >> >
> >> > as I've got recently a few bug reports regarding the stuck with
> >> > request_firmware() in module_init of some sound drivers, I started
> >> > looking at the issue.  Strangely, the problem doesn't happen on
> >> > openSUSE 12.2 although it has the same udev version with libkmod as
> >> > Fedora.  So I installed Fedora 17, and indeed I could see a problem
> >> > there.
> >>
> >> It should be a bug in udev, as discussed in the below link:
> >>
> >>           http://marc.info/?t=134552745100002&r=1&w=2
> >
> > Yeah, if udev has a fix, I'm fine.  I'll proactively ignore these bug
> > reports.  But did it really happen...?
> 
> Linus has expressed that it should be fixed by udev, maybe we can
> wait some time to see if it will happen, :-)

Kay, what is the status?

> There are more than 300 request_firmware called in probe(), even
> adding 2 line code in these drivers will involve much workload, since
> you need to find the matched probe()  for one request_firmware and
> sometimes it is not easy.

Well, this depends on from which perspective you look at the issue.
See below.

> >> > Obviously a solution would be to rewrite the driver code to use
> >> > request_firmware_nowait() instead.  But it'd need a lot of code
> >> > shuffling, and most of such drivers are old stuff I don't want to do a
> >> > serious surgery.
> >> >
> >> > So I tried an easier workaround by using the deferred probing.
> >> > An experimental patch is below.  As you can see, from the driver side,
> >> > it's simple: just add two lines at the head of each probe function.
> >>
> >> In fact, the defer probe should only be applied in situations which
> >> driver is built in kernel and request_firmware is called in .probe().
> >
> > Is it?  I thought the deferred probe is basically not for this problem
> > but rather for the dependency problem with other modules.  That's the
> > reason it's triggered only upon the successful binding.
> 
> Sorry, could you explain the dependency in a bit detail?

When a device has some implicit dependency on another hardware
component (e.g. SDHCI depends on I2C GPIO controller, as in comment in
drivers/base/dd.c), the driver needs to wait until another one gets
ready.  -EPROBE_DEFER was introduced for such a purpose.

So, using this mechanism for the firmware issue is a kind of abuse.

> >From your patch, I understand you just convert the caller of
> request_firmware from module_init into deferred_probe_work_func(),
> so the udev problem is avoided, isn't it?

Yes, that was a hack.  The idea is just offloading the probe, and the
deferred probe is an existing simple way for that.

> Looks making all .probe() run non-module_init context is still a solution, :-)

Sounds interesting.

> > And IMO, the deferred probe for the built-in kernel is also silly.
> > Did anyone really make it working for built-in kernel driver and
> > external firmware files?  (For the resume, it's of course a different
> 
> Yes, my original patch does work for the built-in situations.
> 
> > issue.  And I guess it's been solved by your fw cache patch, right?)
> >
> >> > Do you think this kind of hack is OK?  If not, any better (IOW easier)
> >> > solution?
> >>
> >> Looks your solution is a bit complicated, and I have wrote a similar
> >> patch to address the problem, but Linus thought that it may hide the
> >> problem of drivers.
> >>
> >>         http://marc.info/?t=134278790800004&r=1&w=2
> >>
> >> IMO, driver core needn't to be changed, and the defer probe can be
> >> supported just by changes in request_firmware() and its caller.
> >
> > Ideally, yes.  But it won't work in some drivers like sound drivers,
> > that have its own device number counting changed at each probe call.
> > For such drivers, the deferred probe check must be done before
> > entering the normal probe procedure, and returning -EPROBE_DEFER would
> > be too late (or need more complex fallbacks).
> 
> Simply, you can put the firmware loading at the start of probe to
> avoid the specific
> sound problem, :-)

Well, I can't buy it.  Changing the call order can be often more
problematic.  Anyway...


IMO, the primary question is whether we still regard the
request_firmware() call in module_init as a driver bug.  Or, looking
at the usage numbers, we should accept it as a de facto standard use
case?

If we still see it as a bug, the only right way is to fix the drivers,
not the core.  That's why I suggested to put some fix to each driver.
In that way, it shows obvious that the driver is a kind of buggy but
fixed in a very lazy way (the function name should have been more
obvious one like i_may_call_request_firmware_so_call_me_later()).
The difference is that this fixes the bug, not hiding the problem.

OTOH, if we see it no longer as a bug, we should rather improve the
handling in the kernel core.  (Or, more radically, we can consider a
sort of async probing as default :) 
In anyway, fixing the core side is justified only if we agree that
request_firmware() in module_init is a valid use case.

So, before starting the work, we actually need a consensus.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05  5:53       ` Takashi Iwai
@ 2012-09-05 11:32         ` Ming Lei
  2012-09-05 13:03           ` Alan Cox
  0 siblings, 1 reply; 21+ messages in thread
From: Ming Lei @ 2012-09-05 11:32 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel,
	Linus Torvalds

On Wed, Sep 5, 2012 at 1:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 5 Sep 2012 09:15:34 +0800,
> Ming Lei wrote:
>>
>> On Wed, Sep 5, 2012 at 12:10 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Tue, 4 Sep 2012 23:52:15 +0800,
>> > Ming Lei wrote:
>> >>
>> >> On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> > Hi,
>> >> >
>> >> > as I've got recently a few bug reports regarding the stuck with
>> >> > request_firmware() in module_init of some sound drivers, I started
>> >> > looking at the issue.  Strangely, the problem doesn't happen on
>> >> > openSUSE 12.2 although it has the same udev version with libkmod as
>> >> > Fedora.  So I installed Fedora 17, and indeed I could see a problem
>> >> > there.
>> >>
>> >> It should be a bug in udev, as discussed in the below link:
>> >>
>> >>           http://marc.info/?t=134552745100002&r=1&w=2
>> >
>> > Yeah, if udev has a fix, I'm fine.  I'll proactively ignore these bug
>> > reports.  But did it really happen...?
>>
>> Linus has expressed that it should be fixed by udev, maybe we can
>> wait some time to see if it will happen, :-)
>
> Kay, what is the status?
>
>> There are more than 300 request_firmware called in probe(), even
>> adding 2 line code in these drivers will involve much workload, since
>> you need to find the matched probe()  for one request_firmware and
>> sometimes it is not easy.
>
> Well, this depends on from which perspective you look at the issue.
> See below.
>
>> >> > Obviously a solution would be to rewrite the driver code to use
>> >> > request_firmware_nowait() instead.  But it'd need a lot of code
>> >> > shuffling, and most of such drivers are old stuff I don't want to do a
>> >> > serious surgery.
>> >> >
>> >> > So I tried an easier workaround by using the deferred probing.
>> >> > An experimental patch is below.  As you can see, from the driver side,
>> >> > it's simple: just add two lines at the head of each probe function.
>> >>
>> >> In fact, the defer probe should only be applied in situations which
>> >> driver is built in kernel and request_firmware is called in .probe().
>> >
>> > Is it?  I thought the deferred probe is basically not for this problem
>> > but rather for the dependency problem with other modules.  That's the
>> > reason it's triggered only upon the successful binding.
>>
>> Sorry, could you explain the dependency in a bit detail?
>
> When a device has some implicit dependency on another hardware
> component (e.g. SDHCI depends on I2C GPIO controller, as in comment in
> drivers/base/dd.c), the driver needs to wait until another one gets
> ready.  -EPROBE_DEFER was introduced for such a purpose.
>
> So, using this mechanism for the firmware issue is a kind of abuse.
>
>> >From your patch, I understand you just convert the caller of
>> request_firmware from module_init into deferred_probe_work_func(),
>> so the udev problem is avoided, isn't it?
>
> Yes, that was a hack.  The idea is just offloading the probe, and the
> deferred probe is an existing simple way for that.
>
>> Looks making all .probe() run non-module_init context is still a solution, :-)
>
> Sounds interesting.
>
>> > And IMO, the deferred probe for the built-in kernel is also silly.
>> > Did anyone really make it working for built-in kernel driver and
>> > external firmware files?  (For the resume, it's of course a different
>>
>> Yes, my original patch does work for the built-in situations.
>>
>> > issue.  And I guess it's been solved by your fw cache patch, right?)
>> >
>> >> > Do you think this kind of hack is OK?  If not, any better (IOW easier)
>> >> > solution?
>> >>
>> >> Looks your solution is a bit complicated, and I have wrote a similar
>> >> patch to address the problem, but Linus thought that it may hide the
>> >> problem of drivers.
>> >>
>> >>         http://marc.info/?t=134278790800004&r=1&w=2
>> >>
>> >> IMO, driver core needn't to be changed, and the defer probe can be
>> >> supported just by changes in request_firmware() and its caller.
>> >
>> > Ideally, yes.  But it won't work in some drivers like sound drivers,
>> > that have its own device number counting changed at each probe call.
>> > For such drivers, the deferred probe check must be done before
>> > entering the normal probe procedure, and returning -EPROBE_DEFER would
>> > be too late (or need more complex fallbacks).
>>
>> Simply, you can put the firmware loading at the start of probe to
>> avoid the specific
>> sound problem, :-)
>
> Well, I can't buy it.  Changing the call order can be often more
> problematic.  Anyway...

More or less, these sound drivers may not support probe deferral well.
So the similar problem may be triggered if these device/drivers want
to apply probe deferral to solve some resource dependency problem.

>
>
> IMO, the primary question is whether we still regard the
> request_firmware() call in module_init as a driver bug.  Or, looking
> at the usage numbers, we should accept it as a de facto standard use
> case?

IMO, if the driver is built as module, request_firmware should be
allowed to be called in module_init context since user space is
already ready for handling firmware request. Also some devices
can't work without firmware downloading, and some device can
only be allowed to download firmware one time,  so it is reasonable
to call request_firmware() in .probe().

The current problem is that udev thinks it is not good and may timeout
the request, and looks no reasonable explanation about that.

If the driver is built in kernel, the request_firmware in .probe() may
prolong kernel init, and it might be a problem. But looks it is not a
big deal since most of drivers are built as module.

>
> If we still see it as a bug, the only right way is to fix the drivers,
> not the core.  That's why I suggested to put some fix to each driver.
> In that way, it shows obvious that the driver is a kind of buggy but
> fixed in a very lazy way (the function name should have been more
> obvious one like i_may_call_request_firmware_so_call_me_later()).
> The difference is that this fixes the bug, not hiding the problem.
>
> OTOH, if we see it no longer as a bug, we should rather improve the
> handling in the kernel core.  (Or, more radically, we can consider a
> sort of async probing as default :)

Agree, :-)

> In anyway, fixing the core side is justified only if we agree that
> request_firmware() in module_init is a valid use case.
>
> So, before starting the work, we actually need a consensus.

Yes.

Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 11:32         ` Ming Lei
@ 2012-09-05 13:03           ` Alan Cox
  2012-09-05 14:01             ` Takashi Iwai
                               ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Alan Cox @ 2012-09-05 13:03 UTC (permalink / raw)
  To: Ming Lei
  Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers,
	linux-kernel, Linus Torvalds

> If the driver is built in kernel, the request_firmware in .probe() may
> prolong kernel init, and it might be a problem. But looks it is not a
> big deal since most of drivers are built as module.

Doing it by deferring the load also fixes that. The built in ones will
defer their final probe until the firmware appears and all will be well.

If your rootfs needs firmware not in your initrd you already broke it and
there is a certain level beyond which you just have to give up trying to
save people from themselves.

It may actually make sense to push more of it into the core driver layer
and take some of the ability to make mistakes away from driver authors.
For the general case of "load firmware if we see one" there isn't really
any reason we can't have a firmware_name entry in the probe table
entries themselves. If that was present the core bus probe would kick a
firmware load off and only when the firmware had loaded would it call
->probe with dev->firmware pointing at a refcounted firmware struct.

At that point it should be much faster to fix existing drivers and much
harder for a random device driver to get it wrong. We can even add
helpers which manage dev->firmware, and free the relevant objects when
needed, plus doing automatic ref/deref on probe/remove so that for a
typical driver the author only has to do

	{PCI_blah , ... .firmware_name="wibble500.xcr", }

and all the loading, unloading, not loading twice happens by "magic" for
the driver author.

Add a dev_discard_firmware() for drivers that do this and know they can
then dump the file and all is good 8)

Alan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 13:03           ` Alan Cox
@ 2012-09-05 14:01             ` Takashi Iwai
  2012-09-05 15:22             ` Ming Lei
  2012-09-05 16:59             ` Lucas De Marchi
  2 siblings, 0 replies; 21+ messages in thread
From: Takashi Iwai @ 2012-09-05 14:01 UTC (permalink / raw)
  To: Alan Cox
  Cc: Ming Lei, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers,
	linux-kernel, Linus Torvalds

At Wed, 5 Sep 2012 14:03:04 +0100,
Alan Cox wrote:
> 
> > If the driver is built in kernel, the request_firmware in .probe() may
> > prolong kernel init, and it might be a problem. But looks it is not a
> > big deal since most of drivers are built as module.
> 
> Doing it by deferring the load also fixes that. The built in ones will
> defer their final probe until the firmware appears and all will be well.
> 
> If your rootfs needs firmware not in your initrd you already broke it and
> there is a certain level beyond which you just have to give up trying to
> save people from themselves.
> 
> It may actually make sense to push more of it into the core driver layer
> and take some of the ability to make mistakes away from driver authors.
> For the general case of "load firmware if we see one" there isn't really
> any reason we can't have a firmware_name entry in the probe table
> entries themselves. If that was present the core bus probe would kick a
> firmware load off and only when the firmware had loaded would it call
> ->probe with dev->firmware pointing at a refcounted firmware struct.
> 
> At that point it should be much faster to fix existing drivers and much
> harder for a random device driver to get it wrong. We can even add
> helpers which manage dev->firmware, and free the relevant objects when
> needed, plus doing automatic ref/deref on probe/remove so that for a
> typical driver the author only has to do
> 
> 	{PCI_blah , ... .firmware_name="wibble500.xcr", }
> 
> and all the loading, unloading, not loading twice happens by "magic" for
> the driver author.
> 
> Add a dev_discard_firmware() for drivers that do this and know they can
> then dump the file and all is good 8)

This sounds like an interesting idea.  I guess majority of drivers can
be covered by this scenario.  Of course, there are a few drivers that
need more complex handling (e.g. iwlwifi handles fallback fw loading),
but they can be implemented manually if needed anyway.


Takashi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 13:03           ` Alan Cox
  2012-09-05 14:01             ` Takashi Iwai
@ 2012-09-05 15:22             ` Ming Lei
  2012-09-05 16:30               ` Alan Cox
  2012-09-05 16:59             ` Lucas De Marchi
  2 siblings, 1 reply; 21+ messages in thread
From: Ming Lei @ 2012-09-05 15:22 UTC (permalink / raw)
  To: Alan Cox
  Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers,
	linux-kernel, Linus Torvalds, Benjamin Herrenschmidt

On Wed, Sep 5, 2012 at 9:03 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> If the driver is built in kernel, the request_firmware in .probe() may
>> prolong kernel init, and it might be a problem. But looks it is not a
>> big deal since most of drivers are built as module.
>
> Doing it by deferring the load also fixes that. The built in ones will
> defer their final probe until the firmware appears and all will be well.

Yes, deferring the load may fix the built in case, but which also
introduces much work on changes of current drivers. In fact,
there are few guys who complained the built in case.

The current complaint is from that udev may timeout request inside probe()
when drivers are built as module. As pointed by Linus and Benjamin,
it is better to fix it in udev, and looks not good to introduce great changes
(such as Takashi's defer probe patch) in kernel to workaround the problem.

>
> If your rootfs needs firmware not in your initrd you already broke it and
> there is a certain level beyond which you just have to give up trying to
> save people from themselves.
>
> It may actually make sense to push more of it into the core driver layer
> and take some of the ability to make mistakes away from driver authors.
> For the general case of "load firmware if we see one" there isn't really
> any reason we can't have a firmware_name entry in the probe table
> entries themselves. If that was present the core bus probe would kick a

Linus has said that he doesn't like to load firmware in probe(), but in some
situation the drivers have to load firmware in its probe():

        http://marc.info/?l=linux-kernel&m=134592122811866&w=2

In fact, it is better for drivers to load firmware just when user wants to use
the device, and some drivers have already changed to load firmware in
the open() callback.

So looks loading firmware always before probe in driver core is
against the above idea.

> firmware load off and only when the firmware had loaded would it call
> ->probe with dev->firmware pointing at a refcounted firmware struct.

IMO, introduce refcount for the firmware doesn't make sense. The lifetime
of firmware is completely different with lifetime of driver or device:

    - firmware needn't be kept in memory in the device/driver's lifetime, and
      should be loaded just in need, and be released after downloading
      it into device.

   - sometimes devices may disappear, but it is better to keep the
     firmware in memory, for example, device may be disconnected
     during resume but will come back later.

>
> At that point it should be much faster to fix existing drivers and much
> harder for a random device driver to get it wrong. We can even add
> helpers which manage dev->firmware, and free the relevant objects when
> needed, plus doing automatic ref/deref on probe/remove so that for a

As said above, ref/deref on probe/remove is not a good idea since
we needn't to keep the firmware in memory during the whole device/driver
lifetime.

Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 15:22             ` Ming Lei
@ 2012-09-05 16:30               ` Alan Cox
  2012-09-05 21:08                 ` Benjamin Herrenschmidt
                                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Alan Cox @ 2012-09-05 16:30 UTC (permalink / raw)
  To: Ming Lei
  Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers,
	linux-kernel, Linus Torvalds, Benjamin Herrenschmidt

> Yes, deferring the load may fix the built in case, but which also
> introduces much work on changes of current drivers. In fact,
> there are few guys who complained the built in case.

It fixes the modular case too.

> The current complaint is from that udev may timeout request inside probe()
> when drivers are built as module. As pointed by Linus and Benjamin,
> it is better to fix it in udev, and looks not good to introduce great changes
> (such as Takashi's defer probe patch) in kernel to workaround the problem.

It's not about a workaround but about doing it properly for the long term
and doing it in one place. It's also not a "great change", its a small
change.

> Linus has said that he doesn't like to load firmware in probe(), but in some
> situation the drivers have to load firmware in its probe():

You don't want to load firmware in probe because of the locking problems
- you can trigger a load of another device on the same bus - the defer
  dodges that nicely
 
> In fact, it is better for drivers to load firmware just when user wants to use
> the device, and some drivers have already changed to load firmware in
> the open() callback.

For those devices sure but they are if anything a minority as far as I
can see.

> So looks loading firmware always before probe in driver core is
> against the above idea.

I never said "always"

> > firmware load off and only when the firmware had loaded would it call
> > ->probe with dev->firmware pointing at a refcounted firmware struct.
> 
> IMO, introduce refcount for the firmware doesn't make sense. The lifetime
> of firmware is completely different with lifetime of driver or device:

Exactly. Which is why the moment you have multiple devices you need
refcounts. It's also why the propsoal included a 

dev_discard_firmware()

so you an instance can drop its firmware reference if it doesn't need it
post probe.

>     - firmware needn't be kept in memory in the device/driver's lifetime, and
>       should be loaded just in need, and be released after downloading
>       it into device.

You broke suspend/resume for lots of devices.

>    - sometimes devices may disappear, but it is better to keep the
>      firmware in memory, for example, device may be disconnected
>      during resume but will come back later.

So the moment you have multiple instances of a device with their own
lifetime and you have the need to pin it sometimes you need a refcount

> As said above, ref/deref on probe/remove is not a good idea since
> we needn't to keep the firmware in memory during the whole device/driver
> lifetime.

Often you do. And in the case you don't you still have to deal with
multiple probes doing asynchronous loads of the same firmware so you want
to do matching and refcounting. It's pretty much essential.

The other big value apart from making it harder for driver writers to
screw up is that it takes some of the control and puts it in one place.
That means you can change it later easily not in each driver.

This is enabling for device drivers. With no intention of offending
driver authors the reality is that we should have driver interfaces that

- work the right way by default
- allow driver authors to do things themselves if they need to instead
  (ie opt out)
- are hard to f**k up

because we want it to just work.

Alan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-04 13:06 A workaround for request_firmware() stuck in module_init Takashi Iwai
  2012-09-04 15:52 ` Ming Lei
@ 2012-09-05 16:51 ` Lucas De Marchi
  2012-09-05 17:08   ` Takashi Iwai
  1 sibling, 1 reply; 21+ messages in thread
From: Lucas De Marchi @ 2012-09-05 16:51 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Greg Kroah-Hartman, Ming Lei, Rafael J. Wysocki, Kay Sievers,
	linux-kernel

On Tue, Sep 4, 2012 at 10:06 AM, Takashi Iwai <tiwai@suse.de> wrote:
> Hi,
>
> as I've got recently a few bug reports regarding the stuck with
> request_firmware() in module_init of some sound drivers, I started
> looking at the issue.  Strangely, the problem doesn't happen on
> openSUSE 12.2 although it has the same udev version with libkmod as
> Fedora.  So I installed Fedora 17, and indeed I could see a problem
> there.

I fail to see how this is related to libkmod. It's indeed related to
the fact that udev now timeout a modprobe request so people stop doing
crazy things and like start daemons as modprobe's install rules. And
that's true even with old module-init-tools.



Lucas De Marchi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 13:03           ` Alan Cox
  2012-09-05 14:01             ` Takashi Iwai
  2012-09-05 15:22             ` Ming Lei
@ 2012-09-05 16:59             ` Lucas De Marchi
  2012-09-05 17:09               ` Takashi Iwai
  2 siblings, 1 reply; 21+ messages in thread
From: Lucas De Marchi @ 2012-09-05 16:59 UTC (permalink / raw)
  To: Alan Cox
  Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki,
	Kay Sievers, linux-kernel, Linus Torvalds

On Wed, Sep 5, 2012 at 10:03 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> If the driver is built in kernel, the request_firmware in .probe() may
>> prolong kernel init, and it might be a problem. But looks it is not a
>> big deal since most of drivers are built as module.
>
> Doing it by deferring the load also fixes that. The built in ones will
> defer their final probe until the firmware appears and all will be well.
>
> If your rootfs needs firmware not in your initrd you already broke it and
> there is a certain level beyond which you just have to give up trying to
> save people from themselves.
>
> It may actually make sense to push more of it into the core driver layer
> and take some of the ability to make mistakes away from driver authors.
> For the general case of "load firmware if we see one" there isn't really
> any reason we can't have a firmware_name entry in the probe table
> entries themselves. If that was present the core bus probe would kick a
> firmware load off and only when the firmware had loaded would it call
> ->probe with dev->firmware pointing at a refcounted firmware struct.
>
> At that point it should be much faster to fix existing drivers and much
> harder for a random device driver to get it wrong. We can even add
> helpers which manage dev->firmware, and free the relevant objects when
> needed, plus doing automatic ref/deref on probe/remove so that for a
> typical driver the author only has to do
>
>         {PCI_blah , ... .firmware_name="wibble500.xcr", }
>
> and all the loading, unloading, not loading twice happens by "magic" for
> the driver author.
>
> Add a dev_discard_firmware() for drivers that do this and know they can
> then dump the file and all is good 8)


It seems like a good plan. So drivers that call request_module()
inside init_module() can be easily converted to this new scheme.

For those drivers that load the firmware upon open() syscal can be
left as is, right?

Then we can write the rule in stone: *don't call request_firmware from
init_module, instead give the name of the firmware*. I even see
drivers whose only purpose is to load the firmware and change the PID
so it's handled by another module (like drivers/bluetooth/bcm203x.c)
to be simplified by some extent.


Lucas De Marchi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 16:51 ` Lucas De Marchi
@ 2012-09-05 17:08   ` Takashi Iwai
  0 siblings, 0 replies; 21+ messages in thread
From: Takashi Iwai @ 2012-09-05 17:08 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Greg Kroah-Hartman, Ming Lei, Rafael J. Wysocki, Kay Sievers,
	linux-kernel

At Wed, 5 Sep 2012 13:51:27 -0300,
Lucas De Marchi wrote:
> 
> On Tue, Sep 4, 2012 at 10:06 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > Hi,
> >
> > as I've got recently a few bug reports regarding the stuck with
> > request_firmware() in module_init of some sound drivers, I started
> > looking at the issue.  Strangely, the problem doesn't happen on
> > openSUSE 12.2 although it has the same udev version with libkmod as
> > Fedora.  So I installed Fedora 17, and indeed I could see a problem
> > there.
> 
> I fail to see how this is related to libkmod. It's indeed related to
> the fact that udev now timeout a modprobe request so people stop doing
> crazy things and like start daemons as modprobe's install rules. And
> that's true even with old module-init-tools.

Hm, OK, then it must be my misinterpretation.


Takashi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 16:59             ` Lucas De Marchi
@ 2012-09-05 17:09               ` Takashi Iwai
  0 siblings, 0 replies; 21+ messages in thread
From: Takashi Iwai @ 2012-09-05 17:09 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Alan Cox, Ming Lei, Greg Kroah-Hartman, Rafael J. Wysocki,
	Kay Sievers, linux-kernel, Linus Torvalds

At Wed, 5 Sep 2012 13:59:56 -0300,
Lucas De Marchi wrote:
> 
> On Wed, Sep 5, 2012 at 10:03 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >> If the driver is built in kernel, the request_firmware in .probe() may
> >> prolong kernel init, and it might be a problem. But looks it is not a
> >> big deal since most of drivers are built as module.
> >
> > Doing it by deferring the load also fixes that. The built in ones will
> > defer their final probe until the firmware appears and all will be well.
> >
> > If your rootfs needs firmware not in your initrd you already broke it and
> > there is a certain level beyond which you just have to give up trying to
> > save people from themselves.
> >
> > It may actually make sense to push more of it into the core driver layer
> > and take some of the ability to make mistakes away from driver authors.
> > For the general case of "load firmware if we see one" there isn't really
> > any reason we can't have a firmware_name entry in the probe table
> > entries themselves. If that was present the core bus probe would kick a
> > firmware load off and only when the firmware had loaded would it call
> > ->probe with dev->firmware pointing at a refcounted firmware struct.
> >
> > At that point it should be much faster to fix existing drivers and much
> > harder for a random device driver to get it wrong. We can even add
> > helpers which manage dev->firmware, and free the relevant objects when
> > needed, plus doing automatic ref/deref on probe/remove so that for a
> > typical driver the author only has to do
> >
> >         {PCI_blah , ... .firmware_name="wibble500.xcr", }
> >
> > and all the loading, unloading, not loading twice happens by "magic" for
> > the driver author.
> >
> > Add a dev_discard_firmware() for drivers that do this and know they can
> > then dump the file and all is good 8)
> 
> 
> It seems like a good plan. So drivers that call request_module()
> inside init_module() can be easily converted to this new scheme.
> 
> For those drivers that load the firmware upon open() syscal can be
> left as is, right?
> 
> Then we can write the rule in stone: *don't call request_firmware from
> init_module, instead give the name of the firmware*.

And we can even add a WARNING() if the call still happens, once when
the new implementation is done.


Takashi

> I even see
> drivers whose only purpose is to load the firmware and change the PID
> so it's handled by another module (like drivers/bluetooth/bcm203x.c)
> to be simplified by some extent.
> 
> 
> Lucas De Marchi
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 16:30               ` Alan Cox
@ 2012-09-05 21:08                 ` Benjamin Herrenschmidt
  2012-09-05 23:18                   ` Alan Cox
  2012-09-06  2:47                 ` Linus Torvalds
  2012-09-06  4:12                 ` Ming Lei
  2 siblings, 1 reply; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2012-09-05 21:08 UTC (permalink / raw)
  To: Alan Cox
  Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki,
	Kay Sievers, linux-kernel, Linus Torvalds

On Wed, 2012-09-05 at 17:30 +0100, Alan Cox wrote:
> 
> > Linus has said that he doesn't like to load firmware in probe(), but
> in some
> > situation the drivers have to load firmware in its probe():
> 
> You don't want to load firmware in probe because of the locking
> problems
> - you can trigger a load of another device on the same bus - the defer
>   dodges that nicely 

But then you have cases where probe() -> register_with_my_subsystem() ->
open(). Network devices come to mind. IE. udev must be able to deal with
a synchronous firmware load from probe I'm afraid.

The only other option would be to make full operation of the driver
delayed even from open(), which is possible with network devices at
least but generally very complex to implement.

Cheers,
Ben.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 21:08                 ` Benjamin Herrenschmidt
@ 2012-09-05 23:18                   ` Alan Cox
  2012-09-06  5:06                     ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Cox @ 2012-09-05 23:18 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki,
	Kay Sievers, linux-kernel, Linus Torvalds

> But then you have cases where probe() -> register_with_my_subsystem() ->
> open(). Network devices come to mind. IE. udev must be able to deal with
> a synchronous firmware load from probe I'm afraid.

I don't believe so. You have

	begin probe
		find match .. .firmware_name is set
		issue firmware load request
		set defer flag
	end probe

Later..

	firmware load complete
	kick probe up arse

	begin probe
		find match .. .firmware is loaded
		call ->probe()
		register
		open
		happiness
	end probe

Alan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 16:30               ` Alan Cox
  2012-09-05 21:08                 ` Benjamin Herrenschmidt
@ 2012-09-06  2:47                 ` Linus Torvalds
  2012-09-06  4:12                 ` Ming Lei
  2 siblings, 0 replies; 21+ messages in thread
From: Linus Torvalds @ 2012-09-06  2:47 UTC (permalink / raw)
  To: Alan Cox
  Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki,
	Kay Sievers, linux-kernel, Benjamin Herrenschmidt

On Wed, Sep 5, 2012 at 9:30 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> It's not about a workaround but about doing it properly for the long term
> and doing it in one place. It's also not a "great change", its a small
> change.

udev needs to get fixed regardless.

Stop this "we can break stuff" crap. Who maintains udev? Regressions
are not acceptable. I'm not going to change the kernel because udev
broke, f*ck it.

Seriously. More projects need to realize that regressions are totally
and utterly unacceptable.

The "long term cleaner issues" can be handled separately, but are
*not* an excuse to work around clear regressions in core packages.
That just encourages those package maintainers to be shit maintainers.

Just fix udev, which had a regression. And stop blaming the kernel for
user space breakage! Tying these kinds of things together ("udev
broke, so now we need to change the kernel") is *wrong*. It's totally
unacceptable to tie the two together that way.

          Linus

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 16:30               ` Alan Cox
  2012-09-05 21:08                 ` Benjamin Herrenschmidt
  2012-09-06  2:47                 ` Linus Torvalds
@ 2012-09-06  4:12                 ` Ming Lei
  2012-09-06 12:59                   ` Alan Cox
  2 siblings, 1 reply; 21+ messages in thread
From: Ming Lei @ 2012-09-06  4:12 UTC (permalink / raw)
  To: Alan Cox
  Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers,
	linux-kernel, Linus Torvalds, Benjamin Herrenschmidt

On Thu, Sep 6, 2012 at 12:30 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Yes, deferring the load may fix the built in case, but which also
>> introduces much work on changes of current drivers. In fact,
>> there are few guys who complained the built in case.
>
> It fixes the modular case too.

Sorry, I don't see anyone explained clearly why request_firmware()
can't be called inside module_init() in module case, so maybe it is
a bit early to say it is a fix on 'bug', :-)

>
>> The current complaint is from that udev may timeout request inside probe()
>> when drivers are built as module. As pointed by Linus and Benjamin,
>> it is better to fix it in udev, and looks not good to introduce great changes
>> (such as Takashi's defer probe patch) in kernel to workaround the problem.
>
> It's not about a workaround but about doing it properly for the long term
> and doing it in one place. It's also not a "great change", its a small
> change.
>
>> Linus has said that he doesn't like to load firmware in probe(), but in some
>> situation the drivers have to load firmware in its probe():
>
> You don't want to load firmware in probe because of the locking problems
> - you can trigger a load of another device on the same bus - the defer
>   dodges that nicely

Yes, it is alike with the patches from Takashi and me.

Also it is a kind of async probe, all these drivers may convert to
async probe to fix the problem for built-in case, I guess. If it is doable,
it may be a easier approach.

>
>> In fact, it is better for drivers to load firmware just when user wants to use
>> the device, and some drivers have already changed to load firmware in
>> the open() callback.
>
> For those devices sure but they are if anything a minority as far as I
> can see.
>
>> So looks loading firmware always before probe in driver core is
>> against the above idea.
>
> I never said "always"

Sorry for misunderstanding your idea.

>
>> > firmware load off and only when the firmware had loaded would it call
>> > ->probe with dev->firmware pointing at a refcounted firmware struct.
>>
>> IMO, introduce refcount for the firmware doesn't make sense. The lifetime
>> of firmware is completely different with lifetime of driver or device:
>
> Exactly. Which is why the moment you have multiple devices you need
> refcounts. It's also why the propsoal included a
>
> dev_discard_firmware()
>
> so you an instance can drop its firmware reference if it doesn't need it
> post probe.

This kind of mechanism has been implemented already: request_firmware()
and release_firmware() will get and put a refcount.  And, the reference
count is associated with firmware name, and it should be so, IMO.

>
>>     - firmware needn't be kept in memory in the device/driver's lifetime, and
>>       should be loaded just in need, and be released after downloading
>>       it into device.
>
> You broke suspend/resume for lots of devices.

The firmware cache mechanism will keep the firmware during suspend/resume
cycle to address the problem.

>
>>    - sometimes devices may disappear, but it is better to keep the
>>      firmware in memory, for example, device may be disconnected
>>      during resume but will come back later.
>
> So the moment you have multiple instances of a device with their own
> lifetime and you have the need to pin it sometimes you need a refcount
>
>> As said above, ref/deref on probe/remove is not a good idea since
>> we needn't to keep the firmware in memory during the whole device/driver
>> lifetime.
>
> Often you do. And in the case you don't you still have to deal with
> multiple probes doing asynchronous loads of the same firmware so you want
> to do matching and refcounting. It's pretty much essential.

For drivers, I understand request_firmware()/request_firmware_nowait()
and release_firmware() are enough. If many devices share one firmware,
there is only one firmware kept in memory for their requests if one holds
the firmware, and there is a refcount for it already, :-)

So I don't see why it is difficult to use request/release_firmware() inside
drivers, :-)

> The other big value apart from making it harder for driver writers to
> screw up is that it takes some of the control and puts it in one place.
> That means you can change it later easily not in each driver.
>
> This is enabling for device drivers. With no intention of offending
> driver authors the reality is that we should have driver interfaces that
>
> - work the right way by default
> - allow driver authors to do things themselves if they need to instead
>   (ie opt out)
> - are hard to f**k up
>
> because we want it to just work.

Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-05 23:18                   ` Alan Cox
@ 2012-09-06  5:06                     ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 21+ messages in thread
From: Benjamin Herrenschmidt @ 2012-09-06  5:06 UTC (permalink / raw)
  To: Alan Cox
  Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki,
	Kay Sievers, linux-kernel, Linus Torvalds

On Thu, 2012-09-06 at 00:18 +0100, Alan Cox wrote:
> > But then you have cases where probe() -> register_with_my_subsystem() ->
> > open(). Network devices come to mind. IE. udev must be able to deal with
> > a synchronous firmware load from probe I'm afraid.
> 
> I don't believe so. You have
> 
> 	begin probe
> 		find match .. .firmware_name is set
> 		issue firmware load request
> 		set defer flag
> 	end probe
> 
> Later..
> 
> 	firmware load complete
> 	kick probe up arse
> 
> 	begin probe
> 		find match .. .firmware is loaded
> 		call ->probe()
> 		register
> 		open
> 		happiness
> 	end probe

Ok, so this is my fault for picking the thread half way through and
missing the crucial point ... deferring probe() itself. BTW. Deferring
probe is useful for other things.

I've long advocated this as a way to deal with various soft dependencies
between services for example. Cases of a driver hooking up to two or 3
different busses, and wanting all the bits to be actually probed &
connected to each other in order, that sort of thing.

This can be done by defining the initialization order, have the "late"
ones defer when the depend aren't initialized yet, and kick probe arse
from bus notifiers (tho that could be moved to the core via some
helpers).

Cheers,
Ben.




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-06  4:12                 ` Ming Lei
@ 2012-09-06 12:59                   ` Alan Cox
  2012-09-06 15:38                     ` Ming Lei
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Cox @ 2012-09-06 12:59 UTC (permalink / raw)
  To: Ming Lei
  Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers,
	linux-kernel, Linus Torvalds, Benjamin Herrenschmidt

> Sorry, I don't see anyone explained clearly why request_firmware()
> can't be called inside module_init() in module case, so maybe it is
> a bit early to say it is a fix on 'bug', :-)

Because the firmware load may trigger a need to load a driver to load the
firmware.

> > dev_discard_firmware()
> >
> > so you an instance can drop its firmware reference if it doesn't need it
> > post probe.
> 
> This kind of mechanism has been implemented already: request_firmware()
> and release_firmware() will get and put a refcount.  And, the reference
> count is associated with firmware name, and it should be so, IMO.

Yes - so a dev_ firmware interface is very thin.

> > You broke suspend/resume for lots of devices.
> 
> The firmware cache mechanism will keep the firmware during suspend/resume
> cycle to address the problem.

Ok

> For drivers, I understand request_firmware()/request_firmware_nowait()
> and release_firmware() are enough. If many devices share one firmware,
> there is only one firmware kept in memory for their requests if one holds
> the firmware, and there is a refcount for it already, :-)
> 
> So I don't see why it is difficult to use request/release_firmware() inside
> drivers, :-)

The big problem can be summed up in one word "asynchronous". Having
either an automated handler for it before ->probe is called or having the
driver author cut and paste in

	if (!dev_request_firmware(dev, blah))
		return -EPROBE_DEFER;

avoids the need to deal with async completion after probe (and the
*horrible* case of 

	probe
		request firmware
	remove
	
	firmware ready
)

in each driver

Having an auto unload for it at the end is just neatness. Perhaps in fact
it should be devm_request_firmware() and use the mechanism we have ?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: A workaround for request_firmware() stuck in module_init
  2012-09-06 12:59                   ` Alan Cox
@ 2012-09-06 15:38                     ` Ming Lei
  0 siblings, 0 replies; 21+ messages in thread
From: Ming Lei @ 2012-09-06 15:38 UTC (permalink / raw)
  To: Alan Cox
  Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers,
	linux-kernel, Linus Torvalds, Benjamin Herrenschmidt

On Thu, Sep 6, 2012 at 8:59 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Sorry, I don't see anyone explained clearly why request_firmware()
>> can't be called inside module_init() in module case, so maybe it is
>> a bit early to say it is a fix on 'bug', :-)
>
> Because the firmware load may trigger a need to load a driver to load the
> firmware.

Could you explain the problem in a bit detail?

I understand once the request_firmware is called, the firmware load request
is sent to userspace, so why is a driver loading involved? and what is the
problem?

>
>> > dev_discard_firmware()
>> >
>> > so you an instance can drop its firmware reference if it doesn't need it
>> > post probe.
>>
>> This kind of mechanism has been implemented already: request_firmware()
>> and release_firmware() will get and put a refcount.  And, the reference
>> count is associated with firmware name, and it should be so, IMO.
>
> Yes - so a dev_ firmware interface is very thin.
>
>> > You broke suspend/resume for lots of devices.
>>
>> The firmware cache mechanism will keep the firmware during suspend/resume
>> cycle to address the problem.
>
> Ok
>
>> For drivers, I understand request_firmware()/request_firmware_nowait()
>> and release_firmware() are enough. If many devices share one firmware,
>> there is only one firmware kept in memory for their requests if one holds
>> the firmware, and there is a refcount for it already, :-)
>>
>> So I don't see why it is difficult to use request/release_firmware() inside
>> drivers, :-)
>
> The big problem can be summed up in one word "asynchronous". Having

OK, I agree on it, but I think it just happens in built in situation. For module
case, I think the synchronous request_firmware should be OK.

> either an automated handler for it before ->probe is called or having the
> driver author cut and paste in
>
>         if (!dev_request_firmware(dev, blah))
>                 return -EPROBE_DEFER;
>
> avoids the need to deal with async completion after probe (and the
> *horrible* case of
>
>         probe
>                 request firmware
>         remove
>
>         firmware ready
> )
>
> in each driver

OK, it is one solution for asynchronous firmware loading.

IMO, another candidate solution is to implement asynchronous probe
for these drivers which will call request_firmware in its probe().

>
> Having an auto unload for it at the end is just neatness. Perhaps in fact
> it should be devm_request_firmware() and use the mechanism we have ?

IMO, the kind of devm_request_firmware() mechanism may have its
disadvantage because the firmware in memory may or should be
released just after it has been downloaded into device, otherwise it will
consume memory unnecessarily to keep it in the whole device lifetime
if it will be released inside device release handler just like devm resources.


Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2012-09-06 15:39 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-04 13:06 A workaround for request_firmware() stuck in module_init Takashi Iwai
2012-09-04 15:52 ` Ming Lei
2012-09-04 16:10   ` Takashi Iwai
2012-09-05  1:15     ` Ming Lei
2012-09-05  5:53       ` Takashi Iwai
2012-09-05 11:32         ` Ming Lei
2012-09-05 13:03           ` Alan Cox
2012-09-05 14:01             ` Takashi Iwai
2012-09-05 15:22             ` Ming Lei
2012-09-05 16:30               ` Alan Cox
2012-09-05 21:08                 ` Benjamin Herrenschmidt
2012-09-05 23:18                   ` Alan Cox
2012-09-06  5:06                     ` Benjamin Herrenschmidt
2012-09-06  2:47                 ` Linus Torvalds
2012-09-06  4:12                 ` Ming Lei
2012-09-06 12:59                   ` Alan Cox
2012-09-06 15:38                     ` Ming Lei
2012-09-05 16:59             ` Lucas De Marchi
2012-09-05 17:09               ` Takashi Iwai
2012-09-05 16:51 ` Lucas De Marchi
2012-09-05 17:08   ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).