| Message ID | 20121002222333.GA32207@kroah.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 2, 2012 at 3:23 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > which went into udev release 187 which I think corresponds to the place > when people started having problems, right Mauro? According to what I've seen, people started complaining in 182, not 187. See for example http://patchwork.linuxtv.org/patch/13085/ which is a thread where you were involved too.. See also the arch linux thread: https://bbs.archlinux.org/viewtopic.php?id=134012&p=1 And see this email from Kay Sievers that shows that it was all known about and intentional in the udev camp: http://www.spinics.net/lists/netdev/msg185742.html There's a possible patch suggested here: http://lists.freedesktop.org/archives/systemd-devel/2012-August/006357.html but quite frankly, I am leery of the fact that the udev maintenance seems to have gone into some "crazy mode" where they have made changes that were known to be problematic, and are pure and utter stupidity. Having the module init path load the firmware IS THE SENSIBLE AND OBVIOUS THING TO DO for many cases. The fact that udev people have apparently unilaterally decided that it's somehow wrong is scary. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2 Oct 2012, Linus Torvalds wrote: > And see this email from Kay Sievers that shows that it was all known > about and intentional in the udev camp: > > http://www.spinics.net/lists/netdev/msg185742.html This seems confusing indeed. That e-mail referenced above is talking about loading firmware at ifup time. While that might work for network device drivers (I am not sure even about that), what are the udev maintainers advice for other drivers, where there is no analogy to ifup?
On Tue, Oct 2, 2012 at 5:01 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Tue, 2 Oct 2012, Linus Torvalds wrote: > >> And see this email from Kay Sievers that shows that it was all known >> about and intentional in the udev camp: >> >> http://www.spinics.net/lists/netdev/msg185742.html > > This seems confusing indeed. > > That e-mail referenced above is talking about loading firmware at ifup > time. While that might work for network device drivers (I am not sure even > about that), what are the udev maintainers advice for other drivers, where > there is no analogy to ifup? Yeah, it's an udev bug. It really is that simple. This is why I'm complaining. There's no way in hell we're fixing this in kernel space, unless we call the "bypass udev entirely because the maintainership quality of it has taken a nose dive". Yes, I've seen some work-around patches, but quite frankly, I think it would be absolutely insane for the kernel to work around the fact that udev is buggy. The fact is, doing request_firmware() from within module_init() is simply the easiest approach for some devices. Now, at the same time, I do agree that network devices should generally try to delay it until ifup time, so I'm not arguing against that part per se. I do think that when possible, people should aim to delay firmware loading until as late as reasonable. But as you point out, it's simply not always reasonable, and the media people are clearly hitting the cases where it's just painful. Now, those cases seem to be happily fairly *rare*, so this isn't getting a ton of attention, but we should fix it. Because the udev behavior is all pain, no gain. There's no *reason* for udev to be pissy about this. And it didn't use to be. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 02-10-2012 19:23, Greg KH escreveu: > On Tue, Oct 02, 2012 at 03:12:39PM -0700, Greg KH wrote: >> On Tue, Oct 02, 2012 at 09:33:03AM -0700, Linus Torvalds wrote: >>> I don't know where the problem started in udev, but the report I saw >>> was that udev175 was fine, and udev182 was broken, and would deadlock >>> if module_init() did a request_firmware(). That kind of nested >>> behavior is absolutely *required* to work, in order to not cause >>> idiotic problems for the kernel for no good reason. >>> >>> What kind of insane udev maintainership do we have? And can we fix it? >>> >>> Greg, I think you need to step up here too. You were the one who let >>> udev go. If the new maintainers are causing problems, they need to be >>> fixed some way. >> >> I've talked about this with Kay in the past (Plumbers conference I >> think) and I thought he said it was all fixed in the latest version of >> udev so there shouldn't be any problems anymore with this. >> >> Mauro, what version of udev are you using that is still showing this >> issue? >> >> Kay, didn't you resolve this already? If not, what was the reason why? > > Hm, in digging through the udev tree, the only change I found was this > one: > > commit 39177382a4f92a834b568d6ae5d750eb2a5a86f9 > Author: Kay Sievers <kay@vrfy.org> > Date: Thu Jul 19 12:32:24 2012 +0200 > > udev: firmware - do not cancel requests in the initrd > > diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c > index 56dc8fc..de93d7b 100644 > --- a/src/udev/udev-builtin-firmware.c > +++ b/src/udev/udev-builtin-firmware.c > @@ -129,7 +129,13 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo > err = -errno; > } while (err == -ENOENT); > rc = EXIT_FAILURE; > - set_loading(udev, loadpath, "-1"); > + /* > + * Do not cancel the request in the initrd, the real root might have > + * the firmware file and the 'coldplug' run in the real root will find > + * this pending request and fulfill or cancel it. > + * */ > + if (!in_initrd()) > + set_loading(udev, loadpath, "-1"); > goto exit; > } > > > which went into udev release 187 which I think corresponds to the place > when people started having problems, right Mauro? I'm using here udev-182. > If so, Mauro, is the solution just putting the firmware into the initrd? I don't think that putting firmware on initrd is something that we want to for media drivers. None of the webcam drivers currently need a firmware; those are required on more complex devices (typically digital TV ones). While there are a number of PCI devices that require firmware, in practice, we're seeing more people using USB devices. IMO, it doesn't make any sense that a hot-pluggable USB device to require a firmware at initrd/initramfs, with is available only at boot time. > No wait, it looks like this change was trying to fix the problem where > firmware files were not in the initrd, so it would stick around for the > real root to show up so that they could be loaded. > > So this looks like it was fixing firmware loading problems for people? I'll run some tests with this patch applied and see what happens. > Kay, am I just looking at the totally wrong place here, and this file in > udev didn't have anything to do with the breakage? > > thanks, > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 02-10-2012 19:47, Linus Torvalds escreveu: > On Tue, Oct 2, 2012 at 3:23 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >> >> which went into udev release 187 which I think corresponds to the place >> when people started having problems, right Mauro? > > According to what I've seen, people started complaining in 182, not 187. Yes. The issue was noticed with media drivers when people started using the drivers on Fedora 17, witch came with udev-182. There's an open bugzilla there: https://bugzilla.redhat.com/show_bug.cgi?id=827538 > See for example > > http://patchwork.linuxtv.org/patch/13085/ > > which is a thread where you were involved too.. > > See also the arch linux thread: > > https://bbs.archlinux.org/viewtopic.php?id=134012&p=1 > > And see this email from Kay Sievers that shows that it was all known > about and intentional in the udev camp: > > http://www.spinics.net/lists/netdev/msg185742.html > > There's a possible patch suggested here: > > http://lists.freedesktop.org/archives/systemd-devel/2012-August/006357.html > > but quite frankly, I am leery of the fact that the udev maintenance > seems to have gone into some "crazy mode" where they have made changes > that were known to be problematic, and are pure and utter stupidity. > > Having the module init path load the firmware IS THE SENSIBLE AND > OBVIOUS THING TO DO for many cases. Yes, that is the case for most media devices. Some devices can only be detected as a supported device after the firmware load, as we need the firmware for the USB (or PCI) bridge to be there, in order to talk with the media components under the board's internal I2C bus, as sometimes the same USB/PCI ID is used by boards with different internal components. > The fact that udev people have > apparently unilaterally decided that it's somehow wrong is scary. > Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 8:13 AM, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > > Yes. The issue was noticed with media drivers when people started using the > drivers on Fedora 17, witch came with udev-182. There's an open > bugzilla there: > https://bugzilla.redhat.com/show_bug.cgi?id=827538 Yeah, that bugzilla shows the problem with Kay as a maintainer too, not willing to own up to problems he caused. Can you actually see the problem? I did add the attached patch as an attachment to the bugzilla, so the reporter there may be able to test it, but it's been open for a long while.. Anyway. Attached is a really stupid patch that tries to do the "direct firmware load" as suggested by Ivan. It has not been tested very extensively at all (but I did test that it loaded the brcmsmac firmware images on my laptop so it has the *potential* to work). It has a few extra printk's sprinkled in (to show whether it does anything or not), and it has a few other issues, but it might be worth testing as a starting point. We are apparently better off trying to avoid udev like the plague. Doing something very similar to this for module loading is probably a good idea too. I'm adding Ming Lei to the participants too, because hooking into the firmware loader like this means that the image doesn't get cached. Which is sad. I'm hoping Ming Lei might be open to trying to fix that. Hmm? Linus
On Wed, Oct 3, 2012 at 9:38 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Anyway. Attached is a really stupid patch that tries to do the "direct > firmware load" as suggested by Ivan. It has not been tested very > extensively at all (but I did test that it loaded the brcmsmac > firmware images on my laptop so it has the *potential* to work). Oh, and I stupidly put the new functions next to the builtin firmware loading function, which means that the patch only works if you have CONFIG_FW_LOADER enabled. That's bogus, and the functions should be moved out of that #ifdef, but I don't think it should hurt testing. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 03, 2012 at 09:38:52AM -0700, Linus Torvalds wrote: > Yeah, that bugzilla shows the problem with Kay as a maintainer too, > not willing to own up to problems he caused. > > Can you actually see the problem? I did add the attached patch as an > attachment to the bugzilla, so the reporter there may be able to test > it, but it's been open for a long while.. > > Anyway. Attached is a really stupid patch that tries to do the "direct > firmware load" as suggested by Ivan. It has not been tested very > extensively at all (but I did test that it loaded the brcmsmac > firmware images on my laptop so it has the *potential* to work). + if (!S_ISREG(inode->i_mode)) + return false; + size = i_size_read(inode); Probably better to do vfs_getattr() and check mode and size in kstat; if it's sufficiently hot for that to hurt, we are fucked anyway. + file = filp_open(path, O_RDONLY, 0); + if (IS_ERR(file)) + continue; +printk("from file '%s' ", path); + success = fw_read_file_contents(file, fw); + filp_close(file, NULL); fput(file), please. We have enough misuses of filp_close() as it is... > We are apparently better off trying to avoid udev like the plague. > Doing something very similar to this for module loading is probably a > good idea too. That, or just adding usr/udev in the kernel git tree and telling the vertical integrators to go kiss a lamprey... -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > + if (!S_ISREG(inode->i_mode)) > + return false; > + size = i_size_read(inode); > > Probably better to do vfs_getattr() and check mode and size in kstat; if > it's sufficiently hot for that to hurt, we are fucked anyway. > > + file = filp_open(path, O_RDONLY, 0); > + if (IS_ERR(file)) > + continue; > +printk("from file '%s' ", path); > + success = fw_read_file_contents(file, fw); > + filp_close(file, NULL); > > fput(file), please. We have enough misuses of filp_close() as it is... Ok, like this? Linus
On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote: > On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > + if (!S_ISREG(inode->i_mode)) > > + return false; > > + size = i_size_read(inode); > > > > Probably better to do vfs_getattr() and check mode and size in kstat; if > > it's sufficiently hot for that to hurt, we are fucked anyway. > > > > + file = filp_open(path, O_RDONLY, 0); > > + if (IS_ERR(file)) > > + continue; > > +printk("from file '%s' ", path); > > + success = fw_read_file_contents(file, fw); > > + filp_close(file, NULL); > > > > fput(file), please. We have enough misuses of filp_close() as it is... > > Ok, like this? Looks sane. TBH, I'd still prefer to see udev forcibly taken over and put into usr/udev in kernel tree - I don't trust that crowd at all and the fewer critical userland bits they can play leverage games with, the safer we are. Al, that -><- close to volunteering for maintaining that FPOS kernel-side... -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 03, 2012 at 09:38:52AM -0700, Linus Torvalds wrote: >+static bool fw_get_filesystem_firmware(struct firmware *fw, const char *name) >+{ >+ int i; >+ bool success = false; >+ const char *fw_path[] = { "/lib/firmware/update", "/firmware", "/lib/firmware" }; >+ char *path = __getname(); >+ >+printk("Trying to load fw '%s' ", name); >+ for (i = 0; i < ARRAY_SIZE(fw_path); i++) { >+ struct file *file; >+ snprintf(path, PATH_MAX, "%s/%s", fw_path[i], name); >+ >+ file = filp_open(path, O_RDONLY, 0); AFAIK, accessing files on filesystem form kernel directly was no-go for a long time. What's the new rule here? Is it worth to introduce an execption, if it's possible to solve the problem in userspace.
On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote: > On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > + if (!S_ISREG(inode->i_mode)) > > + return false; > > + size = i_size_read(inode); > > > > Probably better to do vfs_getattr() and check mode and size in kstat; if > > it's sufficiently hot for that to hurt, we are fucked anyway. > > > > + file = filp_open(path, O_RDONLY, 0); > > + if (IS_ERR(file)) > > + continue; > > +printk("from file '%s' ", path); > > + success = fw_read_file_contents(file, fw); > > + filp_close(file, NULL); > > > > fput(file), please. We have enough misuses of filp_close() as it is... > > Ok, like this? This looks good to me. Having udev do firmware loading and tieing it to the driver model may have not been such a good idea so many years ago. Doing it this way makes more sense. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 12:48 PM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > AFAIK, accessing files on filesystem form kernel directly was no-go for a > long time. What's the new rule here? Oh, we've *always* accessed files from the kernel. What we don't want is random drivers doing so directly and without a good abstraction layer, because that just ends up being a total nightmare. Still, they've done that too. Ugh. Too many drivers having random hacks like that. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >> >> Ok, like this? > > This looks good to me. Having udev do firmware loading and tieing it to > the driver model may have not been such a good idea so many years ago. > Doing it this way makes more sense. Ok, I wish this had been getting more testing in Linux-next or something, but I suspect that what I'll do is to commit this patch asap, and then commit another patch that turns off udev firmware loading entirely for the synchronous firmware loading case. Why? Just to get more testing, and seeing if there are reports of breakage. Maybe some udev out there has a different search path (or because udev runs in a different filesystem namespace or whatever), in which case running udev as a fallback would otherwise hide the fact that he direct kernel firmware loading isn't working. We can (and will) revert things if that turns out to break things, but I'd like to make any failures of the firmware direct-load path be fast and hard, so that we can see when/what it breaks. Ok? Comments? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 10:39 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >>> >>> Ok, like this? >> >> This looks good to me. Having udev do firmware loading and tieing it to >> the driver model may have not been such a good idea so many years ago. >> Doing it this way makes more sense. > > Ok, I wish this had been getting more testing in Linux-next or > something, but I suspect that what I'll do is to commit this patch > asap, and then commit another patch that turns off udev firmware > loading entirely for the synchronous firmware loading case. > > Why? Just to get more testing, and seeing if there are reports of > breakage. Maybe some udev out there has a different search path (or > because udev runs in a different filesystem namespace or whatever), in > which case running udev as a fallback would otherwise hide the fact > that he direct kernel firmware loading isn't working. > Ok? Comments? The current udev directory search order is: /lib/firmware/updates/$(uname -r)/ /lib/firmware/updates/ /lib/firmware/$(uname -r)/ /lib/firmware/ There is no commonly known /firmware directory. http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/udev-builtin-firmware.c#n100 http://cgit.freedesktop.org/systemd/systemd/tree/configure.ac#n548 Kay -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 03, 2012 at 01:39:23PM -0700, Linus Torvalds wrote: > On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > >> > >> Ok, like this? > > > > This looks good to me. Having udev do firmware loading and tieing it to > > the driver model may have not been such a good idea so many years ago. > > Doing it this way makes more sense. > > Ok, I wish this had been getting more testing in Linux-next or > something, but I suspect that what I'll do is to commit this patch > asap, and then commit another patch that turns off udev firmware > loading entirely for the synchronous firmware loading case. > > Why? Just to get more testing, and seeing if there are reports of > breakage. Maybe some udev out there has a different search path (or > because udev runs in a different filesystem namespace or whatever), in > which case running udev as a fallback would otherwise hide the fact > that he direct kernel firmware loading isn't working. > > We can (and will) revert things if that turns out to break things, but > I'd like to make any failures of the firmware direct-load path be fast > and hard, so that we can see when/what it breaks. > > Ok? Comments? I have no objection to this. As for the firmware path, maybe we should change that to be modified by userspace (much like /sbin/hotplug was) in a proc file so that distros can override the location if they need to. But for now, that's probably overkill. This solves the problem that Mauro and others have reported and can be easily backported by any affected distros if needed. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg KH <gregkh@linuxfoundation.org> wrote: >On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote: >> On Wed, Oct 3, 2012 at 10:09 AM, Al Viro <viro@zeniv.linux.org.uk> >wrote: >> > >> > + if (!S_ISREG(inode->i_mode)) >> > + return false; >> > + size = i_size_read(inode); >> > >> > Probably better to do vfs_getattr() and check mode and size in >kstat; if >> > it's sufficiently hot for that to hurt, we are fucked anyway. >> > >> > + file = filp_open(path, O_RDONLY, 0); >> > + if (IS_ERR(file)) >> > + continue; >> > +printk("from file '%s' ", path); >> > + success = fw_read_file_contents(file, fw); >> > + filp_close(file, NULL); >> > >> > fput(file), please. We have enough misuses of filp_close() as it >is... >> >> Ok, like this? > >This looks good to me. Having udev do firmware loading and tieing it >to >the driver model may have not been such a good idea so many years ago. >Doing it this way makes more sense. > >greg k-h >-- >To unsubscribe from this list: send the line "unsubscribe linux-media" >in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html I agree that not calling out to userspace for firmware load is better. Here is an old, unresolved bug about Oops on firmware loading race condition https://bugzilla.kernel.org/show_bug.cgi?id=15294 The firmware loading timeout in the kernel was cleaning things up, just as udev what trying to say "I'm done loading the firmware" via sysfs; and then *boom*. Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 11:05 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > As for the firmware path, maybe we should > change that to be modified by userspace (much like /sbin/hotplug was) in > a proc file so that distros can override the location if they need to. If that's needed, a CONFIG_FIRMWARE_PATH= with the array of locations would probably be sufficient. Like udev's defaults here: http://cgit.freedesktop.org/systemd/systemd/tree/configure.ac#n550 Kay -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 3 Oct 2012 23:18:06 +0200 Kay Sievers <kay@vrfy.org> wrote: > On Wed, Oct 3, 2012 at 11:05 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > > As for the firmware path, maybe we should > > change that to be modified by userspace (much like /sbin/hotplug was) in > > a proc file so that distros can override the location if they need to. > > If that's needed, a CONFIG_FIRMWARE_PATH= with the array of locations > would probably be sufficient. The current system permits firmware to be served by a daemon, or even assembled on the fly from parts. You break that for one. Just fix udev, and if you can't fix it someone please just fork the last working one. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 5:39 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> wrote: >>> >>> Ok, like this? >> >> This looks good to me. Having udev do firmware loading and tieing it to >> the driver model may have not been such a good idea so many years ago. >> Doing it this way makes more sense. > > Ok, I wish this had been getting more testing in Linux-next or > something, but I suspect that what I'll do is to commit this patch > asap, and then commit another patch that turns off udev firmware > loading entirely for the synchronous firmware loading case. This would break non-udev users with different paths. Namely Android, which uses /system/etc/firmware and /system/vendor/firmware (not sure about them though, I'm not keen on Android camp) So maintaining the fallback or adding a configurable entry to set the firmware paths might be good. Lucas De Marchi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 3, 2012 at 2:58 PM, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > > So maintaining the fallback or adding a configurable entry to set the > firmware paths might be good. Yeah, I do think we need to make it configurable. Probably both at kernel compile time and dynamically. The aim of having a user-mode daemon do this was that it would be easy to configure. Sadly, if we can't trust the daemon, that is all totally worthless. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Torvalds <torvalds@linux-foundation.org> wrote: >On Wed, Oct 3, 2012 at 12:50 PM, Greg KH <gregkh@linuxfoundation.org> >wrote: >>> >>> Ok, like this? >> >> This looks good to me. Having udev do firmware loading and tieing it >to >> the driver model may have not been such a good idea so many years >ago. >> Doing it this way makes more sense. > >Ok, I wish this had been getting more testing in Linux-next or >something, but I suspect that what I'll do is to commit this patch >asap, and then commit another patch that turns off udev firmware >loading entirely for the synchronous firmware loading case. > >Why? Just to get more testing, and seeing if there are reports of >breakage. Maybe some udev out there has a different search path (or >because udev runs in a different filesystem namespace or whatever), in >which case running udev as a fallback would otherwise hide the fact >that he direct kernel firmware loading isn't working. > >We can (and will) revert things if that turns out to break things, but >I'd like to make any failures of the firmware direct-load path be fast >and hard, so that we can see when/what it breaks. > >Ok? Comments? > > Linus >-- >To unsubscribe from this list: send the line "unsubscribe linux-media" >in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html I don't know if you can remove the /sys/.../firmware ABI altogether, because there is at least one, somewhat popular udev replacement that also uses it: mdev http://git.busybox.net/busybox/plain/docs/mdev.txt Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Wed, 3 Oct 2012 13:39:23 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Ok, I wish this had been getting more testing in Linux-next or > something If you ever want a patch tested for a few days, just send it to me and I will put it in my "fixes" tree which is merged into linux-next immediately on top of your tree. If nothing else, that will give it wide build testing (see http://kisskb.ellerman.id.au/linux-next).
On Wed, Oct 3, 2012 at 3:48 PM, Andy Walls <awalls@md.metrocast.net> wrote: > > I don't know if you can remove the /sys/.../firmware ABI altogether, because there is at least one, somewhat popular udev replacement that also uses it: mdev > > http://git.busybox.net/busybox/plain/docs/mdev.txt Heh. That web doc documents /lib/firmware as being the place to be. That said, there's clearly enough variation here that I think that for now I won't take the step to disable the udev part. I'll do the patch to support "direct filesystem firmware loading" using the udev default paths, and that hopefully fixes the particular case people see with media modules. We definitely want to have configurable paths and a way to configure udev entirely off for firmware (together with a lack of paths configuring the direct filesystem loading off - that way people can set things up just the way they like), but since I'm travelling tomorrow and this clearly needs more work, I'll do the first step only for now.. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 Oct 2012, Al Viro spake thusly: > Looks sane. TBH, I'd still prefer to see udev forcibly taken over and put into > usr/udev in kernel tree - I don't trust that crowd at all and the fewer > critical userland bits they can play leverage games with, the safer we are. > > Al, that -><- close to volunteering for maintaining that FPOS kernel-side... <flamebait type="heartfelt" subtype="fever-induced"> Please! It has already been forked at least once in userspace by people who have the temerity to *not use systemd*, imagine that! and still want a udev that is up-to-date in other ways. (We are now being told that, contrary to what was said when udev was migrated into the systemd tree, running udev without systemd is now deprecated and untested and might go away completely. How surprising, nobody ever predicted that when it migrated in, oh wait yes we did, and were assured that we were wrong, that standalone udev would always be supported for those of us who weren't using systemd. Way to destroy your userbase's trust in you...) Possibly udev 175, the last standalone udev as far as I know, is a good place to start from: but you might want to go back a release or two before that, since 174 was where they started doing insane backward-compatibility breaks. By 175 they were requiring devtmpfs, no longer creating device nodes (which I thought was the original *point* of udev), moving lots of install locations on the assumption that / and /usr were on the same filesystem, and migrating udevd from /sbin into /lib/udev without bothering to provide a backward-compatibility symlink. Net benefit of all this thrashing about to udev users: nil. Net sysadmin overhead on upgrade: substantial, oh and if you don't do it your system won't boot. By udev 175 I, and a lot of other people, had simply stopped upgrading udev entirely on the grounds that we could no longer tolerate the uncertainty over whether our systems would boot every time we upgraded it, for no discernible benefit. Yes, all the incompatible changes are (or were, as of udev 175) called out in the release notes -- but there are so *many* of them, it's easy to miss one. And they all seem so completely unnecessary, and their implications for your system configuration grow more and more invasive all the time. When gregkh was maintaining udev it was nicely robust and kept working from release to release, and just did its job without requiring us to change the way our system booted in backwardly-incompatible ways on every release, merge filesystems together or mount /usr in early boot (which is SSH-tunneled over a network in the case of one of my systems, that was fun to do in the initramfs), or install invasive packages that extend tentacles throughout the entire system, require a complete rewriting of my boot process and require millions of kernel features that may not always be turned on. I'd like those days back. I can trust the kernel people to maintain some semblance of userspace compatibility between releases, as is crucial for boot-critical processes. It is now quite clear that I cannot trust the present udev maintainers, or anyone else involved in the ongoing Linux desktop trainwreck, to do any such thing. </flamebait>
On Wed, Oct 3, 2012 at 6:33 PM, Ming Lei <ming.lei@canonical.com> wrote: > > Yes, the patch will make firmware cache not working, I would like to fix > that when I return from one trip next week. > > BTW, firmware cache is still needed even direct loading is taken. I agree 100%, I'd have liked to do the caching for the direct-loading case too. It's just that the freeing case for that is so intimately tied to the firmware_buf format which is actually very inconvenient for direct-loading, that making that happen looked a lot more involved. And I was indeed hoping you'd look at it, since you touched the code last. "Tag, you're it" It shouldn't be *too* bad to instead of doing the "vmalloc()" allocate an array of pages and then using "vmap()" instead in order to read them (we end up doing the vmap anyway, since the firmware *user* wants a virtually contiguous buffer), but the code will definitely get a bit more opaque. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 4, 2012 at 12:58 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > That said, there's clearly enough variation here that I think that for > now I won't take the step to disable the udev part. I'll do the patch > to support "direct filesystem firmware loading" using the udev default > paths, and that hopefully fixes the particular case people see with > media modules. If that approach looks like it works out, please aim for full in-kernel-*only* support. I would absolutely like to get udev entirely out of the sick game of firmware loading here. I would welcome if we are not falling back to the blocking timeouted behaviour again. The whole story would be contained entirely in the kernel, and we get rid of the rather fragile "userspace transaction" to execute module_init(), where the kernel has no idea if userspace is even up to ever responding to its requests. There would be no coordination with userspace tools needed, which sounds like a better fit in the way we develop things with the loosely coupled kernel <-> udev requirements. If that works out, it would a bit like devtmpfs which turned out to be very simple, reliable and absolutely the right thing we could do to primarily mange /dev content. The whole dance with the fake firmware struct device, which has a 60 second timeout to wait for userspace, is a long story of weird failures at various aspects. It would not only solve the unfortunate modprobe lockup with init=/bin/sh we see here, also big servers with an insane amount of devices happen to run into the 60 sec timeout, because udev, which runs with 4000-8000 threads in parallel handling things like 30.000 disks is not scheduled in time to fulfill network card firmware requests. It would be nice if we don't have that arbitrary timeout at all. Having any timeout at all to answer the simple question if a file stored in the rootfs exists, should be a hint that there is something really wrong with the model that stuff is done. Thanks, Kay -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Kay removed because I don't like emailing arguable flamebait directly to the person flamed.] On 4 Oct 2012, nix@esperi.org.uk stated: > By udev 175 I, and a lot of other people, had simply stopped upgrading > udev entirely on the grounds that we could no longer tolerate the > uncertainty over whether our systems would boot every time we upgraded > it, for no discernible benefit. Yes, all the incompatible changes are > (or were, as of udev 175) called out in the release notes -- but there > are so *many* of them, it's easy to miss one. And they all seem so > completely unnecessary, and their implications for your system > configuration grow more and more invasive all the time. In the bright light of day I realize that this post is not as off-topic for this thread as it appears. This is all of a piece. The udev maintainer insists that everyone else adapt to udev's demands: before now, it has been users, sysadmins, and userspace who must adapt, but now is is pushing its demands in the other direction as well: this thread is the result.
On Wed, Oct 3, 2012 at 6:58 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Oct 3, 2012 at 3:48 PM, Andy Walls <awalls@md.metrocast.net> wrote: >> >> I don't know if you can remove the /sys/.../firmware ABI altogether, because there is at least one, somewhat popular udev replacement that also uses it: mdev >> >> http://git.busybox.net/busybox/plain/docs/mdev.txt > > Heh. That web doc documents /lib/firmware as being the place to be. > > That said, there's clearly enough variation here that I think that for > now I won't take the step to disable the udev part. I'll do the patch > to support "direct filesystem firmware loading" using the udev default > paths, and that hopefully fixes the particular case people see with > media modules. As you probably noticed, we had a tester in the RH bug report success with the commit you included yesterday. Do you think this is something worth including in the stable kernels after it gets some further testing during the merge window? Perhaps not that specific commit as there seems to be some additional changes needed for configurable paths, etc, but a backport of the fleshed out changeset might be wanted. We have a new enough udev in Fedora 17 to hit this issue with 3.5 and 3.6 when we rebase. I'm sure other distributions will be in similar circumstances soon if they aren't already. Udev isn't going to be fixed, so having something working in these cases would be great. josh -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 04, 2012 at 09:39:41AM -0400, Josh Boyer wrote: > > That said, there's clearly enough variation here that I think that for > > now I won't take the step to disable the udev part. I'll do the patch > > to support "direct filesystem firmware loading" using the udev default > > paths, and that hopefully fixes the particular case people see with > > media modules. > > As you probably noticed, we had a tester in the RH bug report success > with the commit you included yesterday. > > Do you think this is something worth including in the stable kernels > after it gets some further testing during the merge window? Perhaps > not that specific commit as there seems to be some additional changes > needed for configurable paths, etc, but a backport of the fleshed out > changeset might be wanted. > > We have a new enough udev in Fedora 17 to hit this issue with 3.5 and > 3.6 when we rebase. I'm sure other distributions will be in similar > circumstances soon if they aren't already. Udev isn't going to be > fixed, so having something working in these cases would be great. Yes, I don't have a problem taking this into the stable kernel releases once it gets some testing and fleshed out. I'll be watching it to see how it goes. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2 Oct 2012, Linus Torvalds wrote: > Now, at the same time, I do agree that network devices should generally > try to delay it until ifup time Slightly tangential to the ongoing discussion, but still ... I think that even "all network drivers should delay firmware loading to ifup time" might be too general. I would expect that there are network cards which require firmware to be present for PHY to work, right? On such cards, if you want to have link detection even on interfaces that are down (so that things like ifplugd can detect the link presence and configure the interface), ifup is too late. I admit I haven't checked whether there actually are such cards out there, but it doesn't seem to be completely unrealistic to me.
Kay Sievers <kay@vrfy.org> writes: > If that works out, it would a bit like devtmpfs which turned out to be > very simple, reliable and absolutely the right thing we could do to > primarily mange /dev content. ROFL. There are still quite a few interesting cases that devtmpfs does not even think about supporting. Cases that were reported when devtmpfs was being reviewed. Additionally the devtmpfs maintainership has not dealt with legitimate concerns any better than this firmware issue has been dealt with. I still haven't even hear a productive suggestion back on the hole /dev/ptmx mess. As it happens devtmpfs wound up being a userspace process that happens to reside in the kernel and call mknod. How it makes sense two layers of messaging and device management instead of just one I don't know. Certainly I would not crow about that being a success of anything except passing the buck. There is debacle written all over the user space interface for dealing with devices right now. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 04, 2012 at 10:29:51AM -0700, Eric W. Biederman wrote: > There are still quite a few interesting cases that devtmpfs does not > even think about supporting. Cases that were reported when devtmpfs was > being reviewed. Care to refresh my memory? > Additionally the devtmpfs maintainership has not dealt with legitimate > concerns any better than this firmware issue has been dealt with. I > still haven't even hear a productive suggestion back on the hole > /dev/ptmx mess. I don't know how to handle the /dev/ptmx issue properly from within devtmpfs, does anyone? Proposals are always welcome, the last time this came up a week or so ago, I don't recall seeing any proposals, just a general complaint. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I don't know how to handle the /dev/ptmx issue properly from within > devtmpfs, does anyone? Proposals are always welcome, the last time this > came up a week or so ago, I don't recall seeing any proposals, just a > general complaint. Is it really a problem - devtmpfs is optional. It's a problem for the userspace folks to handle and if they made it mandatory in their code diddums, someone better go fork working versions. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 4, 2012 at 9:17 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> I don't know how to handle the /dev/ptmx issue properly from within >> devtmpfs, does anyone? Proposals are always welcome, the last time this >> came up a week or so ago, I don't recall seeing any proposals, just a >> general complaint. > > Is it really a problem - devtmpfs is optional. It's a problem for the > userspace folks to handle and if they made it mandatory in their code > diddums, someone better go fork working versions. If only there was a viable alternative to udev. Distributions are being pushed around by the udev+systemd project precisely because of this reason; udev maintainers have said that udev on non-systemd systems is a dead end, so everyone that uses udev (everyone) is being forced to switch to systemd if they want to receive proper support, and at some point there might not be even a choice. I for one would like an alternative to both systemd and udev on my Linux systems, and as of yet, I don't know of one. Cheers.
On Wed, Oct 10, 2012 at 5:19 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote: > On Thu, Oct 4, 2012 at 9:17 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >>> I don't know how to handle the /dev/ptmx issue properly from within >>> devtmpfs, does anyone? Proposals are always welcome, the last time this >>> came up a week or so ago, I don't recall seeing any proposals, just a >>> general complaint. >> >> Is it really a problem - devtmpfs is optional. It's a problem for the >> userspace folks to handle and if they made it mandatory in their code >> diddums, someone better go fork working versions. > > If only there was a viable alternative to udev. > > Distributions are being pushed around by the udev+systemd project > precisely because of this reason; udev maintainers have said that udev > on non-systemd systems is a dead end, so everyone that uses udev > (everyone) is being forced to switch to systemd if they want to > receive proper support, and at some point there might not be even a > choice. > > I for one would like an alternative to both systemd and udev on my > Linux systems, and as of yet, I don't know of one. A few years ago, the OpenWRT people pointed me to hotplug2 when I mentioned udev made my poor m68k box with 12 MiB of RAM immediately go OOM. Don't know if it's suitable for "bigger" machines, though. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c index 56dc8fc..de93d7b 100644 --- a/src/udev/udev-builtin-firmware.c +++ b/src/udev/udev-builtin-firmware.c @@ -129,7 +129,13 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo err = -errno; } while (err == -ENOENT); rc = EXIT_FAILURE; - set_loading(udev, loadpath, "-1"); + /* + * Do not cancel the request in the initrd, the real root might have + * the firmware file and the 'coldplug' run in the real root will find + * this pending request and fulfill or cancel it. + * */ + if (!in_initrd()) + set_loading(udev, loadpath, "-1"); goto exit; }