Re: [PATCH] request_firmware: skip timeout if userspace was not notified

From: Cornelia Huck
Date: Mon Aug 06 2007 - 08:24:31 EST


On Fri, 03 Aug 2007 21:07:35 +0200,
Javier Pello <javier.pello@xxxxxxx> wrote:

> 1. The first part changes kobject_uevent_env in lib/kobject_uevent.c
> to report a failure if both netlink_broadcast (if applicable) and
> call_usermodehelper fail to send the event to userspace. Nothing in
> the kernel seems to care about the return value of kobject_uevent_env,
> so this should not break anything.

Famous last words ;) I recall that things broke when the driver core
tried to care about the return code of kobject_uevent(), so this may
work if you check just in your special case.

>
> 2. The second part changes _request_firmware in
> drivers/base/firmware_class.c to actually check the return value of
> kobject_uevent and skip the loading_timeout delay if the loading event
> was not delivered to userspace at all.

Note that kobject_uevent() returns 0 if the event has been filtered.

>
> The patches apply cleanly against 2.6.23-rc1. Any suggestions or feedback
> will be welcome.

It would be better if you sent the patches seperately.

>
> Javier
>
> PS Please CC: me on replies, as I am not subscribed to the list.
>
>
> ==========
> From: Javier Pello <javier.pello@xxxxxxx>
>
> kobject_uevent: return an error if event was not delivered to userspace
>
> Make kobject_uevent_env return an error to the caller if the event was
> not delivered to userspace either via netlink or via uevent_helper.
>
> Signed-off-by: Javier Pello <javier.pello@xxxxxxx>
>
> ---
> diff -u linux-2.6.22/lib/kobject_uevent.c linux-2.6.22-p/lib/kobject_uevent.c
> --- linux-2.6.22/lib/kobject_uevent.c 2007-07-09 01:32:17.000000000 +0200
> +++ linux-2.6.22-p/lib/kobject_uevent.c 2007-07-18 20:17:10.000000000 +0200
> @@ -186,7 +186,8 @@
> }
>
> NETLINK_CB(skb).dst_group = 1;
> - netlink_broadcast(uevent_sock, skb, 0, 1, GFP_KERNEL);
> + retval = netlink_broadcast(uevent_sock, skb, 0, 1,
> + GFP_KERNEL);

Maybe add a pr_debug() here for debugging?
> }
> }
> #endif
> @@ -198,7 +199,18 @@
> argv [0] = uevent_helper;
> argv [1] = (char *)subsystem;
> argv [2] = NULL;
> - call_usermodehelper (argv[0], argv, envp, UMH_WAIT_EXEC);
> +#if defined(CONFIG_NET)
> + if (retval) {
> + retval = call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> + } else {
> + call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> + }
> +#else
> + retval = call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> +#endif
> }
>
> exit:

Here a pr_debug() may also be nice.

>
> ==========
> From: Javier Pello <javier.pello@xxxxxxx>
>
> request_firmware: skip timeout if userspace was not notified
>
> Stop _request_firmware from setting up a timer and waiting for
> the firmware image to appear if kobject_uevent returns an error
> (meaning userspace was not notified at all).
>
> This prevents useless delays if there is no userspace tool to
> provide the firmware image (eg during boot).
>
> Signed-off-by: Javier Pello <javier.pello@xxxxxxx>
>
> ---
> diff -u linux-2.6.22/drivers/base/firmware_class.c linux-2.6.22-p/drivers/base/firmware_class.c
> --- linux-2.6.22/drivers/base/firmware_class.c 2007-07-09 01:32:17.000000000 +0200
> +++ linux-2.6.22-p/drivers/base/firmware_class.c 2007-07-18 20:06:53.000000000 +0200
> @@ -420,8 +420,12 @@
> add_timer(&fw_priv->timeout);
> }
>
> - kobject_uevent(&f_dev->kobj, KOBJ_ADD);
> - wait_for_completion(&fw_priv->completion);
> + retval = kobject_uevent(&f_dev->kobj, KOBJ_ADD);
> + if (retval) {
> + fw_load_abort(fw_priv);
> + } else {
> + wait_for_completion(&fw_priv->completion);
> + }
> set_bit(FW_STATUS_DONE, &fw_priv->status);
> del_timer_sync(&fw_priv->timeout);
> } else

Maybe add a small comment here?
-
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/