All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ata: sata_dwc_460ex: fix error path
@ 2010-09-12 18:57 ` Vasiliy Kulikov
  0 siblings, 0 replies; 10+ 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] 10+ messages in thread
* [PATCH] ata: sata_dwc_460ex: fix error path
@ 2010-09-12 18:57 ` Vasiliy Kulikov
  0 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] ata: sata_dwc_460ex: fix error path
  2010-09-12 18:57 ` Vasiliy Kulikov
@ 2011-02-16 18:10   ` Sergei Shtylyov
  -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] ata: sata_dwc_460ex: fix error path
@ 2011-02-16 18:10   ` Sergei Shtylyov
  0 siblings, 0 replies; 10+ 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] 10+ 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
  -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] ata: sata_dwc_460ex: fix error path
@ 2011-02-18 17:27     ` Vasiliy Kulikov
  0 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] ata: sata_dwc_460ex: fix error path
  2011-02-16 18:10   ` Sergei Shtylyov
@ 2011-02-21 15:31     ` Sergei Shtylyov
  -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] ata: sata_dwc_460ex: fix error path
@ 2011-02-21 15:31     ` Sergei Shtylyov
  0 siblings, 0 replies; 10+ 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] 10+ 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
  -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] ata: sata_dwc_460ex: fix error path
@ 2011-06-27  9:21       ` Sergei Shtylyov
  0 siblings, 0 replies; 10+ 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] 10+ messages in thread
end of thread, other threads:[~2011-06-27  9:21 UTC | newest]

Thread overview: 10+ 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
2010-09-12 18:57 ` Vasiliy Kulikov
2011-02-16 18:10 ` Sergei Shtylyov
2011-02-16 18:10   ` Sergei Shtylyov
2011-02-18 17:27   ` Vasiliy Kulikov
2011-02-18 17:27     ` Vasiliy Kulikov
2011-02-21 15:31   ` Sergei Shtylyov
2011-02-21 15:31     ` Sergei Shtylyov
2011-06-27  9:21     ` Sergei Shtylyov
2011-06-27  9:21       ` Sergei Shtylyov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.