dt3155v4l: unlock on error path

PATCH NAK Dan Carpenter 3 participants 4 messages Apr 9, 2013

Subsystems (3)

Media (V4L/DVB) Staging Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2013-04-09 05:15 UTC #0 lore raw

PATCH 1/1 dt3155v4l: unlock on error path

We should unlock here and do some cleanup before returning.

We can't actually hit this return path with the current code, so this patch is a basically a cleanup and doesn't change how the code works.

Signed-off-by Dan Carpenter <[email protected]>

drivers/staging/media/dt3155v4l/dt3155v4l.c +3 -3

index 073b3b3..3da17bc 100644
@@ -398,7 +398,7 @@ dt3155_open(struct file *filp)
 		pd->field_count = 0;
 		ret = vb2_queue_init(pd->q);
 		if (ret < 0)
-			return ret;
+			goto err_free_q;
 		INIT_LIST_HEAD(&pd->dmaq);
 		spin_lock_init(&pd->lock);
 		/* disable all irqs, clear all irq flags */

@@ -407,11 +407,11 @@ dt3155_open(struct file *filp)
 		ret = request_irq(pd->pdev->irq, dt3155_irq_handler_even,
 						IRQF_SHARED, DT3155_NAME, pd);
 		if (ret)
-			goto err_request_irq;
+			goto err_free_q;
 	}
 	pd->users++;
 	return 0; /* success */
-err_request_irq:
+err_free_q:
 	kfree(pd->q);
 	pd->q = NULL;
 err_alloc_queue:

Julia Lawall

2013-04-09 05:20 UTC #1 lore raw

Quoted 4 lines · Dan Carpenter

We should unlock here and do some cleanup before returning.
We can't actually hit this return path with the current code, so this patch is a basically a cleanup and doesn't change how the code works.

Why keep the return path then? If the code is there, someone reading it could naturally assume that it is necessary.

julia

Quoted 35 lines

Signed-off-by Dan Carpenter <[email protected]>

drivers/staging/media/dt3155v4l/dt3155v4l.c +3 -4

index 073b3b3..3da17bc 100644
@@ -398,7 +398,7 @@ dt3155_open(struct file *filp)
 		pd->field_count = 0;
 		ret = vb2_queue_init(pd->q);
 		if (ret < 0)
-			return ret;
+			goto err_free_q;
 		INIT_LIST_HEAD(&pd->dmaq);
 		spin_lock_init(&pd->lock);
 		/* disable all irqs, clear all irq flags */

@@ -407,11 +407,11 @@ dt3155_open(struct file *filp)
 		ret = request_irq(pd->pdev->irq, dt3155_irq_handler_even,
 						IRQF_SHARED, DT3155_NAME, pd);
 		if (ret)
-			goto err_request_irq;
+			goto err_free_q;
 	}
 	pd->users++;
 	return 0; /* success */
-err_request_irq:
+err_free_q:
 	kfree(pd->q);
 	pd->q = NULL;
 err_alloc_queue:
--

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2013-04-09 06:28 UTC #2 lore raw

Quoted 9 lines · Julia Lawall

Quoted 4 lines · Dan Carpenter
We should unlock here and do some cleanup before returning.
We can't actually hit this return path with the current code, so this patch is a basically a cleanup and doesn't change how the code works.
Why keep the return path then? If the code is there, someone reading it could naturally assume that it is necessary.

There are sanity checks in vb2_queue_init() but this caller always passes a sane "pd->q" pointer so the sanity checks always succeed. That might change in later code. ;)

I like the code as it is, but I just wanted to note that the impact of this patch is zero in case anyone was backporting fixes.

regards,
dan carpenter

Hans Verkuil

Nacked-by on v1 1/1 NAK

2013-04-09 06:14 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 6 lines · On Tue April 9 2013 07:15:40 Dan Carpenter wrote:

We should unlock here and do some cleanup before returning.
We can't actually hit this return path with the current code, so this patch is a basically a cleanup and doesn't change how the code works.
Signed-off-by Dan Carpenter <[email protected]>

Nacked-by Hans Verkuil <[email protected]>

I have a patch pending that will actually fix the locking problem
(http://git.linuxtv.org/hverkuil/media_tree.git/commit/b7926f8ac185fe914a5a97d0206d7a3e8eb3e688)

I plan on working some more on this driver on Friday or the weekend and post a full patch series of fixes and improvements. I recently managed to get hold of a dt3155 card, so I can actually test the code.

Regards,

Hans

Quoted 33 lines

drivers/staging/media/dt3155v4l/dt3155v4l.c +3 -4

index 073b3b3..3da17bc 100644
@@ -398,7 +398,7 @@ dt3155_open(struct file *filp)
 		pd->field_count = 0;
 		ret = vb2_queue_init(pd->q);
 		if (ret < 0)
-			return ret;
+			goto err_free_q;
 		INIT_LIST_HEAD(&pd->dmaq);
 		spin_lock_init(&pd->lock);
 		/* disable all irqs, clear all irq flags */

@@ -407,11 +407,11 @@ dt3155_open(struct file *filp)
 		ret = request_irq(pd->pdev->irq, dt3155_irq_handler_even,
 						IRQF_SHARED, DT3155_NAME, pd);
 		if (ret)
-			goto err_request_irq;
+			goto err_free_q;
 	}
 	pd->users++;
 	return 0; /* success */
-err_request_irq:
+err_free_q:
 	kfree(pd->q);
 	pd->q = NULL;
 err_alloc_queue:
--

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