Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933459Ab3HGVZP (ORCPT ); Wed, 7 Aug 2013 17:25:15 -0400 Received: from mail-vb0-f48.google.com ([209.85.212.48]:52414 "EHLO mail-vb0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933342Ab3HGVZK (ORCPT ); Wed, 7 Aug 2013 17:25:10 -0400 MIME-Version: 1.0 In-Reply-To: <5201FCA4.30804@gmail.com> References: <325b19bb936d7ebae11edad86aac8f0931e8abd9.1375719828.git.luto@amacapital.net> <5200B1BD.7030307@gmail.com> <20130806163158.GA23093@kadzban.is-a-geek.net> <5201FCA4.30804@gmail.com> From: Andy Lutomirski Date: Wed, 7 Aug 2013 14:24:49 -0700 Message-ID: Subject: Re: [PATCH] udev: fail firmware loading immediately if no search path is defined To: Maarten Lankhorst 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 Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11361 Lines: 266 On Wed, Aug 7, 2013 at 12:52 AM, Maarten Lankhorst wrote: > 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. Seems reasonable to me. It might be worth adding something to the message to suggest turning off CONFIG_FW_LOADER_USER_HELPER. > > ~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<---- > 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] == '/') { > -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/