I'm sorry but the probe error handling is totally broken. It has a bunch of double frees. Here is how it should work:

On Fri, Nov 15, 2019 at 05:40:18PM +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;
• u32 addr, val, i;

Make i an int. Loop iterators should be int unless we are going to loop more than INT_MAX times (which hopefully is seldom in the kernel).

if (pci_enable_device(pci)) {

Ideally this should preserve the error code from pci_enable_device().

ret = pci_enable_device(pci); if (ret) return ret;

But 1) you didn't introduce this. 2) This code is basically fine. This is the first resource allocation so there is no error handling, we just return ret. But after this the most recently allocated resource is 'pci enable' if we hit an error now we will want to do "goto err_pci_disable;"

dev_err(&pci->dev, "pci_enable_device failed\n");

@@ -43,7 +43,7 @@ static int snd_acp3x_probe(struct pci_dev *pci,

There is a bit where we request regions so now that's the most recent resource.

	     GFP_KERNEL);

if (!adata) { ret = -ENOMEM;

• goto release_regions;
  

• goto adata_free;
  

We failed to allocate "adata" so we have to release the most recent resource "goto err_release_regions;". "adata" is allocated with devm_kzalloc() so it gets freed automatically after probe(). If we free it ourselves with kfree() that will lead to a double free. So let's remember requet regions still as our most recent allocation.

}

/* check for msi interrupt support */ @@ -68,11 +68,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;
  

unmap_mmio is still the most recent so the labal was correct. The advantage of this style of error handling is that when we add new allocations, we don't have to change a lot of labels. adata->res uses devm_kzalloc() again so we keep unmap_mmio in our head as the most recent allocation.

}

adata->res[0].name = "acp3x_i2s_iomem";

@@ -80,28 +80,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;
  

• 	}
  

Loops are a bit complicated. What we do is if we allocate more than one thing inside the loop, then we free until the start of the most recent iteration: for (i = 0; i < end; i++) { foo->a = alloc(); if (!foo->a) goto unwind_loop; foo->b = alloc(); if (!foo->b) { free(foo->a); goto unwind_loop; } foo->c = alloc(); if (!foo->c) { free(foo->b); free(foo->a); goto unwind_loop; } }

But this loop only allocates one thing so it's fine. We can just goto unwind_loop;

}
break;

default: @@ -112,10 +136,22 @@ static int snd_acp3x_probe(struct pci_dev *pci, return 0;

Then at the end it looks like:

136 return 0; 137 138 unwind_loop: 139 if (val == I2S_MODE) { 140 while (--i >= 0) 141 platform_device_unregister(adata->pdev[i]); 142 } 143 unmap_mmio: 144 iounmap(adata->acp3x_base); 145 disable_msi: 146 pci_disable_msi(pci); 147 release_regions: 148 pci_release_regions(pci); 149 disable_pci: 150 pci_disable_device(pci); 151 152 return ret; 153 }

Note, that I have an if (val == I2S_MODE). It's not required but the unwind code should be a mirror image of the allocation code and it's better for future proofing.

Also if "i" is unsigned this error handling will break. We see that bug with unsigned loop iterators pretty frequently.

regards, dan carpenter