2012-08-09 06:45:29

by Thierry Reding

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

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 <[email protected]>
---
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


2012-08-09 06:57:20

by Takashi Iwai

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

At Thu, 9 Aug 2012 08:45:23 +0200,
Thierry Reding wrote:
>
> Recent changes to the firmware loading helpers cause drivers to stall
> when firmware is loaded during the module_init() call. The snd-hda-intel
> module requests firmware if the patch= parameter is used to load a patch
> file. This patch works around the problem by deferring the probe in such
> cases, which will cause the module to load successfully and the driver
> binding to the device outside the module_init() call.

Is the "recent" change meant 3.6 kernel, or in linux-next?

In anyway, I don't understand why such a change was allowed. Most
drivers do call request_firmware() at the device probing time.
If this really has to be resolved in the driver side, it must be a bug
in the firmware loader core code.


thanks,

Takashi

>
> Signed-off-by: Thierry Reding <[email protected]>
> ---
> 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
>

2012-08-09 07:08:24

by Thierry Reding

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

On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 08:45:23 +0200,
> Thierry Reding wrote:
> >
> > Recent changes to the firmware loading helpers cause drivers to stall
> > when firmware is loaded during the module_init() call. The snd-hda-intel
> > module requests firmware if the patch= parameter is used to load a patch
> > file. This patch works around the problem by deferring the probe in such
> > cases, which will cause the module to load successfully and the driver
> > binding to the device outside the module_init() call.
>
> Is the "recent" change meant 3.6 kernel, or in linux-next?
>
> In anyway, I don't understand why such a change was allowed. Most
> drivers do call request_firmware() at the device probing time.
> If this really has to be resolved in the driver side, it must be a bug
> in the firmware loader core code.

A good explanation of the problem and subsequent discussion can be found
here:

http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975

Thierry


