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
