Re: [PATCH v2 39/79] ALSA: korg1212: Allocate resources with device-managed APIs

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Jul 15, 2021 at 09:59:01AM +0200, Takashi Iwai wrote:
> This patch converts the resource management in PCI korg1212 driver
> with devres as a clean up.  Each manual resource management is
> converted with the corresponding devres helper, the page allocations
> are done with the devres helper, and the card object release is
> managed now via card->private_free instead of a lowlevel snd_device.
> 
> This should give no user-visible functional changes.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/korg1212/korg1212.c | 211 +++++++++-------------------------
>  1 file changed, 55 insertions(+), 156 deletions(-)
> 
> diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
> index 030e01b062e4..7872abbd4587 100644
> --- a/sound/pci/korg1212/korg1212.c
> +++ b/sound/pci/korg1212/korg1212.c
> @@ -320,10 +320,10 @@ struct snd_korg1212 {
>          unsigned long inIRQ;
>          void __iomem *iobase;
>  
> -	struct snd_dma_buffer dma_dsp;
> -        struct snd_dma_buffer dma_play;
> -        struct snd_dma_buffer dma_rec;
> -	struct snd_dma_buffer dma_shared;
> +	struct snd_dma_buffer *dma_dsp;
> +	struct snd_dma_buffer *dma_play;
> +	struct snd_dma_buffer *dma_rec;
> +	struct snd_dma_buffer *dma_shared;
>  
>  	u32 DataBufsSize;
>  
> @@ -1200,8 +1200,8 @@ static int snd_korg1212_downloadDSPCode(struct snd_korg1212 *korg1212)
>          snd_korg1212_setCardState(korg1212, K1212_STATE_DSP_IN_PROCESS);
>  
>          rc = snd_korg1212_Send1212Command(korg1212, K1212_DB_StartDSPDownload,
> -                                     UpperWordSwap(korg1212->dma_dsp.addr),
> -                                     0, 0, 0);
> +					  UpperWordSwap(korg1212->dma_dsp->addr),
> +					  0, 0, 0);
>  	if (rc)
>  		K1212_DEBUG_PRINTK("K1212_DEBUG: Start DSP Download RC = %d [%s]\n",
>  				   rc, stateName[korg1212->cardState]);
> @@ -1382,7 +1382,7 @@ static int snd_korg1212_playback_open(struct snd_pcm_substream *substream)
>  	snd_korg1212_OpenCard(korg1212);
>  
>          runtime->hw = snd_korg1212_playback_info;
> -	snd_pcm_set_runtime_buffer(substream, &korg1212->dma_play);
> +	snd_pcm_set_runtime_buffer(substream, korg1212->dma_play);
>  
>          spin_lock_irqsave(&korg1212->lock, flags);
>  
> @@ -1413,7 +1413,7 @@ static int snd_korg1212_capture_open(struct snd_pcm_substream *substream)
>  	snd_korg1212_OpenCard(korg1212);
>  
>          runtime->hw = snd_korg1212_capture_info;
> -	snd_pcm_set_runtime_buffer(substream, &korg1212->dma_rec);
> +	snd_pcm_set_runtime_buffer(substream, korg1212->dma_rec);
>  
>          spin_lock_irqsave(&korg1212->lock, flags);
>  
> @@ -2080,71 +2080,16 @@ static void snd_korg1212_proc_init(struct snd_korg1212 *korg1212)
>  			     snd_korg1212_proc_read);
>  }
>  
> -static int
> -snd_korg1212_free(struct snd_korg1212 *korg1212)
> +static void
> +snd_korg1212_free(struct snd_card *card)
>  {
> -        snd_korg1212_TurnOffIdleMonitor(korg1212);
> -
> -        if (korg1212->irq >= 0) {
> -                snd_korg1212_DisableCardInterrupts(korg1212);
> -                free_irq(korg1212->irq, korg1212);
> -                korg1212->irq = -1;
> -        }
> -        
> -        if (korg1212->iobase != NULL) {
> -                iounmap(korg1212->iobase);
> -                korg1212->iobase = NULL;
> -        }
> -        
> -	pci_release_regions(korg1212->pci);
> -
> -        // ----------------------------------------------------
> -        // free up memory resources used for the DSP download.
> -        // ----------------------------------------------------
> -        if (korg1212->dma_dsp.area) {
> -        	snd_dma_free_pages(&korg1212->dma_dsp);
> -        	korg1212->dma_dsp.area = NULL;
> -        }
> -
> -#ifndef K1212_LARGEALLOC
> -
> -        // ------------------------------------------------------
> -        // free up memory resources used for the Play/Rec Buffers
> -        // ------------------------------------------------------
> -	if (korg1212->dma_play.area) {
> -		snd_dma_free_pages(&korg1212->dma_play);
> -		korg1212->dma_play.area = NULL;
> -        }
> +	struct snd_korg1212 *korg1212 = card->private_data;
>  
> -	if (korg1212->dma_rec.area) {
> -		snd_dma_free_pages(&korg1212->dma_rec);
> -		korg1212->dma_rec.area = NULL;
> -        }
> -
> -#endif
> -
> -        // ----------------------------------------------------
> -        // free up memory resources used for the Shared Buffers
> -        // ----------------------------------------------------
> -	if (korg1212->dma_shared.area) {
> -		snd_dma_free_pages(&korg1212->dma_shared);
> -		korg1212->dma_shared.area = NULL;
> -        }
> -        
> -	pci_disable_device(korg1212->pci);
> -        kfree(korg1212);
> -        return 0;
> -}
> -
> -static int snd_korg1212_dev_free(struct snd_device *device)
> -{
> -        struct snd_korg1212 *korg1212 = device->device_data;
> -        K1212_DEBUG_PRINTK("K1212_DEBUG: Freeing device\n");
> -	return snd_korg1212_free(korg1212);
> +	snd_korg1212_TurnOffIdleMonitor(korg1212);
> +	snd_korg1212_DisableCardInterrupts(korg1212);
>  }
>  
> -static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> -			       struct snd_korg1212 **rchip)
> +static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci)
>  
>  {
>          int err, rc;
> @@ -2152,24 +2097,13 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>  	unsigned iomem_size;
>  	__maybe_unused unsigned ioport_size;
>  	__maybe_unused unsigned iomem2_size;
> -        struct snd_korg1212 * korg1212;
> +	struct snd_korg1212 *korg1212 = card->private_data;
>  	const struct firmware *dsp_code;
>  
> -	static const struct snd_device_ops ops = {
> -                .dev_free = snd_korg1212_dev_free,
> -        };
> -
> -        * rchip = NULL;
> -	err = pci_enable_device(pci);
> +	err = pcim_enable_device(pci);
>  	if (err < 0)
>                  return err;
>  
> -        korg1212 = kzalloc(sizeof(*korg1212), GFP_KERNEL);
> -        if (korg1212 == NULL) {
> -		pci_disable_device(pci);
> -                return -ENOMEM;
> -	}
> -
>  	korg1212->card = card;
>  	korg1212->pci = pci;
>  
> @@ -2198,12 +2132,9 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>          for (i=0; i<kAudioChannels; i++)
>                  korg1212->volumePhase[i] = 0;
>  
> -	err = pci_request_regions(pci, "korg1212");
> -	if (err < 0) {
> -		kfree(korg1212);
> -		pci_disable_device(pci);
> +	err = pcim_iomap_regions_request_all(pci, 1 << 0, "korg1212");
> +	if (err < 0)
>  		return err;
> -	}
>  
>          korg1212->iomem = pci_resource_start(korg1212->pci, 0);
>          korg1212->ioport = pci_resource_start(korg1212->pci, 1);
> @@ -2223,26 +2154,20 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>  		   korg1212->iomem2, iomem2_size,
>  		   stateName[korg1212->cardState]);
>  
> -	korg1212->iobase = ioremap(korg1212->iomem, iomem_size);
> -	if (!korg1212->iobase) {
> -		snd_printk(KERN_ERR "korg1212: unable to remap memory region 0x%lx-0x%lx\n", korg1212->iomem,
> -                           korg1212->iomem + iomem_size - 1);
> -                snd_korg1212_free(korg1212);
> -                return -EBUSY;
> -        }
> +	korg1212->iobase = pcim_iomap_table(pci)[0];
>  
> -        err = request_irq(pci->irq, snd_korg1212_interrupt,
> +	err = devm_request_irq(&pci->dev, pci->irq, snd_korg1212_interrupt,
>                            IRQF_SHARED,
>                            KBUILD_MODNAME, korg1212);
>  
>          if (err) {
>  		snd_printk(KERN_ERR "korg1212: unable to grab IRQ %d\n", pci->irq);
> -                snd_korg1212_free(korg1212);
>                  return -EBUSY;
>          }
>  
>          korg1212->irq = pci->irq;
>  	card->sync_irq = korg1212->irq;
> +	card->private_free = snd_korg1212_free;
>  
>  	pci_set_master(korg1212->pci);
>  
> @@ -2281,41 +2206,36 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>                     korg1212->idRegPtr,
>  		   stateName[korg1212->cardState]);
>  
> -	if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> -				sizeof(struct KorgSharedBuffer), &korg1212->dma_shared) < 0) {
> -		snd_printk(KERN_ERR "korg1212: can not allocate shared buffer memory (%zd bytes)\n", sizeof(struct KorgSharedBuffer));
> -                snd_korg1212_free(korg1212);
> -                return -ENOMEM;
> -        }
> -        korg1212->sharedBufferPtr = (struct KorgSharedBuffer *)korg1212->dma_shared.area;
> -        korg1212->sharedBufferPhy = korg1212->dma_shared.addr;
> +	korg1212->dma_shared = snd_devm_alloc_pages(&pci->dev,
> +						    SNDRV_DMA_TYPE_DEV,
> +						    sizeof(struct KorgSharedBuffer));
> +	if (!korg1212->dma_shared)
> +		return -ENOMEM;
> +	korg1212->sharedBufferPtr = (struct KorgSharedBuffer *)korg1212->dma_shared->area;
> +	korg1212->sharedBufferPhy = korg1212->dma_shared->addr;
>  
>          K1212_DEBUG_PRINTK("K1212_DEBUG: Shared Buffer Area = 0x%p (0x%08lx), %d bytes\n", korg1212->sharedBufferPtr, korg1212->sharedBufferPhy, sizeof(struct KorgSharedBuffer));
>  
>  #ifndef K1212_LARGEALLOC
> -
>          korg1212->DataBufsSize = sizeof(struct KorgAudioBuffer) * kNumBuffers;
> +	korg1212->dma_play = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV,
> +						  korg1212->DataBufsSize);
> +	if (!korg1212->dma_play)
> +		return -ENOMEM;
>  
> -	if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> -				korg1212->DataBufsSize, &korg1212->dma_play) < 0) {
> -		snd_printk(KERN_ERR "korg1212: can not allocate play data buffer memory (%d bytes)\n", korg1212->DataBufsSize);
> -                snd_korg1212_free(korg1212);
> -                return -ENOMEM;
> -        }
> -	korg1212->playDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_play.area;
> -	korg1212->PlayDataPhy = korg1212->dma_play.addr;
> +	korg1212->playDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_play->area;
> +	korg1212->PlayDataPhy = korg1212->dma_play->addr;
>  
>          K1212_DEBUG_PRINTK("K1212_DEBUG: Play Data Area = 0x%p (0x%08x), %d bytes\n",
>  		korg1212->playDataBufsPtr, korg1212->PlayDataPhy, korg1212->DataBufsSize);
>  
> -	if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> -				korg1212->DataBufsSize, &korg1212->dma_rec) < 0) {
> -		snd_printk(KERN_ERR "korg1212: can not allocate record data buffer memory (%d bytes)\n", korg1212->DataBufsSize);
> -                snd_korg1212_free(korg1212);
> -                return -ENOMEM;
> -        }
> -        korg1212->recordDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_rec.area;
> -        korg1212->RecDataPhy = korg1212->dma_rec.addr;
> +	korg1212->dma_rec = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV,
> +						 korg1212->DataBufsSize);
> +	if (!korg1212->dma_rec)
> +		return -ENOMEM;
> +
> +	korg1212->recordDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_rec->area;
> +	korg1212->RecDataPhy = korg1212->dma_rec->addr;
>  
>          K1212_DEBUG_PRINTK("K1212_DEBUG: Record Data Area = 0x%p (0x%08x), %d bytes\n",
>  		korg1212->recordDataBufsPtr, korg1212->RecDataPhy, korg1212->DataBufsSize);
> @@ -2336,26 +2256,22 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>          korg1212->AdatTimeCodePhy = korg1212->sharedBufferPhy +
>  		offsetof(struct KorgSharedBuffer, AdatTimeCode);
>  
> +	korg1212->dma_dsp = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV,
> +						 dsp_code->size);
> +	if (!korg1212->dma_dsp)
> +		return -ENOMEM;
> +

