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

[David Henningsson <david.henningsson@canonical.com>]

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