Re: [PATCH 2/3] firmware: Avoid deadlock of usermodehelper lock at shutdown

[Ming Lei]

On Thu, May 9, 2013 at 12:15 AM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> At Wed, 8 May 2013 23:56:51 +0800,
> Ming Lei wrote:
>>
>> On Wed, May 8, 2013 at 2:56 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
>> > When a system goes to reboot/shutdown, it tries to disable the
>> > usermode helper via usermodehelper_disable().  This might be blocked
>> > when a driver tries to load a firmware beforehand and it's stuck by
>> > some reason.
>>
>> IMO, it is better to find why the loading is stuck. Also we already provides
>> the timeout sysfs file to help to deal with the situation.
>
> The loading is done manually in the case of dell_rbu driver, so it may
> happen at any time that the f/w loading doesn't finish properly.
>
>
>> > In this patch, the firmware class driver registers a reboot notifier
>> > so that it can abort all pending f/w bufs.  Also enable a flag for
>> > avoiding the call of usermodehelper after the reboot/shutdown starts.
>>
>> With this patch, maybe we only hide the real problem.
>
> No, you can simulate the hang easily.  Try the below (you can run it
> on every x86 machine; it just loads the data onto the memory.)
>
> - modprobe dell_rbu
>
> - echo init > /sys/devices/platform/dell_rbu/image_type
>
>   (... now /sys/class/firmware/dell_rbu/* appear)
>
> - echo 1 > /sys/class/firmware/dell_rbu/loading
>
> - halt -p
>
> Now the machine gets stuck.
>
> Yes, the real problem is obvious: you didn't finish the f/w loading in
> the above, and it will never happen.  This doesn't justify that the
> machine can be stalled at shutdown, though.

OK, got it, thanks for your explanation, but anyway the stall is caused
by not concluding the load in userspace(we have document about it),
not by firmware loader itself.

I think there is no reason to disable the loading timeout for !event, could
you test the below patch to see if it may avoid the stall?

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b1f926..caf399e 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -848,11 +848,12 @@ static int _request_firmware_load(struct
firmware_priv *fw_priv, bool uevent,
 		goto err_del_bin_attr;
 	}

+	if (timeout != MAX_SCHEDULE_TIMEOUT)
+		schedule_delayed_work(&fw_priv->timeout_work, timeout);
+
 	if (uevent) {
 		dev_set_uevent_suppress(f_dev, false);
 		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
-		if (timeout != MAX_SCHEDULE_TIMEOUT)
-			schedule_delayed_work(&fw_priv->timeout_work, timeout);

 		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
 	}


Thanks,
--
Ming Lei
--
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/