From patchwork Wed Aug 7 07:52:04 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maarten Lankhorst X-Patchwork-Id: 2839972 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 8F0DCBF535 for ; Wed, 7 Aug 2013 07:52:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0A9C5201F0 for ; Wed, 7 Aug 2013 07:52:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A1E92201EB for ; Wed, 7 Aug 2013 07:52:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932254Ab3HGHwJ (ORCPT ); Wed, 7 Aug 2013 03:52:09 -0400 Received: from mail-ee0-f50.google.com ([74.125.83.50]:60656 "EHLO mail-ee0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932073Ab3HGHwH (ORCPT ); Wed, 7 Aug 2013 03:52:07 -0400 Received: by mail-ee0-f50.google.com with SMTP id d51so673940eek.9 for ; Wed, 07 Aug 2013 00:52:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=/QybJIHFCHWeMlGSyqiAUL2AIHFL71ySCBunvgMVCJ8=; b=FnQqelKK0VxNZrx1J5PwHI9TQcYeF2yy6si9rQ5DF5F1eOV8hGUMDLwhSajRISOxoI aWJNcIo8vl2rl60L1ctlaJWbBmoJYzqSFJBNqmxoiXmiWCXnjFl6XRxJzaB1lM2n/QdN K31Qufpvu/O2tz9VCcBxuX4dAxht4AtUbYlKjE+a62ZLTW/bDV1rMwKViyh17VdQ0GnD 23HnGG8b+fCuuVMwYaZUFKwnAbrrckTqoyQYk6AdT4pV8x5bw0gtvfZ2Fv4sDOUQ/2Bd TkzGrx/41cP04BdaXBohrtjaB6rL8KIbi2rXetOumeAwWS1Gydj2PpoiUzy+qAG4A0Wm 2O/g== X-Received: by 10.15.31.9 with SMTP id x9mr1796221eeu.103.1375861925842; Wed, 07 Aug 2013 00:52:05 -0700 (PDT) Received: from [192.168.1.128] (5ED49945.cm-7-5c.dynamic.ziggo.nl. [94.212.153.69]) by mx.google.com with ESMTPSA id c3sm7444388eev.3.2013.08.07.00.52.04 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 07 Aug 2013 00:52:05 -0700 (PDT) Message-ID: <5201FCA4.30804@gmail.com> Date: Wed, 07 Aug 2013 09:52:04 +0200 From: Maarten Lankhorst User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130623 Thunderbird/17.0.7 MIME-Version: 1.0 To: Andy Lutomirski CC: Tom Gundersen , Bryan Kadzban , systemd Mailing List , Linux Wireless List , Johannes Berg , Intel Linux Wireless , linux-hotplug@vger.kernel.org, Kay Sievers , linux-kernel@vger.kernel.org Subject: [PATCH] udev: fail firmware loading immediately if no search path is defined References: <325b19bb936d7ebae11edad86aac8f0931e8abd9.1375719828.git.luto@amacapital.net> <5200B1BD.7030307@gmail.com> <20130806163158.GA23093@kadzban.is-a-geek.net> In-Reply-To: Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Op 07-08-13 02:26, Andy Lutomirski schreef: > On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen wrote: >> On 6 Aug 2013 18:32, "Bryan Kadzban" wrote: >>> On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote: >>>> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen wrote: >>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst >>>>> wrote: >>>>>> Op 05-08-13 18:29, Andy Lutomirski schreef: >>>>>>> The systemd commit below can delay firmware loading by multiple >>>>>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y. Unfortunately no one >>>>>>> noticed that the systemd-udev change would break new kernels as well >>>>>>> as old kernels. >>>>>>> >>>>>>> Since the kernel apparently can't count on reasonable userspace >>>>>>> support, turn this thing off by default. >>>>>>> >>>>>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a >>>>>>> Author: Tom Gundersen >>>>>>> Date: Mon Mar 18 15:12:18 2013 +0100 >>>>>>> >>>>>>> udev: make firmware loading optional and disable by default >>>>>>> >>>>>>> Distros that whish to support old kernels should set >>>>>>> >>>>>>> --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware" >>>>>>> to retain the old behaviour. >>>>>>> >>>>>> methinks this patch should be reverted then, >>>>> Well, all the code is still there, so it can be enabled if anyone >>>>> wants it. >>>>> >>>>>> or a stub should be added to udev to always fail firmware loading so >>>>>> timeouts don't occur. >>>>> I think the only use (if any) of a userspace firmware loader would be >>>>> for anyone who wants a custom one (i.e., not udev), so we shouldn't >>>>> just fail the loading from udev unconditionally. >>>>> >>>>> How about we just improve the udev documentation a bit, similar to >>>>> Andy's kernel patch? >>>> Sorry, I should first have checked. We already document this in the >>>> README: >>>> >>>>> Userspace firmware loading is deprecated, will go away, and >>>>> sometimes causes problems: >>>>> CONFIG_FW_LOADER_USER_HELPER=n >>> ...And this patch is making the kernel default to the correct behavior, >>> instead of the now-broken-by-udev behavior. >>> >>> I'm not sure I see the issue with it? :-) >> Oh yeah this patch is totally the right thing to do, I was just arguing that >> there is nothing to be done on the udev side. >> >>> (Add me to the list of people that think udev is broken too, fwiw. But >>> let's at least not leave *both* sides in a broken-by-default state.) >> Well I don't think it is too much to ask that the kernel and udev should be >> configured in a consistent way. Especially as thing still work even if you >> get it wrong, albeit with a delay. > Except that the current defaults are inconsistent and there is no > explanation anywhere in the logs when this is screwed up. > So what is wrong with my 'fail in udev immediately if not configured' idea? In that case it doesn't matter whether CONFIG_FW_LOADER_USER_HELPER is set or not. You could even print a useful message for the user in udev to the log, so they have an idea of what happened. Breaking udev on older still supported kernels by default without printing any debug info is silly, and the only cost is a small increase in disk space when unused. I did so in below patch. ~Maarten I converted < ELEMENTSOF to != ELEMENTSOF in the loop to work correctly when the array is empty. Most code in udev-builtin-firmware is eliminated at -O2 optimization level when FIRMWARE_PATH is not explicitly set. 8<---- --- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/Makefile.am b/Makefile.am index b8b8d06..2097629 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2235,6 +2235,7 @@ libudev_core_la_SOURCES = \ src/udev/udev-ctrl.c \ src/udev/udev-builtin.c \ src/udev/udev-builtin-btrfs.c \ + src/udev/udev-builtin-firmware.c \ src/udev/udev-builtin-hwdb.c \ src/udev/udev-builtin-input_id.c \ src/udev/udev-builtin-keyboard.c \ @@ -2271,14 +2272,6 @@ libudev_core_la_CPPFLAGS = \ $(AM_CPPFLAGS) \ -DFIRMWARE_PATH="$(FIRMWARE_PATH)" -if ENABLE_FIRMWARE -libudev_core_la_SOURCES += \ - src/udev/udev-builtin-firmware.c - -dist_udevrules_DATA += \ - rules/50-firmware.rules -endif - if HAVE_KMOD libudev_core_la_SOURCES += \ src/udev/udev-builtin-kmod.c diff --git a/configure.ac b/configure.ac index 0ecc716..dc7a3e3 100644 --- a/configure.ac +++ b/configure.ac @@ -823,8 +823,6 @@ for i in $with_firmware_path; do done IFS=$OLD_IFS AC_SUBST(FIRMWARE_PATH) -AS_IF([test "x${FIRMWARE_PATH}" != "x"], [ AC_DEFINE(HAVE_FIRMWARE, 1, [Define if FIRMWARE is available]) ]) -AM_CONDITIONAL(ENABLE_FIRMWARE, [test "x${FIRMWARE_PATH}" != "x"]) # ------------------------------------------------------------------------------ AC_ARG_ENABLE([gudev], diff --git a/rules/50-firmware.rules b/rules/50-firmware.rules deleted file mode 100644 index f0ae684..0000000 --- a/rules/50-firmware.rules +++ /dev/null @@ -1,3 +0,0 @@ -# do not edit this file, it will be overwritten on update - -SUBSYSTEM=="firmware", ACTION=="add", RUN{builtin}="firmware" diff --git a/rules/50-udev-default.rules b/rules/50-udev-default.rules index f764789..645830e 100644 --- a/rules/50-udev-default.rules +++ b/rules/50-udev-default.rules @@ -8,6 +8,7 @@ SUBSYSTEM=="rtc", KERNEL=="rtc0", SYMLINK+="rtc", OPTIONS+="link_priority=-100" SUBSYSTEM=="usb", ENV{DEVTYPE}=="usb_device", IMPORT{builtin}="usb_id", IMPORT{builtin}="hwdb --subsystem=usb" SUBSYSTEM=="input", ENV{ID_INPUT}=="", IMPORT{builtin}="input_id" +SUBSYSTEM=="firmware", ACTION=="add", IMPORT{builtin}="firmware" ENV{MODALIAS}!="", IMPORT{builtin}="hwdb --subsystem=$env{SUBSYSTEM}" ACTION!="add", GOTO="default_permissions_end" diff --git a/shell-completion/bash/udevadm b/shell-completion/bash/udevadm index 8ad8550..c22a3e2 100644 --- a/shell-completion/bash/udevadm +++ b/shell-completion/bash/udevadm @@ -83,7 +83,7 @@ _udevadm() { fi ;; 'test-builtin') - comps='blkid btrfs hwdb input_id keyboard kmod net_id path_id usb_id uaccess' + comps='blkid btrfs firmware hwdb input_id keyboard kmod net_id path_id usb_id uaccess' ;; *) comps=${VERBS[*]} diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c index b80940b..e678545 100644 --- a/src/udev/udev-builtin-firmware.c +++ b/src/udev/udev-builtin-firmware.c @@ -97,7 +97,7 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo /* lookup firmware file */ uname(&kernel); - for (i = 0; i < ELEMENTSOF(searchpath); i++) { + for (i = 0; i != ELEMENTSOF(searchpath); i++) { strscpyl(fwpath, sizeof(fwpath), searchpath[i], kernel.release, "/", firmware, NULL); fwfile = fopen(fwpath, "re"); if (fwfile != NULL) @@ -112,7 +112,10 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo strscpyl(loadpath, sizeof(loadpath), udev_device_get_syspath(dev), "/loading", NULL); if (fwfile == NULL) { - log_debug("did not find firmware file '%s'\n", firmware); + if (ELEMENTSOF(searchpath)) + log_debug("did not find firmware file '%s'\n", firmware); + else + log_error("could not load firmware file '%s', firmware loading not enabled\n", firmware); rc = EXIT_FAILURE; /* * Do not cancel the request in the initrd, the real root might have diff --git a/src/udev/udev-builtin.c b/src/udev/udev-builtin.c index 6b3a518..5a5cb30 100644 --- a/src/udev/udev-builtin.c +++ b/src/udev/udev-builtin.c @@ -34,9 +34,7 @@ static const struct udev_builtin *builtins[] = { [UDEV_BUILTIN_BLKID] = &udev_builtin_blkid, #endif [UDEV_BUILTIN_BTRFS] = &udev_builtin_btrfs, -#ifdef HAVE_FIRMWARE [UDEV_BUILTIN_FIRMWARE] = &udev_builtin_firmware, -#endif [UDEV_BUILTIN_HWDB] = &udev_builtin_hwdb, [UDEV_BUILTIN_INPUT_ID] = &udev_builtin_input_id, [UDEV_BUILTIN_KEYBOARD] = &udev_builtin_keyboard, diff --git a/src/udev/udev.h b/src/udev/udev.h index 8395926..31fa606 100644 --- a/src/udev/udev.h +++ b/src/udev/udev.h @@ -140,9 +140,7 @@ enum udev_builtin_cmd { UDEV_BUILTIN_BLKID, #endif UDEV_BUILTIN_BTRFS, -#ifdef HAVE_FIRMWARE UDEV_BUILTIN_FIRMWARE, -#endif UDEV_BUILTIN_HWDB, UDEV_BUILTIN_INPUT_ID, UDEV_BUILTIN_KEYBOARD, @@ -170,9 +168,7 @@ struct udev_builtin { extern const struct udev_builtin udev_builtin_blkid; #endif extern const struct udev_builtin udev_builtin_btrfs; -#ifdef HAVE_FIRMWARE extern const struct udev_builtin udev_builtin_firmware; -#endif extern const struct udev_builtin udev_builtin_hwdb; extern const struct udev_builtin udev_builtin_input_id; extern const struct udev_builtin udev_builtin_keyboard; diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 45ec3d6..e27a33a 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -98,9 +98,7 @@ struct event { dev_t devnum; int ifindex; bool is_block; -#ifdef HAVE_FIRMWARE bool nodelay; -#endif }; static inline struct event *node_to_event(struct udev_list_node *node) @@ -444,10 +442,8 @@ static int event_queue_insert(struct udev_device *dev) event->devnum = udev_device_get_devnum(dev); event->is_block = streq("block", udev_device_get_subsystem(dev)); event->ifindex = udev_device_get_ifindex(dev); -#ifdef HAVE_FIRMWARE if (streq(udev_device_get_subsystem(dev), "firmware")) event->nodelay = true; -#endif udev_queue_export_device_queued(udev_queue_export, dev); log_debug("seq %llu queued, '%s' '%s'\n", udev_device_get_seqnum(dev), @@ -527,11 +523,9 @@ static bool is_devpath_busy(struct event *event) return true; } -#ifdef HAVE_FIRMWARE /* allow to bypass the dependency tracking */ if (event->nodelay) continue; -#endif /* parent device event found */ if (event->devpath[common] == '/') {