[alsa-devel] [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 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;
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
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/4997...
Thierry
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/4997...
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
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/4997...
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
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/4997...
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
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/4997...
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
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/4997...
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
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/4997...
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
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/4997...
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; }
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/4997...
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
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/4997...
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;
}
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/4997...
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
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/4997... > > 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
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/4997... > > > > 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
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
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) {
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
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;
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);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;
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);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
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;
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);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;
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
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
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;
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);probe_now = false; /* continued in azx_firmware_cb() */
- if (!chip->disabled) {
- if (probe_now) { err = azx_probe_continue(chip); if (err < 0) goto out_free;
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
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
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
participants (3)
-
David Henningsson -
Takashi Iwai -
Thierry Reding