diff mbox

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

Message ID alpine.DEB.2.02.1208132219060.2355@localhost6.localdomain6 (mailing list archive)
State New, archived
Headers show

Commit Message

Julia Lawall Aug. 13, 2012, 8:20 p.m. UTC
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(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mauro Carvalho Chehab Aug. 14, 2012, 1:40 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall Aug. 14, 2012, 6:30 a.m. UTC | #2
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Aug. 14, 2012, 9:54 a.m. UTC | #3
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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