Re: [PATCH] firmware loader: don't cancel _nowait requests when helper is not yet available

[Rafael J. Wysocki]

On Sunday, March 11, 2012, Kay Sievers wrote:
> On Sat, Mar 10, 2012 at 00:36, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Fri, Mar 09, 2012 at 11:30:24PM +0100, Christian Lamparter wrote:
> >> This patch fixes a regression which was introduced by:
> >> "PM: Print a warning if firmware is requested when tasks are frozen"
> >>
> >> request_firmware_nowait does not stall in any system resume paths.
> >> Therefore, I think it is perfectly save to use request_firmware_nowait
> >> from at least the ->complete() callback.
> >
> > Is there code somewhere in the kernel that wants to do this?  Has commit
> > a144c6a broken it somehow that this fix would resolve it?
> >
> >>
> >> Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
> >> ---
> >>  drivers/base/firmware_class.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> >> index 6c9387d..017e020 100644
> >> --- a/drivers/base/firmware_class.c
> >> +++ b/drivers/base/firmware_class.c
> >> @@ -535,7 +535,7 @@ static int _request_firmware(const struct firmware **firmware_p,
> >>
> >>       read_lock_usermodehelper();
> >>
> >> -     if (WARN_ON(usermodehelper_is_disabled())) {
> >> +     if (WARN_ON(usermodehelper_is_disabled() && !(nowait && uevent))) {
> >
> > What does uevent have to do with things here?
> 
> I don't think that the firmware loader should care about the
> usermodehelper at all, and that stuff fiddling should just be removed
> from the firmware class.

It's there to warn people that their drivers do stupid things like
loading frimware during system resume, which is guaranteed not to work.

IOW, it's there very much on purpose.

> Forking /sbin/hotplug is disabled by default, it is a broken concept,
> and it cannot work reliably on today's systems.
> 
> Firmware is not loaded by /sbin/hotplug since many years, but by udev
> or whatever service handles uevents, like ueventd on android.

Which I'm not sure why is relevant here.

Thanks,
Rafael
--
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/