[PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia Lawall]

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

Using devm_kzalloc and devm_clk_get simplifies the code and ensures that
the use of devm_request_irq is safe.  When kzalloc and kfree were used, the
interrupt could be triggered after the handler's data argument had been
freed.

The problem of a free after a devm_request_irq was found using the
following semantic match (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
expression e1,e2,x,a,b,c,d;
identifier free;
position p1,p2;
@@

  devm_request_irq@p1(e1,e2,...,x)
  ... when any
      when != e2 = a
      when != x = b
  if (...) {
    ... when != e2 = c
        when != x = d
    free@p2(...,x,...);
    ...
    return ...;
  }
// </smpl>

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

---
 drivers/media/video/mx2_emmaprp.c |   25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/mx2_emmaprp.c b/drivers/media/video/mx2_emmaprp.c
index 5f8a6f5..78c5dc9 100644
--- a/drivers/media/video/mx2_emmaprp.c
+++ b/drivers/media/video/mx2_emmaprp.c
@@ -874,29 +874,27 @@ static int emmaprp_probe(struct platform_device *pdev)
 	int irq_emma;
 	int ret;
 
-	pcdev = kzalloc(sizeof *pcdev, GFP_KERNEL);
+	pcdev = devm_kzalloc(&pdev->dev, sizeof(*pcdev), GFP_KERNEL);
 	if (!pcdev)
 		return -ENOMEM;
 
 	spin_lock_init(&pcdev->irqlock);
 
-	pcdev->clk_emma = clk_get(&pdev->dev, NULL);
+	pcdev->clk_emma = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pcdev->clk_emma)) {
-		ret = PTR_ERR(pcdev->clk_emma);
-		goto free_dev;
+		return PTR_ERR(pcdev->clk_emma);
 	}
 
 	irq_emma = platform_get_irq(pdev, 0);
 	res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (irq_emma < 0 || res_emma == NULL) {
 		dev_err(&pdev->dev, "Missing platform resources data\n");
-		ret = -ENODEV;
-		goto free_clk;
+		return -ENODEV;
 	}
 
 	ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
 	if (ret)
-		goto free_clk;
+		return ret;
 
 	mutex_init(&pcdev->dev_mutex);
 
@@ -922,12 +920,7 @@ static int emmaprp_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcdev);
 
-	if (devm_request_mem_region(&pdev->dev, res_emma->start,
-	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
-		goto rel_vdev;
-
-	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
-					resource_size(res_emma));
+	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
 	if (!pcdev->base_emma)
 		goto rel_vdev;
 
@@ -969,10 +962,6 @@ rel_vdev:
 	video_device_release(vfd);
 unreg_dev:
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-free_clk:
-	clk_put(pcdev->clk_emma);
-free_dev:
-	kfree(pcdev);
 
 	return ret;
 }
@@ -987,8 +976,6 @@ static int emmaprp_remove(struct platform_device *pdev)
 	v4l2_m2m_release(pcdev->m2m_dev);
 	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-	clk_put(pcdev->clk_emma);
-	kfree(pcdev);
 
 	return 0;
 }

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Dan Carpenter]

On Sat, Aug 04, 2012 at 08:23:27PM +0200, Julia Lawall wrote:
> @@ -922,12 +920,7 @@ static int emmaprp_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, pcdev);
>  
> -	if (devm_request_mem_region(&pdev->dev, res_emma->start,
> -	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
> -		goto rel_vdev;
> -
> -	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
> -					resource_size(res_emma));
> +	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
>  	if (!pcdev->base_emma)
>  		goto rel_vdev;

This was in the original code, but there is a "ret = -ENOMEM;"
missing here, and again a couple lines down in the original code.

regards,
dan carpenter

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Dan Carpenter]

