On Mon, Nov 18, 2019 at 07:34:16PM +0530, Ravulapati Vishnu vardhan rao wrote:
> static int snd_acp3x_probe(struct pci_dev *pci,
> const struct pci_device_id *pci_id)
> {
> - int ret;
> - u32 addr, val;
> struct acp3x_dev_data *adata;
> - struct platform_device_info pdevinfo;
> + struct platform_device_info pdevinfo[ACP3x_DEVS];
> unsigned int irqflags;
> + int ret, val, i;
> + u32 addr;
>
> if (pci_enable_device(pci)) {
> dev_err(&pci->dev, "pci_enable_device failed\n");
> @@ -40,10 +40,10 @@ static int snd_acp3x_probe(struct pci_dev *pci,
> }
>
> adata = devm_kzalloc(&pci->dev, sizeof(struct acp3x_dev_data),
> - GFP_KERNEL);
> + GFP_KERNEL);
> if (!adata) {
> ret = -ENOMEM;
> - goto release_regions;
> + goto adata_free;
> }
>
> /* check for msi interrupt support */
> @@ -56,7 +56,8 @@ static int snd_acp3x_probe(struct pci_dev *pci,
> irqflags = 0;
>
> addr = pci_resource_start(pci, 0);
> - adata->acp3x_base = ioremap(addr, pci_resource_len(pci, 0));
> + adata->acp3x_base = devm_ioremap(&pci->dev, addr,
> + pci_resource_len(pci, 0));
> if (!adata->acp3x_base) {
> ret = -ENOMEM;
> goto release_regions;
> @@ -68,11 +69,11 @@ static int snd_acp3x_probe(struct pci_dev *pci,
> switch (val) {
> case I2S_MODE:
> adata->res = devm_kzalloc(&pci->dev,
> - sizeof(struct resource) * 2,
> + sizeof(struct resource) * 4,
> GFP_KERNEL);
> if (!adata->res) {
> ret = -ENOMEM;
> - goto unmap_mmio;
> + goto release_regions;
> }
>
> adata->res[0].name = "acp3x_i2s_iomem";
> @@ -80,28 +81,52 @@ static int snd_acp3x_probe(struct pci_dev *pci,
> adata->res[0].start = addr;
> adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START);
>
> - adata->res[1].name = "acp3x_i2s_irq";
> - adata->res[1].flags = IORESOURCE_IRQ;
> - adata->res[1].start = pci->irq;
> - adata->res[1].end = pci->irq;
> + adata->res[1].name = "acp3x_i2s_sp";
> + adata->res[1].flags = IORESOURCE_MEM;
> + adata->res[1].start = addr + ACP3x_I2STDM_REG_START;
> + adata->res[1].end = addr + ACP3x_I2STDM_REG_END;
> +
> + adata->res[2].name = "acp3x_i2s_bt";
> + adata->res[2].flags = IORESOURCE_MEM;
> + adata->res[2].start = addr + ACP3x_BT_TDM_REG_START;
> + adata->res[2].end = addr + ACP3x_BT_TDM_REG_END;
> +
> + adata->res[3].name = "acp3x_i2s_irq";
> + adata->res[3].flags = IORESOURCE_IRQ;
> + adata->res[3].start = pci->irq;
> + adata->res[3].end = adata->res[3].start;
>
> adata->acp3x_audio_mode = ACP3x_I2S_MODE;
>
> memset(&pdevinfo, 0, sizeof(pdevinfo));
> - pdevinfo.name = "acp3x_rv_i2s";
> - pdevinfo.id = 0;
> - pdevinfo.parent = &pci->dev;
> - pdevinfo.num_res = 2;
> - pdevinfo.res = adata->res;
> - pdevinfo.data = &irqflags;
> - pdevinfo.size_data = sizeof(irqflags);
> -
> - adata->pdev = platform_device_register_full(&pdevinfo);
> - if (IS_ERR(adata->pdev)) {
> - dev_err(&pci->dev, "cannot register %s device\n",
> - pdevinfo.name);
> - ret = PTR_ERR(adata->pdev);
> - goto unmap_mmio;
> + pdevinfo[0].name = "acp3x_rv_i2s_dma";
> + pdevinfo[0].id = 0;
> + pdevinfo[0].parent = &pci->dev;
> + pdevinfo[0].num_res = 4;
> + pdevinfo[0].res = &adata->res[0];
> + pdevinfo[0].data = &irqflags;
> + pdevinfo[0].size_data = sizeof(irqflags);
> +
> + pdevinfo[1].name = "acp3x_i2s_playcap";
> + pdevinfo[1].id = 0;
> + pdevinfo[1].parent = &pci->dev;
> + pdevinfo[1].num_res = 1;
> + pdevinfo[1].res = &adata->res[1];
> +
> + pdevinfo[2].name = "acp3x_i2s_playcap";
> + pdevinfo[2].id = 1;
> + pdevinfo[2].parent = &pci->dev;
> + pdevinfo[2].num_res = 1;
> + pdevinfo[2].res = &adata->res[2];
> + for (i = 0; i < ACP3x_DEVS ; i++) {
> + adata->pdev[i] =
> + platform_device_register_full(&pdevinfo[i]);
> + if (IS_ERR(adata->pdev[i])) {
> + dev_err(&pci->dev, "cannot register %s device\n",
> + pdevinfo[i].name);
> + ret = PTR_ERR(adata->pdev[i]);
> + goto unmap_mmio;
> + }
> }
> break;
> default:
> @@ -112,10 +137,22 @@ static int snd_acp3x_probe(struct pci_dev *pci,
> return 0;
>
> unmap_mmio:
> + if (val == I2S_MODE)
> + for (i = 0 ; i < ACP3x_DEVS ; i++)
> + platform_device_unregister(adata->pdev[i]);
I really wish you had done this the way, I said. That's the easiest
way to audit. Otherwise you have to check that adata->pdev[] is
initialized and that platform_device_unregister() accepts both error
pointers and NULLs. It does. But it's easier to audit if you only
free things which have been allocated and never free things which were
not allocated.
> + devm_kfree(&pci->dev, adata->res);
Don't free devm_ allocated data. It gets freed for you automatically.
> + devm_kfree(&pci->dev, adata);
> pci_disable_msi(pci);
> - iounmap(adata->acp3x_base);
> + pci_release_regions(pci);
> + pci_disable_device(pci);
> release_regions:
> + devm_kfree(&pci->dev, adata);
> + pci_disable_msi(pci);
This disables the msi twice so it's a double free situation. It would
have been better to just cut and paste the code that I wrote for you.
regards,
dan carpenter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
