linux-kernel - Re: [RFC] firmware loader: retry _nowait requests when userhelper is not yet available

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201203162325.56303.chunkeey@googlemail.com>
Date:	Fri, 16 Mar 2012 23:25:55 +0100
From:	Christian Lamparter <chunkeey@...glemail.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
	alan@...rguk.ukuu.org.uk,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux PM mailing list <linux-pm@...r.kernel.org>
Subject: Re: [RFC] firmware loader: retry _nowait requests when userhelper is not yet available

On Friday 16 March 2012 23:19:53 Rafael J. Wysocki wrote:
> > On 03/04/2012 01:52 AM, Christian Lamparter wrote:
> > 
> > > During resume, the userhelper might not be available. However for
> > > drivers which use the request_firmware_nowait interface, this will
> > > only lead to a pointless WARNING and a device which no longer works
> > > after the resume [since it couldn't get the firmware, because the
> > > userhelper was not available to take the request].
> > > 
> > > In order to solve this "chicken or egg" dilemma, the code now
> > > retries _nowait requests at one second intervals until the
> > > "loading_timeout" time is up.
> > > 
> > > ---
> > > I'm aware about the previous "request_firmware* in probe" discussions.
> > > Unfortunately, the hardware needs firmware so there is no other way
> > > around it. So please, I just wanted to know what the general opinion
> > > about the idea behind this patch is.
> 
> BTW, I wonder what comments on this patch were posted?
Only Alan Cox was kind enough to drop me a few words.

Why? Do you think it is actually sane from a specific POV?
[Don't tell me you do :D !]

Really, it wasn't until:
"[PATCH] firmware loader: don't cancel _nowait requests when helper is not yet available"
https://lkml.org/lkml/2012/3/9/612 
that things started to pick up.

Regards,
	Chr

> > > ---
> > >  drivers/base/firmware_class.c |   24 +++++++++++++++++++++++-
> > >  1 files changed, 23 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 6c9387d..9f70096 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/kthread.h>
> > >  #include <linux/highmem.h>
> > >  #include <linux/firmware.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/slab.h>
> > > 
> > >  #define to_dev(obj) container_of(obj, struct device, kobj)
> > > @@ -535,6 +536,11 @@ static int _request_firmware(const struct firmware **firmware_p,
> > > 
> > >  	read_lock_usermodehelper();
> > > 
> > > +	if (nowait && usermodehelper_is_disabled()) {
> > > +		retval = -EBUSY;
> > > +		goto out;
> > > +	}
> > > +
> > >  	if (WARN_ON(usermodehelper_is_disabled())) {
> > >  		dev_err(device, "firmware: %s will not be loaded\n", name);
> > >  		retval = -EBUSY;
> > > @@ -633,7 +639,7 @@ static int request_firmware_work_func(void *arg)
> > >  {
> > >  	struct firmware_work *fw_work = arg;
> > >  	const struct firmware *fw;
> > > -	int ret;
> > > +	int ret, timeout = loading_timeout;
> > > 
> > >  	if (!arg) {
> > >  		WARN_ON(1);
> > > @@ -642,6 +648,22 @@ static int request_firmware_work_func(void *arg)
> > > 
> > >  	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
> > >  				fw_work->uevent, true);
> > > +
> > > +	while (ret == -EBUSY) {
> > > +		/*
> > > +		 * Try to retrieve the firmware within the loading timeout.
> > > +		 * To stick with the loading timeout convention from above:
> > > +		 *	loading_timeout = 0 means 'try forever' as well.
> > > +		 */
> > > +
> > > +		msleep(1000);
> > > +		ret = _request_firmware(&fw, fw_work->name, fw_work->device,
> > > +					fw_work->uevent, true);
> > > +
> > > +		if (timeout != 0 && timeout-- == 1)
> > 
> > > +			break;
> > 
> > > +	};
> > 
> > > +
> > 
> > >  	fw_work->cont(fw, fw_work->context);
> > > 
> > >  	module_put(fw_work->module);
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/