On Mon, Aug 06, 2012 at 05:23:23PM +0300, Dan Carpenter wrote:
> On Sat, Aug 04, 2012 at 08:23:27PM +0200, Julia Lawall wrote:
> > @@ -922,12 +920,7 @@ static int emmaprp_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, pcdev);
> >  
> > -	if (devm_request_mem_region(&pdev->dev, res_emma->start,
> > -	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
> > -		goto rel_vdev;
> > -
> > -	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
> > -					resource_size(res_emma));
> > +	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
> >  	if (!pcdev->base_emma)
> >  		goto rel_vdev;
> 
> This was in the original code, but there is a "ret = -ENOMEM;"
> missing here, and again a couple lines down in the original code.
> 

Or should that be -EIO instead of -ENOMEM?  I'm not sure.

regards,
dan carpenter

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Lars-Peter Clausen]

On 08/06/2012 04:26 PM, Dan Carpenter wrote:
> On Mon, Aug 06, 2012 at 05:23:23PM +0300, Dan Carpenter wrote:
>> On Sat, Aug 04, 2012 at 08:23:27PM +0200, Julia Lawall wrote:
>>> @@ -922,12 +920,7 @@ static int emmaprp_probe(struct platform_device *pdev)
>>>  
>>>  	platform_set_drvdata(pdev, pcdev);
>>>  
>>> -	if (devm_request_mem_region(&pdev->dev, res_emma->start,
>>> -	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
>>> -		goto rel_vdev;
>>> -
>>> -	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
>>> -					resource_size(res_emma));
>>> +	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
>>>  	if (!pcdev->base_emma)
>>>  		goto rel_vdev;
>>
>> This was in the original code, but there is a "ret = -ENOMEM;"
>> missing here, and again a couple lines down in the original code.
>>
> 
> Or should that be -EIO instead of -ENOMEM?  I'm not sure.

-ENXIO is usually used in such a case.

- Lars

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia.Lawall]

Quoting Lars-Peter Clausen <lars@metafoo.de>:

> On 08/06/2012 04:26 PM, Dan Carpenter wrote:
>> On Mon, Aug 06, 2012 at 05:23:23PM +0300, Dan Carpenter wrote:
>>> On Sat, Aug 04, 2012 at 08:23:27PM +0200, Julia Lawall wrote:
>>>> @@ -922,12 +920,7 @@ static int emmaprp_probe(struct  
>>>> platform_device *pdev)
>>>>
>>>>  	platform_set_drvdata(pdev, pcdev);
>>>>
>>>> -	if (devm_request_mem_region(&pdev->dev, res_emma->start,
>>>> -	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
>>>> -		goto rel_vdev;
>>>> -
>>>> -	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
>>>> -					resource_size(res_emma));
>>>> +	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
>>>>  	if (!pcdev->base_emma)
>>>>  		goto rel_vdev;
>>>
>>> This was in the original code, but there is a "ret = -ENOMEM;"
>>> missing here, and again a couple lines down in the original code.
>>>
>>
>> Or should that be -EIO instead of -ENOMEM?  I'm not sure.
>
> -ENXIO is usually used in such a case.

Thanks for the feedback.  I won't be able to access my work machine  
until the end of the week, so I will fix it then.

julia

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia.Lawall]

Quoting Lars-Peter Clausen <lars@metafoo.de>:

> On 08/06/2012 04:26 PM, Dan Carpenter wrote:
>> On Mon, Aug 06, 2012 at 05:23:23PM +0300, Dan Carpenter wrote:
>>> On Sat, Aug 04, 2012 at 08:23:27PM +0200, Julia Lawall wrote:
>>>> @@ -922,12 +920,7 @@ static int emmaprp_probe(struct  
>>>> platform_device *pdev)
>>>>
>>>>  	platform_set_drvdata(pdev, pcdev);
>>>>
>>>> -	if (devm_request_mem_region(&pdev->dev, res_emma->start,
>>>> -	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
>>>> -		goto rel_vdev;
>>>> -
>>>> -	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
>>>> -					resource_size(res_emma));
>>>> +	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
>>>>  	if (!pcdev->base_emma)
>>>>  		goto rel_vdev;
>>>
>>> This was in the original code, but there is a "ret = -ENOMEM;"
>>> missing here, and again a couple lines down in the original code.
>>>
>>
>> Or should that be -EIO instead of -ENOMEM?  I'm not sure.
>
> -ENXIO is usually used in such a case.

Thanks for the feedback.  I won't be able to access my work machine  
until the end of the week, so I will fix it then.

julia

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia Lawall]

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

