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

On Tue, 20 Jul 2021 21:41:51 +0200, Nathan Chancellor wrote:

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.

Indeed, the wrong code shuffle caused it. The fix is below.

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: korg1212: Fix wrongly shuffled firmware loader code

The recent change for the devres introduced the wrong code shuffling in the korg1212 firmware loader function that may lead to a bad pointer access. Restore the calls in the right order (and put back the release_firmware() call in the error path, too).

Fixes: b5cde369b618 ("ALSA: korg1212: Allocate resources with device-managed APIs") Reported-by: Nathan Chancellor nathan@kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/korg1212/korg1212.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c index 7872abbd4587..49ed2bfaf11f 100644 --- a/sound/pci/korg1212/korg1212.c +++ b/sound/pci/korg1212/korg1212.c @@ -2256,17 +2256,19 @@ 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; - err = request_firmware(&dsp_code, "korg/k1212.dsp", &pci->dev); if (err < 0) { snd_printk(KERN_ERR "firmware not available\n"); return err; }

+ korg1212->dma_dsp = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV, + dsp_code->size); + if (!korg1212->dma_dsp) { + 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, stateName[korg1212->cardState]);