Re: [systemd-devel] [PATCH] udev: fail firmware loading immediately if no search path is defined

2013-08-10 Thread Tom Gundersen
Hi Maarten,

On 7 Aug 2013 09:52, Maarten Lankhorst m.b.lankho...@gmail.com wrote:
 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.

Well that would break the case for anyone who actually wants to use a
non-udev user space helper (which is the only situation where this
configuration actually makes sense). I don't necessarily think that using
such a helper is a sensible thing to do, but if we don't want to support
that we should just drop the option from the kernel rather than make udev
override the kernel configuration.

 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.

It would be simple enough to add an udev rule to just print 'ignoring
firmware event' to the logs. We should really ignore the event though, and
not cancel it. Not sure if this is something we want upstream (after all,
there are plenty of situations where we don't warn if the recommendations
in the README file are not followed), or if distros and whoever wants it
should ship that themselves. I'll leave that for Kay to decide.

Lastly, note that the plan is to drop all the firmware code from udev in
the not too distant future, so it doesn't really maker much sense to add
new functionality to that at this point.

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel


Re: [systemd-devel] [PATCH] udev: fail firmware loading immediately if no search path is defined

2013-08-07 Thread Andy Lutomirski
On Wed, Aug 7, 2013 at 12:52 AM, Maarten Lankhorst
m.b.lankho...@gmail.com wrote:
 Op 07-08-13 02:26, Andy Lutomirski schreef:
 On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen t...@jklm.no wrote:
 On 6 Aug 2013 18:32, Bryan Kadzban br...@kadzban.is-a-geek.net 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 t...@jklm.no wrote:
 On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
 m.b.lankho...@gmail.com 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 t...@jklm.no
 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..000
 --- a/rules/50-firmware.rules
 +++ /dev/null
 @@