* Re: udevadm trigger --type=devices calling ifup on all devices, even devices with ONBOOT=no
From: Kay Sievers @ 2013-09-11 15:42 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <870142449.13579622.1378910415316.JavaMail.root@redhat.com>
On Wed, Sep 11, 2013 at 4:40 PM, Assaf Muller <amuller@redhat.com> wrote:
> When I run "/sbin/udevadm trigger --typeÞvices" on my RHEL 6.4 machine (udev-147-2.46.el6.x86_64),
> udevadm runs ifup on all of the network devices on my machine, including devices who have ONBOOT=no in their ifcfg files.
>
> Is this intended behavior?
This list is about upstream general kernel and userspace development
regarding device management.
You question is Red Hat Enterprise Linux network setup specific.
Please use the Red Hat Support for that, most people here have no idea
about distribution details.
Thanks,
Kay
^ permalink raw reply
* udevadm trigger --type=devices calling ifup on all devices, even devices with ONBOOT=no
From: Assaf Muller @ 2013-09-11 14:40 UTC (permalink / raw)
To: linux-hotplug
Heya all,
When I run "/sbin/udevadm trigger --typefivices" on my RHEL 6.4 machine (udev-147-2.46.el6.x86_64),
udevadm runs ifup on all of the network devices on my machine, including devices who have ONBOOT=no in their ifcfg files.
Is this intended behavior?
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: loading 'acpiphp' fails after the system boot?
From: gregkh @ 2013-09-10 15:51 UTC (permalink / raw)
To: Shiro Itou 伊東
Cc: linux-hotplug@vger.kernel.org, linux-acpi@vger.kernel.org
In-Reply-To: <SNT149-W205633FE9047D530867E639A380@phx.gbl>
On Tue, Sep 10, 2013 at 08:43:05AM -0700, Shiro Itou 伊東 wrote:
> Hi All,
>
> In kernel version 3.0, when ‘apciphp’ driver module is loaded, after
> the system comes up, it fails with error “no such device”. When I
> tried to debug I see that it fails in acpiphp_get_num_slots ()
> function with error "Total 0 slots". In this case ‘bridge_list’ seem
> to be NULL.
> But when the ‘acpiphp’ is loaded during boot time, it seems to work file.
What happens in the 3.11 kernel release? Lots of work has been done in
this area in the past 2 years, please try a newer kernel. If there are
still problems, please let the linux-pci@vger.kernel.org mailing list
know about it.
thanks,
greg k-h
^ permalink raw reply
* loading 'acpiphp' fails after the system =?utf-8?
From: Shiro Itou 伊東 @ 2013-09-10 15:43 UTC (permalink / raw)
To: linux-hotplug@vger.kernel.org, gregkh@linuxfoundation.org
Cc: linux-acpi@vger.kernel.org
SGkgQWxsLArCoApJbiBrZXJuZWwgdmVyc2lvbiAzLjAsIHdoZW4g4oCYYXBjaXBocOKAmSBkcml2
ZXIgbW9kdWxlIGlzIGxvYWRlZCwgYWZ0ZXIgdGhlIHN5c3RlbSBjb21lcyB1cCwgaXQgZmFpbHMg
d2l0aCBlcnJvciDigJxubyBzdWNoIGRldmljZeKAnS4gV2hlbiBJIHRyaWVkIHRvIGRlYnVnIEkg
c2VlIHRoYXQgaXQgZmFpbHMgaW4gYWNwaXBocF9nZXRfbnVtX3Nsb3RzICgpIGZ1bmN0aW9uIHdp
dGggZXJyb3IgIlRvdGFsIDAgc2xvdHMiLiBJbiB0aGlzIGNhc2Ug4oCYYnJpZGdlX2xpc3TigJkg
wqBzZWVtIHRvIGJlIE5VTEwuCsKgCkJ1dCB3aGVuIHRoZSDigJhhY3BpcGhw4oCZIGlzIGxvYWRl
ZCBkdXJpbmcgYm9vdCB0aW1lLCDCoGl0IHNlZW1zIHRvIHdvcmsgZmlsZS4KwqAKV2hlcmUgZG9l
cyB0aGUg4oCYYnJpZGdlX2xpc3TigJkgZ2V0IHBvcHVsYXRlZCBhbmQgd2h5IGRvZXMgbG9hZGlu
ZyDigJhhcGNpcGhw4oCZIGFmdGVyIHRoZSBzeXN0ZW0gY29tZXMgdXAgZmFpbHMgdG8gbG9hZC4K
wqAKUmVnYXJkcywKU2hpcm8gCQkgCSAgIAkJICA
^ permalink raw reply
* Re: Logitech iTouch keyboard broken scan codes
From: Götz Christ @ 2013-08-27 16:34 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <CAGc31=qQe1d9f76roQE-wXhzHVA2CRtLTRqXqYuy3083ke-=zQ@mail.gmail.com>
Hello Martin,
don't worry, you helped me with this some month ago in Launchpad
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1152377
regards,
Götz
^ permalink raw reply
* Re: Logitech iTouch USB keyboard scan codes
From: Martin Pitt @ 2013-08-27 10:58 UTC (permalink / raw)
To: linux-hotplug
Hey Albrecht,
Albrecht Kolthoff [2013-07-15 23:15 +0200]:
> Anyway, here are my scancodes for this keyboard "Logitech Internet Navigator" (Logitech's product number Y-BF37), VID 046d, PID c309. Product web page:
> http://www.logitech.com/de-de/support/3373
>
> I added this line to my 95-keymap.rules:
>
> ENV{ID_VENDOR}="Logitech*", ATTRS{idProduct}="c309", RUN+="keymap $name logitech-int-nav-046d-c309-usb-complete"
Thanks for this! I translated your keymap to the new hwdb format that
current udev is using, and pushed this to master:
http://cgit.freedesktop.org/systemd/systemd/commit/?idy2d6163
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
^ permalink raw reply
* Re: Logitech iTouch keyboard broken scan codes
From: Martin Pitt @ 2013-08-27 10:49 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <CAGc31=qQe1d9f76roQE-wXhzHVA2CRtLTRqXqYuy3083ke-=zQ@mail.gmail.com>
Hello Götz,
sorry for the ridiculously late answer, catching up on old ML posts.
Götz Christ [2013-02-23 16:42 -0500]:
> Hi, I have a Logitech "iTouch Internet Navigator" keyboard, with 5
> broken scan codes.
> Following the instructions from
> /usr/share/doc/systemd/README.keymap.txt, I have:
>
> $ /usr/lib/udev/findkeyboards
> AT keyboard: input/event2
I need some way of identifying that keyboard properly. Can you please
do
udevadm info --export-db > /tmp/udev.txt
and send /tmp/udev.txt to me? (Please ensure the keyboard is
connected).
> # /usr/lib/udev/keymap -i input/event2
>
> (which corresponds to the keys labeled as)
>
> 0x91 shop # Shopping
> 0x92 config # iTouch
> 0x93 finance # Finance
> 0x94 prog1 # My Sites
> 0x95 prog2 # Community
>
> I couldn't find something better in input.h for the last two, so I
> chose prog1 and prog2.
That sounds fine.
Thanks,
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
^ permalink raw reply
* Re: eMachines E725 keymap
From: Zbigniew Jędrzejewski-Szmek @ 2013-08-23 19:22 UTC (permalink / raw)
To: linux-hotplug
On Fri, Jul 26, 2013 at 05:28:47PM -0300, Ludvig wrote:
> Keynao for eMachines E725. Nearly identical to the already existing
> Acer one, which isn't surprising since eMachines is a sub-brand of
> acer, but I'm sending it anyway.
>
> % cat /sys/class/dmi/id/{sys_vendor,product_name}
> eMachines
> eMachines E725
> 0xA9 switchvideomode # Fn+F5
> 0xCE brightnessup # Fn+Right
> 0xD5 wlan # (toggle) on-to-off
> 0xD6 wlan # (toggle) off-to-on
> 0xD7 bluetooth # (toggle) on-to-off
> 0xD8 bluetooth # (toggle) off-to-on
> 0xEF brightnessdown # Fn+Left
> 0xF1 f22 # Fn+F7 Touchpad toggle (off-to-on)
> 0xF2 f23 # Fn+F7 Touchpad toggle (on-to-off)
> 0xF8 fn
Committed to systemd as
http://cgit.freedesktop.org/systemd/systemd/commit/?idña5d37e08.
Please check if it works if you have a chance to try systemd/udev >= 207.
Zbyszek
^ permalink raw reply
* Re: udev: Why non-blocking poll() with blocking recvmsg()?
From: Tetsuo Handa @ 2013-08-23 12:48 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <201308192039.ACD60413.HFOQSFtLFMOJVO@I-love.SAKURA.ne.jp>
Hello.
> I haven't identified which NULL in udev_monitor_receive_device() is returned.
I identified that "recvmsg() in udev_monitor_receive_device() returning ENOBUFS
error" is the cause of "udev_monitor_receive_device() returning NULL".
Therefore, I moved the discussion to LP #1215911.
https://launchpad.net/bugs/1215911
Thanks.
^ permalink raw reply
* Re: automatic sysfs attribute creation
From: Greg KH @ 2013-08-21 17:44 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <7214233.AEam3A6dd5@ws-stein>
On Wed, Aug 21, 2013 at 05:17:15PM +0200, Alexander Stein wrote:
> Hello,
>
> I've read Greg's blog post about sysfs attributes at
> http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/.
> We've used attribute groups for the whole time, but the post made me
> wondering whether our drivers might be affected by that too.
It depends on the driver type.
> Unfortunately the blog mentions only struct class, struct bus, struct
> device and so on. I couldn't find anything about USB interfaces or
> struct net_device. At least the latter has const struct
> attribute_group *sysfs_groups[4]; itself. So I tried setting
> sysfs_groups[0] in the probe function of my usb_driver before calling
> register_candev (which in return set the attribute groups in struct
> device later). So IMHO the sysfs files should be created before the
> new CAN interface is announced to userspace.
Yes, they "should".
> But I get the exact problem Greg describes: A udev rules shall set an
> attribute during ACTION="add". This USB device create 2 CAN
> interfaces and only on one device the attribute is set correctly.
> _Unless_: the rule file is named 75-usbcan.rules in /etc/udev/rules.d
> or has a higher number at the beginning. This indicates a race
> condition to me. Am I doing anything wrong here? The described way
> should have prevented this.
> BTW: Is there something similar for the USB device driver?
The big issue is that for things like USB interfaces, where the device
is already created and announced to userspace, the driver binding
happens in the kernel afterward. So, in the probe function for the USB
driver, if you want to attach to this device, and add sysfs files to it,
it will be "too late".
Which in a way makes sense, as the USB device is "owned" by the USB
core, not the driver. For a network device, it doesn't make sense, and
I'm working on fixing this, right now there's not a way for you to
resolve this, it needs to be done in the network core, and it's on my
TODO list to resolve.
But then there is the issue of "platform" devices. They get announced
to userspace before the driver is bound to them, but there's really only
ever one driver that can bind to it, so their sysfs files are also
created "late". I don't know what to do about this example (which is
the same thing for the USB devices), but I'm open to ideas.
Right now I'm working on fixing up all of the attribute code in the
drivers (part of it will show up in 3.12), then I can move to resolving
the network driver issue you have pointed out, and then worry about
platform devices (and USB and the rest). So it will be a while, unless
people wish to help out.
thanks,
greg k-h
^ permalink raw reply
* automatic sysfs attribute creation
From: Alexander Stein @ 2013-08-21 15:17 UTC (permalink / raw)
To: linux-hotplug
Hello,
I've read Greg's blog post about sysfs attributes at http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/. We've used attribute groups for the whole time, but the post made me wondering whether our drivers might be affected by that too. Unfortunately the blog mentions only struct class, struct bus, struct device and so on. I couldn't find anything about USB interfaces or struct net_device. At least the latter has const struct attribute_group *sysfs_groups[4]; itself. So I tried setting sysfs_groups[0] in the probe function of my usb_driver before calling register_candev (which in return set the attribute groups in struct device later). So IMHO the sysfs files should be created before the new CAN interface is announced to userspace.
But I get the exact problem Greg describes: A udev rules shall set an attribute during ACTION="add". This USB device create 2 CAN interfaces and only on one device the attribute is set correctly. _Unless_: the rule file is named 75-usbcan.rules in /etc/udev/rules.d or has a higher number at the beginning. This indicates a race condition to me.
Am I doing anything wrong here? The described way should have prevented this.
BTW: Is there something similar for the USB device driver?
Regards and thanks in advance
Alexander
^ permalink raw reply
* Re: udev: Why non-blocking poll() with blocking recvmsg()?
From: Tetsuo Handa @ 2013-08-19 13:24 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <201308192039.ACD60413.HFOQSFtLFMOJVO@I-love.SAKURA.ne.jp>
Hello Kay.
Kay Sievers wrote:
> > recvmsg() waits until something is queued if udev_monitor->sock is blocking,
>
> Only on a blocking socket, which it isn't by default:
I confirmed using strace utility that Ubuntu 12.04's wait-for-root
(source code shown below) calls recvmsg() using blocking mode.
# strace /usr/lib/initramfs-tools/bin/wait-for-root /dev/sdaX 5
> It's from a time where the socket was still blocking, but we still
> used poll() in calling code. We didn't want to block the caller, hence
> the poll() inside the library.
So, "poll() in udev_monitor_receive_device() does not wait" is a feature?
Then, wait-for-root is responsible for retrying the loop (line 91 to 104) when
udev_monitor_receive_device() returned NULL (without waiting for a "block"
subsystem event) caused by udev_monitor_receive_device() getting a non-"block"
subsystem event at recvmsg(), isn't it?
---------- wait-for-root.c start ----------
1:#include <libudev.h>
2:
3:#include <sys/types.h>
4:#include <sys/stat.h>
5:
6:#include <time.h>
7:#include <stdio.h>
8:#include <limits.h>
9:#include <signal.h>
10:#include <stdlib.h>
11:#include <string.h>
12:#include <unistd.h>
13:
14:
15:static int device_queued (struct udev *udev, const char *path);
16:static int matching_device (struct udev_device *device, const char *path);
17:
18:static void alarm_handler (int signum);
19:
20:
21:int
22:main (int argc,
23: char *argv[])
24:{
25: const char * devpath;
26: char path[PATH_MAX];
27: int timeout;
28: struct udev * udev;
29: struct udev_monitor *udev_monitor;
30: struct stat devstat;
31: struct udev_device * udev_device;
32: const char * type;
33:
34: if (argc != 3) {
35: fprintf (stderr, "Usage: %s DEVICE TIMEOUT\n", argv[0]);
36: exit (2);
37: }
38:
39: devpath = argv[1];
40: if (! strncmp (devpath, "UUID=", 5)) {
41: strcpy (path, "/dev/disk/by-uuid/");
42: strcat (path, devpath + 5);
43: } else if (! strncmp (devpath, "LABEL=", 6)) {
44: strcpy (path, "/dev/disk/by-label/");
45: strcat (path, devpath + 6);
46: } else {
47: strcpy (path, devpath);
48: }
49:
50: timeout = atoi (argv[2]);
51:
52: signal (SIGALRM, alarm_handler);
53: alarm (timeout);
54:
55: /* Connect to the udev monitor first; if we stat() first, the
56: * event might happen between the stat() and the time we actually
57: * get hooked up.
58: */
59: udev = udev_new ();
60: udev_monitor = udev_monitor_new_from_netlink (udev, "udev");
61:
62: udev_monitor_filter_add_match_subsystem_devtype (udev_monitor, "block", NULL);
63: udev_monitor_enable_receiving (udev_monitor);
64:
65: /* If the device is not being processed, check to see whether it
66: * exists already on the filesystem. If this is true, we don't need
67: * to wait for it can obtain the filesystem type by looking up the
68: * udevdb record by major/minor.
69: */
70: if ((! device_queued (udev, devpath))
71: && (stat (path, &devstat) = 0)
72: && S_ISBLK (devstat.st_mode))
73: {
74: udev_device = udev_device_new_from_devnum (udev, 'b', devstat.st_rdev);
75: if (udev_device) {
76: type = udev_device_get_property_value (udev_device, "ID_FS_TYPE");
77: if (type) {
78: printf ("%s\n", type);
79:
80: udev_device_unref (udev_device);
81: goto exit;
82: }
83:
84: udev_device_unref (udev_device);
85: }
86: }
87:
88: /* When the device doesn't exist yet, or is still being processed
89: * by udev, use the monitor socket to wait it to be done.
90: */
91: while ((udev_device = udev_monitor_receive_device (udev_monitor)) != NULL) {
92: if (matching_device (udev_device, devpath)) {
93: type = udev_device_get_property_value (udev_device, "ID_FS_TYPE");
94: if (type) {
95: printf ("%s\n", type);
96:
97: udev_device_unref (udev_device);
98: goto exit;
99: }
100:
101: }
102:
103: udev_device_unref (udev_device);
104: }
105:
106:exit:
107: udev_monitor_unref (udev_monitor);
108: udev_unref (udev);
109:
110: exit (0);
111:}
112:
113:
114:static int
115:device_queued (struct udev *udev,
116: const char * devpath)
117:{
118: struct udev_queue * udev_queue;
119: struct udev_list_entry *queue_entry;
120: int found = 0;
121:
122: udev_queue = udev_queue_new (udev);
123:
124: for (queue_entry = udev_queue_get_queued_list_entry (udev_queue);
125: queue_entry != NULL;
126: queue_entry = udev_list_entry_get_next (queue_entry)) {
127: const char * syspath;
128: struct udev_device *udev_device;
129:
130: syspath = udev_list_entry_get_name (queue_entry);
131: udev_device = udev_device_new_from_syspath (udev, syspath);
132: if (udev_device) {
133: if (matching_device (udev_device, devpath))
134: found = 1;
135:
136: udev_device_unref (udev_device);
137: }
138: }
139:
140: udev_queue_unref (udev_queue);
141:
142: return found;
143:}
144:
145:static int
146:matching_device (struct udev_device *device,
147: const char * path)
148:{
149: const char * devnode;
150: struct udev_list_entry *devlinks_entry;
151:
152: /* Match by name */
153: devnode = udev_device_get_devnode (device);
154: if (devnode && (! strcmp (path, devnode)))
155: return 1;
156:
157: /* Match by UUID */
158: if (! strncmp (path, "UUID=", 5)) {
159: const char *uuid;
160:
161: uuid = udev_device_get_property_value (device, "ID_FS_UUID");
162: if (uuid && (! strcmp (path + 5, uuid)))
163: return 1;
164: }
165:
166: /* Match by LABEL */
167: if (! strncmp (path, "LABEL=", 6)) {
168: const char *label;
169:
170: label = udev_device_get_property_value (device, "ID_FS_LABEL");
171: if (label && (! strcmp (path + 6, label)))
172: return 1;
173: }
174:
175: /* Match by symlink */
176: for (devlinks_entry = udev_device_get_devlinks_list_entry (device);
177: devlinks_entry != NULL;
178: devlinks_entry = udev_list_entry_get_next (devlinks_entry))
179: if (! strcmp (path, udev_list_entry_get_name (devlinks_entry)))
180: return 1;
181:
182: return 0;
183:}
184:
185:
186:static void
187:alarm_handler (int signum)
188:{
189: exit (1);
190:}
---------- wait-for-root.c end ----------
^ permalink raw reply
* Re: udev: Why non-blocking poll() with blocking recvmsg()?
From: Kay Sievers @ 2013-08-19 12:45 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <201308192039.ACD60413.HFOQSFtLFMOJVO@I-love.SAKURA.ne.jp>
On Mon, Aug 19, 2013 at 1:39 PM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> recvmsg() waits until something is queued if udev_monitor->sock is blocking,
Only on a blocking socket, which it isn't by default:
http://www.freedesktop.org/software/systemd/libudev/libudev-udev-monitor.html#udev-monitor-receive-device
> but poll() does not wait until something is queued even if udev_monitor->sock
> is blocking.
>
> I think that this inconsistency makes udev_monitor_receive_device()
> to immediately return NULL when an event where passes_filter() returns 0 was
> already queued but another event where passes_filter() returns 1 is not yet
> queued, making wait-for-root utility which is waiting for only "block" subsystem
> events fail.
>
> Why don't we unconditionally go to retry rather than poll(pfd, 1, 0) so that
> "blocking socket can wait for next event" and "non-blocking socket can return
> immediately"?
It's from a time where the socket was still blocking, but we still
used poll() in calling code. We didn't want to block the caller, hence
the poll() inside the library.
The poll() inside of libudev-monitor.c can probably just be removed
today. Or even the whole retry be removed, it might not be a useful
optimization at all.
Kay
^ permalink raw reply
* Re: udev: Why non-blocking poll() with blocking recvmsg()?
From: Tetsuo Handa @ 2013-08-19 12:37 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <201308192039.ACD60413.HFOQSFtLFMOJVO@I-love.SAKURA.ne.jp>
Hello Martin.
Martin Pitt wrote:
> > Although wait-for-root uses blocking socket, udev_monitor_receive_device() sometimes
> > immediately returns NULL.
>
> It *assumed* a blocking socket, but didn't make sure that it actually
> was. Originally libudev used blocking sockets, but at some point [1]
> this was switched to non-blocking by default, and wait-for-root needed
> to be adjusted to that. Please see https://launchpad.net/bugs/1154813,
> it's fixed in newer Ubuntu releases.
Yes, I have checked LP #1154813. But I think that my case is different.
I'm using Ubuntu 12.04 (Precise Pangolin), not Ubuntu 13.04 (Raring Ringtail);
I think [1] is not yet applied as of Ubuntu 12.04.
What #1154813 solves is that "make sure that udev_monitor_receive_device()
called by wait-for-root waits for event at recvmsg()".
What I'm talking is that "udev_monitor_receive_device() called by wait-for-root
does not wait for next event at poll() whereas it waits for first event at
recvmsg()".
^ permalink raw reply
* Re: udev: Why non-blocking poll() with blocking recvmsg()?
From: Martin Pitt @ 2013-08-19 12:15 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <201308192039.ACD60413.HFOQSFtLFMOJVO@I-love.SAKURA.ne.jp>
Hello Tetsuo,
Tetsuo Handa [2013-08-19 20:39 +0900]:
> I'm experiencing a boot failure problem with wait-for-root utility in Ubuntu 12.04.
Note that this is an Ubuntu specific program.
> Although wait-for-root uses blocking socket, udev_monitor_receive_device() sometimes
> immediately returns NULL.
It *assumed* a blocking socket, but didn't make sure that it actually
was. Originally libudev used blocking sockets, but at some point [1]
this was switched to non-blocking by default, and wait-for-root needed
to be adjusted to that. Please see https://launchpad.net/bugs/1154813,
it's fixed in newer Ubuntu releases.
[1] http://git.kernel.org/cgit/linux/hotplug/udev.git/commit/?id\x13d83b8
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
^ permalink raw reply
* udev: Why non-blocking poll() with blocking recvmsg()?
From: Tetsuo Handa @ 2013-08-19 11:39 UTC (permalink / raw)
To: linux-hotplug
Hello.
I'm experiencing a boot failure problem with wait-for-root utility in Ubuntu 12.04.
Although wait-for-root uses blocking socket, udev_monitor_receive_device() sometimes
immediately returns NULL.
I haven't identified which NULL in udev_monitor_receive_device() is returned.
But I came to wonder why poll() in udev_monitor_receive_device() uses timeout = 0.
---------- Quoting from http://git.kernel.org/cgit/linux/hotplug/udev.git/tree/src/libudev-monitor.c start ----------
UDEV_EXPORT struct udev_device *udev_monitor_receive_device(struct udev_monitor *udev_monitor)
{
(...snipped...)
retry:
(...snipped...)
buflen = recvmsg(udev_monitor->sock, &smsg, 0);
if (buflen < 0) {
if (errno != EINTR)
info(udev_monitor->udev, "unable to receive message\n");
return NULL;
}
(...snipped...)
/* skip device, if it does not pass the current filter */
if (!passes_filter(udev_monitor, udev_device)) {
struct pollfd pfd[1];
int rc;
udev_device_unref(udev_device);
/* if something is queued, get next device */
pfd[0].fd = udev_monitor->sock;
pfd[0].events = POLLIN;
rc = poll(pfd, 1, 0);
if (rc > 0)
goto retry;
return NULL;
}
(...snipped...)
}
---------- Quoting from http://git.kernel.org/cgit/linux/hotplug/udev.git/tree/src/libudev-monitor.c end ----------
recvmsg() waits until something is queued if udev_monitor->sock is blocking,
but poll() does not wait until something is queued even if udev_monitor->sock
is blocking. I think that this inconsistency makes udev_monitor_receive_device()
to immediately return NULL when an event where passes_filter() returns 0 was
already queued but another event where passes_filter() returns 1 is not yet
queued, making wait-for-root utility which is waiting for only "block" subsystem
events fail.
Why don't we unconditionally go to retry rather than poll(pfd, 1, 0) so that
"blocking socket can wait for next event" and "non-blocking socket can return
immediately"?
Regards.
^ permalink raw reply
* Re: [PATCH] udev: fail firmware loading immediately if no search path is defined
From: Kay Sievers @ 2013-08-10 21:28 UTC (permalink / raw)
To: Tom Gundersen
Cc: Maarten Lankhorst, Linux Wireless List, Andy Lutomirski,
Intel Linux Wireless, Johannes Berg, linux-hotplug, Bryan Kadzban,
systemd Mailing List, linux-kernel
In-Reply-To: <CAG-2HqUT3hbFPSEqYnJvBOgS6p4dKf=nhqZtzqS-on8FFe9ipA@mail.gmail.com>
On Sat, Aug 10, 2013 at 11:00 PM, Tom Gundersen <teg@jklm.no> wrote:
> It would be simple enough to add an udev rule to just print 'ignoring
> firmware event' to the logs.
This and I guess:
SUBSYSTEM="firmware", ACTION="add", ATTR{loading}="-1"
would also just cancel the request at the same time without any other
code needed.
The udev firmware support just a configure option, just like the
kernel has them. So distributions should enable it in udev and the
kernel if they need it.
We simply cannot coordinate the defaults of systemd and the kernel
because the rules of the kernel are different. The kernel does "keep
defaults like stuff has been in the past" and udev does "make default
what makes the most sense on current systems".
> 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.
The proper fix is that userspace firmware should be disabled in the
kernel for new systems, and kept enabled only for old systems. Old
systems need to enable a new udev version to support firmware loading.
There are currently broken in-kernel mis-users of the firmware
interface that use the firmware interface but disable uevents, they
still pull-in the user interface of the firmware loader. If nobody
wants to fix them, the code for the common users of the firmware
loader should at least get rid of the userspace fallback to call out
to userspace. At that point the udev configure option would not matter
any more.
> 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.
Right, I think all is fine. It's something that people can control
with the kernel and udev configuration options. It's just that the
defaults of the kernel and udev don't match at the moment, because
they have different policies of setting default values.
Kay
^ permalink raw reply
* Re: [PATCH] udev: fail firmware loading immediately if no search path is defined
From: Andy Lutomirski @ 2013-08-07 21:24 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Tom Gundersen, Bryan Kadzban, systemd Mailing List,
Linux Wireless List, Johannes Berg, Intel Linux Wireless,
linux-hotplug, Kay Sievers, linux-kernel
In-Reply-To: <5201FCA4.30804@gmail.com>
On Wed, Aug 7, 2013 at 12:52 AM, Maarten Lankhorst
<m.b.lankhorst@gmail.com> wrote:
> Op 07-08-13 02:26, Andy Lutomirski schreef:
>> On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>>> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@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 <teg@jklm.no> wrote:
>>>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>>>>>> <m.b.lankhorst@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 <teg@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..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
^ permalink raw reply
* [PATCH] udev: fail firmware loading immediately if no search path is defined
From: Maarten Lankhorst @ 2013-08-07 7:52 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tom Gundersen, Bryan Kadzban, systemd Mailing List,
Linux Wireless List, Johannes Berg, Intel Linux Wireless,
linux-hotplug, Kay Sievers, linux-kernel
In-Reply-To: <CALCETrU-CWuefE5XUpC=xFuCXaL56Cox95nL35Bm9jMi_B+6_A@mail.gmail.com>
Op 07-08-13 02:26, Andy Lutomirski schreef:
> On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@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 <teg@jklm.no> wrote:
>>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>>>>> <m.b.lankhorst@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 <teg@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..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] = '/') {
^ permalink raw reply related
* Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it
From: Andy Lutomirski @ 2013-08-07 0:26 UTC (permalink / raw)
To: Tom Gundersen
Cc: Bryan Kadzban, systemd Mailing List, Linux Wireless List,
Johannes Berg, Intel Linux Wireless, linux-hotplug,
Maarten Lankhorst, Kay Sievers, linux-kernel
In-Reply-To: <CAG-2HqU=yyxomNTg9-2+bxMKP=e5_pdVT7bhB_CHhFzB-ac_mQ@mail.gmail.com>
On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>
> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@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 <teg@jklm.no> wrote:
>> > > On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>> > > <m.b.lankhorst@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 <teg@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.
--Andy
^ permalink raw reply
* Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it
From: Bryan Kadzban @ 2013-08-06 16:31 UTC (permalink / raw)
To: Tom Gundersen
Cc: Maarten Lankhorst, Andy Lutomirski, systemd-devel, linux-hotplug,
Linux Wireless List, linux-kernel, Intel Linux Wireless,
Kay Sievers, Johannes Berg
In-Reply-To: <CAG-2HqVvoQK8ggErGGinO9i8Pbu-O-X=eYFW445ce24dpt7HyA@mail.gmail.com>
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@jklm.no> wrote:
> > On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> > <m.b.lankhorst@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 <teg@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? :-)
(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.)
^ permalink raw reply
* Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it
From: Andy Lutomirski @ 2013-08-06 16:08 UTC (permalink / raw)
To: Tom Gundersen
Cc: Maarten Lankhorst, systemd-devel, linux-hotplug,
Linux Wireless List, linux-kernel, Intel Linux Wireless,
Kay Sievers, Johannes Berg
In-Reply-To: <CAG-2HqVvoQK8ggErGGinO9i8Pbu-O-X=eYFW445ce24dpt7HyA@mail.gmail.com>
On Tue, Aug 6, 2013 at 2:17 AM, Tom Gundersen <teg@jklm.no> wrote:
> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>> <m.b.lankhorst@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 <teg@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
TBH, the udev README is the last thing I'm going to check to figure
out why I don't have wifi for a couple minutes after boot. Also, the
message is missing the point. It's not that it's deprecated and
sometimes causes problems -- it's that udev *changed behavior* and
breaks your system if you have CONFIG_FW_LOADER_USER_HELPER=y.
If udev logged something (for a couple of years), that would be a
different story.
--Andy
^ permalink raw reply
* Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it
From: Tom Gundersen @ 2013-08-06 9:17 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Andy Lutomirski, systemd-devel, linux-hotplug,
Linux Wireless List, linux-kernel, Intel Linux Wireless,
Kay Sievers, Johannes Berg
In-Reply-To: <CAG-2HqXEj06JbUOWee=NfuRXxL75uRf=55FZcXRLHroDzYUthQ@mail.gmail.com>
On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> <m.b.lankhorst@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 <teg@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
-t
^ permalink raw reply
* Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it
From: Tom Gundersen @ 2013-08-06 9:11 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Andy Lutomirski, systemd-devel, linux-hotplug,
Linux Wireless List, linux-kernel, Intel Linux Wireless,
Kay Sievers, Johannes Berg
In-Reply-To: <5200B1BD.7030307@gmail.com>
On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
<m.b.lankhorst@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 <teg@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?
Cheers,
Tom
^ permalink raw reply
* Re: [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it
From: Maarten Lankhorst @ 2013-08-06 8:20 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-kernel, Kay Sievers, Zbigniew Jędrzejewski-Szmek,
systemd-devel, Linux Wireless List, linux-hotplug,
Intel Linux Wireless, Johannes Berg
In-Reply-To: <325b19bb936d7ebae11edad86aac8f0931e8abd9.1375719828.git.luto@amacapital.net>
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@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, or a stub should be added to udev to always fail firmware loading so timeouts don't occur.
~Maarten
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox