Patchwork ata: sata_dwc_460ex: fix error path

login
register
mail settings
Submitter Kulikov Vasiliy
Date Sept. 12, 2010, 6:57 p.m.
Message ID <1284317830-5022-1-git-send-email-segooon@gmail.com>
Download mbox | patch
Permalink /patch/64560/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Kulikov Vasiliy - Sept. 12, 2010, 6:57 p.m.
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(-)
Sergei Shtylyov - Feb. 16, 2011, 6:10 p.m.
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
Kulikov Vasiliy - Feb. 18, 2011, 5:27 p.m.
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,
Sergei Shtylyov - Feb. 21, 2011, 3:31 p.m.
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
Sergei Shtylyov - June 27, 2011, 9:21 a.m.
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

Patch

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;
 }