Should this section be moved below the next one?

sound/pci/korg1212/korg1212.c:2260:8: error: variable 'dsp_code' is uninitialized when used here [-Werror,-Wuninitialized]
                                                 dsp_code->size);
                                                 ^~~~~~~~
sound/pci/korg1212/korg1212.c:2101:33: note: initialize the variable 'dsp_code' to silence this warning
        const struct firmware *dsp_code;
                                       ^
                                        = NULL
1 error generated.

>  	err = request_firmware(&dsp_code, "korg/k1212.dsp", &pci->dev);
>  	if (err < 0) {
>  		snd_printk(KERN_ERR "firmware not available\n");
> -		snd_korg1212_free(korg1212);
>  		return err;
>  	}
>  
> -	if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> -				dsp_code->size, &korg1212->dma_dsp) < 0) {
> -		snd_printk(KERN_ERR "korg1212: cannot allocate dsp code memory (%zd bytes)\n", dsp_code->size);
> -                snd_korg1212_free(korg1212);
> -		release_firmware(dsp_code);
> -                return -ENOMEM;
> -        }
> -
>          K1212_DEBUG_PRINTK("K1212_DEBUG: DSP Code area = 0x%p (0x%08x) %d bytes [%s]\n",
> -		   korg1212->dma_dsp.area, korg1212->dma_dsp.addr, dsp_code->size,
> +		   korg1212->dma_dsp->area, korg1212->dma_dsp->addr, dsp_code->size,
>  		   stateName[korg1212->cardState]);
>  
> -	memcpy(korg1212->dma_dsp.area, dsp_code->data, dsp_code->size);
> +	memcpy(korg1212->dma_dsp->area, dsp_code->data, dsp_code->size);
>  
>  	release_firmware(dsp_code);
>  
> @@ -2364,12 +2280,6 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>  	if (rc)
>  		K1212_DEBUG_PRINTK("K1212_DEBUG: Reboot Card - RC = %d [%s]\n", rc, stateName[korg1212->cardState]);
>  
> -	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, korg1212, &ops);
> -	if (err < 0) {
> -                snd_korg1212_free(korg1212);
> -                return err;
> -        }
> -        
>  	snd_korg1212_EnableCardInterrupts(korg1212);
>  
>  	mdelay(CARD_BOOT_DELAY_IN_MS);
> @@ -2411,10 +2321,8 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
>          }
>  
>          snd_korg1212_proc_init(korg1212);
> -        
> -        * rchip = korg1212;
> -	return 0;
>  
> +	return 0;
>  }
>  
>  /*
> @@ -2437,16 +2345,15 @@ snd_korg1212_probe(struct pci_dev *pci,
>  		dev++;
>  		return -ENOENT;
>  	}
> -	err = snd_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
> -			   0, &card);
> +	err = snd_devm_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
> +				sizeof(*korg1212), &card);
>  	if (err < 0)
>  		return err;
> +	korg1212 = card->private_data;
>  
> -	err = snd_korg1212_create(card, pci, &korg1212);
> -	if (err < 0) {
> -		snd_card_free(card);
> +	err = snd_korg1212_create(card, pci);
> +	if (err < 0)
>  		return err;
> -	}
>  
>  	strcpy(card->driver, "korg1212");
>  	strcpy(card->shortname, "korg1212");
> @@ -2456,25 +2363,17 @@ snd_korg1212_probe(struct pci_dev *pci,
>          K1212_DEBUG_PRINTK("K1212_DEBUG: %s\n", card->longname);
>  
>  	err = snd_card_register(card);
> -	if (err < 0) {
> -		snd_card_free(card);
> +	if (err < 0)
>  		return err;
> -	}
>  	pci_set_drvdata(pci, card);
>  	dev++;
>  	return 0;
>  }
>  
> -static void snd_korg1212_remove(struct pci_dev *pci)
> -{
> -	snd_card_free(pci_get_drvdata(pci));
> -}
> -
>  static struct pci_driver korg1212_driver = {
>  	.name = KBUILD_MODNAME,
>  	.id_table = snd_korg1212_ids,
>  	.probe = snd_korg1212_probe,
> -	.remove = snd_korg1212_remove,
>  };
>  
>  module_pci_driver(korg1212_driver);
> -- 
> 2.26.2