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<
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
-li
