| Message ID | 1573819823-23731-2-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/6] ASoC: amd:Create multiple I2S platform device endpoint | expand |
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
diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h index 4f2cadd..2f15fe1 100644 --- a/sound/soc/amd/raven/acp3x.h +++ b/sound/soc/amd/raven/acp3x.h @@ -7,10 +7,15 @@ #include "chip_offset_byte.h" +#define ACP3x_DEVS 3 #define ACP3x_PHY_BASE_ADDRESS 0x1240000 #define ACP3x_I2S_MODE 0 #define ACP3x_REG_START 0x1240000 #define ACP3x_REG_END 0x1250200 +#define ACP3x_I2STDM_REG_START 0x1242400 +#define ACP3x_I2STDM_REG_END 0x1242410 +#define ACP3x_BT_TDM_REG_START 0x1242800 +#define ACP3x_BT_TDM_REG_END 0x1242810 #define I2S_MODE 0x04 #define BT_TX_THRESHOLD 26 #define BT_RX_THRESHOLD 25 diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c index facec24..4c4601d 100644 --- a/sound/soc/amd/raven/pci-acp3x.c +++ b/sound/soc/amd/raven/pci-acp3x.c @@ -16,17 +16,17 @@ struct acp3x_dev_data { void __iomem *acp3x_base; bool acp3x_audio_mode; struct resource *res; - struct platform_device *pdev; + struct platform_device *pdev[ACP3x_DEVS]; }; 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; if (pci_enable_device(pci)) { dev_err(&pci->dev, "pci_enable_device failed\n"); @@ -43,7 +43,7 @@ static int snd_acp3x_probe(struct pci_dev *pci, GFP_KERNEL); if (!adata) { ret = -ENOMEM; - goto release_regions; + goto adata_free; } /* 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; } 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; + } } break; default: @@ -112,10 +136,22 @@ static int snd_acp3x_probe(struct pci_dev *pci, return 0; unmap_mmio: - pci_disable_msi(pci); + for (i = 0 ; i < ACP3x_DEVS ; i++) + platform_device_unregister(adata->pdev[i]); iounmap(adata->acp3x_base); + kfree(adata->res); + kfree(adata); + pci_disable_msi(pci); + pci_release_regions(pci); + pci_disable_device(pci); release_regions: + kfree(adata); + pci_disable_msi(pci); + pci_release_regions(pci); + pci_disable_device(pci); +adata_free: pci_release_regions(pci); + pci_disable_device(pci); disable_pci: pci_disable_device(pci); @@ -125,10 +161,13 @@ static int snd_acp3x_probe(struct pci_dev *pci, static void snd_acp3x_remove(struct pci_dev *pci) { struct acp3x_dev_data *adata = pci_get_drvdata(pci); + int i; - platform_device_unregister(adata->pdev); + if (adata->acp3x_audio_mode == ACP3x_I2S_MODE) { + for (i = 0 ; i < ACP3x_DEVS ; i++) + platform_device_unregister(adata->pdev[i]); + } iounmap(adata->acp3x_base); - pci_disable_msi(pci); pci_release_regions(pci); pci_disable_device(pci); @@ -151,6 +190,7 @@ static struct pci_driver acp3x_driver = { module_pci_driver(acp3x_driver); +MODULE_AUTHOR("Vishnuvardhanrao.Ravulapati@amd.com"); MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com"); MODULE_DESCRIPTION("AMD ACP3x PCI driver"); MODULE_LICENSE("GPL v2");
Creates Platform Device endpoints for multiple I2S instances: SP and BT endpoints device. Pass PCI resources like MMIO, irq to the platform devices. Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> --- sound/soc/amd/raven/acp3x.h | 5 +++ sound/soc/amd/raven/pci-acp3x.c | 96 +++++++++++++++++++++++++++++------------ 2 files changed, 73 insertions(+), 28 deletions(-)