Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 14:49:04 +0200, Thierry Reding wrote: > > On Thu, Aug 09, 2012 at 02:32:38PM +0200, Takashi Iwai wrote: > > At Thu, 9 Aug 2012 12:34:30 +0200, > > Thierry Reding wrote: > > > > > > On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote: > > > > At Thu, 9 Aug 2012 10:07:13 +0200, > > > > Thierry Reding wrote: > > > > > > > > > > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: > > > > > > At Thu, 9 Aug 2012 09:36:42 +0200, > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > > > > > > > At Thu, 9 Aug 2012 09:08:13 +0200, > > > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > > > > > > > > > Recent changes to the firmware loading helpers cause > > > > > > > > > > > drivers to stall > > > > > > > > > > > when firmware is loaded during the module_init() call. > > > > > > > > > > > The snd-hda-intel > > > > > > > > > > > module requests firmware if the patch= parameter is used > > > > > > > > > > > to load a patch > > > > > > > > > > > file. This patch works around the problem by deferring > > > > > > > > > > > the probe in such > > > > > > > > > > > cases, which will cause the module to load successfully > > > > > > > > > > > and the driver > > > > > > > > > > > binding to the device outside the module_init() call. > > > > > > > > > > > > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > > > > > > > > > > > > > In anyway, I don't understand why such a change was > > > > > > > > > > allowed. Most > > > > > > > > > > drivers do call request_firmware() at the device probing > > > > > > > > > > time. > > > > > > > > > > If this really has to be resolved in the driver side, it > > > > > > > > > > must be a bug > > > > > > > > > > in the firmware loader core code. > > > > > > > > > > > > > > > > > > A good explanation of the problem and subsequent discussion > > > > > > > > > can be found > > > > > > > > > here: > > > > > > > > > > > > > > > > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > > > > > > > > > > > > > Yeah, but it doesn't justify this ugly module option. > > > > > > > > It's a simple bug. Papering over it with this option doesn't > > > > > > > > fix > > > > > > > > anything. > > > > > > > > > > > > > > It's not an option, all it does is defer probing if and only if > > > > > > > the > > > > > > > patch parameter was specified to make sure the firmware load won't > > > > > > > stall. I realize that this may not be an optimal solution, but at > > > > > > > least > > > > > > > it fixes the problem with no fallout. > > > > > > > > > > > > Ah sorry, I misread the patch. > > > > > > > > > > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the > > > > > > probing code was already split for vga_switcheroo support. > > > > > > > > > > Yes, I saw that. But unless you actually use vga_switcheroo, the > > > > > second > > > > > stage, azx_probe_continue(), will still be called from azx_probe() and > > > > > therefore ultimately from module_init(). > > > > > > > > Yeah, but this could be easily delayed. The split was already done, > > > > so the next step would be to return after the first half at probe, > > > > then call the second half later. > > > > > > > > > Before coming up with this patch I actually did play around a bit with > > > > > using the asynchronous firmware load functions but it turned out to be > > > > > rather difficult to do so I opted for the easy way. The biggest > > > > > problem > > > > > I faced was that since patch loading needs to be done very early on, a > > > > > lot of the initialization would need to be done after .probe() and > > > > > many > > > > > things could still fail, so cleaning up after errors would become > > > > > increasingly difficult. > > > > > > > > async probe is also on my TODO list, but it's deferred ;) > > > > > > > > > > The point you added is the second stage. > > > > > > > > > > I don't understand this sentence. > > > > > > > > I meant that your patch added the check at the second-half probing > > > > function (azx_probe_contine()). That is, it could be already the > > > > point triggered by vga_switcheroo handler, not via module_init any > > > > longer. > > > > > > > > So, after rethinking what you suggested, I wrote a quick patch below. > > > > Could you check whether this works? > > > > > > It oopses, though I can't quite tell where. I need to test some more > > > later to see where it goes wrong. > > > > Yeah, I tested it here and noticed, too. As mentioned, the behavior > > of -EPROBE_DEFER is somehow flaky. For example, when it's used for > > modules, the deferred probe will
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 02:32:38PM +0200, Takashi Iwai wrote: > At Thu, 9 Aug 2012 12:34:30 +0200, > Thierry Reding wrote: > > > > On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote: > > > At Thu, 9 Aug 2012 10:07:13 +0200, > > > Thierry Reding wrote: > > > > > > > > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: > > > > > At Thu, 9 Aug 2012 09:36:42 +0200, > > > > > Thierry Reding wrote: > > > > > > > > > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > > > > > > At Thu, 9 Aug 2012 09:08:13 +0200, > > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > > > > > > > Recent changes to the firmware loading helpers cause > > > > > > > > > > drivers to stall > > > > > > > > > > when firmware is loaded during the module_init() call. The > > > > > > > > > > snd-hda-intel > > > > > > > > > > module requests firmware if the patch= parameter is used to > > > > > > > > > > load a patch > > > > > > > > > > file. This patch works around the problem by deferring the > > > > > > > > > > probe in such > > > > > > > > > > cases, which will cause the module to load successfully and > > > > > > > > > > the driver > > > > > > > > > > binding to the device outside the module_init() call. > > > > > > > > > > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > > > > > > > > > > > In anyway, I don't understand why such a change was allowed. > > > > > > > > > Most > > > > > > > > > drivers do call request_firmware() at the device probing time. > > > > > > > > > If this really has to be resolved in the driver side, it must > > > > > > > > > be a bug > > > > > > > > > in the firmware loader core code. > > > > > > > > > > > > > > > > A good explanation of the problem and subsequent discussion can > > > > > > > > be found > > > > > > > > here: > > > > > > > > > > > > > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > > > > > > > > > > > Yeah, but it doesn't justify this ugly module option. > > > > > > > It's a simple bug. Papering over it with this option doesn't fix > > > > > > > anything. > > > > > > > > > > > > It's not an option, all it does is defer probing if and only if the > > > > > > patch parameter was specified to make sure the firmware load won't > > > > > > stall. I realize that this may not be an optimal solution, but at > > > > > > least > > > > > > it fixes the problem with no fallout. > > > > > > > > > > Ah sorry, I misread the patch. > > > > > > > > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the > > > > > probing code was already split for vga_switcheroo support. > > > > > > > > Yes, I saw that. But unless you actually use vga_switcheroo, the second > > > > stage, azx_probe_continue(), will still be called from azx_probe() and > > > > therefore ultimately from module_init(). > > > > > > Yeah, but this could be easily delayed. The split was already done, > > > so the next step would be to return after the first half at probe, > > > then call the second half later. > > > > > > > Before coming up with this patch I actually did play around a bit with > > > > using the asynchronous firmware load functions but it turned out to be > > > > rather difficult to do so I opted for the easy way. The biggest problem > > > > I faced was that since patch loading needs to be done very early on, a > > > > lot of the initialization would need to be done after .probe() and many > > > > things could still fail, so cleaning up after errors would become > > > > increasingly difficult. > > > > > > async probe is also on my TODO list, but it's deferred ;) > > > > > > > > The point you added is the second stage. > > > > > > > > I don't understand this sentence. > > > > > > I meant that your patch added the check at the second-half probing > > > function (azx_probe_contine()). That is, it could be already the > > > point triggered by vga_switcheroo handler, not via module_init any > > > longer. > > > > > > So, after rethinking what you suggested, I wrote a quick patch below. > > > Could you check whether this works? > > > > It oopses, though I can't quite tell where. I need to test some more > > later to see where it goes wrong. > > Yeah, I tested it here and noticed, too. As mentioned, the behavior > of -EPROBE_DEFER is somehow flaky. For example, when it's used for > modules, the deferred probe will be never triggered (unless a new > device is bound). Yes, the idea is that probing is only retried after other drivers have bound to other devices because otherwise nothing about any missing resources can have changed. This however would indicate that deferred probing is not the right solution here, after all we're not waiting for another
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 12:34:30 +0200, Thierry Reding wrote: > > On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote: > > At Thu, 9 Aug 2012 10:07:13 +0200, > > Thierry Reding wrote: > > > > > > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: > > > > At Thu, 9 Aug 2012 09:36:42 +0200, > > > > Thierry Reding wrote: > > > > > > > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > > > > > At Thu, 9 Aug 2012 09:08:13 +0200, > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > > > > > Recent changes to the firmware loading helpers cause drivers > > > > > > > > > to stall > > > > > > > > > when firmware is loaded during the module_init() call. The > > > > > > > > > snd-hda-intel > > > > > > > > > module requests firmware if the patch= parameter is used to > > > > > > > > > load a patch > > > > > > > > > file. This patch works around the problem by deferring the > > > > > > > > > probe in such > > > > > > > > > cases, which will cause the module to load successfully and > > > > > > > > > the driver > > > > > > > > > binding to the device outside the module_init() call. > > > > > > > > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > > > > > > > > > In anyway, I don't understand why such a change was allowed. > > > > > > > > Most > > > > > > > > drivers do call request_firmware() at the device probing time. > > > > > > > > If this really has to be resolved in the driver side, it must > > > > > > > > be a bug > > > > > > > > in the firmware loader core code. > > > > > > > > > > > > > > A good explanation of the problem and subsequent discussion can > > > > > > > be found > > > > > > > here: > > > > > > > > > > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > > > > > > > > > Yeah, but it doesn't justify this ugly module option. > > > > > > It's a simple bug. Papering over it with this option doesn't fix > > > > > > anything. > > > > > > > > > > It's not an option, all it does is defer probing if and only if the > > > > > patch parameter was specified to make sure the firmware load won't > > > > > stall. I realize that this may not be an optimal solution, but at > > > > > least > > > > > it fixes the problem with no fallout. > > > > > > > > Ah sorry, I misread the patch. > > > > > > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the > > > > probing code was already split for vga_switcheroo support. > > > > > > Yes, I saw that. But unless you actually use vga_switcheroo, the second > > > stage, azx_probe_continue(), will still be called from azx_probe() and > > > therefore ultimately from module_init(). > > > > Yeah, but this could be easily delayed. The split was already done, > > so the next step would be to return after the first half at probe, > > then call the second half later. > > > > > Before coming up with this patch I actually did play around a bit with > > > using the asynchronous firmware load functions but it turned out to be > > > rather difficult to do so I opted for the easy way. The biggest problem > > > I faced was that since patch loading needs to be done very early on, a > > > lot of the initialization would need to be done after .probe() and many > > > things could still fail, so cleaning up after errors would become > > > increasingly difficult. > > > > async probe is also on my TODO list, but it's deferred ;) > > > > > > The point you added is the second stage. > > > > > > I don't understand this sentence. > > > > I meant that your patch added the check at the second-half probing > > function (azx_probe_contine()). That is, it could be already the > > point triggered by vga_switcheroo handler, not via module_init any > > longer. > > > > So, after rethinking what you suggested, I wrote a quick patch below. > > Could you check whether this works? > > It oopses, though I can't quite tell where. I need to test some more > later to see where it goes wrong. Yeah, I tested it here and noticed, too. As mentioned, the behavior of -EPROBE_DEFER is somehow flaky. For example, when it's used for modules, the deferred probe will be never triggered (unless a new device is bound). Considering the problem again, it's currently an issue only for the built-in sound driver, right? AFAIK, request_firmware() works fine for modules. If so, a simple "fix" to avoid the unexpected behavior is to make CONFIG_SND_HDA_PATCH_LOADER depending on CONFIG_SND_HDA=m. It'd be simple enough for merging 3.6 kernel. I almost finished writing the patch to use request_firmware_nowait() version, but I'm afraid it's too intrusive for 3.6. If disabling the patch loader for the built-in driver is OK, I'd queue the request_firmware_nowait() patches
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 10:07:13 +0200,
> Thierry Reding wrote:
> >
> > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > > At Thu, 9 Aug 2012 09:36:42 +0200,
> > > Thierry Reding wrote:
> > > >
> > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > > Thierry Reding wrote:
> > > > > >
> > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > > Thierry Reding wrote:
> > > > > > > >
> > > > > > > > Recent changes to the firmware loading helpers cause drivers to
> > > > > > > > stall
> > > > > > > > when firmware is loaded during the module_init() call. The
> > > > > > > > snd-hda-intel
> > > > > > > > module requests firmware if the patch= parameter is used to
> > > > > > > > load a patch
> > > > > > > > file. This patch works around the problem by deferring the
> > > > > > > > probe in such
> > > > > > > > cases, which will cause the module to load successfully and the
> > > > > > > > driver
> > > > > > > > binding to the device outside the module_init() call.
> > > > > > >
> > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > > >
> > > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > > If this really has to be resolved in the driver side, it must be
> > > > > > > a bug
> > > > > > > in the firmware loader core code.
> > > > > >
> > > > > > A good explanation of the problem and subsequent discussion can be
> > > > > > found
> > > > > > here:
> > > > > >
> > > > > >
> > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > > >
> > > > > Yeah, but it doesn't justify this ugly module option.
> > > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > > anything.
> > > >
> > > > It's not an option, all it does is defer probing if and only if the
> > > > patch parameter was specified to make sure the firmware load won't
> > > > stall. I realize that this may not be an optimal solution, but at least
> > > > it fixes the problem with no fallout.
> > >
> > > Ah sorry, I misread the patch.
> > >
> > > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > > probing code was already split for vga_switcheroo support.
> >
> > Yes, I saw that. But unless you actually use vga_switcheroo, the second
> > stage, azx_probe_continue(), will still be called from azx_probe() and
> > therefore ultimately from module_init().
>
> Yeah, but this could be easily delayed. The split was already done,
> so the next step would be to return after the first half at probe,
> then call the second half later.
>
> > Before coming up with this patch I actually did play around a bit with
> > using the asynchronous firmware load functions but it turned out to be
> > rather difficult to do so I opted for the easy way. The biggest problem
> > I faced was that since patch loading needs to be done very early on, a
> > lot of the initialization would need to be done after .probe() and many
> > things could still fail, so cleaning up after errors would become
> > increasingly difficult.
>
> async probe is also on my TODO list, but it's deferred ;)
>
> > > The point you added is the second stage.
> >
> > I don't understand this sentence.
>
> I meant that your patch added the check at the second-half probing
> function (azx_probe_contine()). That is, it could be already the
> point triggered by vga_switcheroo handler, not via module_init any
> longer.
>
> So, after rethinking what you suggested, I wrote a quick patch below.
> Could you check whether this works?
It oopses, though I can't quite tell where. I need to test some more
later to see where it goes wrong.
Thierry
>
>
> Takashi
>
> ---
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c8aced1..4e5839a 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -559,13 +559,17 @@ enum {
> * VGA-switcher support
> */
> #ifdef SUPPORT_VGA_SWITCHEROO
> +#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
> +#else
> +#define use_vga_switcheroo(chip) 0
> +#endif
> +
> +#if defined(SUPPORT_VGA_SWITCHEROO) || defined(CONFIG_SND_HDA_PATCH_LOADER)
> #define DELAYED_INIT_MARK
> #define DELAYED_INITDATA_MARK
> -#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
> #else
> #define DELAYED_INIT_MARK__devinit
> #define DELAYED_INITDATA_MARK__devinitdata
> -#define use_vga_switcheroo(chip) 0
> #endif
>
> static char *driver_short_names[] DELAYED_INITDATA_MARK = {
> @@ -3154,6 +3158,20 @@ static int __devinit azx_probe(struct pci_dev *pci,
> struct azx *chip;
> int err;
>
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> +
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 09 Aug 2012 10:21:15 +0200, Takashi Iwai wrote: > > At Thu, 9 Aug 2012 10:07:13 +0200, > Thierry Reding wrote: > > > > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: > > > At Thu, 9 Aug 2012 09:36:42 +0200, > > > Thierry Reding wrote: > > > > > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > > > > At Thu, 9 Aug 2012 09:08:13 +0200, > > > > > Thierry Reding wrote: > > > > > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > > > Recent changes to the firmware loading helpers cause drivers to > > > > > > > > stall > > > > > > > > when firmware is loaded during the module_init() call. The > > > > > > > > snd-hda-intel > > > > > > > > module requests firmware if the patch= parameter is used to > > > > > > > > load a patch > > > > > > > > file. This patch works around the problem by deferring the > > > > > > > > probe in such > > > > > > > > cases, which will cause the module to load successfully and the > > > > > > > > driver > > > > > > > > binding to the device outside the module_init() call. > > > > > > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > > > > > > > In anyway, I don't understand why such a change was allowed. Most > > > > > > > drivers do call request_firmware() at the device probing time. > > > > > > > If this really has to be resolved in the driver side, it must be > > > > > > > a bug > > > > > > > in the firmware loader core code. > > > > > > > > > > > > A good explanation of the problem and subsequent discussion can be > > > > > > found > > > > > > here: > > > > > > > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > > > > > > > Yeah, but it doesn't justify this ugly module option. > > > > > It's a simple bug. Papering over it with this option doesn't fix > > > > > anything. > > > > > > > > It's not an option, all it does is defer probing if and only if the > > > > patch parameter was specified to make sure the firmware load won't > > > > stall. I realize that this may not be an optimal solution, but at least > > > > it fixes the problem with no fallout. > > > > > > Ah sorry, I misread the patch. > > > > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the > > > probing code was already split for vga_switcheroo support. > > > > Yes, I saw that. But unless you actually use vga_switcheroo, the second > > stage, azx_probe_continue(), will still be called from azx_probe() and > > therefore ultimately from module_init(). > > Yeah, but this could be easily delayed. The split was already done, > so the next step would be to return after the first half at probe, > then call the second half later. > > > Before coming up with this patch I actually did play around a bit with > > using the asynchronous firmware load functions but it turned out to be > > rather difficult to do so I opted for the easy way. The biggest problem > > I faced was that since patch loading needs to be done very early on, a > > lot of the initialization would need to be done after .probe() and many > > things could still fail, so cleaning up after errors would become > > increasingly difficult. > > async probe is also on my TODO list, but it's deferred ;) > > > > The point you added is the second stage. > > > > I don't understand this sentence. > > I meant that your patch added the check at the second-half probing > function (azx_probe_contine()). That is, it could be already the > point triggered by vga_switcheroo handler, not via module_init any > longer. > > So, after rethinking what you suggested, I wrote a quick patch below. > Could you check whether this works? Obviously it won't work if the module is re-loaded manually. The -EPROBE_DEFER would work only at boot time, as it seems. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 10:07:13 +0200,
Thierry Reding wrote:
>
> On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > At Thu, 9 Aug 2012 09:36:42 +0200,
> > Thierry Reding wrote:
> > >
> > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > Thierry Reding wrote:
> > > > >
> > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > Thierry Reding wrote:
> > > > > > >
> > > > > > > Recent changes to the firmware loading helpers cause drivers to
> > > > > > > stall
> > > > > > > when firmware is loaded during the module_init() call. The
> > > > > > > snd-hda-intel
> > > > > > > module requests firmware if the patch= parameter is used to load
> > > > > > > a patch
> > > > > > > file. This patch works around the problem by deferring the probe
> > > > > > > in such
> > > > > > > cases, which will cause the module to load successfully and the
> > > > > > > driver
> > > > > > > binding to the device outside the module_init() call.
> > > > > >
> > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > >
> > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > If this really has to be resolved in the driver side, it must be a
> > > > > > bug
> > > > > > in the firmware loader core code.
> > > > >
> > > > > A good explanation of the problem and subsequent discussion can be
> > > > > found
> > > > > here:
> > > > >
> > > > >
> > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > >
> > > > Yeah, but it doesn't justify this ugly module option.
> > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > anything.
> > >
> > > It's not an option, all it does is defer probing if and only if the
> > > patch parameter was specified to make sure the firmware load won't
> > > stall. I realize that this may not be an optimal solution, but at least
> > > it fixes the problem with no fallout.
> >
> > Ah sorry, I misread the patch.
> >
> > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > probing code was already split for vga_switcheroo support.
>
> Yes, I saw that. But unless you actually use vga_switcheroo, the second
> stage, azx_probe_continue(), will still be called from azx_probe() and
> therefore ultimately from module_init().
Yeah, but this could be easily delayed. The split was already done,
so the next step would be to return after the first half at probe,
then call the second half later.
> Before coming up with this patch I actually did play around a bit with
> using the asynchronous firmware load functions but it turned out to be
> rather difficult to do so I opted for the easy way. The biggest problem
> I faced was that since patch loading needs to be done very early on, a
> lot of the initialization would need to be done after .probe() and many
> things could still fail, so cleaning up after errors would become
> increasingly difficult.
async probe is also on my TODO list, but it's deferred ;)
> > The point you added is the second stage.
>
> I don't understand this sentence.
I meant that your patch added the check at the second-half probing
function (azx_probe_contine()). That is, it could be already the
point triggered by vga_switcheroo handler, not via module_init any
longer.
So, after rethinking what you suggested, I wrote a quick patch below.
Could you check whether this works?
Takashi
---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..4e5839a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -559,13 +559,17 @@ enum {
* VGA-switcher support
*/
#ifdef SUPPORT_VGA_SWITCHEROO
+#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
+#else
+#define use_vga_switcheroo(chip) 0
+#endif
+
+#if defined(SUPPORT_VGA_SWITCHEROO) || defined(CONFIG_SND_HDA_PATCH_LOADER)
#define DELAYED_INIT_MARK
#define DELAYED_INITDATA_MARK
-#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
#else
#define DELAYED_INIT_MARK __devinit
#define DELAYED_INITDATA_MARK __devinitdata
-#define use_vga_switcheroo(chip) 0
#endif
static char *driver_short_names[] DELAYED_INITDATA_MARK = {
@@ -3154,6 +3158,20 @@ static int __devinit azx_probe(struct pci_dev *pci,
struct azx *chip;
int err;
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+ /* delayed probe */
+ card = pci_get_drvdata(pci);
+ if (card) {
+ struct azx *chip = card->private_data;
+ if (chip->disabled)
+ return 0; /* will be loaded via vga_switcheroo */
+ err = azx_probe_continue(chip);
+ if (err < 0)
+ goto out_free;
+ return 0;
+ }
+#endif
+
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: > At Thu, 9 Aug 2012 09:36:42 +0200, > Thierry Reding wrote: > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > > At Thu, 9 Aug 2012 09:08:13 +0200, > > > Thierry Reding wrote: > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > > Thierry Reding wrote: > > > > > > > > > > > > Recent changes to the firmware loading helpers cause drivers to > > > > > > stall > > > > > > when firmware is loaded during the module_init() call. The > > > > > > snd-hda-intel > > > > > > module requests firmware if the patch= parameter is used to load a > > > > > > patch > > > > > > file. This patch works around the problem by deferring the probe in > > > > > > such > > > > > > cases, which will cause the module to load successfully and the > > > > > > driver > > > > > > binding to the device outside the module_init() call. > > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > > > In anyway, I don't understand why such a change was allowed. Most > > > > > drivers do call request_firmware() at the device probing time. > > > > > If this really has to be resolved in the driver side, it must be a bug > > > > > in the firmware loader core code. > > > > > > > > A good explanation of the problem and subsequent discussion can be found > > > > here: > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > > > Yeah, but it doesn't justify this ugly module option. > > > It's a simple bug. Papering over it with this option doesn't fix > > > anything. > > > > It's not an option, all it does is defer probing if and only if the > > patch parameter was specified to make sure the firmware load won't > > stall. I realize that this may not be an optimal solution, but at least > > it fixes the problem with no fallout. > > Ah sorry, I misread the patch. > > Then it shouldn't be checked at that point. Since 3.5 kernel, the > probing code was already split for vga_switcheroo support. Yes, I saw that. But unless you actually use vga_switcheroo, the second stage, azx_probe_continue(), will still be called from azx_probe() and therefore ultimately from module_init(). Before coming up with this patch I actually did play around a bit with using the asynchronous firmware load functions but it turned out to be rather difficult to do so I opted for the easy way. The biggest problem I faced was that since patch loading needs to be done very early on, a lot of the initialization would need to be done after .probe() and many things could still fail, so cleaning up after errors would become increasingly difficult. > The point you added is the second stage. I don't understand this sentence. Thierry pgp8EbInaoPo2.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 09:50:57AM +0200, Takashi Iwai wrote: > At Thu, 09 Aug 2012 09:42:48 +0200, > Takashi Iwai wrote: > > > > At Thu, 9 Aug 2012 09:36:42 +0200, > > Thierry Reding wrote: > > > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > > > At Thu, 9 Aug 2012 09:08:13 +0200, > > > > Thierry Reding wrote: > > > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > > > Thierry Reding wrote: > > > > > > > > > > > > > > Recent changes to the firmware loading helpers cause drivers to > > > > > > > stall > > > > > > > when firmware is loaded during the module_init() call. The > > > > > > > snd-hda-intel > > > > > > > module requests firmware if the patch= parameter is used to load > > > > > > > a patch > > > > > > > file. This patch works around the problem by deferring the probe > > > > > > > in such > > > > > > > cases, which will cause the module to load successfully and the > > > > > > > driver > > > > > > > binding to the device outside the module_init() call. > > > > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > > > > > In anyway, I don't understand why such a change was allowed. Most > > > > > > drivers do call request_firmware() at the device probing time. > > > > > > If this really has to be resolved in the driver side, it must be a > > > > > > bug > > > > > > in the firmware loader core code. > > > > > > > > > > A good explanation of the problem and subsequent discussion can be > > > > > found > > > > > here: > > > > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > > > > > Yeah, but it doesn't justify this ugly module option. > > > > It's a simple bug. Papering over it with this option doesn't fix > > > > anything. > > > > > > It's not an option, all it does is defer probing if and only if the > > > patch parameter was specified to make sure the firmware load won't > > > stall. I realize that this may not be an optimal solution, but at least > > > it fixes the problem with no fallout. > > > > Ah sorry, I misread the patch. > > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the > > probing code was already split for vga_switcheroo support. The point > > you added is the second stage. > > ... and the patch won't work properly if there are multiple HD-audio > controllers. Hmm. Right... the deferred probe would mess up the matching done by the static dev variable. So maybe a proper implementation that uses asynchronous firmware loading is inevitable. Thierry pgpk0jusaI1rK.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 09 Aug 2012 09:42:48 +0200, Takashi Iwai wrote: > > At Thu, 9 Aug 2012 09:36:42 +0200, > Thierry Reding wrote: > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > > At Thu, 9 Aug 2012 09:08:13 +0200, > > > Thierry Reding wrote: > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > > Thierry Reding wrote: > > > > > > > > > > > > Recent changes to the firmware loading helpers cause drivers to > > > > > > stall > > > > > > when firmware is loaded during the module_init() call. The > > > > > > snd-hda-intel > > > > > > module requests firmware if the patch= parameter is used to load a > > > > > > patch > > > > > > file. This patch works around the problem by deferring the probe in > > > > > > such > > > > > > cases, which will cause the module to load successfully and the > > > > > > driver > > > > > > binding to the device outside the module_init() call. > > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > > > In anyway, I don't understand why such a change was allowed. Most > > > > > drivers do call request_firmware() at the device probing time. > > > > > If this really has to be resolved in the driver side, it must be a bug > > > > > in the firmware loader core code. > > > > > > > > A good explanation of the problem and subsequent discussion can be found > > > > here: > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > > > Yeah, but it doesn't justify this ugly module option. > > > It's a simple bug. Papering over it with this option doesn't fix > > > anything. > > > > It's not an option, all it does is defer probing if and only if the > > patch parameter was specified to make sure the firmware load won't > > stall. I realize that this may not be an optimal solution, but at least > > it fixes the problem with no fallout. > > Ah sorry, I misread the patch. > > Then it shouldn't be checked at that point. Since 3.5 kernel, the > probing code was already split for vga_switcheroo support. The point > you added is the second stage. ... and the patch won't work properly if there are multiple HD-audio controllers. Hmm. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > > At Thu, 9 Aug 2012 09:08:13 +0200, > > Thierry Reding wrote: > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > > Thierry Reding wrote: > > > > > > > > > > Recent changes to the firmware loading helpers cause drivers to stall > > > > > when firmware is loaded during the module_init() call. The > > > > > snd-hda-intel > > > > > module requests firmware if the patch= parameter is used to load a > > > > > patch > > > > > file. This patch works around the problem by deferring the probe in > > > > > such > > > > > cases, which will cause the module to load successfully and the driver > > > > > binding to the device outside the module_init() call. > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > > > In anyway, I don't understand why such a change was allowed. Most > > > > drivers do call request_firmware() at the device probing time. > > > > If this really has to be resolved in the driver side, it must be a bug > > > > in the firmware loader core code. > > > > > > A good explanation of the problem and subsequent discussion can be found > > > here: > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > > > Yeah, but it doesn't justify this ugly module option. > > It's a simple bug. Papering over it with this option doesn't fix > > anything. > > It's not an option, all it does is defer probing if and only if the > patch parameter was specified to make sure the firmware load won't > stall. I realize that this may not be an optimal solution, but at least > it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. The point you added is the second stage. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: > At Thu, 9 Aug 2012 09:08:13 +0200, > Thierry Reding wrote: > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > > At Thu, 9 Aug 2012 08:45:23 +0200, > > > Thierry Reding wrote: > > > > > > > > Recent changes to the firmware loading helpers cause drivers to stall > > > > when firmware is loaded during the module_init() call. The snd-hda-intel > > > > module requests firmware if the patch= parameter is used to load a patch > > > > file. This patch works around the problem by deferring the probe in such > > > > cases, which will cause the module to load successfully and the driver > > > > binding to the device outside the module_init() call. > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > > > In anyway, I don't understand why such a change was allowed. Most > > > drivers do call request_firmware() at the device probing time. > > > If this really has to be resolved in the driver side, it must be a bug > > > in the firmware loader core code. > > > > A good explanation of the problem and subsequent discussion can be found > > here: > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 > > Yeah, but it doesn't justify this ugly module option. > It's a simple bug. Papering over it with this option doesn't fix > anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. A proper fix would require a larger rewrite because it would entail using the asynchronous firmware load operations. That in turn would require the initialization to be split into several stages. Thierry pgpKBr6h5feRG.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > > At Thu, 9 Aug 2012 08:45:23 +0200, > > Thierry Reding wrote: > > > > > > Recent changes to the firmware loading helpers cause drivers to stall > > > when firmware is loaded during the module_init() call. The snd-hda-intel > > > module requests firmware if the patch= parameter is used to load a patch > > > file. This patch works around the problem by deferring the probe in such > > > cases, which will cause the module to load successfully and the driver > > > binding to the device outside the module_init() call. > > > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > > > In anyway, I don't understand why such a change was allowed. Most > > drivers do call request_firmware() at the device probing time. > > If this really has to be resolved in the driver side, it must be a bug > > in the firmware loader core code. > > A good explanation of the problem and subsequent discussion can be found > here: > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: > At Thu, 9 Aug 2012 08:45:23 +0200, > Thierry Reding wrote: > > > > Recent changes to the firmware loading helpers cause drivers to stall > > when firmware is loaded during the module_init() call. The snd-hda-intel > > module requests firmware if the patch= parameter is used to load a patch > > file. This patch works around the problem by deferring the probe in such > > cases, which will cause the module to load successfully and the driver > > binding to the device outside the module_init() call. > > Is the "recent" change meant 3.6 kernel, or in linux-next? > > In anyway, I don't understand why such a change was allowed. Most > drivers do call request_firmware() at the device probing time. > If this really has to be resolved in the driver side, it must be a bug > in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Thierry pgpjtEZRoIpvj.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 08:45:23 +0200,
Thierry Reding wrote:
>
> Recent changes to the firmware loading helpers cause drivers to stall
> when firmware is loaded during the module_init() call. The snd-hda-intel
> module requests firmware if the patch= parameter is used to load a patch
> file. This patch works around the problem by deferring the probe in such
> cases, which will cause the module to load successfully and the driver
> binding to the device outside the module_init() call.
Is the "recent" change meant 3.6 kernel, or in linux-next?
In anyway, I don't understand why such a change was allowed. Most
drivers do call request_firmware() at the device probing time.
If this really has to be resolved in the driver side, it must be a bug
in the firmware loader core code.
thanks,
Takashi
>
> Signed-off-by: Thierry Reding
> ---
> sound/pci/hda/hda_intel.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c8aced1..296616e 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -69,6 +69,7 @@ static int probe_only[SNDRV_CARDS];
> static bool single_cmd;
> static int enable_msi = -1;
> #ifdef CONFIG_SND_HDA_PATCH_LOADER
> +static bool deferred[SNDRV_CARDS];
> static char *patch[SNDRV_CARDS];
> #endif
> #ifdef CONFIG_SND_HDA_INPUT_BEEP
> @@ -3156,6 +3157,16 @@ static int __devinit azx_probe(struct pci_dev *pci,
>
> if (dev >= SNDRV_CARDS)
> return -ENODEV;
> +
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> + if (patch[dev] && *patch[dev] && !deferred[dev]) {
> + snd_printk(KERN_ERR SFX "deferring probe for patch %s\n",
> +patch[dev]);
> + deferred[dev] = true;
> + return -EPROBE_DEFER;
> + }
> +#endif
> +
> if (!enable[dev]) {
> dev++;
> return -ENOENT;
> --
> 1.7.11.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 08:45:23 +0200,
Thierry Reding wrote:
Recent changes to the firmware loading helpers cause drivers to stall
when firmware is loaded during the module_init() call. The snd-hda-intel
module requests firmware if the patch= parameter is used to load a patch
file. This patch works around the problem by deferring the probe in such
cases, which will cause the module to load successfully and the driver
binding to the device outside the module_init() call.
Is the recent change meant 3.6 kernel, or in linux-next?
In anyway, I don't understand why such a change was allowed. Most
drivers do call request_firmware() at the device probing time.
If this really has to be resolved in the driver side, it must be a bug
in the firmware loader core code.
thanks,
Takashi
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
sound/pci/hda/hda_intel.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..296616e 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -69,6 +69,7 @@ static int probe_only[SNDRV_CARDS];
static bool single_cmd;
static int enable_msi = -1;
#ifdef CONFIG_SND_HDA_PATCH_LOADER
+static bool deferred[SNDRV_CARDS];
static char *patch[SNDRV_CARDS];
#endif
#ifdef CONFIG_SND_HDA_INPUT_BEEP
@@ -3156,6 +3157,16 @@ static int __devinit azx_probe(struct pci_dev *pci,
if (dev = SNDRV_CARDS)
return -ENODEV;
+
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+ if (patch[dev] *patch[dev] !deferred[dev]) {
+ snd_printk(KERN_ERR SFX deferring probe for patch %s\n,
+patch[dev]);
+ deferred[dev] = true;
+ return -EPROBE_DEFER;
+ }
+#endif
+
if (!enable[dev]) {
dev++;
return -ENOENT;
--
1.7.11.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Thierry pgpjtEZRoIpvj.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. Takashi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. A proper fix would require a larger rewrite because it would entail using the asynchronous firmware load operations. That in turn would require the initialization to be split into several stages. Thierry pgpKBr6h5feRG.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. The point you added is the second stage. Takashi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 09 Aug 2012 09:42:48 +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. The point you added is the second stage. ... and the patch won't work properly if there are multiple HD-audio controllers. Hmm. Takashi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 09:50:57AM +0200, Takashi Iwai wrote: At Thu, 09 Aug 2012 09:42:48 +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. The point you added is the second stage. ... and the patch won't work properly if there are multiple HD-audio controllers. Hmm. Right... the deferred probe would mess up the matching done by the static dev variable. So maybe a proper implementation that uses asynchronous firmware loading is inevitable. Thierry pgpk0jusaI1rK.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. Yes, I saw that. But unless you actually use vga_switcheroo, the second stage, azx_probe_continue(), will still be called from azx_probe() and therefore ultimately from module_init(). Before coming up with this patch I actually did play around a bit with using the asynchronous firmware load functions but it turned out to be rather difficult to do so I opted for the easy way. The biggest problem I faced was that since patch loading needs to be done very early on, a lot of the initialization would need to be done after .probe() and many things could still fail, so cleaning up after errors would become increasingly difficult. The point you added is the second stage. I don't understand this sentence. Thierry pgp8EbInaoPo2.pgp Description: PGP signature
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 10:07:13 +0200,
Thierry Reding wrote:
On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
At Thu, 9 Aug 2012 09:36:42 +0200,
Thierry Reding wrote:
On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
At Thu, 9 Aug 2012 09:08:13 +0200,
Thierry Reding wrote:
On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
At Thu, 9 Aug 2012 08:45:23 +0200,
Thierry Reding wrote:
Recent changes to the firmware loading helpers cause drivers to
stall
when firmware is loaded during the module_init() call. The
snd-hda-intel
module requests firmware if the patch= parameter is used to load
a patch
file. This patch works around the problem by deferring the probe
in such
cases, which will cause the module to load successfully and the
driver
binding to the device outside the module_init() call.
Is the recent change meant 3.6 kernel, or in linux-next?
In anyway, I don't understand why such a change was allowed. Most
drivers do call request_firmware() at the device probing time.
If this really has to be resolved in the driver side, it must be a
bug
in the firmware loader core code.
A good explanation of the problem and subsequent discussion can be
found
here:
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
Yeah, but it doesn't justify this ugly module option.
It's a simple bug. Papering over it with this option doesn't fix
anything.
It's not an option, all it does is defer probing if and only if the
patch parameter was specified to make sure the firmware load won't
stall. I realize that this may not be an optimal solution, but at least
it fixes the problem with no fallout.
Ah sorry, I misread the patch.
Then it shouldn't be checked at that point. Since 3.5 kernel, the
probing code was already split for vga_switcheroo support.
Yes, I saw that. But unless you actually use vga_switcheroo, the second
stage, azx_probe_continue(), will still be called from azx_probe() and
therefore ultimately from module_init().
Yeah, but this could be easily delayed. The split was already done,
so the next step would be to return after the first half at probe,
then call the second half later.
Before coming up with this patch I actually did play around a bit with
using the asynchronous firmware load functions but it turned out to be
rather difficult to do so I opted for the easy way. The biggest problem
I faced was that since patch loading needs to be done very early on, a
lot of the initialization would need to be done after .probe() and many
things could still fail, so cleaning up after errors would become
increasingly difficult.
async probe is also on my TODO list, but it's deferred ;)
The point you added is the second stage.
I don't understand this sentence.
I meant that your patch added the check at the second-half probing
function (azx_probe_contine()). That is, it could be already the
point triggered by vga_switcheroo handler, not via module_init any
longer.
So, after rethinking what you suggested, I wrote a quick patch below.
Could you check whether this works?
Takashi
---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..4e5839a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -559,13 +559,17 @@ enum {
* VGA-switcher support
*/
#ifdef SUPPORT_VGA_SWITCHEROO
+#define use_vga_switcheroo(chip) ((chip)-use_vga_switcheroo)
+#else
+#define use_vga_switcheroo(chip) 0
+#endif
+
+#if defined(SUPPORT_VGA_SWITCHEROO) || defined(CONFIG_SND_HDA_PATCH_LOADER)
#define DELAYED_INIT_MARK
#define DELAYED_INITDATA_MARK
-#define use_vga_switcheroo(chip) ((chip)-use_vga_switcheroo)
#else
#define DELAYED_INIT_MARK __devinit
#define DELAYED_INITDATA_MARK __devinitdata
-#define use_vga_switcheroo(chip) 0
#endif
static char *driver_short_names[] DELAYED_INITDATA_MARK = {
@@ -3154,6 +3158,20 @@ static int __devinit azx_probe(struct pci_dev *pci,
struct azx *chip;
int err;
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+ /* delayed probe */
+ card = pci_get_drvdata(pci);
+ if (card) {
+ struct azx *chip = card-private_data;
+ if (chip-disabled)
+ return 0; /* will be loaded via vga_switcheroo */
+ err = azx_probe_continue(chip);
+ if (err 0)
+ goto out_free;
+ return 0;
+ }
+#endif
+
if (dev = SNDRV_CARDS)
return -ENODEV;
if (!enable[dev]) {
@@ -3175,19 +3193,28 @@ static int __devinit azx_probe(struct pci_dev *pci,
goto out_free;
card-private_data = chip;
+#ifndef CONFIG_SND_HDA_PATCH_LOADER
+
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 09 Aug 2012 10:21:15 +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 10:07:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. Yes, I saw that. But unless you actually use vga_switcheroo, the second stage, azx_probe_continue(), will still be called from azx_probe() and therefore ultimately from module_init(). Yeah, but this could be easily delayed. The split was already done, so the next step would be to return after the first half at probe, then call the second half later. Before coming up with this patch I actually did play around a bit with using the asynchronous firmware load functions but it turned out to be rather difficult to do so I opted for the easy way. The biggest problem I faced was that since patch loading needs to be done very early on, a lot of the initialization would need to be done after .probe() and many things could still fail, so cleaning up after errors would become increasingly difficult. async probe is also on my TODO list, but it's deferred ;) The point you added is the second stage. I don't understand this sentence. I meant that your patch added the check at the second-half probing function (azx_probe_contine()). That is, it could be already the point triggered by vga_switcheroo handler, not via module_init any longer. So, after rethinking what you suggested, I wrote a quick patch below. Could you check whether this works? Obviously it won't work if the module is re-loaded manually. The -EPROBE_DEFER would work only at boot time, as it seems. Takashi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote:
At Thu, 9 Aug 2012 10:07:13 +0200,
Thierry Reding wrote:
On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
At Thu, 9 Aug 2012 09:36:42 +0200,
Thierry Reding wrote:
On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
At Thu, 9 Aug 2012 09:08:13 +0200,
Thierry Reding wrote:
On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
At Thu, 9 Aug 2012 08:45:23 +0200,
Thierry Reding wrote:
Recent changes to the firmware loading helpers cause drivers to
stall
when firmware is loaded during the module_init() call. The
snd-hda-intel
module requests firmware if the patch= parameter is used to
load a patch
file. This patch works around the problem by deferring the
probe in such
cases, which will cause the module to load successfully and the
driver
binding to the device outside the module_init() call.
Is the recent change meant 3.6 kernel, or in linux-next?
In anyway, I don't understand why such a change was allowed. Most
drivers do call request_firmware() at the device probing time.
If this really has to be resolved in the driver side, it must be
a bug
in the firmware loader core code.
A good explanation of the problem and subsequent discussion can be
found
here:
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
Yeah, but it doesn't justify this ugly module option.
It's a simple bug. Papering over it with this option doesn't fix
anything.
It's not an option, all it does is defer probing if and only if the
patch parameter was specified to make sure the firmware load won't
stall. I realize that this may not be an optimal solution, but at least
it fixes the problem with no fallout.
Ah sorry, I misread the patch.
Then it shouldn't be checked at that point. Since 3.5 kernel, the
probing code was already split for vga_switcheroo support.
Yes, I saw that. But unless you actually use vga_switcheroo, the second
stage, azx_probe_continue(), will still be called from azx_probe() and
therefore ultimately from module_init().
Yeah, but this could be easily delayed. The split was already done,
so the next step would be to return after the first half at probe,
then call the second half later.
Before coming up with this patch I actually did play around a bit with
using the asynchronous firmware load functions but it turned out to be
rather difficult to do so I opted for the easy way. The biggest problem
I faced was that since patch loading needs to be done very early on, a
lot of the initialization would need to be done after .probe() and many
things could still fail, so cleaning up after errors would become
increasingly difficult.
async probe is also on my TODO list, but it's deferred ;)
The point you added is the second stage.
I don't understand this sentence.
I meant that your patch added the check at the second-half probing
function (azx_probe_contine()). That is, it could be already the
point triggered by vga_switcheroo handler, not via module_init any
longer.
So, after rethinking what you suggested, I wrote a quick patch below.
Could you check whether this works?
It oopses, though I can't quite tell where. I need to test some more
later to see where it goes wrong.
Thierry
Takashi
---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..4e5839a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -559,13 +559,17 @@ enum {
* VGA-switcher support
*/
#ifdef SUPPORT_VGA_SWITCHEROO
+#define use_vga_switcheroo(chip) ((chip)-use_vga_switcheroo)
+#else
+#define use_vga_switcheroo(chip) 0
+#endif
+
+#if defined(SUPPORT_VGA_SWITCHEROO) || defined(CONFIG_SND_HDA_PATCH_LOADER)
#define DELAYED_INIT_MARK
#define DELAYED_INITDATA_MARK
-#define use_vga_switcheroo(chip) ((chip)-use_vga_switcheroo)
#else
#define DELAYED_INIT_MARK__devinit
#define DELAYED_INITDATA_MARK__devinitdata
-#define use_vga_switcheroo(chip) 0
#endif
static char *driver_short_names[] DELAYED_INITDATA_MARK = {
@@ -3154,6 +3158,20 @@ static int __devinit azx_probe(struct pci_dev *pci,
struct azx *chip;
int err;
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+ /* delayed probe */
+ card = pci_get_drvdata(pci);
+ if (card) {
+ struct azx *chip = card-private_data;
+ if (chip-disabled)
+ return 0; /* will be loaded via vga_switcheroo */
+ err = azx_probe_continue(chip);
+ if (err 0)
+ goto out_free;
+ return 0;
+ }
+#endif
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 12:34:30 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 10:07:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. Yes, I saw that. But unless you actually use vga_switcheroo, the second stage, azx_probe_continue(), will still be called from azx_probe() and therefore ultimately from module_init(). Yeah, but this could be easily delayed. The split was already done, so the next step would be to return after the first half at probe, then call the second half later. Before coming up with this patch I actually did play around a bit with using the asynchronous firmware load functions but it turned out to be rather difficult to do so I opted for the easy way. The biggest problem I faced was that since patch loading needs to be done very early on, a lot of the initialization would need to be done after .probe() and many things could still fail, so cleaning up after errors would become increasingly difficult. async probe is also on my TODO list, but it's deferred ;) The point you added is the second stage. I don't understand this sentence. I meant that your patch added the check at the second-half probing function (azx_probe_contine()). That is, it could be already the point triggered by vga_switcheroo handler, not via module_init any longer. So, after rethinking what you suggested, I wrote a quick patch below. Could you check whether this works? It oopses, though I can't quite tell where. I need to test some more later to see where it goes wrong. Yeah, I tested it here and noticed, too. As mentioned, the behavior of -EPROBE_DEFER is somehow flaky. For example, when it's used for modules, the deferred probe will be never triggered (unless a new device is bound). Considering the problem again, it's currently an issue only for the built-in sound driver, right? AFAIK, request_firmware() works fine for modules. If so, a simple fix to avoid the unexpected behavior is to make CONFIG_SND_HDA_PATCH_LOADER depending on CONFIG_SND_HDA=m. It'd be simple enough for merging 3.6 kernel. I almost finished writing the patch to use request_firmware_nowait() version, but I'm afraid it's too intrusive for 3.6. If disabling the patch loader for the built-in driver is OK, I'd queue the request_firmware_nowait() patches to 3.7 queue. Takashi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
On Thu, Aug 09, 2012 at 02:32:38PM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 12:34:30 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 10:07:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. Yes, I saw that. But unless you actually use vga_switcheroo, the second stage, azx_probe_continue(), will still be called from azx_probe() and therefore ultimately from module_init(). Yeah, but this could be easily delayed. The split was already done, so the next step would be to return after the first half at probe, then call the second half later. Before coming up with this patch I actually did play around a bit with using the asynchronous firmware load functions but it turned out to be rather difficult to do so I opted for the easy way. The biggest problem I faced was that since patch loading needs to be done very early on, a lot of the initialization would need to be done after .probe() and many things could still fail, so cleaning up after errors would become increasingly difficult. async probe is also on my TODO list, but it's deferred ;) The point you added is the second stage. I don't understand this sentence. I meant that your patch added the check at the second-half probing function (azx_probe_contine()). That is, it could be already the point triggered by vga_switcheroo handler, not via module_init any longer. So, after rethinking what you suggested, I wrote a quick patch below. Could you check whether this works? It oopses, though I can't quite tell where. I need to test some more later to see where it goes wrong. Yeah, I tested it here and noticed, too. As mentioned, the behavior of -EPROBE_DEFER is somehow flaky. For example, when it's used for modules, the deferred probe will be never triggered (unless a new device is bound). Yes, the idea is that probing is only retried after other drivers have bound to other devices because otherwise nothing about any missing resources can have changed. This however would indicate that deferred probing is not the right solution here, after all we're not waiting for another resource to become available. Something like delayed work might be better suited. Well, asynchronous firmware load is actually the right solution, delayed work just might be a better workaround. =) For completeness I should say that I've been using deferred probing with modules quite successfully on another platform, so it is not a general problem. Rather as you said, it is only triggered if another module is loaded after the deferral. Considering the problem again, it's currently an issue only for the built-in sound driver, right? AFAIK,
Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware
At Thu, 9 Aug 2012 14:49:04 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 02:32:38PM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 12:34:30 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 10:07:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:36:42 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 09:08:13 +0200, Thierry Reding wrote: On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote: At Thu, 9 Aug 2012 08:45:23 +0200, Thierry Reding wrote: Recent changes to the firmware loading helpers cause drivers to stall when firmware is loaded during the module_init() call. The snd-hda-intel module requests firmware if the patch= parameter is used to load a patch file. This patch works around the problem by deferring the probe in such cases, which will cause the module to load successfully and the driver binding to the device outside the module_init() call. Is the recent change meant 3.6 kernel, or in linux-next? In anyway, I don't understand why such a change was allowed. Most drivers do call request_firmware() at the device probing time. If this really has to be resolved in the driver side, it must be a bug in the firmware loader core code. A good explanation of the problem and subsequent discussion can be found here: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975 Yeah, but it doesn't justify this ugly module option. It's a simple bug. Papering over it with this option doesn't fix anything. It's not an option, all it does is defer probing if and only if the patch parameter was specified to make sure the firmware load won't stall. I realize that this may not be an optimal solution, but at least it fixes the problem with no fallout. Ah sorry, I misread the patch. Then it shouldn't be checked at that point. Since 3.5 kernel, the probing code was already split for vga_switcheroo support. Yes, I saw that. But unless you actually use vga_switcheroo, the second stage, azx_probe_continue(), will still be called from azx_probe() and therefore ultimately from module_init(). Yeah, but this could be easily delayed. The split was already done, so the next step would be to return after the first half at probe, then call the second half later. Before coming up with this patch I actually did play around a bit with using the asynchronous firmware load functions but it turned out to be rather difficult to do so I opted for the easy way. The biggest problem I faced was that since patch loading needs to be done very early on, a lot of the initialization would need to be done after .probe() and many things could still fail, so cleaning up after errors would become increasingly difficult. async probe is also on my TODO list, but it's deferred ;) The point you added is the second stage. I don't understand this sentence. I meant that your patch added the check at the second-half probing function (azx_probe_contine()). That is, it could be already the point triggered by vga_switcheroo handler, not via module_init any longer. So, after rethinking what you suggested, I wrote a quick patch below. Could you check whether this works? It oopses, though I can't quite tell where. I need to test some more later to see where it goes wrong. Yeah, I tested it here and noticed, too. As mentioned, the behavior of -EPROBE_DEFER is somehow flaky. For example, when it's used for modules, the deferred probe will be never triggered (unless a new device is bound). Yes, the idea is that probing is only retried after other drivers have bound to other devices because otherwise nothing about any missing resources can have changed. This however would indicate that deferred probing is not the right solution here, after all we're not waiting for another resource to become available. Something like delayed work might be better suited. Well, asynchronous firmware load is actually the right solution, delayed work just might be a better workaround. =) For completeness I should say that I've been using deferred probing with modules quite successfully on another platform, so it is not a
