* [PATCH] ata: sata_dwc_460ex: fix error path
@ 2010-09-12 18:57 Vasiliy Kulikov
2011-02-16 18:10 ` Sergei Shtylyov
0 siblings, 1 reply; 5+ messages in thread
From: Vasiliy Kulikov @ 2010-09-12 18:57 UTC (permalink / raw)
To: kernel-janitors
Cc: Jeff Garzik, Stephen Rothwell, Rupjyoti Sarmah, Prodyut Hazarika,
Grant Likely, linux-ide, linux-kernel
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(-)
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;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] ata: sata_dwc_460ex: fix error path
2010-09-12 18:57 [PATCH] ata: sata_dwc_460ex: fix error path Vasiliy Kulikov
@ 2011-02-16 18:10 ` Sergei Shtylyov
2011-02-18 17:27 ` Vasiliy Kulikov
2011-02-21 15:31 ` Sergei Shtylyov
0 siblings, 2 replies; 5+ messages in thread
From: Sergei Shtylyov @ 2011-02-16 18:10 UTC (permalink / raw)
To: Vasiliy Kulikov, Jeff Garzik
Cc: kernel-janitors, Stephen Rothwell, Rupjyoti Sarmah,
Prodyut Hazarika, Grant Likely, linux-ide, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ata: sata_dwc_460ex: fix error path
2011-02-16 18:10 ` Sergei Shtylyov
@ 2011-02-18 17:27 ` Vasiliy Kulikov
2011-02-21 15:31 ` Sergei Shtylyov
1 sibling, 0 replies; 5+ messages in thread
From: Vasiliy Kulikov @ 2011-02-18 17:27 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Jeff Garzik, kernel-janitors, Stephen Rothwell, Rupjyoti Sarmah,
Prodyut Hazarika, Grant Likely, linux-ide, linux-kernel
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,
--
Vasiliy
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ata: sata_dwc_460ex: fix error path
2011-02-16 18:10 ` Sergei Shtylyov
2011-02-18 17:27 ` Vasiliy Kulikov
@ 2011-02-21 15:31 ` Sergei Shtylyov
2011-06-27 9:21 ` Sergei Shtylyov
1 sibling, 1 reply; 5+ messages in thread
From: Sergei Shtylyov @ 2011-02-21 15:31 UTC (permalink / raw)
To: Vasiliy Kulikov, Jeff Garzik
Cc: kernel-janitors, Stephen Rothwell, Rupjyoti Sarmah,
Prodyut Hazarika, Grant Likely, linux-ide, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ata: sata_dwc_460ex: fix error path
2011-02-21 15:31 ` Sergei Shtylyov
@ 2011-06-27 9:21 ` Sergei Shtylyov
0 siblings, 0 replies; 5+ messages in thread
From: Sergei Shtylyov @ 2011-06-27 9:21 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: Jeff Garzik, kernel-janitors, Stephen Rothwell, Rupjyoti Sarmah,
Prodyut Hazarika, Grant Likely, linux-ide, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-06-27 9:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-12 18:57 [PATCH] ata: sata_dwc_460ex: fix error path Vasiliy Kulikov
2011-02-16 18:10 ` Sergei Shtylyov
2011-02-18 17:27 ` Vasiliy Kulikov
2011-02-21 15:31 ` Sergei Shtylyov
2011-06-27 9:21 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).