* A workaround for request_firmware() stuck in module_init @ 2012-09-04 13:06 Takashi Iwai 2012-09-04 15:52 ` Ming Lei 2012-09-05 16:51 ` Lucas De Marchi 0 siblings, 2 replies; 21+ messages in thread From: Takashi Iwai @ 2012-09-04 13:06 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Ming Lei, Rafael J. Wysocki, Kay Sievers, linux-kernel Hi, as I've got recently a few bug reports regarding the stuck with request_firmware() in module_init of some sound drivers, I started looking at the issue. Strangely, the problem doesn't happen on openSUSE 12.2 although it has the same udev version with libkmod as Fedora. So I installed Fedora 17, and indeed I could see a problem there. Obviously a solution would be to rewrite the driver code to use request_firmware_nowait() instead. But it'd need a lot of code shuffling, and most of such drivers are old stuff I don't want to do a serious surgery. So I tried an easier workaround by using the deferred probing. An experimental patch is below. As you can see, from the driver side, it's simple: just add two lines at the head of each probe function. Do you think this kind of hack is OK? If not, any better (IOW easier) solution? thanks, Takashi === Subject: [PATCH] driver-core: Add a helper to work around the stuck with request_firmware() Since the recent udev loads the module with libkmod, the module loading works no longer properly when a driver calls request_firmware() in module_init. Certainly we can fix all these with request_firmware_nowait(), but it'd need fairly lots of code changes, so it's no preferred for a lazy person like me. This patch adds an easier workaround: use the deferred probing. The driver that may call request_firmware() in module_init should call the new helper function dev_defer_for_fw_load(), and returns immediately with -EPROBE_DEFER if it's true. If it's false, simply continues the rest. That's all. In the driver core side, a new bit flag field is added to the device private data for bookkeeping and triggering the deferred probe explicitly. (Otherwise the trigger won't happen unless any new driver binding occurs.) As an example implementation, the patch contains the fix for sound/pci/rme9652/hdsp.c, too. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/base/base.h | 6 ++++++ drivers/base/dd.c | 25 +++++++++++++++++++++++++ include/linux/firmware.h | 6 ++++++ sound/pci/rme9652/hdsp.c | 3 +++ 4 files changed, 40 insertions(+) diff --git a/drivers/base/base.h b/drivers/base/base.h index 6ee17bb..de5a7ca 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -67,6 +67,7 @@ struct driver_private { * list soon. * @device - pointer back to the struct class that this structure is * associated with. + * @flags - extra bit flags, currently used for deferred probing for fw loader * * Nothing outside of the driver core should ever touch these fields. */ @@ -78,6 +79,7 @@ struct device_private { struct list_head deferred_probe; void *driver_data; struct device *device; + unsigned int flags; }; #define to_device_private_parent(obj) \ container_of(obj, struct device_private, knode_parent) @@ -86,6 +88,10 @@ struct device_private { #define to_device_private_bus(obj) \ container_of(obj, struct device_private, knode_bus) +/* bit flags */ +#define DEV_FLAG_IN_DEFERRED_PROBE (1 << 0) +#define DEV_FLAG_NEED_TRIGGER (1 << 1) + extern int device_private_init(struct device *dev); /* initialisation functions */ diff --git a/drivers/base/dd.c b/drivers/base/dd.c index e3bbed8..aaefa7e 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -97,7 +97,9 @@ static void deferred_probe_work_func(struct work_struct *work) device_pm_unlock(); dev_dbg(dev, "Retrying from deferred list\n"); + dev->p->flags |= DEV_FLAG_IN_DEFERRED_PROBE; bus_probe_device(dev); + dev->p->flags &= ~DEV_FLAG_IN_DEFERRED_PROBE; mutex_lock(&deferred_probe_mutex); @@ -301,6 +303,10 @@ probe_failed: /* Driver requested deferred probing */ dev_info(dev, "Driver %s requests probe deferral\n", drv->name); driver_deferred_probe_add(dev); + if (dev->p->flags & DEV_FLAG_NEED_TRIGGER) { + driver_deferred_probe_trigger(); + dev->p->flags &= ~DEV_FLAG_NEED_TRIGGER; + } } else if (ret != -ENODEV && ret != -ENXIO) { /* driver matched but the probe failed */ printk(KERN_WARNING @@ -587,3 +593,22 @@ int dev_set_drvdata(struct device *dev, void *data) return 0; } EXPORT_SYMBOL(dev_set_drvdata); + +#ifdef CONFIG_FW_LOADER +/** + * dev_defer_for_fw_load - check if deferred probe for fw loader is needed + * @dev: device + * + * When a driver may invoke request_firmware() in its module init, call this + * function at the beginning of the probe function. When it's true, the probe + * should return immediately with -EPROBE_DEFER. + */ +bool dev_defer_for_fw_load(struct device *dev) +{ + if (dev->p->flags & DEV_FLAG_IN_DEFERRED_PROBE) + return false; + dev->p->flags |= DEV_FLAG_NEED_TRIGGER; + return true; +} +EXPORT_SYMBOL_GPL(dev_defer_for_fw_load); +#endif diff --git a/include/linux/firmware.h b/include/linux/firmware.h index 1e7c011..7caa96c 100644 --- a/include/linux/firmware.h +++ b/include/linux/firmware.h @@ -44,6 +44,7 @@ int request_firmware_nowait( void (*cont)(const struct firmware *fw, void *context)); void release_firmware(const struct firmware *fw); +bool dev_defer_for_fw_load(struct device *dev); #else static inline int request_firmware(const struct firmware **fw, const char *name, @@ -62,6 +63,11 @@ static inline int request_firmware_nowait( static inline void release_firmware(const struct firmware *fw) { } + +static inline bool dev_defer_for_fw_load(struct device *dev) +{ + return false; +} #endif #endif diff --git a/sound/pci/rme9652/hdsp.c b/sound/pci/rme9652/hdsp.c index 0d6930c..427ac24 100644 --- a/sound/pci/rme9652/hdsp.c +++ b/sound/pci/rme9652/hdsp.c @@ -5594,6 +5594,9 @@ static int __devinit snd_hdsp_probe(struct pci_dev *pci, struct snd_card *card; int err; + if (dev_defer_for_fw_load(&pci->dev)) + return -EPROBE_DEFER; + if (dev >= SNDRV_CARDS) return -ENODEV; if (!enable[dev]) { -- 1.7.11.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-04 13:06 A workaround for request_firmware() stuck in module_init Takashi Iwai @ 2012-09-04 15:52 ` Ming Lei 2012-09-04 16:10 ` Takashi Iwai 2012-09-05 16:51 ` Lucas De Marchi 1 sibling, 1 reply; 21+ messages in thread From: Ming Lei @ 2012-09-04 15:52 UTC (permalink / raw) To: Takashi Iwai Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote: > Hi, > > as I've got recently a few bug reports regarding the stuck with > request_firmware() in module_init of some sound drivers, I started > looking at the issue. Strangely, the problem doesn't happen on > openSUSE 12.2 although it has the same udev version with libkmod as > Fedora. So I installed Fedora 17, and indeed I could see a problem > there. It should be a bug in udev, as discussed in the below link: http://marc.info/?t=134552745100002&r=1&w=2 > > Obviously a solution would be to rewrite the driver code to use > request_firmware_nowait() instead. But it'd need a lot of code > shuffling, and most of such drivers are old stuff I don't want to do a > serious surgery. > > So I tried an easier workaround by using the deferred probing. > An experimental patch is below. As you can see, from the driver side, > it's simple: just add two lines at the head of each probe function. In fact, the defer probe should only be applied in situations which driver is built in kernel and request_firmware is called in .probe(). > > Do you think this kind of hack is OK? If not, any better (IOW easier) > solution? Looks your solution is a bit complicated, and I have wrote a similar patch to address the problem, but Linus thought that it may hide the problem of drivers. http://marc.info/?t=134278790800004&r=1&w=2 IMO, driver core needn't to be changed, and the defer probe can be supported just by changes in request_firmware() and its caller. Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-04 15:52 ` Ming Lei @ 2012-09-04 16:10 ` Takashi Iwai 2012-09-05 1:15 ` Ming Lei 0 siblings, 1 reply; 21+ messages in thread From: Takashi Iwai @ 2012-09-04 16:10 UTC (permalink / raw) To: Ming Lei; +Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel At Tue, 4 Sep 2012 23:52:15 +0800, Ming Lei wrote: > > On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote: > > Hi, > > > > as I've got recently a few bug reports regarding the stuck with > > request_firmware() in module_init of some sound drivers, I started > > looking at the issue. Strangely, the problem doesn't happen on > > openSUSE 12.2 although it has the same udev version with libkmod as > > Fedora. So I installed Fedora 17, and indeed I could see a problem > > there. > > It should be a bug in udev, as discussed in the below link: > > http://marc.info/?t=134552745100002&r=1&w=2 Yeah, if udev has a fix, I'm fine. I'll proactively ignore these bug reports. But did it really happen...? > > Obviously a solution would be to rewrite the driver code to use > > request_firmware_nowait() instead. But it'd need a lot of code > > shuffling, and most of such drivers are old stuff I don't want to do a > > serious surgery. > > > > So I tried an easier workaround by using the deferred probing. > > An experimental patch is below. As you can see, from the driver side, > > it's simple: just add two lines at the head of each probe function. > > In fact, the defer probe should only be applied in situations which > driver is built in kernel and request_firmware is called in .probe(). Is it? I thought the deferred probe is basically not for this problem but rather for the dependency problem with other modules. That's the reason it's triggered only upon the successful binding. And IMO, the deferred probe for the built-in kernel is also silly. Did anyone really make it working for built-in kernel driver and external firmware files? (For the resume, it's of course a different issue. And I guess it's been solved by your fw cache patch, right?) > > Do you think this kind of hack is OK? If not, any better (IOW easier) > > solution? > > Looks your solution is a bit complicated, and I have wrote a similar > patch to address the problem, but Linus thought that it may hide the > problem of drivers. > > http://marc.info/?t=134278790800004&r=1&w=2 > > IMO, driver core needn't to be changed, and the defer probe can be > supported just by changes in request_firmware() and its caller. Ideally, yes. But it won't work in some drivers like sound drivers, that have its own device number counting changed at each probe call. For such drivers, the deferred probe check must be done before entering the normal probe procedure, and returning -EPROBE_DEFER would be too late (or need more complex fallbacks). thanks, Takashi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-04 16:10 ` Takashi Iwai @ 2012-09-05 1:15 ` Ming Lei 2012-09-05 5:53 ` Takashi Iwai 0 siblings, 1 reply; 21+ messages in thread From: Ming Lei @ 2012-09-05 1:15 UTC (permalink / raw) To: Takashi Iwai Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds On Wed, Sep 5, 2012 at 12:10 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Tue, 4 Sep 2012 23:52:15 +0800, > Ming Lei wrote: >> >> On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > Hi, >> > >> > as I've got recently a few bug reports regarding the stuck with >> > request_firmware() in module_init of some sound drivers, I started >> > looking at the issue. Strangely, the problem doesn't happen on >> > openSUSE 12.2 although it has the same udev version with libkmod as >> > Fedora. So I installed Fedora 17, and indeed I could see a problem >> > there. >> >> It should be a bug in udev, as discussed in the below link: >> >> http://marc.info/?t=134552745100002&r=1&w=2 > > Yeah, if udev has a fix, I'm fine. I'll proactively ignore these bug > reports. But did it really happen...? Linus has expressed that it should be fixed by udev, maybe we can wait some time to see if it will happen, :-) There are more than 300 request_firmware called in probe(), even adding 2 line code in these drivers will involve much workload, since you need to find the matched probe() for one request_firmware and sometimes it is not easy. > >> > Obviously a solution would be to rewrite the driver code to use >> > request_firmware_nowait() instead. But it'd need a lot of code >> > shuffling, and most of such drivers are old stuff I don't want to do a >> > serious surgery. >> > >> > So I tried an easier workaround by using the deferred probing. >> > An experimental patch is below. As you can see, from the driver side, >> > it's simple: just add two lines at the head of each probe function. >> >> In fact, the defer probe should only be applied in situations which >> driver is built in kernel and request_firmware is called in .probe(). > > Is it? I thought the deferred probe is basically not for this problem > but rather for the dependency problem with other modules. That's the > reason it's triggered only upon the successful binding. Sorry, could you explain the dependency in a bit detail? >From your patch, I understand you just convert the caller of request_firmware from module_init into deferred_probe_work_func(), so the udev problem is avoided, isn't it? Looks making all .probe() run non-module_init context is still a solution, :-) > > And IMO, the deferred probe for the built-in kernel is also silly. > Did anyone really make it working for built-in kernel driver and > external firmware files? (For the resume, it's of course a different Yes, my original patch does work for the built-in situations. > issue. And I guess it's been solved by your fw cache patch, right?) > >> > Do you think this kind of hack is OK? If not, any better (IOW easier) >> > solution? >> >> Looks your solution is a bit complicated, and I have wrote a similar >> patch to address the problem, but Linus thought that it may hide the >> problem of drivers. >> >> http://marc.info/?t=134278790800004&r=1&w=2 >> >> IMO, driver core needn't to be changed, and the defer probe can be >> supported just by changes in request_firmware() and its caller. > > Ideally, yes. But it won't work in some drivers like sound drivers, > that have its own device number counting changed at each probe call. > For such drivers, the deferred probe check must be done before > entering the normal probe procedure, and returning -EPROBE_DEFER would > be too late (or need more complex fallbacks). Simply, you can put the firmware loading at the start of probe to avoid the specific sound problem, :-) Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 1:15 ` Ming Lei @ 2012-09-05 5:53 ` Takashi Iwai 2012-09-05 11:32 ` Ming Lei 0 siblings, 1 reply; 21+ messages in thread From: Takashi Iwai @ 2012-09-05 5:53 UTC (permalink / raw) To: Ming Lei Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds At Wed, 5 Sep 2012 09:15:34 +0800, Ming Lei wrote: > > On Wed, Sep 5, 2012 at 12:10 AM, Takashi Iwai <tiwai@suse.de> wrote: > > At Tue, 4 Sep 2012 23:52:15 +0800, > > Ming Lei wrote: > >> > >> On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote: > >> > Hi, > >> > > >> > as I've got recently a few bug reports regarding the stuck with > >> > request_firmware() in module_init of some sound drivers, I started > >> > looking at the issue. Strangely, the problem doesn't happen on > >> > openSUSE 12.2 although it has the same udev version with libkmod as > >> > Fedora. So I installed Fedora 17, and indeed I could see a problem > >> > there. > >> > >> It should be a bug in udev, as discussed in the below link: > >> > >> http://marc.info/?t=134552745100002&r=1&w=2 > > > > Yeah, if udev has a fix, I'm fine. I'll proactively ignore these bug > > reports. But did it really happen...? > > Linus has expressed that it should be fixed by udev, maybe we can > wait some time to see if it will happen, :-) Kay, what is the status? > There are more than 300 request_firmware called in probe(), even > adding 2 line code in these drivers will involve much workload, since > you need to find the matched probe() for one request_firmware and > sometimes it is not easy. Well, this depends on from which perspective you look at the issue. See below. > >> > Obviously a solution would be to rewrite the driver code to use > >> > request_firmware_nowait() instead. But it'd need a lot of code > >> > shuffling, and most of such drivers are old stuff I don't want to do a > >> > serious surgery. > >> > > >> > So I tried an easier workaround by using the deferred probing. > >> > An experimental patch is below. As you can see, from the driver side, > >> > it's simple: just add two lines at the head of each probe function. > >> > >> In fact, the defer probe should only be applied in situations which > >> driver is built in kernel and request_firmware is called in .probe(). > > > > Is it? I thought the deferred probe is basically not for this problem > > but rather for the dependency problem with other modules. That's the > > reason it's triggered only upon the successful binding. > > Sorry, could you explain the dependency in a bit detail? When a device has some implicit dependency on another hardware component (e.g. SDHCI depends on I2C GPIO controller, as in comment in drivers/base/dd.c), the driver needs to wait until another one gets ready. -EPROBE_DEFER was introduced for such a purpose. So, using this mechanism for the firmware issue is a kind of abuse. > >From your patch, I understand you just convert the caller of > request_firmware from module_init into deferred_probe_work_func(), > so the udev problem is avoided, isn't it? Yes, that was a hack. The idea is just offloading the probe, and the deferred probe is an existing simple way for that. > Looks making all .probe() run non-module_init context is still a solution, :-) Sounds interesting. > > And IMO, the deferred probe for the built-in kernel is also silly. > > Did anyone really make it working for built-in kernel driver and > > external firmware files? (For the resume, it's of course a different > > Yes, my original patch does work for the built-in situations. > > > issue. And I guess it's been solved by your fw cache patch, right?) > > > >> > Do you think this kind of hack is OK? If not, any better (IOW easier) > >> > solution? > >> > >> Looks your solution is a bit complicated, and I have wrote a similar > >> patch to address the problem, but Linus thought that it may hide the > >> problem of drivers. > >> > >> http://marc.info/?t=134278790800004&r=1&w=2 > >> > >> IMO, driver core needn't to be changed, and the defer probe can be > >> supported just by changes in request_firmware() and its caller. > > > > Ideally, yes. But it won't work in some drivers like sound drivers, > > that have its own device number counting changed at each probe call. > > For such drivers, the deferred probe check must be done before > > entering the normal probe procedure, and returning -EPROBE_DEFER would > > be too late (or need more complex fallbacks). > > Simply, you can put the firmware loading at the start of probe to > avoid the specific > sound problem, :-) Well, I can't buy it. Changing the call order can be often more problematic. Anyway... IMO, the primary question is whether we still regard the request_firmware() call in module_init as a driver bug. Or, looking at the usage numbers, we should accept it as a de facto standard use case? If we still see it as a bug, the only right way is to fix the drivers, not the core. That's why I suggested to put some fix to each driver. In that way, it shows obvious that the driver is a kind of buggy but fixed in a very lazy way (the function name should have been more obvious one like i_may_call_request_firmware_so_call_me_later()). The difference is that this fixes the bug, not hiding the problem. OTOH, if we see it no longer as a bug, we should rather improve the handling in the kernel core. (Or, more radically, we can consider a sort of async probing as default :) In anyway, fixing the core side is justified only if we agree that request_firmware() in module_init is a valid use case. So, before starting the work, we actually need a consensus. thanks, Takashi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 5:53 ` Takashi Iwai @ 2012-09-05 11:32 ` Ming Lei 2012-09-05 13:03 ` Alan Cox 0 siblings, 1 reply; 21+ messages in thread From: Ming Lei @ 2012-09-05 11:32 UTC (permalink / raw) To: Takashi Iwai Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds On Wed, Sep 5, 2012 at 1:53 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 5 Sep 2012 09:15:34 +0800, > Ming Lei wrote: >> >> On Wed, Sep 5, 2012 at 12:10 AM, Takashi Iwai <tiwai@suse.de> wrote: >> > At Tue, 4 Sep 2012 23:52:15 +0800, >> > Ming Lei wrote: >> >> >> >> On Tue, Sep 4, 2012 at 9:06 PM, Takashi Iwai <tiwai@suse.de> wrote: >> >> > Hi, >> >> > >> >> > as I've got recently a few bug reports regarding the stuck with >> >> > request_firmware() in module_init of some sound drivers, I started >> >> > looking at the issue. Strangely, the problem doesn't happen on >> >> > openSUSE 12.2 although it has the same udev version with libkmod as >> >> > Fedora. So I installed Fedora 17, and indeed I could see a problem >> >> > there. >> >> >> >> It should be a bug in udev, as discussed in the below link: >> >> >> >> http://marc.info/?t=134552745100002&r=1&w=2 >> > >> > Yeah, if udev has a fix, I'm fine. I'll proactively ignore these bug >> > reports. But did it really happen...? >> >> Linus has expressed that it should be fixed by udev, maybe we can >> wait some time to see if it will happen, :-) > > Kay, what is the status? > >> There are more than 300 request_firmware called in probe(), even >> adding 2 line code in these drivers will involve much workload, since >> you need to find the matched probe() for one request_firmware and >> sometimes it is not easy. > > Well, this depends on from which perspective you look at the issue. > See below. > >> >> > Obviously a solution would be to rewrite the driver code to use >> >> > request_firmware_nowait() instead. But it'd need a lot of code >> >> > shuffling, and most of such drivers are old stuff I don't want to do a >> >> > serious surgery. >> >> > >> >> > So I tried an easier workaround by using the deferred probing. >> >> > An experimental patch is below. As you can see, from the driver side, >> >> > it's simple: just add two lines at the head of each probe function. >> >> >> >> In fact, the defer probe should only be applied in situations which >> >> driver is built in kernel and request_firmware is called in .probe(). >> > >> > Is it? I thought the deferred probe is basically not for this problem >> > but rather for the dependency problem with other modules. That's the >> > reason it's triggered only upon the successful binding. >> >> Sorry, could you explain the dependency in a bit detail? > > When a device has some implicit dependency on another hardware > component (e.g. SDHCI depends on I2C GPIO controller, as in comment in > drivers/base/dd.c), the driver needs to wait until another one gets > ready. -EPROBE_DEFER was introduced for such a purpose. > > So, using this mechanism for the firmware issue is a kind of abuse. > >> >From your patch, I understand you just convert the caller of >> request_firmware from module_init into deferred_probe_work_func(), >> so the udev problem is avoided, isn't it? > > Yes, that was a hack. The idea is just offloading the probe, and the > deferred probe is an existing simple way for that. > >> Looks making all .probe() run non-module_init context is still a solution, :-) > > Sounds interesting. > >> > And IMO, the deferred probe for the built-in kernel is also silly. >> > Did anyone really make it working for built-in kernel driver and >> > external firmware files? (For the resume, it's of course a different >> >> Yes, my original patch does work for the built-in situations. >> >> > issue. And I guess it's been solved by your fw cache patch, right?) >> > >> >> > Do you think this kind of hack is OK? If not, any better (IOW easier) >> >> > solution? >> >> >> >> Looks your solution is a bit complicated, and I have wrote a similar >> >> patch to address the problem, but Linus thought that it may hide the >> >> problem of drivers. >> >> >> >> http://marc.info/?t=134278790800004&r=1&w=2 >> >> >> >> IMO, driver core needn't to be changed, and the defer probe can be >> >> supported just by changes in request_firmware() and its caller. >> > >> > Ideally, yes. But it won't work in some drivers like sound drivers, >> > that have its own device number counting changed at each probe call. >> > For such drivers, the deferred probe check must be done before >> > entering the normal probe procedure, and returning -EPROBE_DEFER would >> > be too late (or need more complex fallbacks). >> >> Simply, you can put the firmware loading at the start of probe to >> avoid the specific >> sound problem, :-) > > Well, I can't buy it. Changing the call order can be often more > problematic. Anyway... More or less, these sound drivers may not support probe deferral well. So the similar problem may be triggered if these device/drivers want to apply probe deferral to solve some resource dependency problem. > > > IMO, the primary question is whether we still regard the > request_firmware() call in module_init as a driver bug. Or, looking > at the usage numbers, we should accept it as a de facto standard use > case? IMO, if the driver is built as module, request_firmware should be allowed to be called in module_init context since user space is already ready for handling firmware request. Also some devices can't work without firmware downloading, and some device can only be allowed to download firmware one time, so it is reasonable to call request_firmware() in .probe(). The current problem is that udev thinks it is not good and may timeout the request, and looks no reasonable explanation about that. If the driver is built in kernel, the request_firmware in .probe() may prolong kernel init, and it might be a problem. But looks it is not a big deal since most of drivers are built as module. > > If we still see it as a bug, the only right way is to fix the drivers, > not the core. That's why I suggested to put some fix to each driver. > In that way, it shows obvious that the driver is a kind of buggy but > fixed in a very lazy way (the function name should have been more > obvious one like i_may_call_request_firmware_so_call_me_later()). > The difference is that this fixes the bug, not hiding the problem. > > OTOH, if we see it no longer as a bug, we should rather improve the > handling in the kernel core. (Or, more radically, we can consider a > sort of async probing as default :) Agree, :-) > In anyway, fixing the core side is justified only if we agree that > request_firmware() in module_init is a valid use case. > > So, before starting the work, we actually need a consensus. Yes. Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 11:32 ` Ming Lei @ 2012-09-05 13:03 ` Alan Cox 2012-09-05 14:01 ` Takashi Iwai ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Alan Cox @ 2012-09-05 13:03 UTC (permalink / raw) To: Ming Lei Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds > If the driver is built in kernel, the request_firmware in .probe() may > prolong kernel init, and it might be a problem. But looks it is not a > big deal since most of drivers are built as module. Doing it by deferring the load also fixes that. The built in ones will defer their final probe until the firmware appears and all will be well. If your rootfs needs firmware not in your initrd you already broke it and there is a certain level beyond which you just have to give up trying to save people from themselves. It may actually make sense to push more of it into the core driver layer and take some of the ability to make mistakes away from driver authors. For the general case of "load firmware if we see one" there isn't really any reason we can't have a firmware_name entry in the probe table entries themselves. If that was present the core bus probe would kick a firmware load off and only when the firmware had loaded would it call ->probe with dev->firmware pointing at a refcounted firmware struct. At that point it should be much faster to fix existing drivers and much harder for a random device driver to get it wrong. We can even add helpers which manage dev->firmware, and free the relevant objects when needed, plus doing automatic ref/deref on probe/remove so that for a typical driver the author only has to do {PCI_blah , ... .firmware_name="wibble500.xcr", } and all the loading, unloading, not loading twice happens by "magic" for the driver author. Add a dev_discard_firmware() for drivers that do this and know they can then dump the file and all is good 8) Alan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 13:03 ` Alan Cox @ 2012-09-05 14:01 ` Takashi Iwai 2012-09-05 15:22 ` Ming Lei 2012-09-05 16:59 ` Lucas De Marchi 2 siblings, 0 replies; 21+ messages in thread From: Takashi Iwai @ 2012-09-05 14:01 UTC (permalink / raw) To: Alan Cox Cc: Ming Lei, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds At Wed, 5 Sep 2012 14:03:04 +0100, Alan Cox wrote: > > > If the driver is built in kernel, the request_firmware in .probe() may > > prolong kernel init, and it might be a problem. But looks it is not a > > big deal since most of drivers are built as module. > > Doing it by deferring the load also fixes that. The built in ones will > defer their final probe until the firmware appears and all will be well. > > If your rootfs needs firmware not in your initrd you already broke it and > there is a certain level beyond which you just have to give up trying to > save people from themselves. > > It may actually make sense to push more of it into the core driver layer > and take some of the ability to make mistakes away from driver authors. > For the general case of "load firmware if we see one" there isn't really > any reason we can't have a firmware_name entry in the probe table > entries themselves. If that was present the core bus probe would kick a > firmware load off and only when the firmware had loaded would it call > ->probe with dev->firmware pointing at a refcounted firmware struct. > > At that point it should be much faster to fix existing drivers and much > harder for a random device driver to get it wrong. We can even add > helpers which manage dev->firmware, and free the relevant objects when > needed, plus doing automatic ref/deref on probe/remove so that for a > typical driver the author only has to do > > {PCI_blah , ... .firmware_name="wibble500.xcr", } > > and all the loading, unloading, not loading twice happens by "magic" for > the driver author. > > Add a dev_discard_firmware() for drivers that do this and know they can > then dump the file and all is good 8) This sounds like an interesting idea. I guess majority of drivers can be covered by this scenario. Of course, there are a few drivers that need more complex handling (e.g. iwlwifi handles fallback fw loading), but they can be implemented manually if needed anyway. Takashi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 13:03 ` Alan Cox 2012-09-05 14:01 ` Takashi Iwai @ 2012-09-05 15:22 ` Ming Lei 2012-09-05 16:30 ` Alan Cox 2012-09-05 16:59 ` Lucas De Marchi 2 siblings, 1 reply; 21+ messages in thread From: Ming Lei @ 2012-09-05 15:22 UTC (permalink / raw) To: Alan Cox Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds, Benjamin Herrenschmidt On Wed, Sep 5, 2012 at 9:03 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> If the driver is built in kernel, the request_firmware in .probe() may >> prolong kernel init, and it might be a problem. But looks it is not a >> big deal since most of drivers are built as module. > > Doing it by deferring the load also fixes that. The built in ones will > defer their final probe until the firmware appears and all will be well. Yes, deferring the load may fix the built in case, but which also introduces much work on changes of current drivers. In fact, there are few guys who complained the built in case. The current complaint is from that udev may timeout request inside probe() when drivers are built as module. As pointed by Linus and Benjamin, it is better to fix it in udev, and looks not good to introduce great changes (such as Takashi's defer probe patch) in kernel to workaround the problem. > > If your rootfs needs firmware not in your initrd you already broke it and > there is a certain level beyond which you just have to give up trying to > save people from themselves. > > It may actually make sense to push more of it into the core driver layer > and take some of the ability to make mistakes away from driver authors. > For the general case of "load firmware if we see one" there isn't really > any reason we can't have a firmware_name entry in the probe table > entries themselves. If that was present the core bus probe would kick a Linus has said that he doesn't like to load firmware in probe(), but in some situation the drivers have to load firmware in its probe(): http://marc.info/?l=linux-kernel&m=134592122811866&w=2 In fact, it is better for drivers to load firmware just when user wants to use the device, and some drivers have already changed to load firmware in the open() callback. So looks loading firmware always before probe in driver core is against the above idea. > firmware load off and only when the firmware had loaded would it call > ->probe with dev->firmware pointing at a refcounted firmware struct. IMO, introduce refcount for the firmware doesn't make sense. The lifetime of firmware is completely different with lifetime of driver or device: - firmware needn't be kept in memory in the device/driver's lifetime, and should be loaded just in need, and be released after downloading it into device. - sometimes devices may disappear, but it is better to keep the firmware in memory, for example, device may be disconnected during resume but will come back later. > > At that point it should be much faster to fix existing drivers and much > harder for a random device driver to get it wrong. We can even add > helpers which manage dev->firmware, and free the relevant objects when > needed, plus doing automatic ref/deref on probe/remove so that for a As said above, ref/deref on probe/remove is not a good idea since we needn't to keep the firmware in memory during the whole device/driver lifetime. Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 15:22 ` Ming Lei @ 2012-09-05 16:30 ` Alan Cox 2012-09-05 21:08 ` Benjamin Herrenschmidt ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Alan Cox @ 2012-09-05 16:30 UTC (permalink / raw) To: Ming Lei Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds, Benjamin Herrenschmidt > Yes, deferring the load may fix the built in case, but which also > introduces much work on changes of current drivers. In fact, > there are few guys who complained the built in case. It fixes the modular case too. > The current complaint is from that udev may timeout request inside probe() > when drivers are built as module. As pointed by Linus and Benjamin, > it is better to fix it in udev, and looks not good to introduce great changes > (such as Takashi's defer probe patch) in kernel to workaround the problem. It's not about a workaround but about doing it properly for the long term and doing it in one place. It's also not a "great change", its a small change. > Linus has said that he doesn't like to load firmware in probe(), but in some > situation the drivers have to load firmware in its probe(): You don't want to load firmware in probe because of the locking problems - you can trigger a load of another device on the same bus - the defer dodges that nicely > In fact, it is better for drivers to load firmware just when user wants to use > the device, and some drivers have already changed to load firmware in > the open() callback. For those devices sure but they are if anything a minority as far as I can see. > So looks loading firmware always before probe in driver core is > against the above idea. I never said "always" > > firmware load off and only when the firmware had loaded would it call > > ->probe with dev->firmware pointing at a refcounted firmware struct. > > IMO, introduce refcount for the firmware doesn't make sense. The lifetime > of firmware is completely different with lifetime of driver or device: Exactly. Which is why the moment you have multiple devices you need refcounts. It's also why the propsoal included a dev_discard_firmware() so you an instance can drop its firmware reference if it doesn't need it post probe. > - firmware needn't be kept in memory in the device/driver's lifetime, and > should be loaded just in need, and be released after downloading > it into device. You broke suspend/resume for lots of devices. > - sometimes devices may disappear, but it is better to keep the > firmware in memory, for example, device may be disconnected > during resume but will come back later. So the moment you have multiple instances of a device with their own lifetime and you have the need to pin it sometimes you need a refcount > As said above, ref/deref on probe/remove is not a good idea since > we needn't to keep the firmware in memory during the whole device/driver > lifetime. Often you do. And in the case you don't you still have to deal with multiple probes doing asynchronous loads of the same firmware so you want to do matching and refcounting. It's pretty much essential. The other big value apart from making it harder for driver writers to screw up is that it takes some of the control and puts it in one place. That means you can change it later easily not in each driver. This is enabling for device drivers. With no intention of offending driver authors the reality is that we should have driver interfaces that - work the right way by default - allow driver authors to do things themselves if they need to instead (ie opt out) - are hard to f**k up because we want it to just work. Alan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 16:30 ` Alan Cox @ 2012-09-05 21:08 ` Benjamin Herrenschmidt 2012-09-05 23:18 ` Alan Cox 2012-09-06 2:47 ` Linus Torvalds 2012-09-06 4:12 ` Ming Lei 2 siblings, 1 reply; 21+ messages in thread From: Benjamin Herrenschmidt @ 2012-09-05 21:08 UTC (permalink / raw) To: Alan Cox Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds On Wed, 2012-09-05 at 17:30 +0100, Alan Cox wrote: > > > Linus has said that he doesn't like to load firmware in probe(), but > in some > > situation the drivers have to load firmware in its probe(): > > You don't want to load firmware in probe because of the locking > problems > - you can trigger a load of another device on the same bus - the defer > dodges that nicely But then you have cases where probe() -> register_with_my_subsystem() -> open(). Network devices come to mind. IE. udev must be able to deal with a synchronous firmware load from probe I'm afraid. The only other option would be to make full operation of the driver delayed even from open(), which is possible with network devices at least but generally very complex to implement. Cheers, Ben. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 21:08 ` Benjamin Herrenschmidt @ 2012-09-05 23:18 ` Alan Cox 2012-09-06 5:06 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 21+ messages in thread From: Alan Cox @ 2012-09-05 23:18 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds > But then you have cases where probe() -> register_with_my_subsystem() -> > open(). Network devices come to mind. IE. udev must be able to deal with > a synchronous firmware load from probe I'm afraid. I don't believe so. You have begin probe find match .. .firmware_name is set issue firmware load request set defer flag end probe Later.. firmware load complete kick probe up arse begin probe find match .. .firmware is loaded call ->probe() register open happiness end probe Alan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 23:18 ` Alan Cox @ 2012-09-06 5:06 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 21+ messages in thread From: Benjamin Herrenschmidt @ 2012-09-06 5:06 UTC (permalink / raw) To: Alan Cox Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds On Thu, 2012-09-06 at 00:18 +0100, Alan Cox wrote: > > But then you have cases where probe() -> register_with_my_subsystem() -> > > open(). Network devices come to mind. IE. udev must be able to deal with > > a synchronous firmware load from probe I'm afraid. > > I don't believe so. You have > > begin probe > find match .. .firmware_name is set > issue firmware load request > set defer flag > end probe > > Later.. > > firmware load complete > kick probe up arse > > begin probe > find match .. .firmware is loaded > call ->probe() > register > open > happiness > end probe Ok, so this is my fault for picking the thread half way through and missing the crucial point ... deferring probe() itself. BTW. Deferring probe is useful for other things. I've long advocated this as a way to deal with various soft dependencies between services for example. Cases of a driver hooking up to two or 3 different busses, and wanting all the bits to be actually probed & connected to each other in order, that sort of thing. This can be done by defining the initialization order, have the "late" ones defer when the depend aren't initialized yet, and kick probe arse from bus notifiers (tho that could be moved to the core via some helpers). Cheers, Ben. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 16:30 ` Alan Cox 2012-09-05 21:08 ` Benjamin Herrenschmidt @ 2012-09-06 2:47 ` Linus Torvalds 2012-09-06 4:12 ` Ming Lei 2 siblings, 0 replies; 21+ messages in thread From: Linus Torvalds @ 2012-09-06 2:47 UTC (permalink / raw) To: Alan Cox Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Benjamin Herrenschmidt On Wed, Sep 5, 2012 at 9:30 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > > It's not about a workaround but about doing it properly for the long term > and doing it in one place. It's also not a "great change", its a small > change. udev needs to get fixed regardless. Stop this "we can break stuff" crap. Who maintains udev? Regressions are not acceptable. I'm not going to change the kernel because udev broke, f*ck it. Seriously. More projects need to realize that regressions are totally and utterly unacceptable. The "long term cleaner issues" can be handled separately, but are *not* an excuse to work around clear regressions in core packages. That just encourages those package maintainers to be shit maintainers. Just fix udev, which had a regression. And stop blaming the kernel for user space breakage! Tying these kinds of things together ("udev broke, so now we need to change the kernel") is *wrong*. It's totally unacceptable to tie the two together that way. Linus ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 16:30 ` Alan Cox 2012-09-05 21:08 ` Benjamin Herrenschmidt 2012-09-06 2:47 ` Linus Torvalds @ 2012-09-06 4:12 ` Ming Lei 2012-09-06 12:59 ` Alan Cox 2 siblings, 1 reply; 21+ messages in thread From: Ming Lei @ 2012-09-06 4:12 UTC (permalink / raw) To: Alan Cox Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds, Benjamin Herrenschmidt On Thu, Sep 6, 2012 at 12:30 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> Yes, deferring the load may fix the built in case, but which also >> introduces much work on changes of current drivers. In fact, >> there are few guys who complained the built in case. > > It fixes the modular case too. Sorry, I don't see anyone explained clearly why request_firmware() can't be called inside module_init() in module case, so maybe it is a bit early to say it is a fix on 'bug', :-) > >> The current complaint is from that udev may timeout request inside probe() >> when drivers are built as module. As pointed by Linus and Benjamin, >> it is better to fix it in udev, and looks not good to introduce great changes >> (such as Takashi's defer probe patch) in kernel to workaround the problem. > > It's not about a workaround but about doing it properly for the long term > and doing it in one place. It's also not a "great change", its a small > change. > >> Linus has said that he doesn't like to load firmware in probe(), but in some >> situation the drivers have to load firmware in its probe(): > > You don't want to load firmware in probe because of the locking problems > - you can trigger a load of another device on the same bus - the defer > dodges that nicely Yes, it is alike with the patches from Takashi and me. Also it is a kind of async probe, all these drivers may convert to async probe to fix the problem for built-in case, I guess. If it is doable, it may be a easier approach. > >> In fact, it is better for drivers to load firmware just when user wants to use >> the device, and some drivers have already changed to load firmware in >> the open() callback. > > For those devices sure but they are if anything a minority as far as I > can see. > >> So looks loading firmware always before probe in driver core is >> against the above idea. > > I never said "always" Sorry for misunderstanding your idea. > >> > firmware load off and only when the firmware had loaded would it call >> > ->probe with dev->firmware pointing at a refcounted firmware struct. >> >> IMO, introduce refcount for the firmware doesn't make sense. The lifetime >> of firmware is completely different with lifetime of driver or device: > > Exactly. Which is why the moment you have multiple devices you need > refcounts. It's also why the propsoal included a > > dev_discard_firmware() > > so you an instance can drop its firmware reference if it doesn't need it > post probe. This kind of mechanism has been implemented already: request_firmware() and release_firmware() will get and put a refcount. And, the reference count is associated with firmware name, and it should be so, IMO. > >> - firmware needn't be kept in memory in the device/driver's lifetime, and >> should be loaded just in need, and be released after downloading >> it into device. > > You broke suspend/resume for lots of devices. The firmware cache mechanism will keep the firmware during suspend/resume cycle to address the problem. > >> - sometimes devices may disappear, but it is better to keep the >> firmware in memory, for example, device may be disconnected >> during resume but will come back later. > > So the moment you have multiple instances of a device with their own > lifetime and you have the need to pin it sometimes you need a refcount > >> As said above, ref/deref on probe/remove is not a good idea since >> we needn't to keep the firmware in memory during the whole device/driver >> lifetime. > > Often you do. And in the case you don't you still have to deal with > multiple probes doing asynchronous loads of the same firmware so you want > to do matching and refcounting. It's pretty much essential. For drivers, I understand request_firmware()/request_firmware_nowait() and release_firmware() are enough. If many devices share one firmware, there is only one firmware kept in memory for their requests if one holds the firmware, and there is a refcount for it already, :-) So I don't see why it is difficult to use request/release_firmware() inside drivers, :-) > The other big value apart from making it harder for driver writers to > screw up is that it takes some of the control and puts it in one place. > That means you can change it later easily not in each driver. > > This is enabling for device drivers. With no intention of offending > driver authors the reality is that we should have driver interfaces that > > - work the right way by default > - allow driver authors to do things themselves if they need to instead > (ie opt out) > - are hard to f**k up > > because we want it to just work. Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-06 4:12 ` Ming Lei @ 2012-09-06 12:59 ` Alan Cox 2012-09-06 15:38 ` Ming Lei 0 siblings, 1 reply; 21+ messages in thread From: Alan Cox @ 2012-09-06 12:59 UTC (permalink / raw) To: Ming Lei Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds, Benjamin Herrenschmidt > Sorry, I don't see anyone explained clearly why request_firmware() > can't be called inside module_init() in module case, so maybe it is > a bit early to say it is a fix on 'bug', :-) Because the firmware load may trigger a need to load a driver to load the firmware. > > dev_discard_firmware() > > > > so you an instance can drop its firmware reference if it doesn't need it > > post probe. > > This kind of mechanism has been implemented already: request_firmware() > and release_firmware() will get and put a refcount. And, the reference > count is associated with firmware name, and it should be so, IMO. Yes - so a dev_ firmware interface is very thin. > > You broke suspend/resume for lots of devices. > > The firmware cache mechanism will keep the firmware during suspend/resume > cycle to address the problem. Ok > For drivers, I understand request_firmware()/request_firmware_nowait() > and release_firmware() are enough. If many devices share one firmware, > there is only one firmware kept in memory for their requests if one holds > the firmware, and there is a refcount for it already, :-) > > So I don't see why it is difficult to use request/release_firmware() inside > drivers, :-) The big problem can be summed up in one word "asynchronous". Having either an automated handler for it before ->probe is called or having the driver author cut and paste in if (!dev_request_firmware(dev, blah)) return -EPROBE_DEFER; avoids the need to deal with async completion after probe (and the *horrible* case of probe request firmware remove firmware ready ) in each driver Having an auto unload for it at the end is just neatness. Perhaps in fact it should be devm_request_firmware() and use the mechanism we have ? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-06 12:59 ` Alan Cox @ 2012-09-06 15:38 ` Ming Lei 0 siblings, 0 replies; 21+ messages in thread From: Ming Lei @ 2012-09-06 15:38 UTC (permalink / raw) To: Alan Cox Cc: Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds, Benjamin Herrenschmidt On Thu, Sep 6, 2012 at 8:59 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> Sorry, I don't see anyone explained clearly why request_firmware() >> can't be called inside module_init() in module case, so maybe it is >> a bit early to say it is a fix on 'bug', :-) > > Because the firmware load may trigger a need to load a driver to load the > firmware. Could you explain the problem in a bit detail? I understand once the request_firmware is called, the firmware load request is sent to userspace, so why is a driver loading involved? and what is the problem? > >> > dev_discard_firmware() >> > >> > so you an instance can drop its firmware reference if it doesn't need it >> > post probe. >> >> This kind of mechanism has been implemented already: request_firmware() >> and release_firmware() will get and put a refcount. And, the reference >> count is associated with firmware name, and it should be so, IMO. > > Yes - so a dev_ firmware interface is very thin. > >> > You broke suspend/resume for lots of devices. >> >> The firmware cache mechanism will keep the firmware during suspend/resume >> cycle to address the problem. > > Ok > >> For drivers, I understand request_firmware()/request_firmware_nowait() >> and release_firmware() are enough. If many devices share one firmware, >> there is only one firmware kept in memory for their requests if one holds >> the firmware, and there is a refcount for it already, :-) >> >> So I don't see why it is difficult to use request/release_firmware() inside >> drivers, :-) > > The big problem can be summed up in one word "asynchronous". Having OK, I agree on it, but I think it just happens in built in situation. For module case, I think the synchronous request_firmware should be OK. > either an automated handler for it before ->probe is called or having the > driver author cut and paste in > > if (!dev_request_firmware(dev, blah)) > return -EPROBE_DEFER; > > avoids the need to deal with async completion after probe (and the > *horrible* case of > > probe > request firmware > remove > > firmware ready > ) > > in each driver OK, it is one solution for asynchronous firmware loading. IMO, another candidate solution is to implement asynchronous probe for these drivers which will call request_firmware in its probe(). > > Having an auto unload for it at the end is just neatness. Perhaps in fact > it should be devm_request_firmware() and use the mechanism we have ? IMO, the kind of devm_request_firmware() mechanism may have its disadvantage because the firmware in memory may or should be released just after it has been downloaded into device, otherwise it will consume memory unnecessarily to keep it in the whole device lifetime if it will be released inside device release handler just like devm resources. Thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 13:03 ` Alan Cox 2012-09-05 14:01 ` Takashi Iwai 2012-09-05 15:22 ` Ming Lei @ 2012-09-05 16:59 ` Lucas De Marchi 2012-09-05 17:09 ` Takashi Iwai 2 siblings, 1 reply; 21+ messages in thread From: Lucas De Marchi @ 2012-09-05 16:59 UTC (permalink / raw) To: Alan Cox Cc: Ming Lei, Takashi Iwai, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds On Wed, Sep 5, 2012 at 10:03 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> If the driver is built in kernel, the request_firmware in .probe() may >> prolong kernel init, and it might be a problem. But looks it is not a >> big deal since most of drivers are built as module. > > Doing it by deferring the load also fixes that. The built in ones will > defer their final probe until the firmware appears and all will be well. > > If your rootfs needs firmware not in your initrd you already broke it and > there is a certain level beyond which you just have to give up trying to > save people from themselves. > > It may actually make sense to push more of it into the core driver layer > and take some of the ability to make mistakes away from driver authors. > For the general case of "load firmware if we see one" there isn't really > any reason we can't have a firmware_name entry in the probe table > entries themselves. If that was present the core bus probe would kick a > firmware load off and only when the firmware had loaded would it call > ->probe with dev->firmware pointing at a refcounted firmware struct. > > At that point it should be much faster to fix existing drivers and much > harder for a random device driver to get it wrong. We can even add > helpers which manage dev->firmware, and free the relevant objects when > needed, plus doing automatic ref/deref on probe/remove so that for a > typical driver the author only has to do > > {PCI_blah , ... .firmware_name="wibble500.xcr", } > > and all the loading, unloading, not loading twice happens by "magic" for > the driver author. > > Add a dev_discard_firmware() for drivers that do this and know they can > then dump the file and all is good 8) It seems like a good plan. So drivers that call request_module() inside init_module() can be easily converted to this new scheme. For those drivers that load the firmware upon open() syscal can be left as is, right? Then we can write the rule in stone: *don't call request_firmware from init_module, instead give the name of the firmware*. I even see drivers whose only purpose is to load the firmware and change the PID so it's handled by another module (like drivers/bluetooth/bcm203x.c) to be simplified by some extent. Lucas De Marchi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 16:59 ` Lucas De Marchi @ 2012-09-05 17:09 ` Takashi Iwai 0 siblings, 0 replies; 21+ messages in thread From: Takashi Iwai @ 2012-09-05 17:09 UTC (permalink / raw) To: Lucas De Marchi Cc: Alan Cox, Ming Lei, Greg Kroah-Hartman, Rafael J. Wysocki, Kay Sievers, linux-kernel, Linus Torvalds At Wed, 5 Sep 2012 13:59:56 -0300, Lucas De Marchi wrote: > > On Wed, Sep 5, 2012 at 10:03 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > >> If the driver is built in kernel, the request_firmware in .probe() may > >> prolong kernel init, and it might be a problem. But looks it is not a > >> big deal since most of drivers are built as module. > > > > Doing it by deferring the load also fixes that. The built in ones will > > defer their final probe until the firmware appears and all will be well. > > > > If your rootfs needs firmware not in your initrd you already broke it and > > there is a certain level beyond which you just have to give up trying to > > save people from themselves. > > > > It may actually make sense to push more of it into the core driver layer > > and take some of the ability to make mistakes away from driver authors. > > For the general case of "load firmware if we see one" there isn't really > > any reason we can't have a firmware_name entry in the probe table > > entries themselves. If that was present the core bus probe would kick a > > firmware load off and only when the firmware had loaded would it call > > ->probe with dev->firmware pointing at a refcounted firmware struct. > > > > At that point it should be much faster to fix existing drivers and much > > harder for a random device driver to get it wrong. We can even add > > helpers which manage dev->firmware, and free the relevant objects when > > needed, plus doing automatic ref/deref on probe/remove so that for a > > typical driver the author only has to do > > > > {PCI_blah , ... .firmware_name="wibble500.xcr", } > > > > and all the loading, unloading, not loading twice happens by "magic" for > > the driver author. > > > > Add a dev_discard_firmware() for drivers that do this and know they can > > then dump the file and all is good 8) > > > It seems like a good plan. So drivers that call request_module() > inside init_module() can be easily converted to this new scheme. > > For those drivers that load the firmware upon open() syscal can be > left as is, right? > > Then we can write the rule in stone: *don't call request_firmware from > init_module, instead give the name of the firmware*. And we can even add a WARNING() if the call still happens, once when the new implementation is done. Takashi > I even see > drivers whose only purpose is to load the firmware and change the PID > so it's handled by another module (like drivers/bluetooth/bcm203x.c) > to be simplified by some extent. > > > Lucas De Marchi > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-04 13:06 A workaround for request_firmware() stuck in module_init Takashi Iwai 2012-09-04 15:52 ` Ming Lei @ 2012-09-05 16:51 ` Lucas De Marchi 2012-09-05 17:08 ` Takashi Iwai 1 sibling, 1 reply; 21+ messages in thread From: Lucas De Marchi @ 2012-09-05 16:51 UTC (permalink / raw) To: Takashi Iwai Cc: Greg Kroah-Hartman, Ming Lei, Rafael J. Wysocki, Kay Sievers, linux-kernel On Tue, Sep 4, 2012 at 10:06 AM, Takashi Iwai <tiwai@suse.de> wrote: > Hi, > > as I've got recently a few bug reports regarding the stuck with > request_firmware() in module_init of some sound drivers, I started > looking at the issue. Strangely, the problem doesn't happen on > openSUSE 12.2 although it has the same udev version with libkmod as > Fedora. So I installed Fedora 17, and indeed I could see a problem > there. I fail to see how this is related to libkmod. It's indeed related to the fact that udev now timeout a modprobe request so people stop doing crazy things and like start daemons as modprobe's install rules. And that's true even with old module-init-tools. Lucas De Marchi ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: A workaround for request_firmware() stuck in module_init 2012-09-05 16:51 ` Lucas De Marchi @ 2012-09-05 17:08 ` Takashi Iwai 0 siblings, 0 replies; 21+ messages in thread From: Takashi Iwai @ 2012-09-05 17:08 UTC (permalink / raw) To: Lucas De Marchi Cc: Greg Kroah-Hartman, Ming Lei, Rafael J. Wysocki, Kay Sievers, linux-kernel At Wed, 5 Sep 2012 13:51:27 -0300, Lucas De Marchi wrote: > > On Tue, Sep 4, 2012 at 10:06 AM, Takashi Iwai <tiwai@suse.de> wrote: > > Hi, > > > > as I've got recently a few bug reports regarding the stuck with > > request_firmware() in module_init of some sound drivers, I started > > looking at the issue. Strangely, the problem doesn't happen on > > openSUSE 12.2 although it has the same udev version with libkmod as > > Fedora. So I installed Fedora 17, and indeed I could see a problem > > there. > > I fail to see how this is related to libkmod. It's indeed related to > the fact that udev now timeout a modprobe request so people stop doing > crazy things and like start daemons as modprobe's install rules. And > that's true even with old module-init-tools. Hm, OK, then it must be my misinterpretation. Takashi ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-09-06 15:39 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-04 13:06 A workaround for request_firmware() stuck in module_init Takashi Iwai 2012-09-04 15:52 ` Ming Lei 2012-09-04 16:10 ` Takashi Iwai 2012-09-05 1:15 ` Ming Lei 2012-09-05 5:53 ` Takashi Iwai 2012-09-05 11:32 ` Ming Lei 2012-09-05 13:03 ` Alan Cox 2012-09-05 14:01 ` Takashi Iwai 2012-09-05 15:22 ` Ming Lei 2012-09-05 16:30 ` Alan Cox 2012-09-05 21:08 ` Benjamin Herrenschmidt 2012-09-05 23:18 ` Alan Cox 2012-09-06 5:06 ` Benjamin Herrenschmidt 2012-09-06 2:47 ` Linus Torvalds 2012-09-06 4:12 ` Ming Lei 2012-09-06 12:59 ` Alan Cox 2012-09-06 15:38 ` Ming Lei 2012-09-05 16:59 ` Lucas De Marchi 2012-09-05 17:09 ` Takashi Iwai 2012-09-05 16:51 ` Lucas De Marchi 2012-09-05 17:08 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).