* [patch] [media] dt3155v4l: unlock on error path
@ 2013-04-09 5:15 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-04-09 5:15 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Bill Pemberton, Hans Verkuil, Lad, Prabhakar, linux-media,
kernel-janitors
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 <dan.carpenter@oracle.com>
diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
index 073b3b3..3da17bc 100644
--- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
+++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
@@ -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:
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [patch] [media] dt3155v4l: unlock on error path
@ 2013-04-09 5:15 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-04-09 5:15 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Bill Pemberton, Hans Verkuil, Lad, Prabhakar, linux-media,
kernel-janitors
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 <dan.carpenter@oracle.com>
diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
index 073b3b3..3da17bc 100644
--- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
+++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
@@ -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:
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [patch] [media] dt3155v4l: unlock on error path
2013-04-09 5:15 ` Dan Carpenter
@ 2013-04-09 5:20 ` Julia Lawall
-1 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2013-04-09 5:20 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Bill Pemberton, Hans Verkuil,
Lad, Prabhakar, linux-media, kernel-janitors
On Tue, 9 Apr 2013, 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.
Why keep the return path then? If the code is there, someone reading it
could naturally assume that it is necessary.
julia
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> index 073b3b3..3da17bc 100644
> --- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
> +++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] [media] dt3155v4l: unlock on error path
@ 2013-04-09 5:20 ` Julia Lawall
0 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2013-04-09 5:20 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Bill Pemberton, Hans Verkuil,
Lad, Prabhakar, linux-media, kernel-janitors
On Tue, 9 Apr 2013, 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.
Why keep the return path then? If the code is there, someone reading it
could naturally assume that it is necessary.
julia
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> index 073b3b3..3da17bc 100644
> --- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
> +++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] [media] dt3155v4l: unlock on error path
2013-04-09 5:15 ` Dan Carpenter
@ 2013-04-09 6:14 ` Hans Verkuil
-1 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2013-04-09 6:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Bill Pemberton, Hans Verkuil,
Lad, Prabhakar, linux-media, kernel-janitors
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 <dan.carpenter@oracle.com>
Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
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
>
> diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> index 073b3b3..3da17bc 100644
> --- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
> +++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] [media] dt3155v4l: unlock on error path
@ 2013-04-09 6:14 ` Hans Verkuil
0 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2013-04-09 6:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Bill Pemberton, Hans Verkuil,
Lad, Prabhakar, linux-media, kernel-janitors
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 <dan.carpenter@oracle.com>
Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
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
>
> diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> index 073b3b3..3da17bc 100644
> --- a/drivers/staging/media/dt3155v4l/dt3155v4l.c
> +++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] [media] dt3155v4l: unlock on error path
2013-04-09 5:20 ` Julia Lawall
@ 2013-04-09 6:28 ` Dan Carpenter
-1 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-04-09 6:28 UTC (permalink / raw)
To: Julia Lawall
Cc: Mauro Carvalho Chehab, Bill Pemberton, Hans Verkuil,
Lad, Prabhakar, linux-media, kernel-janitors
On Tue, Apr 09, 2013 at 07:20:19AM +0200, Julia Lawall wrote:
> On Tue, 9 Apr 2013, 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.
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] [media] dt3155v4l: unlock on error path
@ 2013-04-09 6:28 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-04-09 6:28 UTC (permalink / raw)
To: Julia Lawall
Cc: Mauro Carvalho Chehab, Bill Pemberton, Hans Verkuil,
Lad, Prabhakar, linux-media, kernel-janitors
On Tue, Apr 09, 2013 at 07:20:19AM +0200, Julia Lawall wrote:
> On Tue, 9 Apr 2013, 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.
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-04-09 6:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-09 5:15 [patch] [media] dt3155v4l: unlock on error path Dan Carpenter
2013-04-09 5:15 ` Dan Carpenter
2013-04-09 5:20 ` Julia Lawall
2013-04-09 5:20 ` Julia Lawall
2013-04-09 6:28 ` Dan Carpenter
2013-04-09 6:28 ` Dan Carpenter
2013-04-09 6:14 ` Hans Verkuil
2013-04-09 6:14 ` Hans Verkuil
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.