Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 6:58 PM, Linus Torvalds torva...@linux-foundation.org wrote: On Wed, Oct 3, 2012 at 3:48 PM, Andy Walls awa...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
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. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 12:12 AM, Greg KH gre...@linuxfoundation.org wrote: 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? It's the same in the current release, we still haven't wrapped our head around how to fix it/work around it. Unlike what the heated and pretty uncivilized and rude emails here claim, udev does not dead-lock or break things, it's just slow. The modprobe event handling runs into a ~30 second event timeout. Everything is still fully functional though, there's only this delay. Udev ensures full dependency resolution between parent and child events. Parent events have to finish the event handling and have to return, before child event handlers are started. We need to ensure such things so that (among other things) disk events have finished their operations before the partition events are started, so they can rely and access their fully set up parent devices. What happens here is that the module_init() call blocks in a userspace transaction, creating a child event that is not started until the parent event has finished. The event handler for modprobe times out then the child event loads the firmware. Having kernel module relying on a running and fully functional userspace to return from module_init() is surely a broken driver model, at least it's not how things should work. If userspace does not respond to firmware requests, module_init() locks up until the firmware timeout happens. This all is not so much about how probe() should behave, it's about a fragile dependency on a specific userspace transaction to link a loadable module into the kernel. Drivers should avoid such loops for many reasons. Also, it's unclear in many cases how such a model should work at all if the module is compiled in and initialized when no userspace is running. If that unfortunate module_init() lockup can't be solved properly in the kernel, we need to find out if we need to make the modprobe handling in udev async, or let firmware events bypass dependency resolving. As mentioned, we haven't decided as of now which road to take here. Thanks, Kay -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 7:36 AM, Kay Sievers k...@vrfy.org wrote: If that unfortunate module_init() lockup can't be solved properly in the kernel Stop this idiocy. The kernel doesn't have a lockup problem. udev does. As even you admit, it is *udev* that has the whole serialization issue, and does excessive (and incorrect) serialization between events. Resulting in the kernel timing out a udev event that takes too long. The fact that you then continually try to make this a kernel issue is just disgusting. Talking about the kernel solving it properly is pretty dishonest, when the kernel wasn't the problem in the first place. The kernel not only does things right, but this all used to work fine. Linus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Em 02-10-2012 19:47, Linus Torvalds escreveu: On Tue, Oct 2, 2012 at 3:23 PM, Greg KH gre...@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=134012p=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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 8:13 AM, Mauro Carvalho Chehab mche...@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 patch.diff Description: Binary data
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 03, 2012 at 04:36:53PM +0200, Kay Sievers wrote: On Wed, Oct 3, 2012 at 12:12 AM, Greg KH gre...@linuxfoundation.org wrote: 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? It's the same in the current release, we still haven't wrapped our head around how to fix it/work around it. Ick, as this is breaking people's previously-working machines, shouldn't this be resolved quickly? Unlike what the heated and pretty uncivilized and rude emails here claim, udev does not dead-lock or break things, it's just slow. The modprobe event handling runs into a ~30 second event timeout. Everything is still fully functional though, there's only this delay. Mauro said it broke the video drivers. Mauro, if you wait 30 seconds, does everything then work? Not to say that waiting 30 seconds is a correct thing here... Udev ensures full dependency resolution between parent and child events. Parent events have to finish the event handling and have to return, before child event handlers are started. We need to ensure such things so that (among other things) disk events have finished their operations before the partition events are started, so they can rely and access their fully set up parent devices. What happens here is that the module_init() call blocks in a userspace transaction, creating a child event that is not started until the parent event has finished. The event handler for modprobe times out then the child event loads the firmware. module_init() can do lots of bad things, sleeping, asking for firmware, and lots of other things. To have userspace block because of this doesn't seem very wise. Having kernel module relying on a running and fully functional userspace to return from module_init() is surely a broken driver model, at least it's not how things should work. If userspace does not respond to firmware requests, module_init() locks up until the firmware timeout happens. But previously this all just worked as we ran 'modprobe' in a new thread/process right? What's wrong with going back to just execing modprobe and letting that process go off and do what ever it wants to do? It can't be that expensive as modprobe is a very slow thing, and it should solve this issue. udev will then have handled the 'a device has shown up, run modprobe' event in the correct order, and then anything else that the module_init() process wants to do, it can do without worrying about stopping anything else in the system that might want to happen at the same time (like load multiple modules in a row). This all is not so much about how probe() should behave, it's about a fragile dependency on a specific userspace transaction to link a loadable module into the kernel. Drivers should avoid such loops for many reasons. Also, it's unclear in many cases how such a model should work at all if the module is compiled in and initialized when no userspace is running. If that unfortunate module_init() lockup can't be solved properly in the kernel, we need to find out if we need to make the modprobe handling in udev async, or let firmware events bypass dependency resolving. As mentioned, we haven't decided as of now which road to take here. It's not a lockup, there have never been rules about what a driver could and could not do in its module_init() function. Sure, there are some not-nice drivers out there, but don't halt the whole system just because of them. I recommend making module loading async, like it used to be, and then all should be fine, right? That's also the way the mdev works, and I don't think that people have been having problems there. :) thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 9:38 AM, Linus Torvalds torva...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 6:57 PM, Greg KH gre...@linuxfoundation.org wrote: It's the same in the current release, we still haven't wrapped our head around how to fix it/work around it. Ick, as this is breaking people's previously-working machines, shouldn't this be resolved quickly? Nothing really breaks, It's slow and it will surely be fixed when we know what's the right fix, which we haven't sorted out at this moment. module_init() can do lots of bad things, sleeping, asking for firmware, and lots of other things. To have userspace block because of this doesn't seem very wise. Not saying that it is right or nice, but it's the kernel itself that blocks. Run init=/bin/sh and do a modprobe of one of these drivers and it hangs un-interruptible until the kernel's internal firmware loading request times out, just because userspace is not there. But previously this all just worked as we ran 'modprobe' in a new thread/process right? No, we used to un-queue events which had a timeout specified in the environment, that code caused other issues and was removed. it can do without worrying about stopping anything else in the system that might want to happen at the same time (like load multiple modules in a row). It should not be an issue, the serialization is strictly parent - child, everything else runs in parallel. If that unfortunate module_init() lockup can't be solved properly in the kernel, we need to find out if we need to make the modprobe handling in udev async, or let firmware events bypass dependency resolving. As mentioned, we haven't decided as of now which road to take here. It's not a lockup, there have never been rules about what a driver could and could not do in its module_init() function. Sure, there are some not-nice drivers out there, but don't halt the whole system just because of them. It is a kind of lock up, just try modprobe with the init=/bin/sh boot. I recommend making module loading async, like it used to be, and then all should be fine, right? That's the current idea, and Tom is looking into it how it could look like. I also have no issues at all if the kernel does load the firmware from the filesystem on its own; it sounds like the simplest and most robust solution from a general look at the problem. It would also make the difference between in-kernel firmware and out-of-kernel firmware less visible, which sounds good. Honestly, requiring firmware-loading userspace-transactions to successfully link a module into the kernel sounds like a pretty bad idea to start with. Unlike module loading which needs the depmod alias database and userspace configuration; with firmware loading, there is no policy involved where userspace would add any single additional value to that step. Thanks, Kay -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 10:09 AM, Al Viro v...@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 patch.diff Description: Binary data
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 10:24 AM, Kay Sievers k...@vrfy.org wrote: Nothing really breaks, It's slow and it will surely be fixed when we know what's the right fix, which we haven't sorted out at this moment. A thirty-second pause at bootup is easily long enough that some people might think the machine is hung. I also call bullshit on your it will surely be fixed when we know what's the right fix excuses. The fact is, you've spent the last several months blaming everybody but yourself, and actively told people to stop blaming you: https://bugzilla.redhat.com/show_bug.cgi?id=827538#c12 and have ignored patches that were sent to you: http://lists.freedesktop.org/archives/systemd-devel/2012-August/006357.html despite having clearly seen the patch (you *replied* to it, for chissake, and I even told you in that same thread why that reply was wrong at the time). I also have no issues at all if the kernel does load the firmware from the filesystem on its own; it sounds like the simplest and most robust solution from a general look at the problem. It would also make the difference between in-kernel firmware and out-of-kernel firmware less visible, which sounds good. So now, after you've dismissed the patch that did the equivalent fix in udev (Ming Lei's patch basically disabled your idiotic and wrong sequence number test for firmware loading), you say it's ok to bypass udev entirely, because that is more robust. Kay, you are so full of sh*t that it's not funny. You're refusing to acknowledge your bugs, you refuse to fix them even when a patch is sent to you, and then you make excuses for the fact that we have to work around *your* bugs, and say that we should have done so from the very beginning. Yes, doing it in the kernel is more robust. But don't play games, and stop the lying. It's more robust because we have maintainers that care, and because we know that regressions are not something we can play fast and loose with. If something breaks, and we don't know what the right fix for that breakage is, we *revert* the thing that broke. So yes, we're clearly better off doing it in the kernel. Not because firmware loading cannot be done in user space. But simply because udev maintenance since Greg gave it up has gone downhill. Linus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote: On Wed, Oct 3, 2012 at 10:09 AM, Al Viro v...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Em 03-10-2012 13:57, Greg KH escreveu: On Wed, Oct 03, 2012 at 04:36:53PM +0200, Kay Sievers wrote: On Wed, Oct 3, 2012 at 12:12 AM, Greg KH gre...@linuxfoundation.org wrote: 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? It's the same in the current release, we still haven't wrapped our head around how to fix it/work around it. Ick, as this is breaking people's previously-working machines, shouldn't this be resolved quickly? Unlike what the heated and pretty uncivilized and rude emails here claim, udev does not dead-lock or break things, it's just slow. The modprobe event handling runs into a ~30 second event timeout. Everything is still fully functional though, there's only this delay. Mauro said it broke the video drivers. Mauro, if you wait 30 seconds, does everything then work? Not to say that waiting 30 seconds is a correct thing here... Before the implementation of the DVB async firmware load, as requested by udev maintainers, and on a test case, the additional 30 seconds wait won't hurt[1]. [1] Well, I didn't test any tm6000 devices with Kernel 3.6. On tm6000, firmwares are needed for xc3028 tuners, via the board internal I2C bus. Those devices only support 10 kHz speed for those transfers. Also, due to a bug at the hardware, extra delay is needed. So, a firmware load there takes ~30 seconds. I suspect that waiting for ~60 seconds will trigger the max allowed time for firmware load to happen, actually breaking completely the driver. There are applications like MythTV, however, that opens all interfaces produced by a media device at the same time (alsa, video nodes, dvb nodes). I'm not a MythTV user myself, but someone told me recently that MythTV has a 5 seconds timeout there. If it can't open an interface on that time, it will give up for that device. As you likely know, MythTV is used as a Media Center: there are distros that load it (or some other applications like XBMC) at boot time. As the media drivers are modular, the DVB, alsa and TV API's are implemented on (almost) independent modules. On such scenario, if the DVB firmware load takes 30 seconds, because the DVB demod requires a firmware, while the analog TV doesn't require a firmware, I suspect that Mythtv will assume that DVB is not there, causing a failure to the end user. [1] The attempt to fix the issues with udev forced us to re-design a few drivers. One of them (drxk) now loads firmware asynchronously. That effectively broke the driver on Kernel 3.6, for PCTV520e and similar devices, as the tuner driver, attached just after drx-k, stopped working, likely due to some missing clock or GPIO signals that drx-k is supposed to rise after firmware load. The fixes for it are at: http://git.linuxtv.org/media_tree.git/commit/6ae5e060840589f567c1837613e8a9d34fc9188a http://git.linuxtv.org/media_tree.git/commit/8e30783b0b3270736b2cff6415c68b894bc411df http://git.linuxtv.org/media_tree.git/commit/2425bb3d4016ed95ce83a90b53bd92c7f31091e4 In order to fix that issues, I had to defer DVB initialization for all em28xx devices, in despite of the fact that only 5 boards (of 86 devices supported there) require DVB firmwares. Patches are likely a little pedantic, as they are doing async DVB initialization even when the drivers are compiled builtin, but, as I do expect to revert the entire async thing from em28xx/drx-k, I didn't want to make too big changes there. FYI, I'm planning to upstream those fixes tomorrow, if nobody detects any issue on this approach. I recommend making module loading async, like it used to be, and then all should be fine, right? IMHO, module loading should be async: it seems a way better and more POSIX compliant to take more time during init, than delaying firmware init to happen open(), especially when open() is called on non-block mode. Regards, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 03, 2012 at 10:32:08AM -0700, Linus Torvalds wrote: On Wed, Oct 3, 2012 at 10:09 AM, Al Viro v...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 12:50 PM, Greg KH gre...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 10:39 PM, Linus Torvalds torva...@linux-foundation.org wrote: On Wed, Oct 3, 2012 at 12:50 PM, Greg KH gre...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 03, 2012 at 01:39:23PM -0700, Linus Torvalds wrote: On Wed, Oct 3, 2012 at 12:50 PM, Greg KH gre...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Greg KH gre...@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 v...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 11:05 PM, Greg KH gre...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, 3 Oct 2012 23:18:06 +0200 Kay Sievers k...@vrfy.org wrote: On Wed, Oct 3, 2012 at 11:05 PM, Greg KH gre...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 5:39 PM, Linus Torvalds torva...@linux-foundation.org wrote: On Wed, Oct 3, 2012 at 12:50 PM, Greg KH gre...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Tue, Oct 2, 2012 at 7:37 PM, Linus Torvalds torva...@linux-foundation.org wrote: On Tue, Oct 2, 2012 at 2:03 PM, Ivan Kalvachev ikalvac...@gmail.com wrote: I'm not kernel developer and probably my opinion would be a little naive, but here it is. Please, make the kernel load firmware from the filesystem on its own. We probably should do that, not just for firmware, but for modules too. It would also simplify the whole built-in firmware thing Afaik, the only thing udev really does is to lok in /lib/firmware/updates and /lib/firmware for the file, load it, and pass it back to the kernel. We could make the kernel try to do it manually first, and only fall back to udev if that fails. Afaik, nobody ever does anything else anyway. I'd prefer to not have to do that, but if the udev code is buggy or the maintainership is flaky, I guess we don't really have much choice. Doing the same thing for module loading is probably a good idea too. humn... I don't think so. It would work perfectly well for firmware, but for modules there are more things involved like fulfilling dependencies, soft-dependencies, aliases and the like. It would create several regressions. There were already people (from the google/Android camp) who wanted to do module loading based on filename rather than the buffer passed to it, because they have a I trust this filesystem model. They wanted to pass a fd instead of a buffer. That is being done in the new finit_module syscall being discussed: http://www.gossamer-threads.com/lists/linux/kernel/1592271?do=post_view_flat Lucas De Marchi -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 2:58 PM, Lucas De Marchi lucas.de.mar...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Linus Torvalds torva...@linux-foundation.org wrote: On Wed, Oct 3, 2012 at 12:50 PM, Greg KH gre...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Hi Linus, On Wed, 3 Oct 2012 13:39:23 -0700 Linus Torvalds torva...@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). -- Cheers, Stephen Rothwells...@canb.auug.org.au pgpuyFMHBy6Dt.pgp Description: PGP signature
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 3:48 PM, Andy Walls awa...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Wed, Oct 3, 2012 at 6:33 PM, Ming Lei ming@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Thu, Oct 4, 2012 at 12:58 AM, Linus Torvalds torva...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Hi Greg, Em Mon, 25 Jun 2012 22:55:41 -0300 Mauro Carvalho Chehab mche...@redhat.com escreveu: Em 25-06-2012 19:33, Greg KH escreveu: On Mon, Jun 25, 2012 at 05:49:25PM -0300, Mauro Carvalho Chehab wrote: Greg, Basically, the recent changes at request_firmware() exposed an issue that affects all media drivers that use firmware (64 drivers). What change was that? How did it break anything? https://bugzilla.redhat.com/show_bug.cgi?id=827538 Basically, userspace changes and some pm-related patches, ... ... As I pinged you back in June, the recent changes on udev made lots of regression on all media drivers that require firmware. The news is that the fixes are also causing more regressions... Btw, Linus also complained about that: https://lkml.org/lkml/2012/8/25/136 I basically tried a few different approaches, including deferred probe(), as you suggested, and request_firmware_async(), as Kay suggested. The deferred probe() doesn't work as-is, as it will re-probe the driver only if a new driver is modprobed. It should likely not be that hard to modify the drivers core to allow it to re-probe after returning the probe status to udev, but I failed to find a way of doing that (mostly because of the lack of time, as I'm being too busy those days). I tried a few other things, like using request_firmware_async() at a few drivers that require firmware. That caused a big regression on Kernel 3.6 that we're currently discussing how to fix, but basically, media devices are very complex, as they have lots of different blocks there: tuners, analog video demod, digital video demod, audio demod, etc. On most cases, each of those extra blocks are implemented via I2C, and the device initialization happens serialized. By letting an I2C driver to do asynchronous initialization, other dependent drivers broke, because there are some signals used by the other blocks that are dependent on a proper initialization of a block previously initialized. One of the specific case is a device very popular in Europe nowadays: the PCTV 520e. This device uses an em28xx USB bridge chip, a tda18271 i2c tuner and a drx-k DVB i2c demod (among other things). The DRX-K chipset requires a firmware; the other chipsets there don't. The drx-k driver is used in combination with several other drivers, like em28xx, az6027, xc5000, tda18271, tda18271dd, etc. On my tests, with the devices I have available (Terratec H5, H6, H7, HTC; Hauppauge HVR 530C), the usage of request_firmware_async() worked as expected. So, such patch that made DRX-K firmware load to be deferred were applied at Kernel 3.6 on this changeset: commit bd02dbcd008f92135b2c7a92b6 Author: Mauro Carvalho Chehab mche...@redhat.com Date: Thu Jun 21 09:36:38 2012 -0300 [media] drxk: change it to use request_firmware_nowait() The firmware blob may not be available when the driver probes. Instead of blocking the whole kernel use request_firmware_nowait() and continue without firmware. This shouldn't be that bad on drx-k devices, as they all seem to have an internal firmware. So, only the firmware update will take a little longer to happen. Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com It should be noticed that, for the DVB demodulation to work, the DVB demod should be attached with the tuner module, via this code snippet[1]: fe = drxk_attach(pctv_520e_drxk_cfg, i2c_adap); tda18271_attach(fe, i2c_tuner_address, i2c_adap, pctv_520e_tda18271_cfg); The evil are in the details: the DRX-K chip produces some clocks needed by the tuner, and it has some generic GPIO pins that allow to control random things, with are device-dependent. Also, there is an I2C switch at the board, used to control I2C access to the tuner: when the switch is off, the tuner is not visible at the I2C bus[2]. The tda18271 tuner driver needs to read one device register, in order to identify the chipset version, as there are a few different setups there, depending if the silicon is version 1 or 2. As this driver doesn't require any firmware, such read happens at driver's attach time, with should be fine. Well, before the patch: 1) drxk_attach() would synchronously read the firmware and initialize the device, setting the needed GPIO pins and clock signals; 2) tda18271_attach() would read the version ID register and initialize the device. However, after the patch, drxk_attach() does nothing but filling some internal data structures and defer the device init job to happen after the firmware load. So, when tda18271_attach() is called, there wasn't enough time yet to initialize the DRX-K device. The end result is that the tda18271 version is not identified and the I2C driver read fails: tda18271_read_regs: [5-0060|M] ERROR: i2c_transfer returned: -19 Unknown device (16) detected @ 5-0060, device not supported. Ok, there are
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Tue, Oct 2, 2012 at 6:03 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: I basically tried a few different approaches, including deferred probe(), as you suggested, and request_firmware_async(), as Kay suggested. Stop this crazy. FIX UDEV ALREADY, DAMMIT. Who maintains udev these days? Is it Lennart/Kai, as part of systemd? Lennart/Kai, fix the udev regression already. Lennart was the one who brought up kernel ABI regressions at some conference, and if you now you have the *gall* to break udev in an incompatible manner that requires basically impossible kernel changes for the kernel to fix the udev interface, I don't know what to say. Two-faced lying weasel would be the most polite thing I could say. But it almost certainly will involve a lot of cursing. However, for 3.7 or 3.8, I think that the better is to revert changeset 177bc7dade38b5 and to stop with udev's insanity of requiring asynchronous firmware load during device driver initialization. If udev's developers are not willing to do that, we'll likely need to add something at the drivers core to trick udev for it to think that the modules got probed before the probe actually happens. The fact is, udev made new - and insane - rules that are simply *invalid*. Modern udev is broken, and needs to be fixed. 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. Linus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On 10/2/12, Linus Torvalds torva...@linux-foundation.org wrote: On Tue, Oct 2, 2012 at 6:03 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: I basically tried a few different approaches, including deferred probe(), as you suggested, and request_firmware_async(), as Kay suggested. Stop this crazy. FIX UDEV ALREADY, DAMMIT. Who maintains udev these days? Is it Lennart/Kai, as part of systemd? Lennart/Kai, fix the udev regression already. Lennart was the one who brought up kernel ABI regressions at some conference, and if you now you have the *gall* to break udev in an incompatible manner that requires basically impossible kernel changes for the kernel to fix the udev interface, I don't know what to say. Two-faced lying weasel would be the most polite thing I could say. But it almost certainly will involve a lot of cursing. However, for 3.7 or 3.8, I think that the better is to revert changeset 177bc7dade38b5 and to stop with udev's insanity of requiring asynchronous firmware load during device driver initialization. If udev's developers are not willing to do that, we'll likely need to add something at the drivers core to trick udev for it to think that the modules got probed before the probe actually happens. The fact is, udev made new - and insane - rules that are simply *invalid*. Modern udev is broken, and needs to be fixed. 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'm not kernel developer and probably my opinion would be a little naive, but here it is. Please, make the kernel load firmware from the filesystem on its own. This should solve almost 99.9% of the problems related to firmware loading. I don't mind if there is still userland component that could be used to request a firmware from repository. You can even keep the udev userland piping as a fallback if you want, but I think you can simplify a lot of code if you phase it out. The firmware loading should follow the same concept as modules loading. I've heard that the udev userland piping of firmware is done to avoid some licensing issues. But honestly, if you can not store the firmware on the user's disk, no free operating system should support it at all. This piping thing have been feature creep that have metastasized all over the kernel while keeping a lot of obscure modules broken (in hard to find way) and requiring increasingly complicated schemes to workaround its flaws. KiSS. Best Regards Ivan Kalvachev -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
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? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
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 k...@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? If so, Mauro, is the solution just putting the firmware into the initrd? 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? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Tue, Oct 2, 2012 at 2:03 PM, Ivan Kalvachev ikalvac...@gmail.com wrote: I'm not kernel developer and probably my opinion would be a little naive, but here it is. Please, make the kernel load firmware from the filesystem on its own. We probably should do that, not just for firmware, but for modules too. It would also simplify the whole built-in firmware thing Afaik, the only thing udev really does is to lok in /lib/firmware/updates and /lib/firmware for the file, load it, and pass it back to the kernel. We could make the kernel try to do it manually first, and only fall back to udev if that fails. Afaik, nobody ever does anything else anyway. I'd prefer to not have to do that, but if the udev code is buggy or the maintainership is flaky, I guess we don't really have much choice. Doing the same thing for module loading is probably a good idea too. There were already people (from the google/Android camp) who wanted to do module loading based on filename rather than the buffer passed to it, because they have a I trust this filesystem model. I've heard that the udev userland piping of firmware is done to avoid some licensing issues. No, I think it was mainly a combination of - some people like the whole let's do things in user land model even when it makes things more complicated - we do tend to try to punt policy issues to user space, and the whole /lib/firmware location is an example of such a policy issue. along with the fact that we already had the hotplug model for these kinds of things (eg module loading used to actually have a big user space component that did the whole relocation etc, so we had real historical reasons to do that in user space) Does anybody want to try to cook up a patch, leaving the udev path as a fallback? We already have the case of builtin firmware as one option, this would go after that.. Linus -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Tue, Oct 2, 2012 at 3:23 PM, Greg KH gre...@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=134012p=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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
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? -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: udev breakages - was: Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Tue, Oct 2, 2012 at 5:01 PM, Jiri Kosina jkos...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Hi Mauro, On Mon, 25 Jun 2012 17:06:28 -0300, Mauro Carvalho Chehab wrote: That's said, IMO, the best approach is to do: 1) add support for asynchronous probe at device core, for devices that requires firmware at probe(). The async_probe() will only be active if !usermodehelper_disabled. 2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface. Both functions are already exported since kernel 2.6.32. However these functions were originally made public for the shared pin case, where two pins can be used for either I2C or something else, and we have to prevent I2C usage when the other function is used. This does not help in your case. What you need additionally is that unlocked flavors of some i2c transfer functions (at least i2c_transfer itself) are exported as well. This isn't necessarily trivial though, as the locking and unlocking are taking place inside i2c_transfer(), not at its boundaries. I'm looking into this now. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote: Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu: The firmware blob may not be available when the driver probes. Instead of blocking the whole kernel use request_firmware_nowait() and continue without firmware. This shouldn't be that bad on drx-k devices, as they all seem to have an internal firmware. So, only the firmware update will take a little longer to happen. While thinking on converting another DVB frontend driver, I just realized that a patch like that won't work fine. As most of you know, there are _several_ I2C chips that don't tolerate any usage of the I2C bus while a firmware is being loaded (I dunno if this is the case of drx-k, but I won't doubt). The current approach makes the device probe() logic is serialized. So, there's no chance that two different I2C drivers to try to access the bus at the same time, if the bridge driver is properly implemented. However, now that firmware is loaded asynchronously, several other I2C drivers may be trying to use the bus at the same time. So, events like IR (and CI) polling, tuner get_status, etc can happen during a firmware transfer, causing the firmware to not load properly. A fix for that will require to lock the firmware load I2C traffic into a single transaction. How about deferring registration or probe of every bus-interface (usb, pci, firewire) drivers we have. If we defer interface driver using work or some other trick we don't need to touch any other chip-drivers that are chained behind interface driver. Demodulator, tuner, decoder, remote and all the other peripheral drivers can be left as those are currently because those are deferred by bus interface driver. regards Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Em 25-06-2012 16:15, Antti Palosaari escreveu: On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote: Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu: The firmware blob may not be available when the driver probes. Instead of blocking the whole kernel use request_firmware_nowait() and continue without firmware. This shouldn't be that bad on drx-k devices, as they all seem to have an internal firmware. So, only the firmware update will take a little longer to happen. While thinking on converting another DVB frontend driver, I just realized that a patch like that won't work fine. As most of you know, there are _several_ I2C chips that don't tolerate any usage of the I2C bus while a firmware is being loaded (I dunno if this is the case of drx-k, but I won't doubt). The current approach makes the device probe() logic is serialized. So, there's no chance that two different I2C drivers to try to access the bus at the same time, if the bridge driver is properly implemented. However, now that firmware is loaded asynchronously, several other I2C drivers may be trying to use the bus at the same time. So, events like IR (and CI) polling, tuner get_status, etc can happen during a firmware transfer, causing the firmware to not load properly. A fix for that will require to lock the firmware load I2C traffic into a single transaction. How about deferring registration or probe of every bus-interface (usb, pci, firewire) drivers we have. If we defer interface driver using work or some other trick we don't need to touch any other chip-drivers that are chained behind interface driver. Demodulator, tuner, decoder, remote and all the other peripheral drivers can be left as those are currently because those are deferred by bus interface driver. There are some issues with regards to it: 1) Currently, driver core doesn't allow a deferred probe. Drivers might implement that, but they'll lie to the core that the driver were properly supported even when probe fails. So, driver's core need an .async_probe() method; 2) The firmware load issue will still happen at resume. So, a lock like that is still needed; 3) It can make some sense to async load the firmware for some drivers, especially when the device detection may not be dependent on a firmware load. I'm not fully convinced about (3), as the amount of changes are significant for not much gain. There's also another related issue: On devices where both bridge and sub-devices (like frontend) needs firmware to be loaded, the load order is important at resume(), as the bridge requires to get the firmware before the sub-devices. That's said, IMO, the best approach is to do: 1) add support for asynchronous probe at device core, for devices that requires firmware at probe(). The async_probe() will only be active if !usermodehelper_disabled. 2) export the I2C i2c_lock_adapter()/i2c_unlock_adapter() interface. We can postpone or get rid of changing the I2C drivers to use request_firmware_async(), if the request_firmware() core is not pedantic enough to complain and it is not gonna to be deprecated. Anyway, I'll open a thead c/c Greg KH (driver's core maintainer) with regards to the need of an async_probe() callback. Regards, Mauro regards Antti -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Greg,
Basically, the recent changes at request_firmware() exposed an issue that
affects all media drivers that use firmware (64 drivers).
Driver's documentation at Documentation/driver-model/driver.txt says that the
.probe() callback should bind the driver to a given device. That includes
verifying that the device is present, that it's a version the driver can
handle,
that driver data structures can be allocated and initialized, and that any
hardware can be initialized.
All media device drivers are complaint with that, returning 0 on success
(meaning that the device was successfully probed) or an error otherwise.
Almost all media devices are made by a SoC or a RISC CPU that works
as a DMA engine and exposes a set of registers to allow I2C access to the
device's internal and/or external I2C buses. Part of them have an internal
EEPROM/ROM that stores firmware internally at the board, but others require
a firmware to be loaded before being able to init/control the device and to
export the I2C bus interface.
The media handling function is then implemented via a series of I2C devices[1]:
- analog video decoders;
- TV tuners;
- radio tuners;
- I2C remote controller decoders;
- DVB frontends;
- mpeg decoders;
- mpeg encoders;
- video enhancement devices;
...
[1] several media chips have part of those function implemented internally,
but almost all require external I2C components to be probed.
In order to properly refer to each component, we call the main kernel module
that talks with the media device via USB/PCI bus is called bridge driver,
and the I2C components are called as sub-devices.
Different vendors use the same bridge driver to work with different sub-devices.
It is a .probe()'s task to detect what sub-devices are there inside the board.
There are several cases where the vendor switched the sub-devices without
changing the PCI ID/USB ID.
So, drivers do things like the code below, inside the .probe() callback:
static int check_if_dvb_frontend_is_supported_and_bind()
{
switch (device_model) {
case VENDOR_A_MODEL_1:
if (test_and_bind_frontend_1()) /* Doesn't require firmware */
return 0;
if (test_and_bind_frontend_2()) /* requires firmware foo */
return 0;
if (test_and_bind_frontend_3()) /* requires firmware bar */
return 0;
if (test_and_bind_frontend_4()) /* doesn't require firmware */
return 0;
break;
case VENDOR_A_MODEL_2:
/* Analog device - no DVB frontend on it */
return 0;
...
}
return -ENODEV;
}
On several devices, before being able to register the bus and do the actual
probe, the kernel needs to load a firmware.
Also, during the I2C device probing time, firmware may be required, in order
to properly expose the device's internal models and their capabilities.
For example, drx-k sub-device can have support for either DVB-C or DVB-T or
both,
depending on the device model. That affects the frontend properties exposed
to the user and might affect the bridge driver's initialization task.
In practice, a driver like em28xx have a few devices like HVR-930C that require
the drx-k sub-device. For those devices, a firmware is required; for other
devices, a firmware is not required.
What's happening is that newer versions of request_firmware and udev are being
more pedantic (for a reason) about not requesting firmwares during module_init
or PCI/USB register's probe callback.
Worse than that, the same device driver may require a firmware or not,
depending on
the I2C devices inside it. One such example is em28xx: for the great majority of
the supported devices, no firmware is needed, but devices like HVR-930C require
a firmware, because it uses a frontend that needs firmware.
After some discussions, it seems that the best model would be to add an
async_probe()
callback to be used by devices similar to media ones. The async_probe() should
be
not probed during the module_init; the probe() will be deferred to happen later,
when firmware's usermodehelper_disabled is false, allowing those drivers to
load their
firmwares if needed.
What do you think?
Regards,
Mauro
Em 25-06-2012 17:06, Mauro Carvalho Chehab escreveu:
Em 25-06-2012 16:15, Antti Palosaari escreveu:
On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.
While thinking on converting another DVB frontend driver, I just
Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Greg,
Basically, the recent changes at request_firmware() exposed an issue that
affects all media drivers that use firmware (64 drivers).
Driver's documentation at Documentation/driver-model/driver.txt says that the
.probe() callback should bind the driver to a given device. That includes
verifying that the device is present, that it's a version the driver can
handle,
that driver data structures can be allocated and initialized, and that any
hardware can be initialized.
All media device drivers are complaint with that, returning 0 on success
(meaning that the device was successfully probed) or an error otherwise.
Almost all media devices are made by a SoC or a RISC CPU that works
as a DMA engine and exposes a set of registers to allow I2C access to the
device's internal and/or external I2C buses. Part of them have an internal
EEPROM/ROM that stores firmware internally at the board, but others require
a firmware to be loaded before being able to init/control the device and to
export the I2C bus interface.
The media handling function is then implemented via a series of I2C devices[1]:
- analog video decoders;
- TV tuners;
- radio tuners;
- I2C remote controller decoders;
- DVB frontends;
- mpeg decoders;
- mpeg encoders;
- video enhancement devices;
...
[1] several media chips have part of those function implemented internally,
but almost all require external I2C components to be probed.
In order to properly refer to each component, we call the main kernel module
that talks with the media device via USB/PCI bus is called bridge driver,
and the I2C components are called as sub-devices.
Different vendors use the same bridge driver to work with different sub-devices.
It is a .probe()'s task to detect what sub-devices are there inside the board.
There are several cases where the vendor switched the sub-devices without
changing the PCI ID/USB ID.
So, drivers do things like the code below, inside the .probe() callback:
static int check_if_dvb_frontend_is_supported_and_bind()
{
switch (device_model) {
case VENDOR_A_MODEL_1:
if (test_and_bind_frontend_1()) /* Doesn't require firmware */
return 0;
if (test_and_bind_frontend_2()) /* requires firmware foo */
return 0;
if (test_and_bind_frontend_3()) /* requires firmware bar */
return 0;
if (test_and_bind_frontend_4()) /* doesn't require firmware */
return 0;
break;
case VENDOR_A_MODEL_2:
/* Analog device - no DVB frontend on it */
return 0;
...
}
return -ENODEV;
}
On several devices, before being able to register the bus and do the actual
probe, the kernel needs to load a firmware.
Also, during the I2C device probing time, firmware may be required, in order
to properly expose the device's internal models and their capabilities.
For example, drx-k sub-device can have support for either DVB-C or DVB-T or
both,
depending on the device model. That affects the frontend properties exposed
to the user and might affect the bridge driver's initialization task.
In practice, a driver like em28xx have a few devices like HVR-930C that require
the drx-k sub-device. For those devices, a firmware is required; for other
devices, a firmware is not required.
What's happening is that newer versions of request_firmware and udev are being
more pedantic (for a reason) about not requesting firmwares during module_init
or PCI/USB register's probe callback.
Worse than that, the same device driver may require a firmware or not,
depending on
the I2C devices inside it. One such example is em28xx: for the great majority of
the supported devices, no firmware is needed, but devices like HVR-930C require
a firmware, because it uses a frontend that needs firmware.
After some discussions, it seems that the best model would be to add an
async_probe()
callback to be used by devices similar to media ones. The async_probe() should
be
not probed during the module_init; the probe() will be deferred to happen later,
when firmware's usermodehelper_disabled is false, allowing those drivers to
load their
firmwares if needed.
What do you think?
Regards,
Mauro
Em 25-06-2012 17:06, Mauro Carvalho Chehab escreveu:
Em 25-06-2012 16:15, Antti Palosaari escreveu:
On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.
While thinking on converting another DVB frontend driver, I just
Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Mon, Jun 25, 2012 at 05:49:25PM -0300, Mauro Carvalho Chehab wrote:
Greg,
Basically, the recent changes at request_firmware() exposed an issue that
affects all media drivers that use firmware (64 drivers).
What change was that? How did it break anything?
Driver's documentation at Documentation/driver-model/driver.txt says that the
.probe() callback should bind the driver to a given device. That includes
verifying that the device is present, that it's a version the driver can
handle,
that driver data structures can be allocated and initialized, and that any
hardware can be initialized.
Yes.
All media device drivers are complaint with that, returning 0 on success
(meaning that the device was successfully probed) or an error otherwise.
Great, no probems then :)
Almost all media devices are made by a SoC or a RISC CPU that works
as a DMA engine and exposes a set of registers to allow I2C access to the
device's internal and/or external I2C buses. Part of them have an internal
EEPROM/ROM that stores firmware internally at the board, but others require
a firmware to be loaded before being able to init/control the device and to
export the I2C bus interface.
The media handling function is then implemented via a series of I2C
devices[1]:
- analog video decoders;
- TV tuners;
- radio tuners;
- I2C remote controller decoders;
- DVB frontends;
- mpeg decoders;
- mpeg encoders;
- video enhancement devices;
...
[1] several media chips have part of those function implemented internally,
but almost all require external I2C components to be probed.
In order to properly refer to each component, we call the main kernel module
that talks with the media device via USB/PCI bus is called bridge driver,
and the I2C components are called as sub-devices.
Different vendors use the same bridge driver to work with different
sub-devices.
It is a .probe()'s task to detect what sub-devices are there inside the board.
There are several cases where the vendor switched the sub-devices without
changing the PCI ID/USB ID.
Hardware vendors suck, we all know that :(
So, drivers do things like the code below, inside the .probe() callback:
static int check_if_dvb_frontend_is_supported_and_bind()
{
switch (device_model) {
case VENDOR_A_MODEL_1:
if (test_and_bind_frontend_1()) /* Doesn't require firmware */
return 0;
if (test_and_bind_frontend_2()) /* requires firmware foo */
return 0;
if (test_and_bind_frontend_3()) /* requires firmware bar */
return 0;
Wait, why would these, if they require firmware, not load the firmware
at this point in time? Why are they saying all is fine here?
if (test_and_bind_frontend_4()) /* doesn't require firmware */
return 0;
break;
case VENDOR_A_MODEL_2:
/* Analog device - no DVB frontend on it */
return 0;
...
}
return -ENODEV;
}
On several devices, before being able to register the bus and do the actual
probe, the kernel needs to load a firmware.
That's common.
Also, during the I2C device probing time, firmware may be required, in order
to properly expose the device's internal models and their capabilities.
For example, drx-k sub-device can have support for either DVB-C or DVB-T or
both,
depending on the device model. That affects the frontend properties exposed
to the user and might affect the bridge driver's initialization task.
In practice, a driver like em28xx have a few devices like HVR-930C that
require
the drx-k sub-device. For those devices, a firmware is required; for other
devices, a firmware is not required.
What's happening is that newer versions of request_firmware and udev are being
more pedantic (for a reason) about not requesting firmwares during module_init
or PCI/USB register's probe callback.
It is? What changed to require this? What commit id?
Worse than that, the same device driver may require a firmware or not,
depending on
the I2C devices inside it. One such example is em28xx: for the great majority
of
the supported devices, no firmware is needed, but devices like HVR-930C
require
a firmware, because it uses a frontend that needs firmware.
After some discussions, it seems that the best model would be to add an
async_probe()
callback to be used by devices similar to media ones. The async_probe()
should be
not probed during the module_init; the probe() will be deferred to happen
later,
when firmware's usermodehelper_disabled is false, allowing those drivers to
load their
firmwares if needed.
What do you think?
We already have deferred probe() calls, you just need to return a
different value (can't remember it at the moment), and your probe
function will be called again
Re: Need of an .async_probe() type of callback at driver's core - Was: Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Em 25-06-2012 19:33, Greg KH escreveu:
On Mon, Jun 25, 2012 at 05:49:25PM -0300, Mauro Carvalho Chehab wrote:
Greg,
Basically, the recent changes at request_firmware() exposed an issue that
affects all media drivers that use firmware (64 drivers).
What change was that? How did it break anything?
https://bugzilla.redhat.com/show_bug.cgi?id=827538
Basically, userspace changes and some pm-related patches, mainly this one [1]:
@@ -613,7 +606,14 @@ request_firmware(const struct firmware **firmware_p, const
char *name,
if (ret = 0)
return ret;
- ret = _request_firmware(*firmware_p, name, device, true, false);
+ ret = usermodehelper_read_trylock();
+ if (WARN_ON(ret)) {
+ dev_err(device, firmware: %s will not be loaded\n, name);
+ } else {
+ ret = _request_firmware(*firmware_p, name, device, true, false,
+ firmware_loading_timeout());
+ usermodehelper_read_unlock();
+ }
if (ret)
_request_firmware_cleanup(firmware_p);
[1] at git commit 9b78c1da60b3c62ccdd1509f0902ad19ceaf776b
Driver's documentation at Documentation/driver-model/driver.txt says that the
.probe() callback should bind the driver to a given device. That includes
verifying that the device is present, that it's a version the driver can
handle,
that driver data structures can be allocated and initialized, and that any
hardware can be initialized.
Yes.
All media device drivers are complaint with that, returning 0 on success
(meaning that the device was successfully probed) or an error otherwise.
Great, no probems then :)
Almost all media devices are made by a SoC or a RISC CPU that works
as a DMA engine and exposes a set of registers to allow I2C access to the
device's internal and/or external I2C buses. Part of them have an internal
EEPROM/ROM that stores firmware internally at the board, but others require
a firmware to be loaded before being able to init/control the device and to
export the I2C bus interface.
The media handling function is then implemented via a series of I2C
devices[1]:
- analog video decoders;
- TV tuners;
- radio tuners;
- I2C remote controller decoders;
- DVB frontends;
- mpeg decoders;
- mpeg encoders;
- video enhancement devices;
...
[1] several media chips have part of those function implemented internally,
but almost all require external I2C components to be probed.
In order to properly refer to each component, we call the main kernel
module
that talks with the media device via USB/PCI bus is called bridge driver,
and the I2C components are called as sub-devices.
Different vendors use the same bridge driver to work with different
sub-devices.
It is a .probe()'s task to detect what sub-devices are there inside the
board.
There are several cases where the vendor switched the sub-devices without
changing the PCI ID/USB ID.
Hardware vendors suck, we all know that :(
So, drivers do things like the code below, inside the .probe() callback:
static int check_if_dvb_frontend_is_supported_and_bind()
{
switch (device_model) {
case VENDOR_A_MODEL_1:
if (test_and_bind_frontend_1()) /* Doesn't require firmware */
return 0;
if (test_and_bind_frontend_2()) /* requires firmware foo */
return 0;
if (test_and_bind_frontend_3()) /* requires firmware bar */
return 0;
Wait, why would these, if they require firmware, not load the firmware
at this point in time? Why are they saying all is fine here?
The above is a prototype of what happens. The test_and_bind_frontend_n()
logic are, in fact, more complex than that: it tries to load the firmware
inside each frontend's code when needed.
This is a real example (taken from ttpci budget driver):
switch(budget-dev-pci-subsystem_device) {
case 0x1003: // Hauppauge/TT Nova budget (stv0299/ALPS BSRU6(tsa5059)
OR ves1893/ALPS BSRV2(sp5659))
case 0x1013:
// try the ALPS BSRV2 first of all
budget-dvb_frontend = dvb_attach(ves1x93_attach,
alps_bsrv2_config, budget-i2c_adap);
if (budget-dvb_frontend) {
budget-dvb_frontend-ops.tuner_ops.set_params =
alps_bsrv2_tuner_set_params;
budget-dvb_frontend-ops.diseqc_send_master_cmd =
budget_diseqc_send_master_cmd;
budget-dvb_frontend-ops.diseqc_send_burst =
budget_diseqc_send_burst;
budget-dvb_frontend-ops.set_tone = budget_set_tone;
break;
}
// try the ALPS BSRU6 now
budget-dvb_frontend = dvb_attach(stv0299_attach,
alps_bsru6_config, budget-i2c_adap);
if (budget-dvb_frontend) {
Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Hi Mauro
I think that your __i2c_transfer() solution is elegant for DVB-C frontend tuner
reprogramming sleep cases:
TDA6650 programming manual, page 9 states:
Main divider data are valid only if no new I2C-bus transmission
is started (START condition) during the computation
period of 50 μs.
http://www.datasheetcatalog.org/datasheet/philips/TDA6650.pdf
It states that after TDA6650 frontend programming, the whole I2C bus
(for any I2C transfer into any I2C device) must be quiet at least 50μs.
My main point for __i2c_transfer() elegance can be seen easilly
from the code examples below.
-
An elegant way to solve only the 50μs TDA6650 sleep problem (not gate control
issue),
is to solve it in tda665x.c's tda665x_set_state():
static int tda665x_set_state(struct dvb_frontend *fe,
enum tuner_param param,
struct tuner_state *tstate)
{
...
i2c_lock_adapter(state-i2c);
/* Set params without taking I2C lock ( uses __i2c_transfer() ) */
err = __tda665x_write(state, buf, 5);
usleep(50);
i2c_unlock_adapter(state-i2c);
...
}
Ugly robust way to solveonly the 50μs TDA6650 sleep problem (not gate control
issue,
is to teach I2C transfer code TDA6650's requirements:
-- warning: ugly --
saa7146_i2c_transfer(..) {
...
mutex_lock_interruptible(dev-i2c_lock);
...
send_i2c_messages();
if (num == 5 is_tda665())
usleep(50);
mutex_unlock(dev-i2c_lock);
}
Here is a robust version with __i2c_transfer(). Main idea is to
protect the whole communicating with I2C Frontend device with I2C lock:
- Open I2C Gate so that you can talk to DVB Frontend Tuner.
- Send 5 bytes into DVB-C Frontend tuner.
- Quiesce I2C bus by sleeping 50μs to protect the Frontend Tuner message.
- Close I2C Gate so that DVB Frontend Tuner isn't reachable by I2C bus.
static int tda665x_set_state(struct dvb_frontend *fe,
enum tuner_param param,
struct tuner_state *tstate)
{
...
i2c_lock_adapter(state-i2c);
__open_i2c_gate(state);
/* Set params without taking I2C lock ( uses __i2c_transfer() ) */
err = __tda665x_write(state, buf, 5);
usleep(50);
__close_i2c_gate(state);
i2c_unlock_adapter(state-i2c);
...
}
-- Ugly solution for the same full problem in SAA7146 I2C transfer function
itself (code with some unsolved problems) --
saa7146_i2c_transfer(..) {
...
mutex_lock_interruptible(dev-i2c_lock);
if (is_tda665(addr))
send_msg_tda665_gate_open(dev);
...
send_i2c_messages();
if (num == 5 is_tda665())
usleep(50);
if (is_tda665(addr))
send_msg_tda665_gate_close(dev);
mutex_unlock(dev-i2c_lock);
}
Regards,
Marko Ristola
On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.
While thinking on converting another DVB frontend driver, I just realized
that a patch like that won't work fine.
As most of you know, there are _several_ I2C chips that don't tolerate any
usage of the I2C bus while a firmware is being loaded (I dunno if this is the
case of drx-k, but I won't doubt).
The current approach makes the device probe() logic is serialized. So, there's
no chance that two different I2C drivers to try to access the bus at the same
time, if the bridge driver is properly implemented.
However, now that firmware is loaded asynchronously, several other I2C drivers
may be trying to use the bus at the same time. So, events like IR (and CI)
polling,
tuner get_status, etc can happen during a firmware transfer, causing the
firmware
to not load properly.
A fix for that will require to lock the firmware load I2C traffic into a single
transaction.
So, the code that sends the firmware to the board need to be changed to
something like:
static int DownloadMicrocode(struct drxk_state *state,
const u8 pMCImage[], u32 Length)
{
...
i2c_lock_adapter(state-i2c);
...
for (i = 0; i nBlocks; i += 1) {
...
status = __i2c_transfer(state-i2c, ...);
...
}
i2c_unlock_adapter(state-i2c);
return status;
}
where __i2c_transfer() would be a variant of i2c_transfer() that won't take the
I2C lock.
Other drivers that load firmware at I2C and use request_firmware_nowait() will
also
need a similar approach.
Jean,
What do you think?
Regards,
Mauro
Cc: Antti Palosaari cr...@iki.fi
Cc: Kay Sievers k...@redhat.com
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
[PATCH] [media] drxk: change it to use request_firmware_nowait()
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.
Cc: Antti Palosaari cr...@iki.fi
Cc: Kay Sievers k...@redhat.com
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/dvb/frontends/drxk_hard.c | 109 +++
drivers/media/dvb/frontends/drxk_hard.h |3 +
2 files changed, 72 insertions(+), 40 deletions(-)
diff --git a/drivers/media/dvb/frontends/drxk_hard.c
b/drivers/media/dvb/frontends/drxk_hard.c
index 60b868f..4cb8d1e 100644
--- a/drivers/media/dvb/frontends/drxk_hard.c
+++ b/drivers/media/dvb/frontends/drxk_hard.c
@@ -5968,29 +5968,9 @@ error:
return status;
}
-static int load_microcode(struct drxk_state *state, const char *mc_name)
-{
- const struct firmware *fw = NULL;
- int err = 0;
-
- dprintk(1, \n);
-
- err = request_firmware(fw, mc_name, state-i2c-dev.parent);
- if (err 0) {
- printk(KERN_ERR
- drxk: Could not load firmware file %s.\n, mc_name);
- printk(KERN_INFO
- drxk: Copy %s to your hotplug directory!\n, mc_name);
- return err;
- }
- err = DownloadMicrocode(state, fw-data, fw-size);
- release_firmware(fw);
- return err;
-}
-
static int init_drxk(struct drxk_state *state)
{
- int status = 0;
+ int status = 0, n = 0;
enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
u16 driverVersion;
@@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
if (status 0)
goto error;
- if (state-microcode_name)
- load_microcode(state, state-microcode_name);
+ if (state-fw) {
+ status = DownloadMicrocode(state, state-fw-data,
+ state-fw-size);
+ if (status 0)
+ goto error;
+ }
/* disable token-ring bus through OFDM block for possible ucode
upload */
status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A,
SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
@@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
state-m_DrxkState = DRXK_POWERED_DOWN;
} else
state-m_DrxkState = DRXK_STOPPED;
+
+ /* Initialize the supported delivery systems */
+ n = 0;
+ if (state-m_hasDVBC) {
+ state-frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
+ state-frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
+ strlcat(state-frontend.ops.info.name, DVB-C,
+ sizeof(state-frontend.ops.info.name));
+ }
+ if (state-m_hasDVBT) {
+ state-frontend.ops.delsys[n++] = SYS_DVBT;
+ strlcat(state-frontend.ops.info.name, DVB-T,
+ sizeof(state-frontend.ops.info.name));
+ }
}
error:
if (status 0)
@@ -6175,11 +6173,44 @@ error:
return status;
}
+static void load_firmware_cb(const struct firmware *fw,
+void *context)
+{
+ struct drxk_state *state = context;
+
+ if (!fw) {
+ printk(KERN_ERR
+ drxk: Could not load firmware file %s.\n,
+ state-microcode_name);
+ printk(KERN_INFO
+ drxk: Copy %s to your hotplug directory!\n,
+ state-microcode_name);
+ state-microcode_name = NULL;
+
+ /*
+* As firmware is now load asynchronous, it is not possible
+* anymore to fail at frontend attach. We might silently
+* return here, and hope that the driver won't crash.
+* We might also change all DVB callbacks to return -ENODEV
+* if the device is not initialized.
+* As the DRX-K devices have their own internal firmware,
+* let's just hope that it will match a firmware revision
+* compatible with this driver and proceed.
+*/
+ }
+ state-fw = fw;
+
+ init_drxk(state);
+}
+
static void drxk_release(struct dvb_frontend *fe)
{
struct drxk_state *state = fe-demodulator_priv;
dprintk(1, \n);
+ if (state-fw)
+ release_firmware(state-fw);
+
kfree(state);
}
@@ -6371,10 +6402,9 @@ static struct dvb_frontend_ops drxk_ops = {
struct dvb_frontend *drxk_attach(const struct drxk_config
Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
On Thu, Jun 21, 2012 at 9:36 AM, Mauro Carvalho Chehab mche...@redhat.com wrote: The firmware blob may not be available when the driver probes. Instead of blocking the whole kernel use request_firmware_nowait() and continue without firmware. This shouldn't be that bad on drx-k devices, as they all seem to have an internal firmware. So, only the firmware update will take a little longer to happen. The patch itself looks fine, however the comment at the end probably isn't valid. Many of the drx-k devices don't have onboard flash, and even for the ones that do have flash, uploading the firmware doesn't actually rewrite the flash. It patches the in-RAM copy (or replaces the *running* image). Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Em 21-06-2012 11:21, Devin Heitmueller escreveu:
On Thu, Jun 21, 2012 at 9:36 AM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.
The patch itself looks fine, however the comment at the end probably
isn't valid. Many of the drx-k devices don't have onboard flash, and
even for the ones that do have flash, uploading the firmware doesn't
actually rewrite the flash. It patches the in-RAM copy (or replaces
the *running* image).
Yeah, that's what I meant to say. Just rewrote the patch description.
See below.
Regards,
Mauro
-
[media] drxk: change it to use request_firmware_nowait()
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on some drx-k devices that have firmware
stored on their ROM's. On those devices, only the RAM version of the firmware
is changed. If that fails, there's still a chance that the device will work.
So, a fail to load the firmware may not be a fatal error.
Cc: Antti Palosaari cr...@iki.fi
Cc: Kay Sievers k...@redhat.com
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/dvb/frontends/drxk_hard.c | 109 +++
drivers/media/dvb/frontends/drxk_hard.h |3 +
2 files changed, 72 insertions(+), 40 deletions(-)
diff --git a/drivers/media/dvb/frontends/drxk_hard.c
b/drivers/media/dvb/frontends/drxk_hard.c
index 60b868f..4cb8d1e 100644
--- a/drivers/media/dvb/frontends/drxk_hard.c
+++ b/drivers/media/dvb/frontends/drxk_hard.c
@@ -5968,29 +5968,9 @@ error:
return status;
}
-static int load_microcode(struct drxk_state *state, const char *mc_name)
-{
- const struct firmware *fw = NULL;
- int err = 0;
-
- dprintk(1, \n);
-
- err = request_firmware(fw, mc_name, state-i2c-dev.parent);
- if (err 0) {
- printk(KERN_ERR
- drxk: Could not load firmware file %s.\n, mc_name);
- printk(KERN_INFO
- drxk: Copy %s to your hotplug directory!\n, mc_name);
- return err;
- }
- err = DownloadMicrocode(state, fw-data, fw-size);
- release_firmware(fw);
- return err;
-}
-
static int init_drxk(struct drxk_state *state)
{
- int status = 0;
+ int status = 0, n = 0;
enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
u16 driverVersion;
@@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
if (status 0)
goto error;
- if (state-microcode_name)
- load_microcode(state, state-microcode_name);
+ if (state-fw) {
+ status = DownloadMicrocode(state, state-fw-data,
+ state-fw-size);
+ if (status 0)
+ goto error;
+ }
/* disable token-ring bus through OFDM block for possible ucode
upload */
status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A,
SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
@@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
state-m_DrxkState = DRXK_POWERED_DOWN;
} else
state-m_DrxkState = DRXK_STOPPED;
+
+ /* Initialize the supported delivery systems */
+ n = 0;
+ if (state-m_hasDVBC) {
+ state-frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
+ state-frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
+ strlcat(state-frontend.ops.info.name, DVB-C,
+ sizeof(state-frontend.ops.info.name));
+ }
+ if (state-m_hasDVBT) {
+ state-frontend.ops.delsys[n++] = SYS_DVBT;
+ strlcat(state-frontend.ops.info.name, DVB-T,
+ sizeof(state-frontend.ops.info.name));
+ }
}
error:
if (status 0)
@@ -6175,11 +6173,44 @@ error:
return status;
}
+static void load_firmware_cb(const struct firmware *fw,
+void *context)
+{
+ struct drxk_state *state = context;
+
+ if (!fw) {
+ printk(KERN_ERR
+ drxk: Could not load firmware file %s.\n,
+ state-microcode_name);
+ printk(KERN_INFO
+ drxk: Copy %s to your hotplug directory!\n,
+ state-microcode_name);
+
Re: [PATCH] [media] drxk: change it to use request_firmware_nowait()
Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.
While thinking on converting another DVB frontend driver, I just realized
that a patch like that won't work fine.
As most of you know, there are _several_ I2C chips that don't tolerate any
usage of the I2C bus while a firmware is being loaded (I dunno if this is the
case of drx-k, but I won't doubt).
The current approach makes the device probe() logic is serialized. So, there's
no chance that two different I2C drivers to try to access the bus at the same
time, if the bridge driver is properly implemented.
However, now that firmware is loaded asynchronously, several other I2C drivers
may be trying to use the bus at the same time. So, events like IR (and CI)
polling,
tuner get_status, etc can happen during a firmware transfer, causing the
firmware
to not load properly.
A fix for that will require to lock the firmware load I2C traffic into a single
transaction.
So, the code that sends the firmware to the board need to be changed to
something like:
static int DownloadMicrocode(struct drxk_state *state,
const u8 pMCImage[], u32 Length)
{
...
i2c_lock_adapter(state-i2c);
...
for (i = 0; i nBlocks; i += 1) {
...
status = __i2c_transfer(state-i2c, ...);
...
}
i2c_unlock_adapter(state-i2c);
return status;
}
where __i2c_transfer() would be a variant of i2c_transfer() that won't take the
I2C lock.
Other drivers that load firmware at I2C and use request_firmware_nowait() will
also
need a similar approach.
Jean,
What do you think?
Regards,
Mauro
Cc: Antti Palosaari cr...@iki.fi
Cc: Kay Sievers k...@redhat.com
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
drivers/media/dvb/frontends/drxk_hard.c | 109
+++
drivers/media/dvb/frontends/drxk_hard.h |3 +
2 files changed, 72 insertions(+), 40 deletions(-)
diff --git a/drivers/media/dvb/frontends/drxk_hard.c
b/drivers/media/dvb/frontends/drxk_hard.c
index 60b868f..4cb8d1e 100644
--- a/drivers/media/dvb/frontends/drxk_hard.c
+++ b/drivers/media/dvb/frontends/drxk_hard.c
@@ -5968,29 +5968,9 @@ error:
return status;
}
-static int load_microcode(struct drxk_state *state, const char *mc_name)
-{
- const struct firmware *fw = NULL;
- int err = 0;
-
- dprintk(1, \n);
-
- err = request_firmware(fw, mc_name, state-i2c-dev.parent);
- if (err 0) {
- printk(KERN_ERR
-drxk: Could not load firmware file %s.\n, mc_name);
- printk(KERN_INFO
-drxk: Copy %s to your hotplug directory!\n, mc_name);
- return err;
- }
- err = DownloadMicrocode(state, fw-data, fw-size);
- release_firmware(fw);
- return err;
-}
-
static int init_drxk(struct drxk_state *state)
{
- int status = 0;
+ int status = 0, n = 0;
enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
u16 driverVersion;
@@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
if (status 0)
goto error;
- if (state-microcode_name)
- load_microcode(state, state-microcode_name);
+ if (state-fw) {
+ status = DownloadMicrocode(state, state-fw-data,
+state-fw-size);
+ if (status 0)
+ goto error;
+ }
/* disable token-ring bus through OFDM block for possible ucode
upload */
status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A,
SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
@@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
state-m_DrxkState = DRXK_POWERED_DOWN;
} else
state-m_DrxkState = DRXK_STOPPED;
+
+ /* Initialize the supported delivery systems */
+ n = 0;
+ if (state-m_hasDVBC) {
+ state-frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
+ state-frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
+ strlcat(state-frontend.ops.info.name, DVB-C,
+ sizeof(state-frontend.ops.info.name));
+ }
+ if (state-m_hasDVBT) {
+ state-frontend.ops.delsys[n++] = SYS_DVBT;
+ strlcat(state-frontend.ops.info.name, DVB-T,
+