Using devm_kzalloc and devm_clk_get simplifies the code and ensures that
the use of devm_request_irq is safe.  When kzalloc and kfree were used, the
interrupt could be triggered after the handler's data argument had been
freed.

Add missing return code initializations in the error handling code for
devm_request_and_ioremap and devm_request_irq.

The problem of a free after a devm_request_irq was found using the
following semantic match (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
expression e1,e2,x,a,b,c,d;
identifier free;
position p1,p2;
@@

  devm_request_irq@p1(e1,e2,...,x)
  ... when any
      when != e2 = a
      when != x = b
  if (...) {
    ... when != e2 = c
        when != x = d
    free@p2(...,x,...);
    ...
    return ...;
  }
// </smpl>

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

---
v2: patch updated to add nearby ret initializations.  Please let me know
if some other constants should be returned on the failure of
devm_request_and_ioremap and devm_request_irq.

 drivers/media/video/mx2_emmaprp.c |   33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/media/video/mx2_emmaprp.c b/drivers/media/video/mx2_emmaprp.c
index 5f8a6f5..9fe9ec6 100644
--- a/drivers/media/video/mx2_emmaprp.c
+++ b/drivers/media/video/mx2_emmaprp.c
@@ -874,29 +874,27 @@ static int emmaprp_probe(struct platform_device *pdev)
 	int irq_emma;
 	int ret;

-	pcdev = kzalloc(sizeof *pcdev, GFP_KERNEL);
+	pcdev = devm_kzalloc(&pdev->dev, sizeof(*pcdev), GFP_KERNEL);
 	if (!pcdev)
 		return -ENOMEM;

 	spin_lock_init(&pcdev->irqlock);

-	pcdev->clk_emma = clk_get(&pdev->dev, NULL);
+	pcdev->clk_emma = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pcdev->clk_emma)) {
-		ret = PTR_ERR(pcdev->clk_emma);
-		goto free_dev;
+		return PTR_ERR(pcdev->clk_emma);
 	}

 	irq_emma = platform_get_irq(pdev, 0);
 	res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (irq_emma < 0 || res_emma == NULL) {
 		dev_err(&pdev->dev, "Missing platform resources data\n");
-		ret = -ENODEV;
-		goto free_clk;
+		return -ENODEV;
 	}

 	ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
 	if (ret)
-		goto free_clk;
+		return ret;

 	mutex_init(&pcdev->dev_mutex);

@@ -922,21 +920,20 @@ static int emmaprp_probe(struct platform_device *pdev)

 	platform_set_drvdata(pdev, pcdev);

-	if (devm_request_mem_region(&pdev->dev, res_emma->start,
-	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
-		goto rel_vdev;
-
-	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
-					resource_size(res_emma));
-	if (!pcdev->base_emma)
+	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
+	if (!pcdev->base_emma) {
+		ret = -ENXIO;
 		goto rel_vdev;
+	}

 	pcdev->irq_emma = irq_emma;
 	pcdev->res_emma = res_emma;

 	if (devm_request_irq(&pdev->dev, pcdev->irq_emma, emmaprp_irq,
-			     0, MEM2MEM_NAME, pcdev) < 0)
+			     0, MEM2MEM_NAME, pcdev) < 0) {
+		ret = -ENODEV;
 		goto rel_vdev;
+	}

 	pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
 	if (IS_ERR(pcdev->alloc_ctx)) {
@@ -969,10 +966,6 @@ rel_vdev:
 	video_device_release(vfd);
 unreg_dev:
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-free_clk:
-	clk_put(pcdev->clk_emma);
-free_dev:
-	kfree(pcdev);

 	return ret;
 }
@@ -987,8 +980,6 @@ static int emmaprp_remove(struct platform_device *pdev)
 	v4l2_m2m_release(pcdev->m2m_dev);
 	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-	clk_put(pcdev->clk_emma);
-	kfree(pcdev);

 	return 0;
 }

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Mauro Carvalho Chehab]

Em 10-08-2012 10:59, Julia Lawall escreveu:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Using devm_kzalloc and devm_clk_get simplifies the code and ensures that
> the use of devm_request_irq is safe.  When kzalloc and kfree were used, the
> interrupt could be triggered after the handler's data argument had been
> freed.
> 
> Add missing return code initializations in the error handling code for
> devm_request_and_ioremap and devm_request_irq.
> 
> The problem of a free after a devm_request_irq was found using the
> following semantic match (http://coccinelle.lip6.fr/)

Hi Julia,

This patch doesn't apply anymore, likely due to this changeset:

commit 33eb46a7c2bdd10f9a761390ce1bf51169ff537a
Author: Javier Martin <javier.martin@vista-silicon.com>
Date:   Mon Jul 30 04:37:30 2012 -0300

    [media] media: i.MX27: Fix mx2_emmaprp mem2mem driver clocks
    
    This driver wasn't converted to the new clock framework
    (e038ed50a4a767add205094c035b6943e7b30140).
    
    Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/mx2_emmaprp.c b/drivers/media/video/mx2_emmaprp.c
index 5f8a6f5..728cac3 100644
--- a/drivers/media/video/mx2_emmaprp.c
+++ b/drivers/media/video/mx2_emmaprp.c
@@ -209,7 +209,7 @@ struct emmaprp_dev {
 
 	int			irq_emma;
 	void __iomem		*base_emma;
-	struct clk		*clk_emma;
+	struct clk		*clk_emma_ahb, *clk_emma_ipg;
 	struct resource		*res_emma;
 
 	struct v4l2_m2m_dev	*m2m_dev;
@@ -804,7 +804,8 @@ static int emmaprp_open(struct file *file)
 		return ret;
 	}
 
-	clk_enable(pcdev->clk_emma);
+	clk_prepare_enable(pcdev->clk_emma_ipg);
+	clk_prepare_enable(pcdev->clk_emma_ahb);
 	ctx->q_data[V4L2_M2M_SRC].fmt = &formats[1];
 	ctx->q_data[V4L2_M2M_DST].fmt = &formats[0];
 
@@ -820,7 +821,8 @@ static int emmaprp_release(struct file *file)
 
 	dprintk(pcdev, "Releasing instance %p\n", ctx);
 
-	clk_disable(pcdev->clk_emma);
+	clk_disable_unprepare(pcdev->clk_emma_ahb);
+	clk_disable_unprepare(pcdev->clk_emma_ipg);
 	v4l2_m2m_ctx_release(ctx->m2m_ctx);
 	kfree(ctx);
 
@@ -880,9 +882,15 @@ static int emmaprp_probe(struct platform_device *pdev)
 
 	spin_lock_init(&pcdev->irqlock);
 
-	pcdev->clk_emma = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(pcdev->clk_emma)) {
-		ret = PTR_ERR(pcdev->clk_emma);
+	pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(pcdev->clk_emma_ipg)) {
+		ret = PTR_ERR(pcdev->clk_emma_ipg);
+		goto free_dev;
+	}
+
+	pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(pcdev->clk_emma_ipg)) {
+		ret = PTR_ERR(pcdev->clk_emma_ahb);
 		goto free_dev;
 	}
 
@@ -891,12 +899,12 @@ static int emmaprp_probe(struct platform_device *pdev)
 	if (irq_emma < 0 || res_emma == NULL) {
 		dev_err(&pdev->dev, "Missing platform resources data\n");
 		ret = -ENODEV;
-		goto free_clk;
+		goto free_dev;
 	}
 
 	ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
 	if (ret)
-		goto free_clk;
+		goto free_dev;
 
 	mutex_init(&pcdev->dev_mutex);
 
@@ -969,8 +977,6 @@ rel_vdev:
 	video_device_release(vfd);
 unreg_dev:
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-free_clk:
-	clk_put(pcdev->clk_emma);
 free_dev:
 	kfree(pcdev);
 
@@ -987,7 +993,6 @@ static int emmaprp_remove(struct platform_device *pdev)
 	v4l2_m2m_release(pcdev->m2m_dev);
 	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-	clk_put(pcdev->clk_emma);
 	kfree(pcdev);
 
 	return 0;

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia Lawall]

On Mon, 13 Aug 2012, Mauro Carvalho Chehab wrote:

> Em 10-08-2012 10:59, Julia Lawall escreveu:
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> Using devm_kzalloc and devm_clk_get simplifies the code and ensures that
>> the use of devm_request_irq is safe.  When kzalloc and kfree were used, the
>> interrupt could be triggered after the handler's data argument had been
>> freed.
>>
>> Add missing return code initializations in the error handling code for
>> devm_request_and_ioremap and devm_request_irq.
>>
>> The problem of a free after a devm_request_irq was found using the
>> following semantic match (http://coccinelle.lip6.fr/)
>
> Hi Julia,
>
> This patch doesn't apply anymore, likely due to this changeset:

OK, I'll fix it.  Indeed, devm_clk_get is already introduced by this 
patch.

julia

> commit 33eb46a7c2bdd10f9a761390ce1bf51169ff537a
> Author: Javier Martin <javier.martin@vista-silicon.com>
> Date:   Mon Jul 30 04:37:30 2012 -0300
>
>    [media] media: i.MX27: Fix mx2_emmaprp mem2mem driver clocks
>
>    This driver wasn't converted to the new clock framework
>    (e038ed50a4a767add205094c035b6943e7b30140).
>
>    Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> diff --git a/drivers/media/video/mx2_emmaprp.c b/drivers/media/video/mx2_emmaprp.c
> index 5f8a6f5..728cac3 100644
> --- a/drivers/media/video/mx2_emmaprp.c
> +++ b/drivers/media/video/mx2_emmaprp.c
> @@ -209,7 +209,7 @@ struct emmaprp_dev {
>
> 	int			irq_emma;
> 	void __iomem		*base_emma;
> -	struct clk		*clk_emma;
> +	struct clk		*clk_emma_ahb, *clk_emma_ipg;
> 	struct resource		*res_emma;
>
> 	struct v4l2_m2m_dev	*m2m_dev;
> @@ -804,7 +804,8 @@ static int emmaprp_open(struct file *file)
> 		return ret;
> 	}
>
> -	clk_enable(pcdev->clk_emma);
> +	clk_prepare_enable(pcdev->clk_emma_ipg);
> +	clk_prepare_enable(pcdev->clk_emma_ahb);
> 	ctx->q_data[V4L2_M2M_SRC].fmt = &formats[1];
> 	ctx->q_data[V4L2_M2M_DST].fmt = &formats[0];
>
> @@ -820,7 +821,8 @@ static int emmaprp_release(struct file *file)
>
> 	dprintk(pcdev, "Releasing instance %p\n", ctx);
>
> -	clk_disable(pcdev->clk_emma);
> +	clk_disable_unprepare(pcdev->clk_emma_ahb);
> +	clk_disable_unprepare(pcdev->clk_emma_ipg);
> 	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> 	kfree(ctx);
>
> @@ -880,9 +882,15 @@ static int emmaprp_probe(struct platform_device *pdev)
>
> 	spin_lock_init(&pcdev->irqlock);
>
> -	pcdev->clk_emma = clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(pcdev->clk_emma)) {
> -		ret = PTR_ERR(pcdev->clk_emma);
> +	pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(pcdev->clk_emma_ipg)) {
> +		ret = PTR_ERR(pcdev->clk_emma_ipg);
> +		goto free_dev;
> +	}
> +
> +	pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(pcdev->clk_emma_ipg)) {
> +		ret = PTR_ERR(pcdev->clk_emma_ahb);
> 		goto free_dev;
> 	}
>
> @@ -891,12 +899,12 @@ static int emmaprp_probe(struct platform_device *pdev)
> 	if (irq_emma < 0 || res_emma == NULL) {
> 		dev_err(&pdev->dev, "Missing platform resources data\n");
> 		ret = -ENODEV;
> -		goto free_clk;
> +		goto free_dev;
> 	}
>
> 	ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
> 	if (ret)
> -		goto free_clk;
> +		goto free_dev;
>
> 	mutex_init(&pcdev->dev_mutex);
>
> @@ -969,8 +977,6 @@ rel_vdev:
> 	video_device_release(vfd);
> unreg_dev:
> 	v4l2_device_unregister(&pcdev->v4l2_dev);
> -free_clk:
> -	clk_put(pcdev->clk_emma);
> free_dev:
> 	kfree(pcdev);
>
> @@ -987,7 +993,6 @@ static int emmaprp_remove(struct platform_device *pdev)
> 	v4l2_m2m_release(pcdev->m2m_dev);
> 	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
> 	v4l2_device_unregister(&pcdev->v4l2_dev);
> -	clk_put(pcdev->clk_emma);
> 	kfree(pcdev);
>
> 	return 0;
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia Lawall]

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

Using devm_kzalloc simplifies the code and ensures that the use of
devm_request_irq is safe.  When kzalloc and kfree were used, the interrupt
could be triggered after the handler's data argument had been freed.

This also introduces some missing initializations of the return variable
ret, and uses devm_request_and_ioremap instead of the combination of
devm_request_mem_region and devm_ioremap.

The problem of a free after a devm_request_irq was found using the
following semantic match (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
expression e1,e2,x,a,b,c,d;
identifier free;
position p1,p2;
@@

   devm_request_irq@p1(e1,e2,...,x)
   ... when any
       when != e2 = a
       when != x = b
   if (...) {
     ... when != e2 = c
         when != x = d
     free@p2(...,x,...);
     ...
     return ...;
   }
// </smpl>

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

---
v3: due to a conflict with another patch

  drivers/media/video/mx2_emmaprp.c |   31 ++++++++++++-------------------
  1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/mx2_emmaprp.c b/drivers/media/video/mx2_emmaprp.c
index 2810015..dab380a 100644
--- a/drivers/media/video/mx2_emmaprp.c
+++ b/drivers/media/video/mx2_emmaprp.c
@@ -896,7 +896,7 @@ static int emmaprp_probe(struct platform_device *pdev)
  	int irq_emma;
  	int ret;

-	pcdev = kzalloc(sizeof *pcdev, GFP_KERNEL);
+	pcdev = devm_kzalloc(&pdev->dev, sizeof(*pcdev), GFP_KERNEL);
  	if (!pcdev)
  		return -ENOMEM;

@@ -904,27 +904,24 @@ static int emmaprp_probe(struct platform_device *pdev)

  	pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "ipg");
  	if (IS_ERR(pcdev->clk_emma_ipg)) {
-		ret = PTR_ERR(pcdev->clk_emma_ipg);
-		goto free_dev;
+		return PTR_ERR(pcdev->clk_emma_ipg);
  	}

  	pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "ahb");
  	if (IS_ERR(pcdev->clk_emma_ipg)) {
-		ret = PTR_ERR(pcdev->clk_emma_ahb);
-		goto free_dev;
+		return PTR_ERR(pcdev->clk_emma_ahb);
  	}

  	irq_emma = platform_get_irq(pdev, 0);
  	res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	if (irq_emma < 0 || res_emma == NULL) {
  		dev_err(&pdev->dev, "Missing platform resources data\n");
-		ret = -ENODEV;
-		goto free_dev;
+		return -ENODEV;
  	}

  	ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
  	if (ret)
-		goto free_dev;
+		return ret;

  	mutex_init(&pcdev->dev_mutex);

@@ -946,21 +943,20 @@ static int emmaprp_probe(struct platform_device *pdev)

  	platform_set_drvdata(pdev, pcdev);

-	if (devm_request_mem_region(&pdev->dev, res_emma->start,
-	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
-		goto rel_vdev;
-
-	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
-					resource_size(res_emma));
-	if (!pcdev->base_emma)
+	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
+	if (!pcdev->base_emma) {
+		ret = -ENXIO;
  		goto rel_vdev;
+	}

  	pcdev->irq_emma = irq_emma;
  	pcdev->res_emma = res_emma;

  	if (devm_request_irq(&pdev->dev, pcdev->irq_emma, emmaprp_irq,
-			     0, MEM2MEM_NAME, pcdev) < 0)
+			     0, MEM2MEM_NAME, pcdev) < 0) {
+		ret = -ENODEV;
  		goto rel_vdev;
+	}

  	pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
  	if (IS_ERR(pcdev->alloc_ctx)) {
@@ -993,8 +989,6 @@ rel_vdev:
  	video_device_release(vfd);
  unreg_dev:
  	v4l2_device_unregister(&pcdev->v4l2_dev);
-free_dev:
-	kfree(pcdev);

  	return ret;
  }
@@ -1009,7 +1003,6 @@ static int emmaprp_remove(struct platform_device *pdev)
  	v4l2_m2m_release(pcdev->m2m_dev);
  	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
  	v4l2_device_unregister(&pcdev->v4l2_dev);
-	kfree(pcdev);

  	return 0;
  }

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Mauro Carvalho Chehab]

Em 13-08-2012 17:20, Julia Lawall escreveu:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Using devm_kzalloc simplifies the code and ensures that the use of
> devm_request_irq is safe.  When kzalloc and kfree were used, the interrupt
> could be triggered after the handler's data argument had been freed.
> 
> This also introduces some missing initializations of the return variable
> ret, and uses devm_request_and_ioremap instead of the combination of
> devm_request_mem_region and devm_ioremap.
> 
> The problem of a free after a devm_request_irq was found using the
> following semantic match (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r exists@
> expression e1,e2,x,a,b,c,d;
> identifier free;
> position p1,p2;
> @@
> 
>   devm_request_irq@p1(e1,e2,...,x)
>   ... when any
>       when != e2 = a
>       when != x = b
>   if (...) {
>     ... when != e2 = c
>         when != x = d
>     free@p2(...,x,...);
>     ...
>     return ...;
>   }
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> v3: due to a conflict with another patch

Not sure what tree you used for it, but the result was
worse ;)

patching file drivers/media/video/mx2_emmaprp.c
Hunk #1 FAILED at 896.
Hunk #2 FAILED at 904.
Hunk #3 FAILED at 946.
Hunk #4 FAILED at 993.
Hunk #5 FAILED at 1009.
5 out of 5 hunks FAILED -- rejects in file drivers/media/video/mx2_emmaprp.c

Well, I've massively applied hundreds of patches today, but not much
on this driver. Maybe it is better for you to wait for a couple of
days for these to be at -next, or use, instead, our tree as the basis for
it:
	git://linuxtv.org/media_tree.git staging/for_v3.7

Regards,
Mauro

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia Lawall]

> Well, I've massively applied hundreds of patches today, but not much
> on this driver. Maybe it is better for you to wait for a couple of
> days for these to be at -next, or use, instead, our tree as the basis for
> it:
> 	git://linuxtv.org/media_tree.git staging/for_v3.7

I cloned this, but it doesn't seem to contain the file:

>:~: cd staging/for_v3.7/
>:for_v3.7: ls drivers/media/video/mx2_emmaprp.c
ls: cannot access drivers/media/video/mx2_emmaprp.c: No such file or 
directory

julia

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Mauro Carvalho Chehab]

Em 14-08-2012 03:30, Julia Lawall escreveu:
>> Well, I've massively applied hundreds of patches today, but not much
>> on this driver. Maybe it is better for you to wait for a couple of
>> days for these to be at -next, or use, instead, our tree as the basis for
>> it:
>>     git://linuxtv.org/media_tree.git staging/for_v3.7
> 
> I cloned this, but it doesn't seem to contain the file:
> 
>> :~: cd staging/for_v3.7/

staging/for_v3.7 is the name of the branch ;)

So, you need to use "--branch staging/for_v3.7" on your git
clone command.

>> :for_v3.7: ls drivers/media/video/mx2_emmaprp.c
> ls: cannot access drivers/media/video/mx2_emmaprp.c: No such file or directory

PS.: I started yesterday to apply a major reorganization of the drivers
along drivers/media/. This driver is still there at the same path, but it
will be moving soon to another place.

Re: [PATCH] drivers/media/video/mx2_emmaprp.c: use devm_kzalloc and devm_clk_get

[Julia Lawall]

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

Using devm_kzalloc simplifies the code and ensures that the use of
devm_request_irq is safe.  When kzalloc and kfree were used, the interrupt
could be triggered after the handler's data argument had been freed.

This also introduces some missing initializations of the return variable
ret, and uses devm_request_and_ioremap instead of the combination of
devm_request_mem_region and devm_ioremap.

The problem of a free after a devm_request_irq was found using the
following semantic match (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
expression e1,e2,x,a,b,c,d;
identifier free;
position p1,p2;
@@

  devm_request_irq@p1(e1,e2,...,x)
  ... when any
      when != e2 = a
      when != x = b
  if (...) {
    ... when != e2 = c
        when != x = d
    free@p2(...,x,...);
    ...
    return ...;
  }
// </smpl>

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

---
This is the same patch I sent before.  I had no trouble applying it after
cloning the staging/for_v3.7 branch.

 drivers/media/video/mx2_emmaprp.c |   31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/mx2_emmaprp.c b/drivers/media/video/mx2_emmaprp.c
index 2810015..dab380a 100644
--- a/drivers/media/video/mx2_emmaprp.c
+++ b/drivers/media/video/mx2_emmaprp.c
@@ -896,7 +896,7 @@ static int emmaprp_probe(struct platform_device *pdev)
 	int irq_emma;
 	int ret;

-	pcdev = kzalloc(sizeof *pcdev, GFP_KERNEL);
+	pcdev = devm_kzalloc(&pdev->dev, sizeof(*pcdev), GFP_KERNEL);
 	if (!pcdev)
 		return -ENOMEM;

@@ -904,27 +904,24 @@ static int emmaprp_probe(struct platform_device *pdev)

 	pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "ipg");
 	if (IS_ERR(pcdev->clk_emma_ipg)) {
-		ret = PTR_ERR(pcdev->clk_emma_ipg);
-		goto free_dev;
+		return PTR_ERR(pcdev->clk_emma_ipg);
 	}

 	pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "ahb");
 	if (IS_ERR(pcdev->clk_emma_ipg)) {
-		ret = PTR_ERR(pcdev->clk_emma_ahb);
-		goto free_dev;
+		return PTR_ERR(pcdev->clk_emma_ahb);
 	}

 	irq_emma = platform_get_irq(pdev, 0);
 	res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (irq_emma < 0 || res_emma == NULL) {
 		dev_err(&pdev->dev, "Missing platform resources data\n");
-		ret = -ENODEV;
-		goto free_dev;
+		return -ENODEV;
 	}

 	ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
 	if (ret)
-		goto free_dev;
+		return ret;

 	mutex_init(&pcdev->dev_mutex);

@@ -946,21 +943,20 @@ static int emmaprp_probe(struct platform_device *pdev)

 	platform_set_drvdata(pdev, pcdev);

-	if (devm_request_mem_region(&pdev->dev, res_emma->start,
-	    resource_size(res_emma), MEM2MEM_NAME) == NULL)
-		goto rel_vdev;
-
-	pcdev->base_emma = devm_ioremap(&pdev->dev, res_emma->start,
-					resource_size(res_emma));
-	if (!pcdev->base_emma)
+	pcdev->base_emma = devm_request_and_ioremap(&pdev->dev, res_emma);
+	if (!pcdev->base_emma) {
+		ret = -ENXIO;
 		goto rel_vdev;
+	}

 	pcdev->irq_emma = irq_emma;
 	pcdev->res_emma = res_emma;

 	if (devm_request_irq(&pdev->dev, pcdev->irq_emma, emmaprp_irq,
-			     0, MEM2MEM_NAME, pcdev) < 0)
+			     0, MEM2MEM_NAME, pcdev) < 0) {
+		ret = -ENODEV;
 		goto rel_vdev;
+	}

 	pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
 	if (IS_ERR(pcdev->alloc_ctx)) {
@@ -993,8 +989,6 @@ rel_vdev:
 	video_device_release(vfd);
 unreg_dev:
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-free_dev:
-	kfree(pcdev);

 	return ret;
 }
@@ -1009,7 +1003,6 @@ static int emmaprp_remove(struct platform_device *pdev)
 	v4l2_m2m_release(pcdev->m2m_dev);
 	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
 	v4l2_device_unregister(&pcdev->v4l2_dev);
-	kfree(pcdev);

 	return 0;
 }