Re: [systemd-devel] [PATCH] udev: fail firmware loading immediately if no search path is defined
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
[systemd-devel] [PATCH] udev: fail firmware loading immediately if no search path is defined
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.
~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
@@ -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
+++
Re: [systemd-devel] [PATCH] udev: fail firmware loading immediately if no search path is defined
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
@@
