All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-07-31 10:09 ` Julia Lawall
  0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 10:09 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: kernel-janitors, linux-iio, linux-kernel

From: Julia Lawall <Julia.Lawall@lip6.fr>

The various devm_ functions allocate memory that is released when a driver
detaches.  This patch uses these functions for data that is allocated in
the probe function of a platform device and is only freed in the remove
function.

The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved closer
to the new call to devm_request_and_ioremap where its result is first
used.  devm_request_and_ioremap takes case of the NULL test on the result
of platform_get_resource(pdev, IORESOURCE_MEM, 0), so that is dropped.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not compiled.

 drivers/iio/adc/at91_adc.c |   56 +++++++++++----------------------------------
 1 file changed, 14 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index f61780a..b0277bf 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "No resource defined\n");
-		ret = -ENXIO;
-		goto error_ret;
-	}
-
 	platform_set_drvdata(pdev, idev);
 
 	idev->dev.parent = &pdev->dev;
@@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}
 
-	if (!request_mem_region(res->start, resource_size(res),
-				"AT91 adc registers")) {
-		dev_err(&pdev->dev, "Resources are unavailable.\n");
-		ret = -EBUSY;
-		goto error_free_device;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-	st->reg_base = ioremap(res->start, resource_size(res));
+	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
 	if (!st->reg_base) {
 		dev_err(&pdev->dev, "Failed to map registers.\n");
 		ret = -ENOMEM;
-		goto error_release_mem;
+		goto error_free_device;
 	}
 
 	/*
@@ -585,27 +573,27 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	 */
 	at91_adc_writel(st, AT91_ADC_CR, AT91_ADC_SWRST);
 	at91_adc_writel(st, AT91_ADC_IDR, 0xFFFFFFFF);
-	ret = request_irq(st->irq,
-			  at91_adc_eoc_trigger,
-			  0,
-			  pdev->dev.driver->name,
-			  idev);
+	ret = devm_request_irq(&pdev->dev, st->irq,
+			       at91_adc_eoc_trigger,
+			       0,
+			       pdev->dev.driver->name,
+			       idev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
-		goto error_unmap_reg;
+		goto error_free_device;
 	}
 
-	st->clk = clk_get(&pdev->dev, "adc_clk");
+	st->clk = devm_clk_get(&pdev->dev, "adc_clk");
 	if (IS_ERR(st->clk)) {
 		dev_err(&pdev->dev, "Failed to get the clock.\n");
 		ret = PTR_ERR(st->clk);
-		goto error_free_irq;
+		goto error_free_device;
 	}
 
 	ret = clk_prepare(st->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the clock.\n");
-		goto error_free_clk;
+		goto error_free_device;
 	}
 
 	ret = clk_enable(st->clk);
@@ -614,7 +602,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_unprepare_clk;
 	}
 
-	st->adc_clk = clk_get(&pdev->dev, "adc_op_clk");
+	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
 	if (IS_ERR(st->adc_clk)) {
 		dev_err(&pdev->dev, "Failed to get the ADC clock.\n");
 		ret = PTR_ERR(st->clk);
@@ -624,7 +612,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	ret = clk_prepare(st->adc_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the ADC clock.\n");
-		goto error_free_adc_clk;
+		goto error_disable_clk;
 	}
 
 	ret = clk_enable(st->adc_clk);
@@ -697,20 +685,10 @@ error_disable_adc_clk:
 	clk_disable(st->adc_clk);
 error_unprepare_adc_clk:
 	clk_unprepare(st->adc_clk);
-error_free_adc_clk:
-	clk_put(st->adc_clk);
 error_disable_clk:
 	clk_disable(st->clk);
 error_unprepare_clk:
 	clk_unprepare(st->clk);
-error_free_clk:
-	clk_put(st->clk);
-error_free_irq:
-	free_irq(st->irq, idev);
-error_unmap_reg:
-	iounmap(st->reg_base);
-error_release_mem:
-	release_mem_region(res->start, resource_size(res));
 error_free_device:
 	iio_device_free(idev);
 error_ret:
@@ -720,20 +698,14 @@ error_ret:
 static int __devexit at91_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *idev = platform_get_drvdata(pdev);
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct at91_adc_state *st = iio_priv(idev);
 
 	iio_device_unregister(idev);
 	at91_adc_trigger_remove(idev);
 	at91_adc_buffer_remove(idev);
 	clk_disable_unprepare(st->adc_clk);
-	clk_put(st->adc_clk);
 	clk_disable(st->clk);
 	clk_unprepare(st->clk);
-	clk_put(st->clk);
-	free_irq(st->irq, idev);
-	iounmap(st->reg_base);
-	release_mem_region(res->start, resource_size(res));
 	iio_device_free(idev);
 
 	return 0;


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-07-31 10:09 ` Julia Lawall
  0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 10:09 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: kernel-janitors, linux-iio, linux-kernel

From: Julia Lawall <Julia.Lawall@lip6.fr>

The various devm_ functions allocate memory that is released when a driver
detaches.  This patch uses these functions for data that is allocated in
the probe function of a platform device and is only freed in the remove
function.

The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved closer
to the new call to devm_request_and_ioremap where its result is first
used.  devm_request_and_ioremap takes case of the NULL test on the result
of platform_get_resource(pdev, IORESOURCE_MEM, 0), so that is dropped.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
Not compiled.

 drivers/iio/adc/at91_adc.c |   56 +++++++++++----------------------------------
 1 file changed, 14 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index f61780a..b0277bf 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "No resource defined\n");
-		ret = -ENXIO;
-		goto error_ret;
-	}
-
 	platform_set_drvdata(pdev, idev);
 
 	idev->dev.parent = &pdev->dev;
@@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}
 
-	if (!request_mem_region(res->start, resource_size(res),
-				"AT91 adc registers")) {
-		dev_err(&pdev->dev, "Resources are unavailable.\n");
-		ret = -EBUSY;
-		goto error_free_device;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-	st->reg_base = ioremap(res->start, resource_size(res));
+	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
 	if (!st->reg_base) {
 		dev_err(&pdev->dev, "Failed to map registers.\n");
 		ret = -ENOMEM;
-		goto error_release_mem;
+		goto error_free_device;
 	}
 
 	/*
@@ -585,27 +573,27 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	 */
 	at91_adc_writel(st, AT91_ADC_CR, AT91_ADC_SWRST);
 	at91_adc_writel(st, AT91_ADC_IDR, 0xFFFFFFFF);
-	ret = request_irq(st->irq,
-			  at91_adc_eoc_trigger,
-			  0,
-			  pdev->dev.driver->name,
-			  idev);
+	ret = devm_request_irq(&pdev->dev, st->irq,
+			       at91_adc_eoc_trigger,
+			       0,
+			       pdev->dev.driver->name,
+			       idev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
-		goto error_unmap_reg;
+		goto error_free_device;
 	}
 
-	st->clk = clk_get(&pdev->dev, "adc_clk");
+	st->clk = devm_clk_get(&pdev->dev, "adc_clk");
 	if (IS_ERR(st->clk)) {
 		dev_err(&pdev->dev, "Failed to get the clock.\n");
 		ret = PTR_ERR(st->clk);
-		goto error_free_irq;
+		goto error_free_device;
 	}
 
 	ret = clk_prepare(st->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the clock.\n");
-		goto error_free_clk;
+		goto error_free_device;
 	}
 
 	ret = clk_enable(st->clk);
@@ -614,7 +602,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_unprepare_clk;
 	}
 
-	st->adc_clk = clk_get(&pdev->dev, "adc_op_clk");
+	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
 	if (IS_ERR(st->adc_clk)) {
 		dev_err(&pdev->dev, "Failed to get the ADC clock.\n");
 		ret = PTR_ERR(st->clk);
@@ -624,7 +612,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	ret = clk_prepare(st->adc_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the ADC clock.\n");
-		goto error_free_adc_clk;
+		goto error_disable_clk;
 	}
 
 	ret = clk_enable(st->adc_clk);
@@ -697,20 +685,10 @@ error_disable_adc_clk:
 	clk_disable(st->adc_clk);
 error_unprepare_adc_clk:
 	clk_unprepare(st->adc_clk);
-error_free_adc_clk:
-	clk_put(st->adc_clk);
 error_disable_clk:
 	clk_disable(st->clk);
 error_unprepare_clk:
 	clk_unprepare(st->clk);
-error_free_clk:
-	clk_put(st->clk);
-error_free_irq:
-	free_irq(st->irq, idev);
-error_unmap_reg:
-	iounmap(st->reg_base);
-error_release_mem:
-	release_mem_region(res->start, resource_size(res));
 error_free_device:
 	iio_device_free(idev);
 error_ret:
@@ -720,20 +698,14 @@ error_ret:
 static int __devexit at91_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *idev = platform_get_drvdata(pdev);
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct at91_adc_state *st = iio_priv(idev);
 
 	iio_device_unregister(idev);
 	at91_adc_trigger_remove(idev);
 	at91_adc_buffer_remove(idev);
 	clk_disable_unprepare(st->adc_clk);
-	clk_put(st->adc_clk);
 	clk_disable(st->clk);
 	clk_unprepare(st->clk);
-	clk_put(st->clk);
-	free_irq(st->irq, idev);
-	iounmap(st->reg_base);
-	release_mem_region(res->start, resource_size(res));
 	iio_device_free(idev);
 
 	return 0;


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-07-31 10:09 ` Julia Lawall
@ 2012-07-31 12:36   ` Lars-Peter Clausen
  -1 siblings, 0 replies; 20+ messages in thread
From: Lars-Peter Clausen @ 2012-07-31 12:36 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

Hi,

On 07/31/2012 12:09 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> @@ -720,20 +698,14 @@ error_ret:
>  static int __devexit at91_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *idev = platform_get_drvdata(pdev);
> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	struct at91_adc_state *st = iio_priv(idev);
>  
>  	iio_device_unregister(idev);
> [...]
> -	free_irq(st->irq, idev);
> [...]
>  	iio_device_free(idev);

I think we have to be careful here. The interrupted is now freed after the
device has been freed, which means that it could trigger after the device
has been freed. And since we use the device in the interrupt handler we'll
get a use after free.

- Lars

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-07-31 12:36   ` Lars-Peter Clausen
  0 siblings, 0 replies; 20+ messages in thread
From: Lars-Peter Clausen @ 2012-07-31 12:36 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

Hi,

On 07/31/2012 12:09 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> @@ -720,20 +698,14 @@ error_ret:
>  static int __devexit at91_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *idev = platform_get_drvdata(pdev);
> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	struct at91_adc_state *st = iio_priv(idev);
>  
>  	iio_device_unregister(idev);
> [...]
> -	free_irq(st->irq, idev);
> [...]
>  	iio_device_free(idev);

I think we have to be careful here. The interrupted is now freed after the
device has been freed, which means that it could trigger after the device
has been freed. And since we use the device in the interrupt handler we'll
get a use after free.

- Lars

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-07-31 12:36   ` Lars-Peter Clausen
@ 2012-07-31 12:41     ` Julia Lawall
  -1 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 12:41 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

On Tue, 31 Jul 2012, Lars-Peter Clausen wrote:

> Hi,
>
> On 07/31/2012 12:09 PM, Julia Lawall wrote:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > @@ -720,20 +698,14 @@ error_ret:
> >  static int __devexit at91_adc_remove(struct platform_device *pdev)
> >  {
> >  	struct iio_dev *idev = platform_get_drvdata(pdev);
> > -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	struct at91_adc_state *st = iio_priv(idev);
> >
> >  	iio_device_unregister(idev);
> > [...]
> > -	free_irq(st->irq, idev);
> > [...]
> >  	iio_device_free(idev);
>
> I think we have to be careful here. The interrupted is now freed after the
> device has been freed, which means that it could trigger after the device
> has been freed. And since we use the device in the interrupt handler we'll
> get a use after free.

OK, thanks for the feedback.  I'll try again, and un-devm_ this function.

julia

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-07-31 12:41     ` Julia Lawall
  0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 12:41 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

On Tue, 31 Jul 2012, Lars-Peter Clausen wrote:

> Hi,
>
> On 07/31/2012 12:09 PM, Julia Lawall wrote:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > @@ -720,20 +698,14 @@ error_ret:
> >  static int __devexit at91_adc_remove(struct platform_device *pdev)
> >  {
> >  	struct iio_dev *idev = platform_get_drvdata(pdev);
> > -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	struct at91_adc_state *st = iio_priv(idev);
> >
> >  	iio_device_unregister(idev);
> > [...]
> > -	free_irq(st->irq, idev);
> > [...]
> >  	iio_device_free(idev);
>
> I think we have to be careful here. The interrupted is now freed after the
> device has been freed, which means that it could trigger after the device
> has been freed. And since we use the device in the interrupt handler we'll
> get a use after free.

OK, thanks for the feedback.  I'll try again, and un-devm_ this function.

julia

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-07-31 12:36   ` Lars-Peter Clausen
@ 2012-07-31 13:09     ` Julia Lawall
  -1 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 13:09 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

From: Julia Lawall <Julia.Lawall@lip6.fr>

The various devm_ functions allocate memory that is released when a driver
detaches.  This patch uses these functions for data that is allocated in
the probe function of a platform device and is only freed in the remove
function.

The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved coser
to the call to devm_request_and_ioremap, which is th first use of the
result of platform_get_resource.

This does not use devm_request_irq to ensure that free_irq is executed
before its idev argument is freed.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/iio/adc/at91_adc.c |   41 ++++++++---------------------------------
 1 file changed, 8 insertions(+), 33 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index f61780a..3506e3d 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}

-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "No resource defined\n");
-		ret = -ENXIO;
-		goto error_ret;
-	}
-
 	platform_set_drvdata(pdev, idev);

 	idev->dev.parent = &pdev->dev;
@@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}

-	if (!request_mem_region(res->start, resource_size(res),
-				"AT91 adc registers")) {
-		dev_err(&pdev->dev, "Resources are unavailable.\n");
-		ret = -EBUSY;
-		goto error_free_device;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

-	st->reg_base = ioremap(res->start, resource_size(res));
+	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
 	if (!st->reg_base) {
 		dev_err(&pdev->dev, "Failed to map registers.\n");
 		ret = -ENOMEM;
-		goto error_release_mem;
+		goto error_free_device;
 	}

 	/*
@@ -592,10 +580,10 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 			  idev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
-		goto error_unmap_reg;
+		goto error_free_device;
 	}

-	st->clk = clk_get(&pdev->dev, "adc_clk");
+	st->clk = devm_clk_get(&pdev->dev, "adc_clk");
 	if (IS_ERR(st->clk)) {
 		dev_err(&pdev->dev, "Failed to get the clock.\n");
 		ret = PTR_ERR(st->clk);
@@ -605,7 +593,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	ret = clk_prepare(st->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the clock.\n");
-		goto error_free_clk;
+		goto error_free_irq;
 	}

 	ret = clk_enable(st->clk);
@@ -614,7 +602,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_unprepare_clk;
 	}

-	st->adc_clk = clk_get(&pdev->dev, "adc_op_clk");
+	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
 	if (IS_ERR(st->adc_clk)) {
 		dev_err(&pdev->dev, "Failed to get the ADC clock.\n");
 		ret = PTR_ERR(st->clk);
@@ -624,7 +612,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	ret = clk_prepare(st->adc_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the ADC clock.\n");
-		goto error_free_adc_clk;
+		goto error_disable_clk;
 	}

 	ret = clk_enable(st->adc_clk);
@@ -697,20 +685,12 @@ error_disable_adc_clk:
 	clk_disable(st->adc_clk);
 error_unprepare_adc_clk:
 	clk_unprepare(st->adc_clk);
-error_free_adc_clk:
-	clk_put(st->adc_clk);
 error_disable_clk:
 	clk_disable(st->clk);
 error_unprepare_clk:
 	clk_unprepare(st->clk);
-error_free_clk:
-	clk_put(st->clk);
 error_free_irq:
 	free_irq(st->irq, idev);
-error_unmap_reg:
-	iounmap(st->reg_base);
-error_release_mem:
-	release_mem_region(res->start, resource_size(res));
 error_free_device:
 	iio_device_free(idev);
 error_ret:
@@ -720,20 +700,15 @@ error_ret:
 static int __devexit at91_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *idev = platform_get_drvdata(pdev);
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct at91_adc_state *st = iio_priv(idev);

 	iio_device_unregister(idev);
 	at91_adc_trigger_remove(idev);
 	at91_adc_buffer_remove(idev);
 	clk_disable_unprepare(st->adc_clk);
-	clk_put(st->adc_clk);
 	clk_disable(st->clk);
 	clk_unprepare(st->clk);
-	clk_put(st->clk);
 	free_irq(st->irq, idev);
-	iounmap(st->reg_base);
-	release_mem_region(res->start, resource_size(res));
 	iio_device_free(idev);

 	return 0;


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-07-31 13:09     ` Julia Lawall
  0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 13:09 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

From: Julia Lawall <Julia.Lawall@lip6.fr>

The various devm_ functions allocate memory that is released when a driver
detaches.  This patch uses these functions for data that is allocated in
the probe function of a platform device and is only freed in the remove
function.

The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved coser
to the call to devm_request_and_ioremap, which is th first use of the
result of platform_get_resource.

This does not use devm_request_irq to ensure that free_irq is executed
before its idev argument is freed.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/iio/adc/at91_adc.c |   41 ++++++++---------------------------------
 1 file changed, 8 insertions(+), 33 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index f61780a..3506e3d 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}

-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(&pdev->dev, "No resource defined\n");
-		ret = -ENXIO;
-		goto error_ret;
-	}
-
 	platform_set_drvdata(pdev, idev);

 	idev->dev.parent = &pdev->dev;
@@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_device;
 	}

-	if (!request_mem_region(res->start, resource_size(res),
-				"AT91 adc registers")) {
-		dev_err(&pdev->dev, "Resources are unavailable.\n");
-		ret = -EBUSY;
-		goto error_free_device;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

-	st->reg_base = ioremap(res->start, resource_size(res));
+	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
 	if (!st->reg_base) {
 		dev_err(&pdev->dev, "Failed to map registers.\n");
 		ret = -ENOMEM;
-		goto error_release_mem;
+		goto error_free_device;
 	}

 	/*
@@ -592,10 +580,10 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 			  idev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
-		goto error_unmap_reg;
+		goto error_free_device;
 	}

-	st->clk = clk_get(&pdev->dev, "adc_clk");
+	st->clk = devm_clk_get(&pdev->dev, "adc_clk");
 	if (IS_ERR(st->clk)) {
 		dev_err(&pdev->dev, "Failed to get the clock.\n");
 		ret = PTR_ERR(st->clk);
@@ -605,7 +593,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	ret = clk_prepare(st->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the clock.\n");
-		goto error_free_clk;
+		goto error_free_irq;
 	}

 	ret = clk_enable(st->clk);
@@ -614,7 +602,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_unprepare_clk;
 	}

-	st->adc_clk = clk_get(&pdev->dev, "adc_op_clk");
+	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
 	if (IS_ERR(st->adc_clk)) {
 		dev_err(&pdev->dev, "Failed to get the ADC clock.\n");
 		ret = PTR_ERR(st->clk);
@@ -624,7 +612,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	ret = clk_prepare(st->adc_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not prepare the ADC clock.\n");
-		goto error_free_adc_clk;
+		goto error_disable_clk;
 	}

 	ret = clk_enable(st->adc_clk);
@@ -697,20 +685,12 @@ error_disable_adc_clk:
 	clk_disable(st->adc_clk);
 error_unprepare_adc_clk:
 	clk_unprepare(st->adc_clk);
-error_free_adc_clk:
-	clk_put(st->adc_clk);
 error_disable_clk:
 	clk_disable(st->clk);
 error_unprepare_clk:
 	clk_unprepare(st->clk);
-error_free_clk:
-	clk_put(st->clk);
 error_free_irq:
 	free_irq(st->irq, idev);
-error_unmap_reg:
-	iounmap(st->reg_base);
-error_release_mem:
-	release_mem_region(res->start, resource_size(res));
 error_free_device:
 	iio_device_free(idev);
 error_ret:
@@ -720,20 +700,15 @@ error_ret:
 static int __devexit at91_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *idev = platform_get_drvdata(pdev);
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct at91_adc_state *st = iio_priv(idev);

 	iio_device_unregister(idev);
 	at91_adc_trigger_remove(idev);
 	at91_adc_buffer_remove(idev);
 	clk_disable_unprepare(st->adc_clk);
-	clk_put(st->adc_clk);
 	clk_disable(st->clk);
 	clk_unprepare(st->clk);
-	clk_put(st->clk);
 	free_irq(st->irq, idev);
-	iounmap(st->reg_base);
-	release_mem_region(res->start, resource_size(res));
 	iio_device_free(idev);

 	return 0;


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-07-31 12:36   ` Lars-Peter Clausen
@ 2012-07-31 13:54     ` Julia Lawall
  -1 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 13:54 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio,
	linux-kernel, rob.herring



On Tue, 31 Jul 2012, Lars-Peter Clausen wrote:

> Hi,
>
> On 07/31/2012 12:09 PM, Julia Lawall wrote:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > @@ -720,20 +698,14 @@ error_ret:
> >  static int __devexit at91_adc_remove(struct platform_device *pdev)
> >  {
> >  	struct iio_dev *idev = platform_get_drvdata(pdev);
> > -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	struct at91_adc_state *st = iio_priv(idev);
> >
> >  	iio_device_unregister(idev);
> > [...]
> > -	free_irq(st->irq, idev);
> > [...]
> >  	iio_device_free(idev);
>
> I think we have to be careful here. The interrupted is now freed after the
> device has been freed, which means that it could trigger after the device
> has been freed. And since we use the device in the interrupt handler we'll
> get a use after free.

Perhaps the same would be true in the following code, from the file
drivers/edac/highbank_l2_edac.c:

        res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
                               highbank_l2_err_handler,
                               0, dev_name(&pdev->dev), dci);
        if (res < 0)
                goto err;

        dci->mod_name = dev_name(&pdev->dev);
        dci->dev_name = dev_name(&pdev->dev);

        if (edac_device_add_device(dci))
                goto err;

        devres_close_group(&pdev->dev, NULL);
        return 0;
err:
    	devres_release_group(&pdev->dev, NULL);
        edac_device_free_ctl_info(dci);

Is devm_request_irq perhaps not a very good idea?

julia

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-07-31 13:54     ` Julia Lawall
  0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-07-31 13:54 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio,
	linux-kernel, rob.herring



On Tue, 31 Jul 2012, Lars-Peter Clausen wrote:

> Hi,
>
> On 07/31/2012 12:09 PM, Julia Lawall wrote:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > @@ -720,20 +698,14 @@ error_ret:
> >  static int __devexit at91_adc_remove(struct platform_device *pdev)
> >  {
> >  	struct iio_dev *idev = platform_get_drvdata(pdev);
> > -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	struct at91_adc_state *st = iio_priv(idev);
> >
> >  	iio_device_unregister(idev);
> > [...]
> > -	free_irq(st->irq, idev);
> > [...]
> >  	iio_device_free(idev);
>
> I think we have to be careful here. The interrupted is now freed after the
> device has been freed, which means that it could trigger after the device
> has been freed. And since we use the device in the interrupt handler we'll
> get a use after free.

Perhaps the same would be true in the following code, from the file
drivers/edac/highbank_l2_edac.c:

        res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
                               highbank_l2_err_handler,
                               0, dev_name(&pdev->dev), dci);
        if (res < 0)
                goto err;

        dci->mod_name = dev_name(&pdev->dev);
        dci->dev_name = dev_name(&pdev->dev);

        if (edac_device_add_device(dci))
                goto err;

        devres_close_group(&pdev->dev, NULL);
        return 0;
err:
    	devres_release_group(&pdev->dev, NULL);
        edac_device_free_ctl_info(dci);

Is devm_request_irq perhaps not a very good idea?

julia

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-07-31 13:54     ` Julia Lawall
@ 2012-07-31 14:23       ` Lars-Peter Clausen
  -1 siblings, 0 replies; 20+ messages in thread
From: Lars-Peter Clausen @ 2012-07-31 14:23 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel, rob.herring

On 07/31/2012 03:54 PM, Julia Lawall wrote:
> 
> 
> On Tue, 31 Jul 2012, Lars-Peter Clausen wrote:
> 
>> Hi,
>>
>> On 07/31/2012 12:09 PM, Julia Lawall wrote:
>>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>> @@ -720,20 +698,14 @@ error_ret:
>>>  static int __devexit at91_adc_remove(struct platform_device *pdev)
>>>  {
>>>  	struct iio_dev *idev = platform_get_drvdata(pdev);
>>> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>  	struct at91_adc_state *st = iio_priv(idev);
>>>
>>>  	iio_device_unregister(idev);
>>> [...]
>>> -	free_irq(st->irq, idev);
>>> [...]
>>>  	iio_device_free(idev);
>>
>> I think we have to be careful here. The interrupted is now freed after the
>> device has been freed, which means that it could trigger after the device
>> has been freed. And since we use the device in the interrupt handler we'll
>> get a use after free.
> 
> Perhaps the same would be true in the following code, from the file
> drivers/edac/highbank_l2_edac.c:
> 
>         res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
>                                highbank_l2_err_handler,
>                                0, dev_name(&pdev->dev), dci);
>         if (res < 0)
>                 goto err;
> 
>         dci->mod_name = dev_name(&pdev->dev);
>         dci->dev_name = dev_name(&pdev->dev);
> 
>         if (edac_device_add_device(dci))
>                 goto err;
> 
>         devres_close_group(&pdev->dev, NULL);
>         return 0;
> err:
>     	devres_release_group(&pdev->dev, NULL);
>         edac_device_free_ctl_info(dci);

Yes looks like this has the same issue.

> 
> Is devm_request_irq perhaps not a very good idea?
> 

devm_request_irq has to be used carefully. It is ok to use it if the objects
which are accessed in the interrupt handler are also devres managed. devres
will free objects in the reverse order of which they are allocated.

E.g. if you do

obj = dev_kzalloc(...);
...
devm_request_irq(..., obj);

it is save to use, because 'obj' will be freed after the IRQ has been freed.

- Lars

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-07-31 14:23       ` Lars-Peter Clausen
  0 siblings, 0 replies; 20+ messages in thread
From: Lars-Peter Clausen @ 2012-07-31 14:23 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel, rob.herring

On 07/31/2012 03:54 PM, Julia Lawall wrote:
> 
> 
> On Tue, 31 Jul 2012, Lars-Peter Clausen wrote:
> 
>> Hi,
>>
>> On 07/31/2012 12:09 PM, Julia Lawall wrote:
>>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>> @@ -720,20 +698,14 @@ error_ret:
>>>  static int __devexit at91_adc_remove(struct platform_device *pdev)
>>>  {
>>>  	struct iio_dev *idev = platform_get_drvdata(pdev);
>>> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>  	struct at91_adc_state *st = iio_priv(idev);
>>>
>>>  	iio_device_unregister(idev);
>>> [...]
>>> -	free_irq(st->irq, idev);
>>> [...]
>>>  	iio_device_free(idev);
>>
>> I think we have to be careful here. The interrupted is now freed after the
>> device has been freed, which means that it could trigger after the device
>> has been freed. And since we use the device in the interrupt handler we'll
>> get a use after free.
> 
> Perhaps the same would be true in the following code, from the file
> drivers/edac/highbank_l2_edac.c:
> 
>         res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
>                                highbank_l2_err_handler,
>                                0, dev_name(&pdev->dev), dci);
>         if (res < 0)
>                 goto err;
> 
>         dci->mod_name = dev_name(&pdev->dev);
>         dci->dev_name = dev_name(&pdev->dev);
> 
>         if (edac_device_add_device(dci))
>                 goto err;
> 
>         devres_close_group(&pdev->dev, NULL);
>         return 0;
> err:
>     	devres_release_group(&pdev->dev, NULL);
>         edac_device_free_ctl_info(dci);

Yes looks like this has the same issue.

> 
> Is devm_request_irq perhaps not a very good idea?
> 

devm_request_irq has to be used carefully. It is ok to use it if the objects
which are accessed in the interrupt handler are also devres managed. devres
will free objects in the reverse order of which they are allocated.

E.g. if you do

obj = dev_kzalloc(...);
...
devm_request_irq(..., obj);

it is save to use, because 'obj' will be freed after the IRQ has been freed.

- Lars

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-07-31 13:09     ` Julia Lawall
@ 2012-08-14 20:32       ` Jonathan Cameron
  -1 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2012-08-14 20:32 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Lars-Peter Clausen, Jonathan Cameron, kernel-janitors, linux-iio,
	linux-kernel

Lars-Peter,

Are you happy with this updated version?  Can't immediately find any response
from you to it.

Jonathan
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> The various devm_ functions allocate memory that is released when a driver
> detaches.  This patch uses these functions for data that is allocated in
> the probe function of a platform device and is only freed in the remove
> function.
> 
> The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved coser
> to the call to devm_request_and_ioremap, which is th first use of the
> result of platform_get_resource.
> 
> This does not use devm_request_irq to ensure that free_irq is executed
> before its idev argument is freed.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/iio/adc/at91_adc.c |   41 ++++++++---------------------------------
>  1 file changed, 8 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index f61780a..3506e3d 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  		goto error_free_device;
>  	}
> 
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "No resource defined\n");
> -		ret = -ENXIO;
> -		goto error_ret;
> -	}
> -
>  	platform_set_drvdata(pdev, idev);
> 
>  	idev->dev.parent = &pdev->dev;
> @@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  		goto error_free_device;
>  	}
> 
> -	if (!request_mem_region(res->start, resource_size(res),
> -				"AT91 adc registers")) {
> -		dev_err(&pdev->dev, "Resources are unavailable.\n");
> -		ret = -EBUSY;
> -		goto error_free_device;
> -	}
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 
> -	st->reg_base = ioremap(res->start, resource_size(res));
> +	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>  	if (!st->reg_base) {
>  		dev_err(&pdev->dev, "Failed to map registers.\n");
>  		ret = -ENOMEM;
> -		goto error_release_mem;
> +		goto error_free_device;
>  	}
> 
>  	/*
> @@ -592,10 +580,10 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  			  idev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
> -		goto error_unmap_reg;
> +		goto error_free_device;
>  	}
> 
> -	st->clk = clk_get(&pdev->dev, "adc_clk");
> +	st->clk = devm_clk_get(&pdev->dev, "adc_clk");
>  	if (IS_ERR(st->clk)) {
>  		dev_err(&pdev->dev, "Failed to get the clock.\n");
>  		ret = PTR_ERR(st->clk);
> @@ -605,7 +593,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	ret = clk_prepare(st->clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Could not prepare the clock.\n");
> -		goto error_free_clk;
> +		goto error_free_irq;
>  	}
> 
>  	ret = clk_enable(st->clk);
> @@ -614,7 +602,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  		goto error_unprepare_clk;
>  	}
> 
> -	st->adc_clk = clk_get(&pdev->dev, "adc_op_clk");
> +	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
>  	if (IS_ERR(st->adc_clk)) {
>  		dev_err(&pdev->dev, "Failed to get the ADC clock.\n");
>  		ret = PTR_ERR(st->clk);
> @@ -624,7 +612,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	ret = clk_prepare(st->adc_clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Could not prepare the ADC clock.\n");
> -		goto error_free_adc_clk;
> +		goto error_disable_clk;
>  	}
> 
>  	ret = clk_enable(st->adc_clk);
> @@ -697,20 +685,12 @@ error_disable_adc_clk:
>  	clk_disable(st->adc_clk);
>  error_unprepare_adc_clk:
>  	clk_unprepare(st->adc_clk);
> -error_free_adc_clk:
> -	clk_put(st->adc_clk);
>  error_disable_clk:
>  	clk_disable(st->clk);
>  error_unprepare_clk:
>  	clk_unprepare(st->clk);
> -error_free_clk:
> -	clk_put(st->clk);
>  error_free_irq:
>  	free_irq(st->irq, idev);
> -error_unmap_reg:
> -	iounmap(st->reg_base);
> -error_release_mem:
> -	release_mem_region(res->start, resource_size(res));
>  error_free_device:
>  	iio_device_free(idev);
>  error_ret:
> @@ -720,20 +700,15 @@ error_ret:
>  static int __devexit at91_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *idev = platform_get_drvdata(pdev);
> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	struct at91_adc_state *st = iio_priv(idev);
> 
>  	iio_device_unregister(idev);
>  	at91_adc_trigger_remove(idev);
>  	at91_adc_buffer_remove(idev);
>  	clk_disable_unprepare(st->adc_clk);
> -	clk_put(st->adc_clk);
>  	clk_disable(st->clk);
>  	clk_unprepare(st->clk);
> -	clk_put(st->clk);
>  	free_irq(st->irq, idev);
> -	iounmap(st->reg_base);
> -	release_mem_region(res->start, resource_size(res));
>  	iio_device_free(idev);
> 
>  	return 0;
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-08-14 20:32       ` Jonathan Cameron
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2012-08-14 20:32 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Lars-Peter Clausen, Jonathan Cameron, kernel-janitors, linux-iio,
	linux-kernel

Lars-Peter,

Are you happy with this updated version?  Can't immediately find any response
from you to it.

Jonathan
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> The various devm_ functions allocate memory that is released when a driver
> detaches.  This patch uses these functions for data that is allocated in
> the probe function of a platform device and is only freed in the remove
> function.
> 
> The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved coser
> to the call to devm_request_and_ioremap, which is th first use of the
> result of platform_get_resource.
> 
> This does not use devm_request_irq to ensure that free_irq is executed
> before its idev argument is freed.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/iio/adc/at91_adc.c |   41 ++++++++---------------------------------
>  1 file changed, 8 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index f61780a..3506e3d 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  		goto error_free_device;
>  	}
> 
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(&pdev->dev, "No resource defined\n");
> -		ret = -ENXIO;
> -		goto error_ret;
> -	}
> -
>  	platform_set_drvdata(pdev, idev);
> 
>  	idev->dev.parent = &pdev->dev;
> @@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  		goto error_free_device;
>  	}
> 
> -	if (!request_mem_region(res->start, resource_size(res),
> -				"AT91 adc registers")) {
> -		dev_err(&pdev->dev, "Resources are unavailable.\n");
> -		ret = -EBUSY;
> -		goto error_free_device;
> -	}
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 
> -	st->reg_base = ioremap(res->start, resource_size(res));
> +	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>  	if (!st->reg_base) {
>  		dev_err(&pdev->dev, "Failed to map registers.\n");
>  		ret = -ENOMEM;
> -		goto error_release_mem;
> +		goto error_free_device;
>  	}
> 
>  	/*
> @@ -592,10 +580,10 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  			  idev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
> -		goto error_unmap_reg;
> +		goto error_free_device;
>  	}
> 
> -	st->clk = clk_get(&pdev->dev, "adc_clk");
> +	st->clk = devm_clk_get(&pdev->dev, "adc_clk");
>  	if (IS_ERR(st->clk)) {
>  		dev_err(&pdev->dev, "Failed to get the clock.\n");
>  		ret = PTR_ERR(st->clk);
> @@ -605,7 +593,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	ret = clk_prepare(st->clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Could not prepare the clock.\n");
> -		goto error_free_clk;
> +		goto error_free_irq;
>  	}
> 
>  	ret = clk_enable(st->clk);
> @@ -614,7 +602,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  		goto error_unprepare_clk;
>  	}
> 
> -	st->adc_clk = clk_get(&pdev->dev, "adc_op_clk");
> +	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
>  	if (IS_ERR(st->adc_clk)) {
>  		dev_err(&pdev->dev, "Failed to get the ADC clock.\n");
>  		ret = PTR_ERR(st->clk);
> @@ -624,7 +612,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	ret = clk_prepare(st->adc_clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Could not prepare the ADC clock.\n");
> -		goto error_free_adc_clk;
> +		goto error_disable_clk;
>  	}
> 
>  	ret = clk_enable(st->adc_clk);
> @@ -697,20 +685,12 @@ error_disable_adc_clk:
>  	clk_disable(st->adc_clk);
>  error_unprepare_adc_clk:
>  	clk_unprepare(st->adc_clk);
> -error_free_adc_clk:
> -	clk_put(st->adc_clk);
>  error_disable_clk:
>  	clk_disable(st->clk);
>  error_unprepare_clk:
>  	clk_unprepare(st->clk);
> -error_free_clk:
> -	clk_put(st->clk);
>  error_free_irq:
>  	free_irq(st->irq, idev);
> -error_unmap_reg:
> -	iounmap(st->reg_base);
> -error_release_mem:
> -	release_mem_region(res->start, resource_size(res));
>  error_free_device:
>  	iio_device_free(idev);
>  error_ret:
> @@ -720,20 +700,15 @@ error_ret:
>  static int __devexit at91_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *idev = platform_get_drvdata(pdev);
> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	struct at91_adc_state *st = iio_priv(idev);
> 
>  	iio_device_unregister(idev);
>  	at91_adc_trigger_remove(idev);
>  	at91_adc_buffer_remove(idev);
>  	clk_disable_unprepare(st->adc_clk);
> -	clk_put(st->adc_clk);
>  	clk_disable(st->clk);
>  	clk_unprepare(st->clk);
> -	clk_put(st->clk);
>  	free_irq(st->irq, idev);
> -	iounmap(st->reg_base);
> -	release_mem_region(res->start, resource_size(res));
>  	iio_device_free(idev);
> 
>  	return 0;
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-08-14 20:32       ` Jonathan Cameron
@ 2012-08-15  9:02         ` Lars-Peter Clausen
  -1 siblings, 0 replies; 20+ messages in thread
From: Lars-Peter Clausen @ 2012-08-15  9:02 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

On 08/14/2012 10:32 PM, Jonathan Cameron wrote:
> Lars-Peter,
> 
> Are you happy with this updated version?  Can't immediately find any response
> from you to it.
> 

I think it is ok, you can add my
Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>.

One minor nitpick though.

> Jonathan
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> The various devm_ functions allocate memory that is released when a driver
>> detaches.  This patch uses these functions for data that is allocated in
>> the probe function of a platform device and is only freed in the remove
>> function.
>>
>> The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved coser
>> to the call to devm_request_and_ioremap, which is th first use of the
>> result of platform_get_resource.
>>
>> This does not use devm_request_irq to ensure that free_irq is executed
>> before its idev argument is freed.
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> ---
>>  drivers/iio/adc/at91_adc.c |   41 ++++++++---------------------------------
>>  1 file changed, 8 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>> index f61780a..3506e3d 100644
>> --- a/drivers/iio/adc/at91_adc.c
>> +++ b/drivers/iio/adc/at91_adc.c
>> @@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  		goto error_free_device;
>>  	}
>>
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -	if (!res) {
>> -		dev_err(&pdev->dev, "No resource defined\n");
>> -		ret = -ENXIO;
>> -		goto error_ret;
>> -	}
>> -
>>  	platform_set_drvdata(pdev, idev);
>>
>>  	idev->dev.parent = &pdev->dev;
>> @@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  		goto error_free_device;
>>  	}
>>
>> -	if (!request_mem_region(res->start, resource_size(res),
>> -				"AT91 adc registers")) {
>> -		dev_err(&pdev->dev, "Resources are unavailable.\n");
>> -		ret = -EBUSY;
>> -		goto error_free_device;
>> -	}
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>
>> -	st->reg_base = ioremap(res->start, resource_size(res));
>> +	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>>  	if (!st->reg_base) {
>>  		dev_err(&pdev->dev, "Failed to map registers.\n");

devm_request_and_ioremap will already print a error messages on it's own if
something goes wrong. So strictly speaking this one is redundant, but I don't
think it is necessary to do a resend just for this, maybe you can remove the
extra dev_err when you apply the patch.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-08-15  9:02         ` Lars-Peter Clausen
  0 siblings, 0 replies; 20+ messages in thread
From: Lars-Peter Clausen @ 2012-08-15  9:02 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Julia Lawall, Jonathan Cameron, kernel-janitors, linux-iio, linux-kernel

On 08/14/2012 10:32 PM, Jonathan Cameron wrote:
> Lars-Peter,
> 
> Are you happy with this updated version?  Can't immediately find any response
> from you to it.
> 

I think it is ok, you can add my
Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>.

One minor nitpick though.

> Jonathan
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> The various devm_ functions allocate memory that is released when a driver
>> detaches.  This patch uses these functions for data that is allocated in
>> the probe function of a platform device and is only freed in the remove
>> function.
>>
>> The call to platform_get_resource(pdev, IORESOURCE_MEM, 0) is moved coser
>> to the call to devm_request_and_ioremap, which is th first use of the
>> result of platform_get_resource.
>>
>> This does not use devm_request_irq to ensure that free_irq is executed
>> before its idev argument is freed.
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> ---
>>  drivers/iio/adc/at91_adc.c |   41 ++++++++---------------------------------
>>  1 file changed, 8 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>> index f61780a..3506e3d 100644
>> --- a/drivers/iio/adc/at91_adc.c
>> +++ b/drivers/iio/adc/at91_adc.c
>> @@ -545,13 +545,6 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  		goto error_free_device;
>>  	}
>>
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -	if (!res) {
>> -		dev_err(&pdev->dev, "No resource defined\n");
>> -		ret = -ENXIO;
>> -		goto error_ret;
>> -	}
>> -
>>  	platform_set_drvdata(pdev, idev);
>>
>>  	idev->dev.parent = &pdev->dev;
>> @@ -566,18 +559,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  		goto error_free_device;
>>  	}
>>
>> -	if (!request_mem_region(res->start, resource_size(res),
>> -				"AT91 adc registers")) {
>> -		dev_err(&pdev->dev, "Resources are unavailable.\n");
>> -		ret = -EBUSY;
>> -		goto error_free_device;
>> -	}
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>
>> -	st->reg_base = ioremap(res->start, resource_size(res));
>> +	st->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>>  	if (!st->reg_base) {
>>  		dev_err(&pdev->dev, "Failed to map registers.\n");

devm_request_and_ioremap will already print a error messages on it's own if
something goes wrong. So strictly speaking this one is redundant, but I don't
think it is necessary to do a resend just for this, maybe you can remove the
extra dev_err when you apply the patch.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-08-15  9:02         ` Lars-Peter Clausen
@ 2012-08-15  9:20           ` Julia Lawall
  -1 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-08-15  9:20 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Jonathan Cameron, Julia Lawall, Jonathan Cameron,
	kernel-janitors, linux-iio, linux-kernel

> devm_request_and_ioremap will already print a error messages on it's own if
> something goes wrong. So strictly speaking this one is redundant, but I don't
> think it is necessary to do a resend just for this, maybe you can remove the
> extra dev_err when you apply the patch.

Thanks for pointing that out.  I will get rid of the messages in the 
future.  That seems easier than figuring out how to adapt the message to 
the new function.

julia

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-08-15  9:20           ` Julia Lawall
  0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2012-08-15  9:20 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Jonathan Cameron, Julia Lawall, Jonathan Cameron,
	kernel-janitors, linux-iio, linux-kernel

> devm_request_and_ioremap will already print a error messages on it's own if
> something goes wrong. So strictly speaking this one is redundant, but I don't
> think it is necessary to do a resend just for this, maybe you can remove the
> extra dev_err when you apply the patch.

Thanks for pointing that out.  I will get rid of the messages in the 
future.  That seems easier than figuring out how to adapt the message to 
the new function.

julia

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
  2012-08-15  9:20           ` Julia Lawall
@ 2012-08-16 19:04             ` Jonathan Cameron
  -1 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2012-08-16 19:04 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Lars-Peter Clausen, Jonathan Cameron, kernel-janitors, linux-iio,
	linux-kernel

On 08/15/2012 10:20 AM, Julia Lawall wrote:
>> devm_request_and_ioremap will already print a error messages on it's own if
>> something goes wrong. So strictly speaking this one is redundant, but I don't
>> think it is necessary to do a resend just for this, maybe you can remove the
>> extra dev_err when you apply the patch.
> 
> Thanks for pointing that out.  I will get rid of the messages in the future.  That seems easier than figuring out how to
> adapt the message to the new function.
> 
merged with that line removed as suggested.  Thanks Julia and Lars-Peter.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions
@ 2012-08-16 19:04             ` Jonathan Cameron
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2012-08-16 19:04 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Lars-Peter Clausen, Jonathan Cameron, kernel-janitors, linux-iio,
	linux-kernel

On 08/15/2012 10:20 AM, Julia Lawall wrote:
>> devm_request_and_ioremap will already print a error messages on it's own if
>> something goes wrong. So strictly speaking this one is redundant, but I don't
>> think it is necessary to do a resend just for this, maybe you can remove the
>> extra dev_err when you apply the patch.
> 
> Thanks for pointing that out.  I will get rid of the messages in the future.  That seems easier than figuring out how to
> adapt the message to the new function.
> 
merged with that line removed as suggested.  Thanks Julia and Lars-Peter.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2012-08-16 19:04 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-31 10:09 [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions Julia Lawall
2012-07-31 10:09 ` Julia Lawall
2012-07-31 12:36 ` Lars-Peter Clausen
2012-07-31 12:36   ` Lars-Peter Clausen
2012-07-31 12:41   ` Julia Lawall
2012-07-31 12:41     ` Julia Lawall
2012-07-31 13:09   ` Julia Lawall
2012-07-31 13:09     ` Julia Lawall
2012-08-14 20:32     ` Jonathan Cameron
2012-08-14 20:32       ` Jonathan Cameron
2012-08-15  9:02       ` Lars-Peter Clausen
2012-08-15  9:02         ` Lars-Peter Clausen
2012-08-15  9:20         ` Julia Lawall
2012-08-15  9:20           ` Julia Lawall
2012-08-16 19:04           ` Jonathan Cameron
2012-08-16 19:04             ` Jonathan Cameron
2012-07-31 13:54   ` Julia Lawall
2012-07-31 13:54     ` Julia Lawall
2012-07-31 14:23     ` Lars-Peter Clausen
2012-07-31 14:23       ` Lars-Peter Clausen

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.