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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
> > ---
> >  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@xxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
 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]);
-- 
2.26.2




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux