A workaround for request_firmware() stuck in module_init

* 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 related	[flat|nested] 21+ messages in thread