| Message ID | 1284317830-5022-1-git-send-email-segooon@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello. Vasiliy Kulikov wrote: > Free all allocated resources on error path. > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> > --- > I cannot compile this driver at all, so it is not tested. I can at least compile-test the patch... > drivers/ata/sata_dwc_460ex.c | 21 ++++++++++++++------- > 1 files changed, 14 insertions(+), 7 deletions(-) > diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c > index 6cf57c5..ad0535f 100644 > --- a/drivers/ata/sata_dwc_460ex.c > +++ b/drivers/ata/sata_dwc_460ex.c > @@ -727,11 +727,15 @@ static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems, > static void dma_dwc_exit(struct sata_dwc_device *hsdev) > { > dev_dbg(host_pvt.dwc_dev, "%s:\n", __func__); > - if (host_pvt.sata_dma_regs) > + if (host_pvt.sata_dma_regs) { > iounmap(host_pvt.sata_dma_regs); > + host_pvt.sata_dma_regs = NULL; > + } > > - if (hsdev->irq_dma) > + if (hsdev->irq_dma) { > free_irq(hsdev->irq_dma, hsdev); > + hsdev->irq_dma = 0; > + } > } Are those changes really necessary, i.e. is dma_dwc_exit() called more than once? > @@ -1606,7 +1610,7 @@ static int sata_dwc_probe(struct platform_device *ofdev, > if (hsdev == NULL) { > dev_err(&ofdev->dev, "kmalloc failed for hsdev\n"); > err = -ENOMEM; > - goto error_out; > + goto error; Why not just: return err; > } > memset(hsdev, 0, sizeof(*hsdev)); > > @@ -1616,7 +1620,7 @@ static int sata_dwc_probe(struct platform_device *ofdev, > dev_err(&ofdev->dev, "ioremap failed for SATA register" > " address\n"); > err = -ENODEV; > - goto error_out; > + goto error_kmalloc; > } > hsdev->reg_base = base; > dev_dbg(&ofdev->dev, "ioremap done for SATA register address\n"); > @@ -1629,7 +1633,7 @@ static int sata_dwc_probe(struct platform_device *ofdev, > if (!host) { > dev_err(&ofdev->dev, "ata_host_alloc_pinfo failed\n"); > err = -ENOMEM; > - goto error_out; > + goto error_iomap; > } > > host->private_data = hsdev; > @@ -1697,8 +1701,11 @@ error_out: > /* Free SATA DMA resources */ > dma_dwc_exit(hsdev); > > - if (base) > - iounmap(base); > +error_iomap: > + iounmap(base); > +error_kmalloc: > + kfree(hsdev); > +error: > return err; > } Jeff, what's become with this patch? Will you queue it? And will you queue my 4 fixes (and 1 cleanup)? WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Wed, Feb 16, 2011 at 21:10 +0300, Sergei Shtylyov wrote: > >@@ -727,11 +727,15 @@ static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems, > > static void dma_dwc_exit(struct sata_dwc_device *hsdev) > > { > > dev_dbg(host_pvt.dwc_dev, "%s:\n", __func__); > >- if (host_pvt.sata_dma_regs) > >+ if (host_pvt.sata_dma_regs) { > > iounmap(host_pvt.sata_dma_regs); > >+ host_pvt.sata_dma_regs = NULL; > >+ } > >- if (hsdev->irq_dma) > >+ if (hsdev->irq_dma) { > > free_irq(hsdev->irq_dma, hsdev); > >+ hsdev->irq_dma = 0; > >+ } > > } > > Are those changes really necessary, i.e. is dma_dwc_exit() called > more than once? Because this function may be called once again before host_pvt.sata_dma_regs gains new value in sata_dwc_probe(). This would lead to double iounmap() and double free_irq(). > >@@ -1606,7 +1610,7 @@ static int sata_dwc_probe(struct platform_device *ofdev, > > if (hsdev == NULL) { > > dev_err(&ofdev->dev, "kmalloc failed for hsdev\n"); > > err = -ENOMEM; > >- goto error_out; > >+ goto error; > > Why not just: > > return err; If some resource allocation is added before this line in future then "return err" should be changed. With goto the single cleanup is just added to the end of the function. Thanks,
Hello. I wrote: >> Free all allocated resources on error path. >> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> >> --- >> I cannot compile this driver at all, so it is not tested. > I can at least compile-test the patch... No new errors/warnings are added by the patch. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 21-02-2011 18:31, Sergei Shtylyov wrote: >>> Free all allocated resources on error path. >>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> >>> --- >>> I cannot compile this driver at all, so it is not tested. >> I can at least compile-test the patch... > No new errors/warnings are added by the patch. Vasiliy, could you repost this patch? I'm not sure why Jeff hasn't taken it, maybe he will now... WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c index 6cf57c5..ad0535f 100644 --- a/drivers/ata/sata_dwc_460ex.c +++ b/drivers/ata/sata_dwc_460ex.c @@ -727,11 +727,15 @@ static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems, static void dma_dwc_exit(struct sata_dwc_device *hsdev) { dev_dbg(host_pvt.dwc_dev, "%s:\n", __func__); - if (host_pvt.sata_dma_regs) + if (host_pvt.sata_dma_regs) { iounmap(host_pvt.sata_dma_regs); + host_pvt.sata_dma_regs = NULL; + } - if (hsdev->irq_dma) + if (hsdev->irq_dma) { free_irq(hsdev->irq_dma, hsdev); + hsdev->irq_dma = 0; + } } /* @@ -1606,7 +1610,7 @@ static int sata_dwc_probe(struct platform_device *ofdev, if (hsdev == NULL) { dev_err(&ofdev->dev, "kmalloc failed for hsdev\n"); err = -ENOMEM; - goto error_out; + goto error; } memset(hsdev, 0, sizeof(*hsdev)); @@ -1616,7 +1620,7 @@ static int sata_dwc_probe(struct platform_device *ofdev, dev_err(&ofdev->dev, "ioremap failed for SATA register" " address\n"); err = -ENODEV; - goto error_out; + goto error_kmalloc; } hsdev->reg_base = base; dev_dbg(&ofdev->dev, "ioremap done for SATA register address\n"); @@ -1629,7 +1633,7 @@ static int sata_dwc_probe(struct platform_device *ofdev, if (!host) { dev_err(&ofdev->dev, "ata_host_alloc_pinfo failed\n"); err = -ENOMEM; - goto error_out; + goto error_iomap; } host->private_data = hsdev; @@ -1697,8 +1701,11 @@ error_out: /* Free SATA DMA resources */ dma_dwc_exit(hsdev); - if (base) - iounmap(base); +error_iomap: + iounmap(base); +error_kmalloc: + kfree(hsdev); +error: return err; }
Free all allocated resources on error path. Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> --- I cannot compile this driver at all, so it is not tested. drivers/ata/sata_dwc_460ex.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)