Re: [PATCH] udev: fail firmware loading immediately if no search pathis defined

[Andy Lutomirski]

On Wed, Aug 7, 2013 at 12:52 AM, Maarten Lankhorst
<m.b.lankhorst@xxxxxxxxx> wrote:
> Op 07-08-13 02:26, Andy Lutomirski schreef:
>> On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@xxxxxxx> wrote:
>>> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@xxxxxxxxxxxxxxxxxxxxx> 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 <teg@xxxxxxx> wrote:
>>>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>>>>>> <m.b.lankhorst@xxxxxxxxx> 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 <teg@xxxxxxx>
>>>>>>>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/