[PATCH] driver core : fix request_firmware_nowait

[PATCH] driver core : fix request_firmware_nowait

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

request_firmware_nowait declares it can be called in non-sleep contexts,
but kthead_run which is called by request_firmware_nowait may sleep.
So fix it by starting thread in workqueue to request firmware asynchronously.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/base/firmware_class.c |   66 ++++++++++++++++++++++++++++++++++------
 1 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 2d296b7..0388cf7 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -573,15 +573,25 @@ release_firmware(const struct firmware *fw)
 
 /* Async support */
 struct firmware_work {
-	struct work_struct work;
 	struct module *module;
 	const char *name;
 	struct device *device;
 	void *context;
 	void (*cont)(const struct firmware *fw, void *context);
 	int uevent;
+	struct list_head list;
+	struct task_struct *task;
+};
+
+struct firmware_schedule{
+	spinlock_t	lock;
+	struct list_head head;
+	struct work_struct work;
 };
 
+static struct firmware_schedule fw_schedule;
+
+
 static int
 request_firmware_work_func(void *arg)
 {
@@ -605,6 +615,35 @@ request_firmware_work_func(void *arg)
 	return ret;
 }
 
+static void fw_schedule_fun(struct work_struct *work)
+{
+	struct firmware_schedule	*fs =
+		container_of(work, struct firmware_schedule, work);
+	unsigned long		flags;
+
+	spin_lock_irqsave(&fs->lock, flags);
+	while (!list_empty(&fs->head)) {
+		struct firmware_work    *fw_work;
+
+		fw_work = list_entry(fs->head.next, struct firmware_work, list);
+		list_del(&fw_work->list);
+
+		spin_unlock_irqrestore(&fs->lock, flags);
+		fw_work->task = kthread_run(request_firmware_work_func, fw_work,
+			    "firmware/%s", fw_work->name);
+		spin_lock_irqsave(&fs->lock, flags);
+
+		if (IS_ERR(fw_work->task)) {
+			fw_work->cont(NULL, fw_work->context);
+			module_put(fw_work->module);
+			kfree(fw_work);
+			dev_err(fw_work->device, "%s: kthread_run failed\n",
+				__func__);
+		}
+	}
+	spin_unlock_irqrestore(&fs->lock, flags);
+}
+
 /**
  * request_firmware_nowait: asynchronous version of request_firmware
  * @module: module requesting the firmware
@@ -626,7 +665,8 @@ request_firmware_nowait(
 	const char *name, struct device *device, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
-	struct task_struct *task;
+	unsigned long		flags;
+	struct firmware_schedule	*fs = &fw_schedule;
 	struct firmware_work *fw_work = kmalloc(sizeof (struct firmware_work),
 						GFP_ATOMIC);
 
@@ -646,15 +686,14 @@ request_firmware_nowait(
 		.uevent = uevent,
 	};
 
-	task = kthread_run(request_firmware_work_func, fw_work,
-			    "firmware/%s", name);
+	INIT_LIST_HEAD(&fw_work->list);
+
+	spin_lock_irqsave(&fs->lock, flags);
+	list_add_tail(&fw_work->list, &fs->head);
+	spin_unlock_irqrestore(&fs->lock, flags);
+
+	schedule_work(&fs->work);
 
-	if (IS_ERR(task)) {
-		fw_work->cont(NULL, fw_work->context);
-		module_put(fw_work->module);
-		kfree(fw_work);
-		return PTR_ERR(task);
-	}
 	return 0;
 }
 
@@ -662,6 +701,13 @@ static int __init
 firmware_class_init(void)
 {
 	int error;
+	struct firmware_schedule	*fs = &fw_schedule;
+
+	memset(fs, 0, sizeof(*fs));
+	spin_lock_init(&fs->lock);
+	INIT_LIST_HEAD(&fs->head);
+	INIT_WORK(&fs->work, fw_schedule_fun);
+
 	error = class_register(&firmware_class);
 	if (error) {
 		printk(KERN_ERR "%s: class_register failed\n", __func__);
-- 
1.6.0.GIT

Re: [PATCH] driver core : fix request_firmware_nowait

[Alan Cox]

On Fri, 22 May 2009 23:06:27 +0800
tom.leiming@gmail.com wrote:

> From: Ming Lei <tom.leiming@gmail.com>
> 
> request_firmware_nowait declares it can be called in non-sleep contexts,
> but kthead_run which is called by request_firmware_nowait may sleep.

Does anyone actually need to call it in a non-sleeping context or can the
documentation simply be fixed instead to avoid all the extra complexity ?

Re: [PATCH] driver core : fix request_firmware_nowait

[Ming Lei]

2009/5/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
> On Fri, 22 May 2009 23:06:27 +0800
> tom.leiming@gmail.com wrote:
>
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> request_firmware_nowait declares it can be called in non-sleep contexts,
>> but kthead_run which is called by request_firmware_nowait may sleep.
>
> Does anyone actually need to call it in a non-sleeping context or can the
> documentation simply be fixed instead to avoid all the extra complexity ?
>

It seems no one calls it in a non-sleeping context now, but we can provide it
without much extra complexity, IMHO.  Also someone has complained it :

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

-- 
Lei Ming

Re: [PATCH] driver core : fix request_firmware_nowait

[Greg KH]

On Fri, May 22, 2009 at 11:26:16PM +0800, Ming Lei wrote:
> 2009/5/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
> > On Fri, 22 May 2009 23:06:27 +0800
> > tom.leiming@gmail.com wrote:
> >
> >> From: Ming Lei <tom.leiming@gmail.com>
> >>
> >> request_firmware_nowait declares it can be called in non-sleep contexts,
> >> but kthead_run which is called by request_firmware_nowait may sleep.
> >
> > Does anyone actually need to call it in a non-sleeping context or can the
> > documentation simply be fixed instead to avoid all the extra complexity ?
> >
> 
> It seems no one calls it in a non-sleeping context now, but we can provide it
> without much extra complexity, IMHO.  Also someone has complained it :
> 
>       http://marc.info/?t=124022846400003&r=1&w=2

That was 3 years ago :)

I say just fix the documentation.

thanks,

greg k-h

Re: [PATCH] driver core : fix request_firmware_nowait

[Ming Lei]

2009/5/29 Greg KH <greg@kroah.com>:
> On Fri, May 22, 2009 at 11:26:16PM +0800, Ming Lei wrote:
>> 2009/5/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> > On Fri, 22 May 2009 23:06:27 +0800
>> > tom.leiming@gmail.com wrote:
>> >
>> >> From: Ming Lei <tom.leiming@gmail.com>
>> >>
>> >> request_firmware_nowait declares it can be called in non-sleep contexts,
>> >> but kthead_run which is called by request_firmware_nowait may sleep.
>> >
>> > Does anyone actually need to call it in a non-sleeping context or can the
>> > documentation simply be fixed instead to avoid all the extra complexity ?
>> >
>>
>> It seems no one calls it in a non-sleeping context now, but we can provide it
>> without much extra complexity, IMHO.  Also someone has complained it :
>>
>>       http://marc.info/?t=124022846400003&r=1&w=2
>
> That was 3 years ago :)

No, just one month ago, also the name of *_nowait is very misleading and only
documentation fix seems not enough,  right?

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

2009-04-20 Am I allowed to call request_firmware_nowait from an
linux-ker John Hughes

>
> I say just fix the documentation.
>
> thanks,
>
> greg k-h
>



-- 
Lei Ming

Re: [PATCH] driver core : fix request_firmware_nowait

[Greg KH]

On Fri, May 29, 2009 at 09:24:32AM +0800, Ming Lei wrote:
> 2009/5/29 Greg KH <greg@kroah.com>:
> > On Fri, May 22, 2009 at 11:26:16PM +0800, Ming Lei wrote:
> >> 2009/5/22 Alan Cox <alan@lxorguk.ukuu.org.uk>:
> >> > On Fri, 22 May 2009 23:06:27 +0800
> >> > tom.leiming@gmail.com wrote:
> >> >
> >> >> From: Ming Lei <tom.leiming@gmail.com>
> >> >>
> >> >> request_firmware_nowait declares it can be called in non-sleep contexts,
> >> >> but kthead_run which is called by request_firmware_nowait may sleep.
> >> >
> >> > Does anyone actually need to call it in a non-sleeping context or can the
> >> > documentation simply be fixed instead to avoid all the extra complexity ?
> >> >
> >>
> >> It seems no one calls it in a non-sleeping context now, but we can provide it
> >> without much extra complexity, IMHO.  Also someone has complained it :
> >>
> >>       http://marc.info/?t=124022846400003&r=1&w=2
> >
> > That was 3 years ago :)
> 
> No, just one month ago, also the name of *_nowait is very misleading and only
> documentation fix seems not enough,  right?
> 
> http://marc.info/?t=124022846400003&r=1&w=2
> 
> 2009-04-20 Am I allowed to call request_firmware_nowait from an
> linux-ker John Hughes

Oops, sorry, you are right, I read the date wrong.

Anyway, I suggest just fixing the documentation, it's easier :)

thanks,

greg k-h

Re: [PATCH] driver core : fix request_firmware_nowait

[Ming Lei]

On Thu, 28 May 2009 18:43:41 -0700
Greg KH <greg@kroah.com> wrote:
> 
> Anyway, I suggest just fixing the documentation, it's easier :)

Ok, it is the patch which fixes the documentation.
Thanks.

>From 32a1fe6013ed796d397417cc60d5e9a2aa66d74a Mon Sep 17 00:00:00 2001
From: Ming Lei <tom.leiming@gmail.com>
Date: Fri, 29 May 2009 11:23:04 +0800
Subject: [PATCH] driver core: fix documentation of request_firmware_nowait

request_firmware_nowait declares it can be called in non-sleep contexts,
but kthead_run called by request_firmware_nowait may sleep. So fix its
documentation and comment to make callers clear about it.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 Documentation/firmware_class/README |    3 ++-
 drivers/base/firmware_class.c       |    5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
index c3480aa..3a37ad3 100644
--- a/Documentation/firmware_class/README
+++ b/Documentation/firmware_class/README
@@ -77,7 +77,8 @@
    seconds for the whole load operation.
 
  - request_firmware_nowait() is also provided for convenience in
-   non-user contexts.
+   user contexts to request firmware asynchronously, but can't be called
+   in atomic contexts.
 
 
  about in-kernel persistence:
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 2d296b7..2427702 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -617,8 +617,9 @@ request_firmware_work_func(void *arg)
  * @cont: function will be called asynchronously when the firmware
  *	request is over.
  *
- *	Asynchronous variant of request_firmware() for contexts where
- *	it is not possible to sleep.
+ *	Asynchronous variant of request_firmware() for user contexts where
+ *	it is not possible to sleep for long time. It can't be called
+ *	in atomic contexts.
  **/
 int
 request_firmware_nowait(
-- 
1.6.0.GIT




-- 
Lei Ming