[PATCH] ALSA: hda - Defer probe when loading patch firmware

[PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

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.

Signed-off-by: Thierry Reding <thierry.reding@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

[PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

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.

Signed-off-by: Thierry Reding <thierry.reding@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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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.reding@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
> 

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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.reding@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
> 

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1070 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1885 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2611 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2858 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2858 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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
+	/* continue probing if no patch loader is required */
 	if (!chip->disabled) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
 			goto out_free;
 	}
+#endif
 
 	pci_set_drvdata(pci, card);
 
 	dev++;
+
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+	return -EPROBE_DEFER; /* continue probing later for request_firmware() */
+#else
 	return 0;
+#endif
 
 out_free:
 	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
 	return err;
 }
 

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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
+	/* continue probing if no patch loader is required */
 	if (!chip->disabled) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
 			goto out_free;
 	}
+#endif
 
 	pci_set_drvdata(pci, card);
 
 	dev++;
+
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+	return -EPROBE_DEFER; /* continue probing later for request_firmware() */
+#else
 	return 0;
+#endif
 
 out_free:
 	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
 	return err;
 }
 

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6031 bytes --]

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
> +
>  	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
> +	/* continue probing if no patch loader is required */
>  	if (!chip->disabled) {
>  		err = azx_probe_continue(chip);
>  		if (err < 0)
>  			goto out_free;
>  	}
> +#endif
>  
>  	pci_set_drvdata(pci, card);
>  
>  	dev++;
> +
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> +	return -EPROBE_DEFER; /* continue probing later for request_firmware() */
> +#else
>  	return 0;
> +#endif
>  
>  out_free:
>  	snd_card_free(card);
> +	pci_set_drvdata(pci, NULL);
>  	return err;
>  }
>  
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6031 bytes --]

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
> +
>  	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
> +	/* continue probing if no patch loader is required */
>  	if (!chip->disabled) {
>  		err = azx_probe_continue(chip);
>  		if (err < 0)
>  			goto out_free;
>  	}
> +#endif
>  
>  	pci_set_drvdata(pci, card);
>  
>  	dev++;
> +
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> +	return -EPROBE_DEFER; /* continue probing later for request_firmware() */
> +#else
>  	return 0;
> +#endif
>  
>  out_free:
>  	snd_card_free(card);
> +	pci_set_drvdata(pci, NULL);
>  	return err;
>  }
>  
> 
> 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6341 bytes --]

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, 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've actually seen this problem with snd-hda-intel built as a module as
well, so I don't think this kind of temporary fix will do.

> 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.

I'm in no hurry, and the patch that I carry works for me. If there are
no other reasons to pull a corresponding fix into 3.6 I can certainly
wait for 3.7. The particular setup that I have requires other patches
that may not go into 3.6 anyway.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6341 bytes --]

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, 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've actually seen this problem with snd-hda-intel built as a module as
well, so I don't think this kind of temporary fix will do.

> 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.

I'm in no hurry, and the patch that I carry works for me. If there are
no other reasons to pull a corresponding fix into 3.6 I can certainly
wait for 3.7. The particular setup that I have requires other patches
that may not go into 3.6 anyway.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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 general
> problem. Rather as you said, it is only triggered if another module is
> loaded after the deferral.

Yes.  But if you reload just snd-hda-intel module, it'll be stuck.

> > 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've actually seen this problem with snd-hda-intel built as a module as
> well, so I don't think this kind of temporary fix will do.

Hmm, for modules, request_firmware() seems working fine on my systems,
no matter whether it succeeds or not.  Could you double-check?
If the user-helper isn't ready at module loading time, of course, it
stalls, just like the kernel driver.  But it should be available
there.

> > 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.
> 
> I'm in no hurry, and the patch that I carry works for me. If there are
> no other reasons to pull a corresponding fix into 3.6 I can certainly
> wait for 3.7. The particular setup that I have requires other patches
> that may not go into 3.6 anyway.

OK.


Takashi

Re: [PATCH] ALSA: hda - Defer probe when loading patch firmware

[Takashi Iwai]

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 general
> problem. Rather as you said, it is only triggered if another module is
> loaded after the deferral.

Yes.  But if you reload just snd-hda-intel module, it'll be stuck.

> > 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've actually seen this problem with snd-hda-intel built as a module as
> well, so I don't think this kind of temporary fix will do.

Hmm, for modules, request_firmware() seems working fine on my systems,
no matter whether it succeeds or not.  Could you double-check?
If the user-helper isn't ready at module loading time, of course, it
stalls, just like the kernel driver.  But it should be available
there.

> > 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.
> 
> I'm in no hurry, and the patch that I carry works for me. If there are
> no other reasons to pull a corresponding fix into 3.6 I can certainly
> wait for 3.7. The particular setup that I have requires other patches
> that may not go into 3.6 anyway.

OK.


Takashi

[PATCH 0/2] HD-audio firmware loading fixes

[Takashi Iwai]

Hi,

this is a series of patches to fix the firmware loading problem.
I split the changes to two for making clean what the patches do.

Basically it moves request_firmware() to hda_intel.c, then replaces it
with request_firmware_nowait().  Through a short testing, it seems
working fine here on my machines.


Takashi

[PATCH 1/2] ALSA: hda - Load firmware in hda_intel.c

[Takashi Iwai]

This is a preliminary work for the deferred probing for
request_firmware() errors at init.

This patch moves the call of request_firmware() to hda_intel.c, and
call it in the earlier stage of probing rather than
azx_probe_continue().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.h |  2 +-
 sound/pci/hda/hda_hwdep.c | 39 +++++++++++++--------------------------
 sound/pci/hda/hda_intel.c | 29 ++++++++++++++++++++++++-----
 3 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index c422d33..39e43753 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -1072,7 +1072,7 @@ static inline void snd_hda_power_down(struct hda_codec *codec) {}
 /*
  * patch firmware
  */