Attachments:
(No filename) (1.02 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-09 07:31:35

by Takashi Iwai

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

At Thu, 9 Aug 2012 09:08:13 +0200,
Thierry Reding wrote:
>
> On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > At Thu, 9 Aug 2012 08:45:23 +0200,
> > Thierry Reding wrote:
> > >
> > > Recent changes to the firmware loading helpers cause drivers to stall
> > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > module requests firmware if the patch= parameter is used to load a patch
> > > file. This patch works around the problem by deferring the probe in such
> > > cases, which will cause the module to load successfully and the driver
> > > binding to the device outside the module_init() call.
> >
> > Is the "recent" change meant 3.6 kernel, or in linux-next?
> >
> > In anyway, I don't understand why such a change was allowed. Most
> > drivers do call request_firmware() at the device probing time.
> > If this really has to be resolved in the driver side, it must be a bug
> > in the firmware loader core code.
>
> A good explanation of the problem and subsequent discussion can be found
> here:
>
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975

Yeah, but it doesn't justify this ugly module option.
It's a simple bug. Papering over it with this option doesn't fix
anything.


Takashi

2012-08-09 07:36:48

by Thierry Reding

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

On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 09:08:13 +0200,
> Thierry Reding wrote:
> >
> > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > Thierry Reding wrote:
> > > >
> > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > module requests firmware if the patch= parameter is used to load a patch
> > > > file. This patch works around the problem by deferring the probe in such
> > > > cases, which will cause the module to load successfully and the driver
> > > > binding to the device outside the module_init() call.
> > >
> > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > >
> > > In anyway, I don't understand why such a change was allowed. Most
> > > drivers do call request_firmware() at the device probing time.
> > > If this really has to be resolved in the driver side, it must be a bug
> > > in the firmware loader core code.
> >
> > A good explanation of the problem and subsequent discussion can be found
> > here:
> >
> > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
>
> Yeah, but it doesn't justify this ugly module option.
> It's a simple bug. Papering over it with this option doesn't fix
> anything.

It's not an option, all it does is defer probing if and only if the
patch parameter was specified to make sure the firmware load won't
stall. I realize that this may not be an optimal solution, but at least
it fixes the problem with no fallout.

A proper fix would require a larger rewrite because it would entail
using the asynchronous firmware load operations. That in turn would
require the initialization to be split into several stages.

Thierry


Attachments:
(No filename) (1.80 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-09 07:42:53

by Takashi Iwai

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

At Thu, 9 Aug 2012 09:36:42 +0200,
Thierry Reding wrote:
>
> On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > At Thu, 9 Aug 2012 09:08:13 +0200,
> > Thierry Reding wrote:
> > >
> > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > Thierry Reding wrote:
> > > > >
> > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > file. This patch works around the problem by deferring the probe in such
> > > > > cases, which will cause the module to load successfully and the driver
> > > > > binding to the device outside the module_init() call.
> > > >
> > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > >
> > > > In anyway, I don't understand why such a change was allowed. Most
> > > > drivers do call request_firmware() at the device probing time.
> > > > If this really has to be resolved in the driver side, it must be a bug
> > > > in the firmware loader core code.
> > >
> > > A good explanation of the problem and subsequent discussion can be found
> > > here:
> > >
> > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> >
> > Yeah, but it doesn't justify this ugly module option.
> > It's a simple bug. Papering over it with this option doesn't fix
> > anything.
>
> It's not an option, all it does is defer probing if and only if the
> patch parameter was specified to make sure the firmware load won't
> stall. I realize that this may not be an optimal solution, but at least
> it fixes the problem with no fallout.

Ah sorry, I misread the patch.

Then it shouldn't be checked at that point. Since 3.5 kernel, the
probing code was already split for vga_switcheroo support. The point
you added is the second stage.


Takashi

2012-08-09 07:51:01

by Takashi Iwai

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

At Thu, 09 Aug 2012 09:42:48 +0200,
Takashi Iwai wrote:
>
> At Thu, 9 Aug 2012 09:36:42 +0200,
> Thierry Reding wrote:
> >
> > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > Thierry Reding wrote:
> > > >
> > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > Thierry Reding wrote:
> > > > > >
> > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > binding to the device outside the module_init() call.
> > > > >
> > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > >
> > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > drivers do call request_firmware() at the device probing time.
> > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > in the firmware loader core code.
> > > >
> > > > A good explanation of the problem and subsequent discussion can be found
> > > > here:
> > > >
> > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > >
> > > Yeah, but it doesn't justify this ugly module option.
> > > It's a simple bug. Papering over it with this option doesn't fix
> > > anything.
> >
> > It's not an option, all it does is defer probing if and only if the
> > patch parameter was specified to make sure the firmware load won't
> > stall. I realize that this may not be an optimal solution, but at least
> > it fixes the problem with no fallout.
>
> Ah sorry, I misread the patch.
>
> Then it shouldn't be checked at that point. Since 3.5 kernel, the
> probing code was already split for vga_switcheroo support. The point
> you added is the second stage.

... and the patch won't work properly if there are multiple HD-audio
controllers. Hmm.


Takashi

2012-08-09 07:57:22

by Thierry Reding

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

On Thu, Aug 09, 2012 at 09:50:57AM +0200, Takashi Iwai wrote:
> At Thu, 09 Aug 2012 09:42:48 +0200,
> Takashi Iwai wrote:
> >
> > At Thu, 9 Aug 2012 09:36:42 +0200,
> > Thierry Reding wrote:
> > >
> > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > Thierry Reding wrote:
> > > > >
> > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > Thierry Reding wrote:
> > > > > > >
> > > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > > binding to the device outside the module_init() call.
> > > > > >
> > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > >
> > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > > in the firmware loader core code.
> > > > >
> > > > > A good explanation of the problem and subsequent discussion can be found
> > > > > here:
> > > > >
> > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > >
> > > > Yeah, but it doesn't justify this ugly module option.
> > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > anything.
> > >
> > > It's not an option, all it does is defer probing if and only if the
> > > patch parameter was specified to make sure the firmware load won't
> > > stall. I realize that this may not be an optimal solution, but at least
> > > it fixes the problem with no fallout.
> >
> > Ah sorry, I misread the patch.
> >
> > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > probing code was already split for vga_switcheroo support. The point
> > you added is the second stage.
>
> ... and the patch won't work properly if there are multiple HD-audio
> controllers. Hmm.

Right... the deferred probe would mess up the matching done by the
static dev variable. So maybe a proper implementation that uses
asynchronous firmware loading is inevitable.

Thierry


Attachments:
(No filename) (2.49 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-09 08:07:31

by Thierry Reding

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

On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 09:36:42 +0200,
> Thierry Reding wrote:
> >
> > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > Thierry Reding wrote:
> > > >
> > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > Thierry Reding wrote:
> > > > > >
> > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > binding to the device outside the module_init() call.
> > > > >
> > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > >
> > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > drivers do call request_firmware() at the device probing time.
> > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > in the firmware loader core code.
> > > >
> > > > A good explanation of the problem and subsequent discussion can be found
> > > > here:
> > > >
> > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > >
> > > Yeah, but it doesn't justify this ugly module option.
> > > It's a simple bug. Papering over it with this option doesn't fix
> > > anything.
> >
> > It's not an option, all it does is defer probing if and only if the
> > patch parameter was specified to make sure the firmware load won't
> > stall. I realize that this may not be an optimal solution, but at least
> > it fixes the problem with no fallout.
>
> Ah sorry, I misread the patch.
>
> Then it shouldn't be checked at that point. Since 3.5 kernel, the
> probing code was already split for vga_switcheroo support.

Yes, I saw that. But unless you actually use vga_switcheroo, the second
stage, azx_probe_continue(), will still be called from azx_probe() and
therefore ultimately from module_init().

Before coming up with this patch I actually did play around a bit with
using the asynchronous firmware load functions but it turned out to be
rather difficult to do so I opted for the easy way. The biggest problem
I faced was that since patch loading needs to be done very early on, a
lot of the initialization would need to be done after .probe() and many
things could still fail, so cleaning up after errors would become
increasingly difficult.

> The point you added is the second stage.

I don't understand this sentence.

Thierry


Attachments:
(No filename) (2.73 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-09 08:21:21

by Takashi Iwai

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

At Thu, 9 Aug 2012 10:07:13 +0200,
Thierry Reding wrote:
>
> On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > At Thu, 9 Aug 2012 09:36:42 +0200,
> > Thierry Reding wrote:
> > >
> > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > Thierry Reding wrote:
> > > > >
> > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > Thierry Reding wrote:
> > > > > > >
> > > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > > binding to the device outside the module_init() call.
> > > > > >
> > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > >
> > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > > in the firmware loader core code.
> > > > >
> > > > > A good explanation of the problem and subsequent discussion can be found
> > > > > here:
> > > > >
> > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > >
> > > > Yeah, but it doesn't justify this ugly module option.
> > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > anything.
> > >
> > > It's not an option, all it does is defer probing if and only if the
> > > patch parameter was specified to make sure the firmware load won't
> > > stall. I realize that this may not be an optimal solution, but at least
> > > it fixes the problem with no fallout.
> >
> > Ah sorry, I misread the patch.
> >
> > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > probing code was already split for vga_switcheroo support.
>
> Yes, I saw that. But unless you actually use vga_switcheroo, the second
> stage, azx_probe_continue(), will still be called from azx_probe() and
> therefore ultimately from module_init().

Yeah, but this could be easily delayed. The split was already done,
so the next step would be to return after the first half at probe,
then call the second half later.

> Before coming up with this patch I actually did play around a bit with
> using the asynchronous firmware load functions but it turned out to be
> rather difficult to do so I opted for the easy way. The biggest problem
> I faced was that since patch loading needs to be done very early on, a
> lot of the initialization would need to be done after .probe() and many
> things could still fail, so cleaning up after errors would become
> increasingly difficult.

async probe is also on my TODO list, but it's deferred ;)

> > The point you added is the second stage.
>
> I don't understand this sentence.

I meant that your patch added the check at the second-half probing
function (azx_probe_contine()). That is, it could be already the
point triggered by vga_switcheroo handler, not via module_init any
longer.

So, after rethinking what you suggested, I wrote a quick patch below.
Could you check whether this works?


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8aced1..4e5839a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -559,13 +559,17 @@ enum {
* VGA-switcher support
*/
#ifdef SUPPORT_VGA_SWITCHEROO
+#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
+#else
+#define use_vga_switcheroo(chip) 0
+#endif
+
+#if defined(SUPPORT_VGA_SWITCHEROO) || defined(CONFIG_SND_HDA_PATCH_LOADER)
#define DELAYED_INIT_MARK
#define DELAYED_INITDATA_MARK
-#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
#else
#define DELAYED_INIT_MARK __devinit
#define DELAYED_INITDATA_MARK __devinitdata
-#define use_vga_switcheroo(chip) 0
#endif

static char *driver_short_names[] DELAYED_INITDATA_MARK = {
@@ -3154,6 +3158,20 @@ static int __devinit azx_probe(struct pci_dev *pci,
struct azx *chip;
int err;

+#ifdef CONFIG_SND_HDA_PATCH_LOADER
+ /* delayed probe */
+ card = pci_get_drvdata(pci);
+ if (card) {
+ struct azx *chip = card->private_data;
+ if (chip->disabled)
+ return 0; /* will be loaded via vga_switcheroo */
+ err = azx_probe_continue(chip);
+ if (err < 0)
+ goto out_free;
+ return 0;
+ }
+#endif
+
if (dev >= SNDRV_CARDS)
return -ENODEV;
if (!enable[dev]) {
@@ -3175,19 +3193,28 @@ static int __devinit azx_probe(struct pci_dev *pci,
goto out_free;
card->private_data = chip;

+#ifndef CONFIG_SND_HDA_PATCH_LOADER
+ /* 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;
}

2012-08-09 09:19:50

by Takashi Iwai

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

At Thu, 09 Aug 2012 10:21:15 +0200,
Takashi Iwai wrote:
>
> At Thu, 9 Aug 2012 10:07:13 +0200,
> Thierry Reding wrote:
> >
> > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > > At Thu, 9 Aug 2012 09:36:42 +0200,
> > > Thierry Reding wrote:
> > > >
> > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > > Thierry Reding wrote:
> > > > > >
> > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > > Thierry Reding wrote:
> > > > > > > >
> > > > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > > > binding to the device outside the module_init() call.
> > > > > > >
> > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > > >
> > > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > > > in the firmware loader core code.
> > > > > >
> > > > > > A good explanation of the problem and subsequent discussion can be found
> > > > > > here:
> > > > > >
> > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > > >
> > > > > Yeah, but it doesn't justify this ugly module option.
> > > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > > anything.
> > > >
> > > > It's not an option, all it does is defer probing if and only if the
> > > > patch parameter was specified to make sure the firmware load won't
> > > > stall. I realize that this may not be an optimal solution, but at least
> > > > it fixes the problem with no fallout.
> > >
> > > Ah sorry, I misread the patch.
> > >
> > > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > > probing code was already split for vga_switcheroo support.
> >
> > Yes, I saw that. But unless you actually use vga_switcheroo, the second
> > stage, azx_probe_continue(), will still be called from azx_probe() and
> > therefore ultimately from module_init().
>
> Yeah, but this could be easily delayed. The split was already done,
> so the next step would be to return after the first half at probe,
> then call the second half later.
>
> > Before coming up with this patch I actually did play around a bit with
> > using the asynchronous firmware load functions but it turned out to be
> > rather difficult to do so I opted for the easy way. The biggest problem
> > I faced was that since patch loading needs to be done very early on, a
> > lot of the initialization would need to be done after .probe() and many
> > things could still fail, so cleaning up after errors would become
> > increasingly difficult.
>
> async probe is also on my TODO list, but it's deferred ;)
>
> > > The point you added is the second stage.
> >
> > I don't understand this sentence.
>
> I meant that your patch added the check at the second-half probing
> function (azx_probe_contine()). That is, it could be already the
> point triggered by vga_switcheroo handler, not via module_init any
> longer.
>
> So, after rethinking what you suggested, I wrote a quick patch below.
> Could you check whether this works?

Obviously it won't work if the module is re-loaded manually.
The -EPROBE_DEFER would work only at boot time, as it seems.


Takashi

2012-08-09 10:34:42

by Thierry Reding

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

On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 10:07:13 +0200,
> Thierry Reding wrote:
> >
> > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > > At Thu, 9 Aug 2012 09:36:42 +0200,
> > > Thierry Reding wrote:
> > > >
> > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > > Thierry Reding wrote:
> > > > > >
> > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > > Thierry Reding wrote:
> > > > > > > >
> > > > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > > > binding to the device outside the module_init() call.
> > > > > > >
> > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > > >
> > > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > > > in the firmware loader core code.
> > > > > >
> > > > > > A good explanation of the problem and subsequent discussion can be found
> > > > > > here:
> > > > > >
> > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > > >
> > > > > Yeah, but it doesn't justify this ugly module option.
> > > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > > anything.
> > > >
> > > > It's not an option, all it does is defer probing if and only if the
> > > > patch parameter was specified to make sure the firmware load won't
> > > > stall. I realize that this may not be an optimal solution, but at least
> > > > it fixes the problem with no fallout.
> > >
> > > Ah sorry, I misread the patch.
> > >
> > > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > > probing code was already split for vga_switcheroo support.
> >
> > Yes, I saw that. But unless you actually use vga_switcheroo, the second
> > stage, azx_probe_continue(), will still be called from azx_probe() and
> > therefore ultimately from module_init().
>
> Yeah, but this could be easily delayed. The split was already done,
> so the next step would be to return after the first half at probe,
> then call the second half later.
>
> > Before coming up with this patch I actually did play around a bit with
> > using the asynchronous firmware load functions but it turned out to be
> > rather difficult to do so I opted for the easy way. The biggest problem
> > I faced was that since patch loading needs to be done very early on, a
> > lot of the initialization would need to be done after .probe() and many
> > things could still fail, so cleaning up after errors would become
> > increasingly difficult.
>
> async probe is also on my TODO list, but it's deferred ;)
>
> > > The point you added is the second stage.
> >
> > I don't understand this sentence.
>
> I meant that your patch added the check at the second-half probing
> function (azx_probe_contine()). That is, it could be already the
> point triggered by vga_switcheroo handler, not via module_init any
> longer.
>
> So, after rethinking what you suggested, I wrote a quick patch below.
> Could you check whether this works?

It oopses, though I can't quite tell where. I need to test some more
later to see where it goes wrong.

Thierry

>
>
> Takashi
>
> ---
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c8aced1..4e5839a 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -559,13 +559,17 @@ enum {
> * VGA-switcher support
> */
> #ifdef SUPPORT_VGA_SWITCHEROO
> +#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
> +#else
> +#define use_vga_switcheroo(chip) 0
> +#endif
> +
> +#if defined(SUPPORT_VGA_SWITCHEROO) || defined(CONFIG_SND_HDA_PATCH_LOADER)
> #define DELAYED_INIT_MARK
> #define DELAYED_INITDATA_MARK
> -#define use_vga_switcheroo(chip) ((chip)->use_vga_switcheroo)
> #else
> #define DELAYED_INIT_MARK __devinit
> #define DELAYED_INITDATA_MARK __devinitdata
> -#define use_vga_switcheroo(chip) 0
> #endif
>
> static char *driver_short_names[] DELAYED_INITDATA_MARK = {
> @@ -3154,6 +3158,20 @@ static int __devinit azx_probe(struct pci_dev *pci,
> struct azx *chip;
> int err;
>
> +#ifdef CONFIG_SND_HDA_PATCH_LOADER
> + /* delayed probe */
> + card = pci_get_drvdata(pci);
> + if (card) {
> + struct azx *chip = card->private_data;
> + if (chip->disabled)
> + return 0; /* will be loaded via vga_switcheroo */
> + err = azx_probe_continue(chip);
> + if (err < 0)
> + goto out_free;
> + return 0;
> + }
> +#endif
> +
> 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;
> }
>
>
>


Attachments:
(No filename) (5.73 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-09 12:32:41

by Takashi Iwai

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

At Thu, 9 Aug 2012 12:34:30 +0200,
Thierry Reding wrote:
>
> On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote:
> > At Thu, 9 Aug 2012 10:07:13 +0200,
> > Thierry Reding wrote:
> > >
> > > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > > > At Thu, 9 Aug 2012 09:36:42 +0200,
> > > > Thierry Reding wrote:
> > > > >
> > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > > > Thierry Reding wrote:
> > > > > > >
> > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > > > Thierry Reding wrote:
> > > > > > > > >
> > > > > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > > > > binding to the device outside the module_init() call.
> > > > > > > >
> > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > > > >
> > > > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > > > > in the firmware loader core code.
> > > > > > >
> > > > > > > A good explanation of the problem and subsequent discussion can be found
> > > > > > > here:
> > > > > > >
> > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > > > >
> > > > > > Yeah, but it doesn't justify this ugly module option.
> > > > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > > > anything.
> > > > >
> > > > > It's not an option, all it does is defer probing if and only if the
> > > > > patch parameter was specified to make sure the firmware load won't
> > > > > stall. I realize that this may not be an optimal solution, but at least
> > > > > it fixes the problem with no fallout.
> > > >
> > > > Ah sorry, I misread the patch.
> > > >
> > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > > > probing code was already split for vga_switcheroo support.
> > >
> > > Yes, I saw that. But unless you actually use vga_switcheroo, the second
> > > stage, azx_probe_continue(), will still be called from azx_probe() and
> > > therefore ultimately from module_init().
> >
> > Yeah, but this could be easily delayed. The split was already done,
> > so the next step would be to return after the first half at probe,
> > then call the second half later.
> >
> > > Before coming up with this patch I actually did play around a bit with
> > > using the asynchronous firmware load functions but it turned out to be
> > > rather difficult to do so I opted for the easy way. The biggest problem
> > > I faced was that since patch loading needs to be done very early on, a
> > > lot of the initialization would need to be done after .probe() and many
> > > things could still fail, so cleaning up after errors would become
> > > increasingly difficult.
> >
> > async probe is also on my TODO list, but it's deferred ;)
> >
> > > > The point you added is the second stage.
> > >
> > > I don't understand this sentence.
> >
> > I meant that your patch added the check at the second-half probing
> > function (azx_probe_contine()). That is, it could be already the
> > point triggered by vga_switcheroo handler, not via module_init any
> > longer.
> >
> > So, after rethinking what you suggested, I wrote a quick patch below.
> > Could you check whether this works?
>
> It oopses, though I can't quite tell where. I need to test some more
> later to see where it goes wrong.

Yeah, I tested it here and noticed, too. As mentioned, the behavior
of -EPROBE_DEFER is somehow flaky. For example, when it's used for
modules, the deferred probe will be never triggered (unless a new
device is bound).

Considering the problem again, it's currently an issue only for the
built-in sound driver, right? AFAIK, request_firmware() works fine
for modules. If so, a simple "fix" to avoid the unexpected behavior
is to make CONFIG_SND_HDA_PATCH_LOADER depending on CONFIG_SND_HDA=m.
It'd be simple enough for merging 3.6 kernel.

I almost finished writing the patch to use request_firmware_nowait()
version, but I'm afraid it's too intrusive for 3.6. If disabling the
patch loader for the built-in driver is OK, I'd queue the
request_firmware_nowait() patches to 3.7 queue.


Takashi

2012-08-09 12:49:15

by Thierry Reding

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

On Thu, Aug 09, 2012 at 02:32:38PM +0200, Takashi Iwai wrote:
> At Thu, 9 Aug 2012 12:34:30 +0200,
> Thierry Reding wrote:
> >
> > On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote:
> > > At Thu, 9 Aug 2012 10:07:13 +0200,
> > > Thierry Reding wrote:
> > > >
> > > > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > > > > At Thu, 9 Aug 2012 09:36:42 +0200,
> > > > > Thierry Reding wrote:
> > > > > >
> > > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > > > > Thierry Reding wrote:
> > > > > > > >
> > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > > > > Thierry Reding wrote:
> > > > > > > > > >
> > > > > > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > > > > > binding to the device outside the module_init() call.
> > > > > > > > >
> > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > > > > >
> > > > > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > > > > > in the firmware loader core code.
> > > > > > > >
> > > > > > > > A good explanation of the problem and subsequent discussion can be found
> > > > > > > > here:
> > > > > > > >
> > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > > > > >
> > > > > > > Yeah, but it doesn't justify this ugly module option.
> > > > > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > > > > anything.
> > > > > >
> > > > > > It's not an option, all it does is defer probing if and only if the
> > > > > > patch parameter was specified to make sure the firmware load won't
> > > > > > stall. I realize that this may not be an optimal solution, but at least
> > > > > > it fixes the problem with no fallout.
> > > > >
> > > > > Ah sorry, I misread the patch.
> > > > >
> > > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > > > > probing code was already split for vga_switcheroo support.
> > > >
> > > > Yes, I saw that. But unless you actually use vga_switcheroo, the second
> > > > stage, azx_probe_continue(), will still be called from azx_probe() and
> > > > therefore ultimately from module_init().
> > >
> > > Yeah, but this could be easily delayed. The split was already done,
> > > so the next step would be to return after the first half at probe,
> > > then call the second half later.
> > >
> > > > Before coming up with this patch I actually did play around a bit with
> > > > using the asynchronous firmware load functions but it turned out to be
> > > > rather difficult to do so I opted for the easy way. The biggest problem
> > > > I faced was that since patch loading needs to be done very early on, a
> > > > lot of the initialization would need to be done after .probe() and many
> > > > things could still fail, so cleaning up after errors would become
> > > > increasingly difficult.
> > >
> > > async probe is also on my TODO list, but it's deferred ;)
> > >
> > > > > The point you added is the second stage.
> > > >
> > > > I don't understand this sentence.
> > >
> > > I meant that your patch added the check at the second-half probing
> > > function (azx_probe_contine()). That is, it could be already the
> > > point triggered by vga_switcheroo handler, not via module_init any
> > > longer.
> > >
> > > So, after rethinking what you suggested, I wrote a quick patch below.
> > > Could you check whether this works?
> >
> > It oopses, though I can't quite tell where. I need to test some more
> > later to see where it goes wrong.
>
> Yeah, I tested it here and noticed, too. As mentioned, the behavior
> of -EPROBE_DEFER is somehow flaky. For example, when it's used for
> modules, the deferred probe will be never triggered (unless a new
> device is bound).

Yes, the idea is that probing is only retried after other drivers have
bound to other devices because otherwise nothing about any missing
resources can have changed. This however would indicate that deferred
probing is not the right solution here, after all we're not waiting for
another resource to become available. Something like delayed work might
be better suited.

Well, asynchronous firmware load is actually the right solution, delayed
work just might be a better workaround. =)

For completeness I should say that I've been using deferred probing with
modules quite successfully on another platform, so it is not a general
problem. Rather as you said, it is only triggered if another module is
loaded after the deferral.

> Considering the problem again, it's currently an issue only for the
> built-in sound driver, right? AFAIK, 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


Attachments:
(No filename) (6.07 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-09 13:06:15

by Takashi Iwai

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

At Thu, 9 Aug 2012 14:49:04 +0200,
Thierry Reding wrote:
>
> On Thu, Aug 09, 2012 at 02:32:38PM +0200, Takashi Iwai wrote:
> > At Thu, 9 Aug 2012 12:34:30 +0200,
> > Thierry Reding wrote:
> > >
> > > On Thu, Aug 09, 2012 at 10:21:15AM +0200, Takashi Iwai wrote:
> > > > At Thu, 9 Aug 2012 10:07:13 +0200,
> > > > Thierry Reding wrote:
> > > > >
> > > > > On Thu, Aug 09, 2012 at 09:42:48AM +0200, Takashi Iwai wrote:
> > > > > > At Thu, 9 Aug 2012 09:36:42 +0200,
> > > > > > Thierry Reding wrote:
> > > > > > >
> > > > > > > On Thu, Aug 09, 2012 at 09:31:30AM +0200, Takashi Iwai wrote:
> > > > > > > > At Thu, 9 Aug 2012 09:08:13 +0200,
> > > > > > > > Thierry Reding wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Aug 09, 2012 at 08:57:13AM +0200, Takashi Iwai wrote:
> > > > > > > > > > At Thu, 9 Aug 2012 08:45:23 +0200,
> > > > > > > > > > Thierry Reding wrote:
> > > > > > > > > > >
> > > > > > > > > > > Recent changes to the firmware loading helpers cause drivers to stall
> > > > > > > > > > > when firmware is loaded during the module_init() call. The snd-hda-intel
> > > > > > > > > > > module requests firmware if the patch= parameter is used to load a patch
> > > > > > > > > > > file. This patch works around the problem by deferring the probe in such
> > > > > > > > > > > cases, which will cause the module to load successfully and the driver
> > > > > > > > > > > binding to the device outside the module_init() call.
> > > > > > > > > >
> > > > > > > > > > Is the "recent" change meant 3.6 kernel, or in linux-next?
> > > > > > > > > >
> > > > > > > > > > In anyway, I don't understand why such a change was allowed. Most
> > > > > > > > > > drivers do call request_firmware() at the device probing time.
> > > > > > > > > > If this really has to be resolved in the driver side, it must be a bug
> > > > > > > > > > in the firmware loader core code.
> > > > > > > > >
> > > > > > > > > A good explanation of the problem and subsequent discussion can be found
> > > > > > > > > here:
> > > > > > > > >
> > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/49975
> > > > > > > >
> > > > > > > > Yeah, but it doesn't justify this ugly module option.
> > > > > > > > It's a simple bug. Papering over it with this option doesn't fix
> > > > > > > > anything.
> > > > > > >
> > > > > > > It's not an option, all it does is defer probing if and only if the
> > > > > > > patch parameter was specified to make sure the firmware load won't
> > > > > > > stall. I realize that this may not be an optimal solution, but at least
> > > > > > > it fixes the problem with no fallout.
> > > > > >
> > > > > > Ah sorry, I misread the patch.
> > > > > >
> > > > > > Then it shouldn't be checked at that point. Since 3.5 kernel, the
> > > > > > probing code was already split for vga_switcheroo support.
> > > > >
> > > > > Yes, I saw that. But unless you actually use vga_switcheroo, the second
> > > > > stage, azx_probe_continue(), will still be called from azx_probe() and
> > > > > therefore ultimately from module_init().
> > > >
> > > > Yeah, but this could be easily delayed. The split was already done,
> > > > so the next step would be to return after the first half at probe,
> > > > then call the second half later.
> > > >
> > > > > Before coming up with this patch I actually did play around a bit with
> > > > > using the asynchronous firmware load functions but it turned out to be
> > > > > rather difficult to do so I opted for the easy way. The biggest problem
> > > > > I faced was that since patch loading needs to be done very early on, a
> > > > > lot of the initialization would need to be done after .probe() and many
> > > > > things could still fail, so cleaning up after errors would become
> > > > > increasingly difficult.
> > > >
> > > > async probe is also on my TODO list, but it's deferred ;)
> > > >
> > > > > > The point you added is the second stage.
> > > > >
> > > > > I don't understand this sentence.
> > > >
> > > > I meant that your patch added the check at the second-half probing
> > > > function (azx_probe_contine()). That is, it could be already the
> > > > point triggered by vga_switcheroo handler, not via module_init any
> > > > longer.
> > > >
> > > > So, after rethinking what you suggested, I wrote a quick patch below.
> > > > Could you check whether this works?
> > >
> > > It oopses, though I can't quite tell where. I need to test some more
> > > later to see where it goes wrong.
> >
> > Yeah, I tested it here and noticed, too. As mentioned, the behavior
> > of -EPROBE_DEFER is somehow flaky. For example, when it's used for
> > modules, the deferred probe will be never triggered (unless a new
> > device is bound).
>
> Yes, the idea is that probing is only retried after other drivers have
> bound to other devices because otherwise nothing about any missing
> resources can have changed. This however would indicate that deferred
> probing is not the right solution here, after all we're not waiting for
> another resource to become available. Something like delayed work might
> be better suited.
>
> Well, asynchronous firmware load is actually the right solution, delayed
> work just might be a better workaround. =)
>
> For completeness I should say that I've been using deferred probing with
> modules quite successfully on another platform, so it is not a 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

2012-08-09 13:09:25

by Takashi Iwai

[permalink] [raw]
Subject: [PATCH 0/2] HD-audio firmware loading fixes

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

2012-08-09 13:11:43

by Takashi Iwai

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

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 <[email protected]>
---
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

2012-08-09 13:26:55

by David Henningsson

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

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

2012-08-09 13:32:22

by Takashi Iwai

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

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
>

2012-08-09 13:36:50

by Takashi Iwai

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

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 <[email protected]>
---
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

2012-08-09 13:54:04

by David Henningsson

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

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

2012-08-09 14:04:12

by Thierry Reding

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

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


Attachments:
(No filename) (606.00 B)
(No filename) (836.00 B)
Download all attachments

2012-08-09 14:04:54

by Takashi Iwai

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

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

2012-08-09 14:10:57

by Takashi Iwai

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

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 <[email protected]>
---
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

2012-08-09 14:19:36

by David Henningsson

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

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 <[email protected]>

Looks better now!

Reviewed-by: David Henningsson <[email protected]>

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

2012-08-09 14:25:44

by Thierry Reding

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

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 <[email protected]>
> ---
> 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 <[email protected]>

Thierry


Attachments:
(No filename) (714.00 B)
(No filename) (836.00 B)
Download all attachments

2012-08-09 14:34:44

by Takashi Iwai

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

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 <[email protected]>
> > ---
> > 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 <[email protected]>

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

2012-08-09 14:38:33

by Thierry Reding

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

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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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


Attachments:
(No filename) (1.13 kB)
(No filename) (836.00 B)
Download all attachments