-int snd_hda_load_patch(struct hda_bus *bus, const char *patch);
+int snd_hda_load_patch(struct hda_bus *bus, size_t size, const void *buf);
 #endif
 
 /*
diff --git a/sound/pci/hda/hda_hwdep.c b/sound/pci/hda/hda_hwdep.c
index 6b2efb8..b9a644c 100644
--- a/sound/pci/hda/hda_hwdep.c
+++ b/sound/pci/hda/hda_hwdep.c
@@ -25,7 +25,6 @@
 #include <linux/mutex.h>
 #include <linux/ctype.h>
 #include <linux/string.h>
-#include <linux/firmware.h>
 #include <linux/export.h>
 #include <sound/core.h>
 #include "hda_codec.h"
@@ -747,18 +746,21 @@ static int parse_line_mode(char *buf, struct hda_bus *bus)
  *
  * the spaces at the beginning and the end of the line are stripped
  */
-static int get_line_from_fw(char *buf, int size, struct firmware *fw)
+static int get_line_from_fw(char *buf, int size, size_t *fw_size_p,
+			    const void **fw_data_p)
 {
 	int len;
-	const char *p = fw->data;
-	while (isspace(*p) && fw->size) {
+	size_t fw_size = *fw_size_p;
+	const char *p = *fw_data_p;
+
+	while (isspace(*p) && fw_size) {
 		p++;
-		fw->size--;
+		fw_size--;
 	}
-	if (!fw->size)
+	if (!fw_size)
 		return 0;
 
-	for (len = 0; len < fw->size; len++) {
+	for (len = 0; len < fw_size; len++) {
 		if (!*p)
 			break;
 		if (*p == '\n') {
@@ -770,8 +772,8 @@ static int get_line_from_fw(char *buf, int size, struct firmware *fw)
 			*buf++ = *p++;
 	}
 	*buf = 0;
-	fw->size -= len;
-	fw->data = p;
+	*fw_size_p = fw_size - len;
+	*fw_data_p = p;
 	remove_trail_spaces(buf);
 	return 1;
 }
@@ -779,29 +781,15 @@ static int get_line_from_fw(char *buf, int size, struct firmware *fw)
 /*
  * load a "patch" firmware file and parse it
  */
-int snd_hda_load_patch(struct hda_bus *bus, const char *patch)
+int snd_hda_load_patch(struct hda_bus *bus, size_t fw_size, const void *fw_buf)
 {
-	int err;
-	const struct firmware *fw;
-	struct firmware tmp;
 	char buf[128];
 	struct hda_codec *codec;
 	int line_mode;
-	struct device *dev = bus->card->dev;
-
-	if (snd_BUG_ON(!dev))
-		return -ENODEV;
-	err = request_firmware(&fw, patch, dev);
-	if (err < 0) {
-		printk(KERN_ERR "hda-codec: Cannot load the patch '%s'\n",
-		       patch);
-		return err;
-	}
 
-	tmp = *fw;
 	line_mode = LINE_MODE_NONE;
 	codec = NULL;
-	while (get_line_from_fw(buf, sizeof(buf) - 1, &tmp)) {
+	while (get_line_from_fw(buf, sizeof(buf) - 1, &fw_size, &fw_buf)) {
 		if (!*buf || *buf == '#' || *buf == '\n')
 			continue;
 		if (*buf == '[')
@@ -810,7 +798,6 @@ int snd_hda_load_patch(struct hda_bus *bus, const char *patch)
 			 (codec || !patch_items[line_mode].need_codec))
 			patch_items[line_mode].parser(buf, bus, &codec);
 	}
-	release_firmware(fw);
 	return 0;
 }
 EXPORT_SYMBOL_HDA(snd_hda_load_patch);
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..b25d539 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -55,6 +55,7 @@
 #include <sound/initval.h>
 #include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
+#include <linux/firmware.h>
 #include "hda_codec.h"
 
 
@@ -470,6 +471,10 @@ struct azx {
 	struct snd_dma_buffer rb;
 	struct snd_dma_buffer posbuf;
 
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+	const struct firmware *fw;
+#endif
+
 	/* flags */
 	int position_fix[2]; /* for both playback/capture streams */
 	int poll_count;
@@ -2639,6 +2644,10 @@ static int azx_free(struct azx *chip)
 		pci_release_regions(chip->pci);
 	pci_disable_device(chip->pci);
 	kfree(chip->azx_dev);
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+	if (chip->fw)
+		release_firmware(chip->fw);
+#endif
 	kfree(chip);
 
 	return 0;
@@ -3167,7 +3176,6 @@ static int __devinit azx_probe(struct pci_dev *pci,
 		return err;
 	}
 
-	/* set this here since it's referred in snd_hda_load_patch() */
 	snd_card_set_dev(card, &pci->dev);
 
 	err = azx_create(card, pci, dev, pci_id->driver_data, &chip);
@@ -3175,6 +3183,16 @@ static int __devinit azx_probe(struct pci_dev *pci,
 		goto out_free;
 	card->private_data = chip;
 
+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+	if (patch[dev] && *patch[dev]) {
+		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
+			   patch[dev]);
+		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
+		if (err < 0)
+			goto out_free;
+	}
+#endif /* CONFIG_SND_HDA_PATCH_LOADER */
+
 	if (!chip->disabled) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
@@ -3205,12 +3223,13 @@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip)
 	if (err < 0)
 		goto out_free;
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
-	if (patch[dev] && *patch[dev]) {
-		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
-			   patch[dev]);
-		err = snd_hda_load_patch(chip->bus, patch[dev]);
+	if (chip->fw) {
+		err = snd_hda_load_patch(chip->bus, chip->fw->size,
+					 chip->fw->data);
 		if (err < 0)
 			goto out_free;
+		release_firmware(chip->fw); /* no longer needed */
+		chip->fw = NULL;
 	}
 #endif
 	if ((probe_only[dev] & 1) == 0) {
-- 
1.7.11.4

[PATCH 2/2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

For processing the firmware handling properly for built-in kernels,
implement an asynchronous f/w loading with request_firmware_nowait().
This means that the codec probing is deferred when the patch option is
specified.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.c |  2 +-
 sound/pci/hda/hda_intel.c | 41 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 88a9c20..408babc 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4184,7 +4184,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
  *
  * This function returns 0 if successful, or a negative error code.
  */
-int __devinit snd_hda_build_pcms(struct hda_bus *bus)
+int snd_hda_build_pcms(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
 
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b25d539..06e7897 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -493,6 +493,7 @@ struct azx {
 	unsigned int use_vga_switcheroo:1;
 	unsigned int init_failed:1; /* delayed init failed */
 	unsigned int disabled:1; /* disabled by VGA-switcher */
+	unsigned int probe_deferred:1; /* deferred probing for patch loader */
 
 	/* for debugging */
 	unsigned int last_cmd[AZX_MAX_CODECS];
@@ -564,13 +565,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 = {
@@ -3155,6 +3160,31 @@ static void power_down_all_codecs(struct azx *chip)
 #endif
 }
 
+/* callback from request_firmware_nowait() */
+static void azx_firmware_cb(const struct firmware *fw, void *context)
+{
+	struct snd_card *card = context;
+	struct azx *chip = card->private_data;
+	struct pci_dev *pci = chip->pci;
+
+	if (!fw) {
+		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
+		goto error;
+	}
+
+	chip->probe_deferred = 0;
+	if (!chip->disabled) {
+		/* continue probing */
+		if (azx_probe_continue(chip))
+			goto error;
+	}
+	return; /* OK */
+
+ error:
+	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
+}
+
 static int __devinit azx_probe(struct pci_dev *pci,
 			       const struct pci_device_id *pci_id)
 {
@@ -3187,13 +3217,16 @@ static int __devinit azx_probe(struct pci_dev *pci,
 	if (patch[dev] && *patch[dev]) {
 		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
 			   patch[dev]);
-		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
+		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
+					      &pci->dev, GFP_KERNEL, card,
+					      azx_firmware_cb);
 		if (err < 0)
 			goto out_free;
+		chip->probe_deferred = 1;
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
-	if (!chip->disabled) {
+	if (!chip->disabled && !chip->probe_deferred) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
 			goto out_free;
-- 
1.7.11.4

Re: [PATCH 2/2] ALSA: hda - Deferred probing with request_firmware_nowait()

[David Henningsson]

On 08/09/2012 03:11 PM, Takashi Iwai wrote:

> @@ -3187,13 +3217,16 @@ static int __devinit azx_probe(struct pci_dev *pci,
>   	if (patch[dev] && *patch[dev]) {
>   		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
>   			   patch[dev]);
> -		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
> +		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
> +					      &pci->dev, GFP_KERNEL, card,
> +					      azx_firmware_cb);
>   		if (err < 0)
>   			goto out_free;
> +		chip->probe_deferred = 1;

I might be out on deep water here, but isn't this racy? Or is 
azx_firmware_cb somehow guaranteed not to execute before this function 
has exited?

>   	}
>   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>
> -	if (!chip->disabled) {
> +	if (!chip->disabled && !chip->probe_deferred) {
>   		err = azx_probe_continue(chip);
>   		if (err < 0)
>   			goto out_free;
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

At Thu, 09 Aug 2012 15:26:56 +0200,
David Henningsson wrote:
> 
> On 08/09/2012 03:11 PM, Takashi Iwai wrote:
> 
> > @@ -3187,13 +3217,16 @@ static int __devinit azx_probe(struct pci_dev *pci,
> >   	if (patch[dev] && *patch[dev]) {
> >   		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
> >   			   patch[dev]);
> > -		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
> > +		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
> > +					      &pci->dev, GFP_KERNEL, card,
> > +					      azx_firmware_cb);
> >   		if (err < 0)
> >   			goto out_free;
> > +		chip->probe_deferred = 1;
> 
> I might be out on deep water here, but isn't this racy? Or is 
> azx_firmware_cb somehow guaranteed not to execute before this function 
> has exited?

chip->probe_deferred is used only at the line below.
So basically it's not necessarily to be recorded in chip, but could be
a local bool variable in azx_probe().  It remains in chip just because
of my older patch had it.

> 
> >   	}
> >   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
> >
> > -	if (!chip->disabled) {
> > +	if (!chip->disabled && !chip->probe_deferred) {

Here it's checked.


Takashi

> >   		err = azx_probe_continue(chip);
> >   		if (err < 0)
> >   			goto out_free;
> >
> 
> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> 

Re: [PATCH 2/2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

At Thu, 09 Aug 2012 15:26:56 +0200,
David Henningsson wrote:
> 
> On 08/09/2012 03:11 PM, Takashi Iwai wrote:
> 
> > @@ -3187,13 +3217,16 @@ static int __devinit azx_probe(struct pci_dev *pci,
> >   	if (patch[dev] && *patch[dev]) {
> >   		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
> >   			   patch[dev]);
> > -		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
> > +		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
> > +					      &pci->dev, GFP_KERNEL, card,
> > +					      azx_firmware_cb);
> >   		if (err < 0)
> >   			goto out_free;
> > +		chip->probe_deferred = 1;
> 
> I might be out on deep water here, but isn't this racy? Or is 
> azx_firmware_cb somehow guaranteed not to execute before this function 
> has exited?

chip->probe_deferred is used only at the line below.
So basically it's not necessarily to be recorded in chip, but could be
a local bool variable in azx_probe().  It remains in chip just because
of my older patch had it.

> 
> >   	}
> >   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
> >
> > -	if (!chip->disabled) {
> > +	if (!chip->disabled && !chip->probe_deferred) {

Here it's checked.


Takashi

> >   		err = azx_probe_continue(chip);
> >   		if (err < 0)
> >   			goto out_free;
> >
> 
> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> 

[PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

For processing the firmware handling properly for built-in kernels,
implement an asynchronous f/w loading with request_firmware_nowait().
This means that the codec probing is deferred when the patch option is
specified.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: drop superfluous chip->probe_deferred field

 sound/pci/hda/hda_codec.c |  2 +-
 sound/pci/hda/hda_intel.c | 41 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 88a9c20..408babc 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4184,7 +4184,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
  *
  * This function returns 0 if successful, or a negative error code.
  */
-int __devinit snd_hda_build_pcms(struct hda_bus *bus)
+int snd_hda_build_pcms(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
 
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b25d539..717acb4 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -564,13 +564,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 = {
@@ -3155,12 +3159,37 @@ static void power_down_all_codecs(struct azx *chip)
 #endif
 }
 
+/* callback from request_firmware_nowait() */
+static void azx_firmware_cb(const struct firmware *fw, void *context)
+{
+	struct snd_card *card = context;
+	struct azx *chip = card->private_data;
+	struct pci_dev *pci = chip->pci;
+
+	if (!fw) {
+		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
+		goto error;
+	}
+
+	if (!chip->disabled) {
+		/* continue probing */
+		if (azx_probe_continue(chip))
+			goto error;
+	}
+	return; /* OK */
+
+ error:
+	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
+}
+
 static int __devinit azx_probe(struct pci_dev *pci,
 			       const struct pci_device_id *pci_id)
 {
 	static int dev;
 	struct snd_card *card;
 	struct azx *chip;
+	bool probe_deferred;
 	int err;
 
 	if (dev >= SNDRV_CARDS)
@@ -3182,18 +3211,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
 	if (err < 0)
 		goto out_free;
 	card->private_data = chip;
+	probe_deferred = chip->disabled;
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	if (patch[dev] && *patch[dev]) {
 		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
 			   patch[dev]);
-		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
+		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
+					      &pci->dev, GFP_KERNEL, card,
+					      azx_firmware_cb);
 		if (err < 0)
 			goto out_free;
+		probe_deferred = true;
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
-	if (!chip->disabled) {
+	if (!probe_deferred) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
 			goto out_free;
-- 
1.7.11.4

[PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

For processing the firmware handling properly for built-in kernels,
implement an asynchronous f/w loading with request_firmware_nowait().
This means that the codec probing is deferred when the patch option is
specified.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: drop superfluous chip->probe_deferred field

 sound/pci/hda/hda_codec.c |  2 +-
 sound/pci/hda/hda_intel.c | 41 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 88a9c20..408babc 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4184,7 +4184,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
  *
  * This function returns 0 if successful, or a negative error code.
  */
-int __devinit snd_hda_build_pcms(struct hda_bus *bus)
+int snd_hda_build_pcms(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
 
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b25d539..717acb4 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -564,13 +564,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 = {
@@ -3155,12 +3159,37 @@ static void power_down_all_codecs(struct azx *chip)
 #endif
 }
 
+/* callback from request_firmware_nowait() */
+static void azx_firmware_cb(const struct firmware *fw, void *context)
+{
+	struct snd_card *card = context;
+	struct azx *chip = card->private_data;
+	struct pci_dev *pci = chip->pci;
+
+	if (!fw) {
+		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
+		goto error;
+	}
+
+	if (!chip->disabled) {
+		/* continue probing */
+		if (azx_probe_continue(chip))
+			goto error;
+	}
+	return; /* OK */
+
+ error:
+	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
+}
+
 static int __devinit azx_probe(struct pci_dev *pci,
 			       const struct pci_device_id *pci_id)
 {
 	static int dev;
 	struct snd_card *card;
 	struct azx *chip;
+	bool probe_deferred;
 	int err;
 
 	if (dev >= SNDRV_CARDS)
@@ -3182,18 +3211,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
 	if (err < 0)
 		goto out_free;
 	card->private_data = chip;
+	probe_deferred = chip->disabled;
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	if (patch[dev] && *patch[dev]) {
 		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
 			   patch[dev]);
-		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
+		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
+					      &pci->dev, GFP_KERNEL, card,
+					      azx_firmware_cb);
 		if (err < 0)
 			goto out_free;
+		probe_deferred = true;
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
-	if (!chip->disabled) {
+	if (!probe_deferred) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
 			goto out_free;
-- 
1.7.11.4

Re: [PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[David Henningsson]

On 08/09/2012 03:36 PM, Takashi Iwai wrote:
> +/* callback from request_firmware_nowait() */
> +static void azx_firmware_cb(const struct firmware *fw, void *context)
> +{
> +	struct snd_card *card = context;
> +	struct azx *chip = card->private_data;
> +	struct pci_dev *pci = chip->pci;
> +
> +	if (!fw) {
> +		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> +		goto error;
> +	}

Another thing, aren't you missing a

chip->fw = fw;

here?

> +
> +	if (!chip->disabled) {
> +		/* continue probing */
> +		if (azx_probe_continue(chip))
> +			goto error;
> +	}
> +	return; /* OK */
> +
> + error:
> +	snd_card_free(card);
> +	pci_set_drvdata(pci, NULL);
> +}
> +
>   static int __devinit azx_probe(struct pci_dev *pci,
>   			       const struct pci_device_id *pci_id)
>   {
>   	static int dev;
>   	struct snd_card *card;
>   	struct azx *chip;
> +	bool probe_deferred;
>   	int err;
>
>   	if (dev >= SNDRV_CARDS)
> @@ -3182,18 +3211,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
>   	if (err < 0)
>   		goto out_free;
>   	card->private_data = chip;

Right, this patch has the race removed, as the variable is no longer set 
in azx_firmware_cb. To be picky, it's slightly confusing that 
probe_deferred is also used for signalling a disabled chip. Maybe 
"probe_now" (inverted) would have been even better, like this:

> +	probe_deferred = chip->disabled;

probe_now = !chip->disabled;

>
>   #ifdef CONFIG_SND_HDA_PATCH_LOADER
>   	if (patch[dev] && *patch[dev]) {

(maybe we should not ask for firmware for a disabled chip either)

>   		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
>   			   patch[dev]);
> -		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
> +		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
> +					      &pci->dev, GFP_KERNEL, card,
> +					      azx_firmware_cb);
>   		if (err < 0)
>   			goto out_free;
> +		probe_deferred = true;

probe_now = false;

>   	}
>   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>
> -	if (!chip->disabled) {
> +	if (!probe_deferred) {

if (probe_now) {

>   		err = azx_probe_continue(chip);
>   		if (err < 0)
>   			goto out_free;
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[David Henningsson]

On 08/09/2012 03:36 PM, Takashi Iwai wrote:
> +/* callback from request_firmware_nowait() */
> +static void azx_firmware_cb(const struct firmware *fw, void *context)
> +{
> +	struct snd_card *card = context;
> +	struct azx *chip = card->private_data;
> +	struct pci_dev *pci = chip->pci;
> +
> +	if (!fw) {
> +		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> +		goto error;
> +	}

Another thing, aren't you missing a

chip->fw = fw;

here?

> +
> +	if (!chip->disabled) {
> +		/* continue probing */
> +		if (azx_probe_continue(chip))
> +			goto error;
> +	}
> +	return; /* OK */
> +
> + error:
> +	snd_card_free(card);
> +	pci_set_drvdata(pci, NULL);
> +}
> +
>   static int __devinit azx_probe(struct pci_dev *pci,
>   			       const struct pci_device_id *pci_id)
>   {
>   	static int dev;
>   	struct snd_card *card;
>   	struct azx *chip;
> +	bool probe_deferred;
>   	int err;
>
>   	if (dev >= SNDRV_CARDS)
> @@ -3182,18 +3211,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
>   	if (err < 0)
>   		goto out_free;
>   	card->private_data = chip;

Right, this patch has the race removed, as the variable is no longer set 
in azx_firmware_cb. To be picky, it's slightly confusing that 
probe_deferred is also used for signalling a disabled chip. Maybe 
"probe_now" (inverted) would have been even better, like this:

> +	probe_deferred = chip->disabled;

probe_now = !chip->disabled;

>
>   #ifdef CONFIG_SND_HDA_PATCH_LOADER
>   	if (patch[dev] && *patch[dev]) {

(maybe we should not ask for firmware for a disabled chip either)

>   		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
>   			   patch[dev]);
> -		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
> +		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
> +					      &pci->dev, GFP_KERNEL, card,
> +					      azx_firmware_cb);
>   		if (err < 0)
>   			goto out_free;
> +		probe_deferred = true;

probe_now = false;

>   	}
>   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>
> -	if (!chip->disabled) {
> +	if (!probe_deferred) {

if (probe_now) {

>   		err = azx_probe_continue(chip);
>   		if (err < 0)
>   			goto out_free;
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On Thu, Aug 09, 2012 at 03:54:04PM +0200, David Henningsson wrote:
> On 08/09/2012 03:36 PM, Takashi Iwai wrote:
> >+/* callback from request_firmware_nowait() */
> >+static void azx_firmware_cb(const struct firmware *fw, void *context)
> >+{
> >+	struct snd_card *card = context;
> >+	struct azx *chip = card->private_data;
> >+	struct pci_dev *pci = chip->pci;
> >+
> >+	if (!fw) {
> >+		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> >+		goto error;
> >+	}
> 
> Another thing, aren't you missing a
> 
> chip->fw = fw;
> 
> here?

Adding that line here fixes the problem for me.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 629 bytes --]

On Thu, Aug 09, 2012 at 03:54:04PM +0200, David Henningsson wrote:
> On 08/09/2012 03:36 PM, Takashi Iwai wrote:
> >+/* callback from request_firmware_nowait() */
> >+static void azx_firmware_cb(const struct firmware *fw, void *context)
> >+{
> >+	struct snd_card *card = context;
> >+	struct azx *chip = card->private_data;
> >+	struct pci_dev *pci = chip->pci;
> >+
> >+	if (!fw) {
> >+		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> >+		goto error;
> >+	}
> 
> Another thing, aren't you missing a
> 
> chip->fw = fw;
> 
> here?

Adding that line here fixes the problem for me.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

At Thu, 09 Aug 2012 15:54:04 +0200,
David Henningsson wrote:
> 
> On 08/09/2012 03:36 PM, Takashi Iwai wrote:
> > +/* callback from request_firmware_nowait() */
> > +static void azx_firmware_cb(const struct firmware *fw, void *context)
> > +{
> > +	struct snd_card *card = context;
> > +	struct azx *chip = card->private_data;
> > +	struct pci_dev *pci = chip->pci;
> > +
> > +	if (!fw) {
> > +		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> > +		goto error;
> > +	}
> 
> Another thing, aren't you missing a
> 
> chip->fw = fw;
> 
> here?

Ah, yes.


> > +
> > +	if (!chip->disabled) {
> > +		/* continue probing */
> > +		if (azx_probe_continue(chip))
> > +			goto error;
> > +	}
> > +	return; /* OK */
> > +
> > + error:
> > +	snd_card_free(card);
> > +	pci_set_drvdata(pci, NULL);
> > +}
> > +
> >   static int __devinit azx_probe(struct pci_dev *pci,
> >   			       const struct pci_device_id *pci_id)
> >   {
> >   	static int dev;
> >   	struct snd_card *card;
> >   	struct azx *chip;
> > +	bool probe_deferred;
> >   	int err;
> >
> >   	if (dev >= SNDRV_CARDS)
> > @@ -3182,18 +3211,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
> >   	if (err < 0)
> >   		goto out_free;
> >   	card->private_data = chip;
> 
> Right, this patch has the race removed, as the variable is no longer set 
> in azx_firmware_cb. To be picky, it's slightly confusing that 
> probe_deferred is also used for signalling a disabled chip. Maybe 
> "probe_now" (inverted) would have been even better, like this:

Yeah, this looks slightly more straightforward.

I'm going to send a revised version shortly.


thanks,

Takashi

Re: [PATCH 2/2 v2] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

At Thu, 09 Aug 2012 15:54:04 +0200,
David Henningsson wrote:
> 
> On 08/09/2012 03:36 PM, Takashi Iwai wrote:
> > +/* callback from request_firmware_nowait() */
> > +static void azx_firmware_cb(const struct firmware *fw, void *context)
> > +{
> > +	struct snd_card *card = context;
> > +	struct azx *chip = card->private_data;
> > +	struct pci_dev *pci = chip->pci;
> > +
> > +	if (!fw) {
> > +		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> > +		goto error;
> > +	}
> 
> Another thing, aren't you missing a
> 
> chip->fw = fw;
> 
> here?

Ah, yes.


> > +
> > +	if (!chip->disabled) {
> > +		/* continue probing */
> > +		if (azx_probe_continue(chip))
> > +			goto error;
> > +	}
> > +	return; /* OK */
> > +
> > + error:
> > +	snd_card_free(card);
> > +	pci_set_drvdata(pci, NULL);
> > +}
> > +
> >   static int __devinit azx_probe(struct pci_dev *pci,
> >   			       const struct pci_device_id *pci_id)
> >   {
> >   	static int dev;
> >   	struct snd_card *card;
> >   	struct azx *chip;
> > +	bool probe_deferred;
> >   	int err;
> >
> >   	if (dev >= SNDRV_CARDS)
> > @@ -3182,18 +3211,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
> >   	if (err < 0)
> >   		goto out_free;
> >   	card->private_data = chip;
> 
> Right, this patch has the race removed, as the variable is no longer set 
> in azx_firmware_cb. To be picky, it's slightly confusing that 
> probe_deferred is also used for signalling a disabled chip. Maybe 
> "probe_now" (inverted) would have been even better, like this:

Yeah, this looks slightly more straightforward.

I'm going to send a revised version shortly.


thanks,

Takashi

[PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

For processing the firmware handling properly for built-in kernels,
implement an asynchronous firmware loading with
request_firmware_nowait().  This means that the codec probing is
deferred when the patch option is specified.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: drop superfluous chip->probe_deferred field
v2->v3: fix missing chip->fw; renamed the variable to "probe_now"

 sound/pci/hda/hda_codec.c |  2 +-
 sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 88a9c20..408babc 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4184,7 +4184,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
  *
  * This function returns 0 if successful, or a negative error code.
  */
-int __devinit snd_hda_build_pcms(struct hda_bus *bus)
+int snd_hda_build_pcms(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
 
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b25d539..3de3a5c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -564,13 +564,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 = {
@@ -3155,12 +3159,38 @@ static void power_down_all_codecs(struct azx *chip)
 #endif
 }
 
+/* callback from request_firmware_nowait() */
+static void azx_firmware_cb(const struct firmware *fw, void *context)
+{
+	struct snd_card *card = context;
+	struct azx *chip = card->private_data;
+	struct pci_dev *pci = chip->pci;
+
+	if (!fw) {
+		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
+		goto error;
+	}
+
+	chip->fw = fw;
+	if (!chip->disabled) {
+		/* continue probing */
+		if (azx_probe_continue(chip))
+			goto error;
+	}
+	return; /* OK */
+
+ error:
+	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
+}
+
 static int __devinit azx_probe(struct pci_dev *pci,
 			       const struct pci_device_id *pci_id)
 {
 	static int dev;
 	struct snd_card *card;
 	struct azx *chip;
+	bool probe_now;
 	int err;
 
 	if (dev >= SNDRV_CARDS)
@@ -3182,18 +3212,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
 	if (err < 0)
 		goto out_free;
 	card->private_data = chip;
+	probe_now = !chip->disabled;
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	if (patch[dev] && *patch[dev]) {
 		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
 			   patch[dev]);
-		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
+		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
+					      &pci->dev, GFP_KERNEL, card,
+					      azx_firmware_cb);
 		if (err < 0)
 			goto out_free;
+		probe_now = false; /* continued in azx_firmware_cb() */
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
-	if (!chip->disabled) {
+	if (probe_now) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
 			goto out_free;
-- 
1.7.11.4

[PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

For processing the firmware handling properly for built-in kernels,
implement an asynchronous firmware loading with
request_firmware_nowait().  This means that the codec probing is
deferred when the patch option is specified.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: drop superfluous chip->probe_deferred field
v2->v3: fix missing chip->fw; renamed the variable to "probe_now"

 sound/pci/hda/hda_codec.c |  2 +-
 sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 88a9c20..408babc 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -4184,7 +4184,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
  *
  * This function returns 0 if successful, or a negative error code.
  */
-int __devinit snd_hda_build_pcms(struct hda_bus *bus)
+int snd_hda_build_pcms(struct hda_bus *bus)
 {
 	struct hda_codec *codec;
 
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b25d539..3de3a5c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -564,13 +564,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 = {
@@ -3155,12 +3159,38 @@ static void power_down_all_codecs(struct azx *chip)
 #endif
 }
 
+/* callback from request_firmware_nowait() */
+static void azx_firmware_cb(const struct firmware *fw, void *context)
+{
+	struct snd_card *card = context;
+	struct azx *chip = card->private_data;
+	struct pci_dev *pci = chip->pci;
+
+	if (!fw) {
+		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
+		goto error;
+	}
+
+	chip->fw = fw;
+	if (!chip->disabled) {
+		/* continue probing */
+		if (azx_probe_continue(chip))
+			goto error;
+	}
+	return; /* OK */
+
+ error:
+	snd_card_free(card);
+	pci_set_drvdata(pci, NULL);
+}
+
 static int __devinit azx_probe(struct pci_dev *pci,
 			       const struct pci_device_id *pci_id)
 {
 	static int dev;
 	struct snd_card *card;
 	struct azx *chip;
+	bool probe_now;
 	int err;
 
 	if (dev >= SNDRV_CARDS)
@@ -3182,18 +3212,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
 	if (err < 0)
 		goto out_free;
 	card->private_data = chip;
+	probe_now = !chip->disabled;
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	if (patch[dev] && *patch[dev]) {
 		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
 			   patch[dev]);
-		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
+		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
+					      &pci->dev, GFP_KERNEL, card,
+					      azx_firmware_cb);
 		if (err < 0)
 			goto out_free;
+		probe_now = false; /* continued in azx_firmware_cb() */
 	}
 #endif /* CONFIG_SND_HDA_PATCH_LOADER */
 
-	if (!chip->disabled) {
+	if (probe_now) {
 		err = azx_probe_continue(chip);
 		if (err < 0)
 			goto out_free;
-- 
1.7.11.4

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[David Henningsson]

On 08/09/2012 04:10 PM, Takashi Iwai wrote:
> For processing the firmware handling properly for built-in kernels,
> implement an asynchronous firmware loading with
> request_firmware_nowait().  This means that the codec probing is
> deferred when the patch option is specified.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Looks better now!

Reviewed-by: David Henningsson <david.henningsson@canonical.com>

> ---
> v1->v2: drop superfluous chip->probe_deferred field
> v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
>
>   sound/pci/hda/hda_codec.c |  2 +-
>   sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
>   2 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 88a9c20..408babc 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -4184,7 +4184,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
>    *
>    * This function returns 0 if successful, or a negative error code.
>    */
> -int __devinit snd_hda_build_pcms(struct hda_bus *bus)
> +int snd_hda_build_pcms(struct hda_bus *bus)
>   {
>   	struct hda_codec *codec;
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index b25d539..3de3a5c 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -564,13 +564,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 = {
> @@ -3155,12 +3159,38 @@ static void power_down_all_codecs(struct azx *chip)
>   #endif
>   }
>
> +/* callback from request_firmware_nowait() */
> +static void azx_firmware_cb(const struct firmware *fw, void *context)
> +{
> +	struct snd_card *card = context;
> +	struct azx *chip = card->private_data;
> +	struct pci_dev *pci = chip->pci;
> +
> +	if (!fw) {
> +		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> +		goto error;
> +	}
> +
> +	chip->fw = fw;
> +	if (!chip->disabled) {
> +		/* continue probing */
> +		if (azx_probe_continue(chip))
> +			goto error;
> +	}
> +	return; /* OK */
> +
> + error:
> +	snd_card_free(card);
> +	pci_set_drvdata(pci, NULL);
> +}
> +
>   static int __devinit azx_probe(struct pci_dev *pci,
>   			       const struct pci_device_id *pci_id)
>   {
>   	static int dev;
>   	struct snd_card *card;
>   	struct azx *chip;
> +	bool probe_now;
>   	int err;
>
>   	if (dev >= SNDRV_CARDS)
> @@ -3182,18 +3212,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
>   	if (err < 0)
>   		goto out_free;
>   	card->private_data = chip;
> +	probe_now = !chip->disabled;
>
>   #ifdef CONFIG_SND_HDA_PATCH_LOADER
>   	if (patch[dev] && *patch[dev]) {
>   		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
>   			   patch[dev]);
> -		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
> +		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
> +					      &pci->dev, GFP_KERNEL, card,
> +					      azx_firmware_cb);
>   		if (err < 0)
>   			goto out_free;
> +		probe_now = false; /* continued in azx_firmware_cb() */
>   	}
>   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>
> -	if (!chip->disabled) {
> +	if (probe_now) {
>   		err = azx_probe_continue(chip);
>   		if (err < 0)
>   			goto out_free;
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[David Henningsson]

On 08/09/2012 04:10 PM, Takashi Iwai wrote:
> For processing the firmware handling properly for built-in kernels,
> implement an asynchronous firmware loading with
> request_firmware_nowait().  This means that the codec probing is
> deferred when the patch option is specified.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Looks better now!

Reviewed-by: David Henningsson <david.henningsson@canonical.com>

> ---
> v1->v2: drop superfluous chip->probe_deferred field
> v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
>
>   sound/pci/hda/hda_codec.c |  2 +-
>   sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
>   2 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 88a9c20..408babc 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -4184,7 +4184,7 @@ int snd_hda_codec_build_pcms(struct hda_codec *codec)
>    *
>    * This function returns 0 if successful, or a negative error code.
>    */
> -int __devinit snd_hda_build_pcms(struct hda_bus *bus)
> +int snd_hda_build_pcms(struct hda_bus *bus)
>   {
>   	struct hda_codec *codec;
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index b25d539..3de3a5c 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -564,13 +564,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 = {
> @@ -3155,12 +3159,38 @@ static void power_down_all_codecs(struct azx *chip)
>   #endif
>   }
>
> +/* callback from request_firmware_nowait() */
> +static void azx_firmware_cb(const struct firmware *fw, void *context)
> +{
> +	struct snd_card *card = context;
> +	struct azx *chip = card->private_data;
> +	struct pci_dev *pci = chip->pci;
> +
> +	if (!fw) {
> +		snd_printk(KERN_ERR SFX "Cannot load firmware, aborting\n");
> +		goto error;
> +	}
> +
> +	chip->fw = fw;
> +	if (!chip->disabled) {
> +		/* continue probing */
> +		if (azx_probe_continue(chip))
> +			goto error;
> +	}
> +	return; /* OK */
> +
> + error:
> +	snd_card_free(card);
> +	pci_set_drvdata(pci, NULL);
> +}
> +
>   static int __devinit azx_probe(struct pci_dev *pci,
>   			       const struct pci_device_id *pci_id)
>   {
>   	static int dev;
>   	struct snd_card *card;
>   	struct azx *chip;
> +	bool probe_now;
>   	int err;
>
>   	if (dev >= SNDRV_CARDS)
> @@ -3182,18 +3212,22 @@ static int __devinit azx_probe(struct pci_dev *pci,
>   	if (err < 0)
>   		goto out_free;
>   	card->private_data = chip;
> +	probe_now = !chip->disabled;
>
>   #ifdef CONFIG_SND_HDA_PATCH_LOADER
>   	if (patch[dev] && *patch[dev]) {
>   		snd_printk(KERN_ERR SFX "Applying patch firmware '%s'\n",
>   			   patch[dev]);
> -		err = request_firmware(&chip->fw, patch[dev], &pci->dev);
> +		err = request_firmware_nowait(THIS_MODULE, true, patch[dev],
> +					      &pci->dev, GFP_KERNEL, card,
> +					      azx_firmware_cb);
>   		if (err < 0)
>   			goto out_free;
> +		probe_now = false; /* continued in azx_firmware_cb() */
>   	}
>   #endif /* CONFIG_SND_HDA_PATCH_LOADER */
>
> -	if (!chip->disabled) {
> +	if (probe_now) {
>   		err = azx_probe_continue(chip);
>   		if (err < 0)
>   			goto out_free;
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Thu, Aug 09, 2012 at 04:10:55PM +0200, Takashi Iwai wrote:
> For processing the firmware handling properly for built-in kernels,
> implement an asynchronous firmware loading with
> request_firmware_nowait().  This means that the codec probing is
> deferred when the patch option is specified.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v1->v2: drop superfluous chip->probe_deferred field
> v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
> 
>  sound/pci/hda/hda_codec.c |  2 +-
>  sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 39 insertions(+), 5 deletions(-)

Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 732 bytes --]

On Thu, Aug 09, 2012 at 04:10:55PM +0200, Takashi Iwai wrote:
> For processing the firmware handling properly for built-in kernels,
> implement an asynchronous firmware loading with
> request_firmware_nowait().  This means that the codec probing is
> deferred when the patch option is specified.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v1->v2: drop superfluous chip->probe_deferred field
> v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
> 
>  sound/pci/hda/hda_codec.c |  2 +-
>  sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 39 insertions(+), 5 deletions(-)

Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ALSA: hda - Load firmware in hda_intel.c

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Thu, Aug 09, 2012 at 03:10:37PM +0200, Takashi Iwai wrote:
> This is a preliminary work for the deferred probing for
> request_firmware() errors at init.
> 
> This patch moves the call of request_firmware() to hda_intel.c, and
> call it in the earlier stage of probing rather than
> azx_probe_continue().
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_codec.h |  2 +-
>  sound/pci/hda/hda_hwdep.c | 39 +++++++++++++--------------------------
>  sound/pci/hda/hda_intel.c | 29 ++++++++++++++++++++++++-----
>  3 files changed, 38 insertions(+), 32 deletions(-)

Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] ALSA: hda - Load firmware in hda_intel.c

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

On Thu, Aug 09, 2012 at 03:10:37PM +0200, Takashi Iwai wrote:
> This is a preliminary work for the deferred probing for
> request_firmware() errors at init.
> 
> This patch moves the call of request_firmware() to hda_intel.c, and
> call it in the earlier stage of probing rather than
> azx_probe_continue().
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_codec.h |  2 +-
>  sound/pci/hda/hda_hwdep.c | 39 +++++++++++++--------------------------
>  sound/pci/hda/hda_intel.c | 29 ++++++++++++++++++++++++-----
>  3 files changed, 38 insertions(+), 32 deletions(-)

Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

At Thu, 9 Aug 2012 16:25:36 +0200,
Thierry Reding wrote:
> 
> On Thu, Aug 09, 2012 at 04:10:55PM +0200, Takashi Iwai wrote:
> > For processing the firmware handling properly for built-in kernels,
> > implement an asynchronous firmware loading with
> > request_firmware_nowait().  This means that the codec probing is
> > deferred when the patch option is specified.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v1->v2: drop superfluous chip->probe_deferred field
> > v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
> > 
> >  sound/pci/hda/hda_codec.c |  2 +-
> >  sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 39 insertions(+), 5 deletions(-)
> 
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

Thanks, now pushed to sound git tree for-next branch.
As mentioned earlier, this is planned for 3.7 merge.

Also, the patches are found in topic/hda-probe-defer branch so that it
can be easily pulled to 3.6 kernel cleanly.


Takashi

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Takashi Iwai]

At Thu, 9 Aug 2012 16:25:36 +0200,
Thierry Reding wrote:
> 
> On Thu, Aug 09, 2012 at 04:10:55PM +0200, Takashi Iwai wrote:
> > For processing the firmware handling properly for built-in kernels,
> > implement an asynchronous firmware loading with
> > request_firmware_nowait().  This means that the codec probing is
> > deferred when the patch option is specified.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v1->v2: drop superfluous chip->probe_deferred field
> > v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
> > 
> >  sound/pci/hda/hda_codec.c |  2 +-
> >  sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 39 insertions(+), 5 deletions(-)
> 
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

Thanks, now pushed to sound git tree for-next branch.
As mentioned earlier, this is planned for 3.7 merge.

Also, the patches are found in topic/hda-probe-defer branch so that it
can be easily pulled to 3.6 kernel cleanly.


Takashi

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Thu, Aug 09, 2012 at 04:34:42PM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 16:25:36 +0200,
> Thierry Reding wrote:
> > 
> > On Thu, Aug 09, 2012 at 04:10:55PM +0200, Takashi Iwai wrote:
> > > For processing the firmware handling properly for built-in kernels,
> > > implement an asynchronous firmware loading with
> > > request_firmware_nowait().  This means that the codec probing is
> > > deferred when the patch option is specified.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > v1->v2: drop superfluous chip->probe_deferred field
> > > v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
> > > 
> > >  sound/pci/hda/hda_codec.c |  2 +-
> > >  sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
> > >  2 files changed, 39 insertions(+), 5 deletions(-)
> > 
> > Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> Thanks, now pushed to sound git tree for-next branch.
> As mentioned earlier, this is planned for 3.7 merge.
> 
> Also, the patches are found in topic/hda-probe-defer branch so that it
> can be easily pulled to 3.6 kernel cleanly.

Great! Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2 v3] ALSA: hda - Deferred probing with request_firmware_nowait()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1186 bytes --]

On Thu, Aug 09, 2012 at 04:34:42PM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 16:25:36 +0200,
> Thierry Reding wrote:
> > 
> > On Thu, Aug 09, 2012 at 04:10:55PM +0200, Takashi Iwai wrote:
> > > For processing the firmware handling properly for built-in kernels,
> > > implement an asynchronous firmware loading with
> > > request_firmware_nowait().  This means that the codec probing is
> > > deferred when the patch option is specified.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > v1->v2: drop superfluous chip->probe_deferred field
> > > v2->v3: fix missing chip->fw; renamed the variable to "probe_now"
> > > 
> > >  sound/pci/hda/hda_codec.c |  2 +-
> > >  sound/pci/hda/hda_intel.c | 42 ++++++++++++++++++++++++++++++++++++++----
> > >  2 files changed, 39 insertions(+), 5 deletions(-)
> > 
> > Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> Thanks, now pushed to sound git tree for-next branch.
> As mentioned earlier, this is planned for 3.7 merge.
> 
> Also, the patches are found in topic/hda-probe-defer branch so that it
> can be easily pulled to 3.6 kernel cleanly.

Great! Thanks,
Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]