* [PATCH] [media] v4l2-async: Always unregister the subdev on failure
@ 2016-05-11 15:40 Alban Bedel
2016-05-11 16:22 ` Javier Martinez Canillas
0 siblings, 1 reply; 5+ messages in thread
From: Alban Bedel @ 2016-05-11 15:40 UTC (permalink / raw)
To: linux-media
Cc: Mauro Carvalho Chehab, Javier Martinez Canillas, Sakari Ailus,
Bryan Wu, linux-kernel, Alban Bedel
In v4l2_async_test_notify() if the registered_async callback or the
complete notifier returns an error the subdev is not unregistered.
This leave paths where v4l2_async_register_subdev() can fail but
leave the subdev still registered.
Add the required calls to v4l2_device_unregister_subdev() to plug
these holes.
Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index ceb28d4..43393f8 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -121,13 +121,19 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
ret = v4l2_subdev_call(sd, core, registered_async);
if (ret < 0 && ret != -ENOIOCTLCMD) {
+ v4l2_device_unregister_subdev(sd);
if (notifier->unbind)
notifier->unbind(notifier, sd, asd);
return ret;
}
- if (list_empty(¬ifier->waiting) && notifier->complete)
- return notifier->complete(notifier);
+ if (list_empty(¬ifier->waiting) && notifier->complete) {
+ ret = notifier->complete(notifier);
+ if (ret < 0) {
+ v4l2_device_unregister_subdev(sd);
+ return ret;
+ }
+ }
return 0;
}
--
2.8.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
2016-05-11 15:40 [PATCH] [media] v4l2-async: Always unregister the subdev on failure Alban Bedel
@ 2016-05-11 16:22 ` Javier Martinez Canillas
2016-05-11 16:32 ` Alban Bedel
0 siblings, 1 reply; 5+ messages in thread
From: Javier Martinez Canillas @ 2016-05-11 16:22 UTC (permalink / raw)
To: Alban Bedel, linux-media
Cc: Mauro Carvalho Chehab, Sakari Ailus, Bryan Wu, linux-kernel
Hello Alban,
On 05/11/2016 11:40 AM, Alban Bedel wrote:
> In v4l2_async_test_notify() if the registered_async callback or the
> complete notifier returns an error the subdev is not unregistered.
> This leave paths where v4l2_async_register_subdev() can fail but
> leave the subdev still registered.
>
> Add the required calls to v4l2_device_unregister_subdev() to plug
> these holes.
>
> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index ceb28d4..43393f8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -121,13 +121,19 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>
> ret = v4l2_subdev_call(sd, core, registered_async);
> if (ret < 0 && ret != -ENOIOCTLCMD) {
> + v4l2_device_unregister_subdev(sd);
> if (notifier->unbind)
> notifier->unbind(notifier, sd, asd);
> return ret;
> }
>
> - if (list_empty(¬ifier->waiting) && notifier->complete)
> - return notifier->complete(notifier);
> + if (list_empty(¬ifier->waiting) && notifier->complete) {
> + ret = notifier->complete(notifier);
> + if (ret < 0) {
> + v4l2_device_unregister_subdev(sd);
Isn't a call to notifier->unbind() missing here as well?
Also, I think the error path is becoming too duplicated and complex, so
maybe we can have a single error path and use goto labels as is common
in Linux? For example something like the following (not tested) can be
squashed on top of your change:
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 43393f8c1312..abe512d0b4cb 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -113,29 +113,28 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
list_move(&sd->async_list, ¬ifier->done);
ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
- if (ret < 0) {
- if (notifier->unbind)
- notifier->unbind(notifier, sd, asd);
- return ret;
- }
+ if (ret < 0)
+ goto err_subdev_register;
ret = v4l2_subdev_call(sd, core, registered_async);
- if (ret < 0 && ret != -ENOIOCTLCMD) {
- v4l2_device_unregister_subdev(sd);
- if (notifier->unbind)
- notifier->unbind(notifier, sd, asd);
- return ret;
- }
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ goto err_subdev_call;
if (list_empty(¬ifier->waiting) && notifier->complete) {
ret = notifier->complete(notifier);
- if (ret < 0) {
- v4l2_device_unregister_subdev(sd);
- return ret;
- }
+ if (ret < 0)
+ goto err_subdev_call;
}
return 0;
+
+err_subdev_call:
+ v4l2_device_unregister_subdev(sd);
+err_subdev_register:
+ if (notifier->unbind)
+ notifier->unbind(notifier, sd, asd);
+
+ return ret;
}
static void v4l2_async_cleanup(struct v4l2_subdev *sd)
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
2016-05-11 16:22 ` Javier Martinez Canillas
@ 2016-05-11 16:32 ` Alban Bedel
2016-07-01 11:55 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Alban Bedel @ 2016-05-11 16:32 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Alban Bedel, linux-media, Mauro Carvalho Chehab, Sakari Ailus,
Bryan Wu, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]
On Wed, 11 May 2016 12:22:44 -0400
Javier Martinez Canillas <javier@osg.samsung.com> wrote:
> Hello Alban,
>
> On 05/11/2016 11:40 AM, Alban Bedel wrote:
> > In v4l2_async_test_notify() if the registered_async callback or the
> > complete notifier returns an error the subdev is not unregistered.
> > This leave paths where v4l2_async_register_subdev() can fail but
> > leave the subdev still registered.
> >
> > Add the required calls to v4l2_device_unregister_subdev() to plug
> > these holes.
> >
> > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index ceb28d4..43393f8 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -121,13 +121,19 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> >
> > ret = v4l2_subdev_call(sd, core, registered_async);
> > if (ret < 0 && ret != -ENOIOCTLCMD) {
> > + v4l2_device_unregister_subdev(sd);
> > if (notifier->unbind)
> > notifier->unbind(notifier, sd, asd);
> > return ret;
> > }
> >
> > - if (list_empty(¬ifier->waiting) && notifier->complete)
> > - return notifier->complete(notifier);
> > + if (list_empty(¬ifier->waiting) && notifier->complete) {
> > + ret = notifier->complete(notifier);
> > + if (ret < 0) {
> > + v4l2_device_unregister_subdev(sd);
>
> Isn't a call to notifier->unbind() missing here as well?
>
> Also, I think the error path is becoming too duplicated and complex, so
> maybe we can have a single error path and use goto labels as is common
> in Linux? For example something like the following (not tested) can be
> squashed on top of your change:
Yes, that look better. I'll test it and report tomorrow.
Alban
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
2016-05-11 16:32 ` Alban Bedel
@ 2016-07-01 11:55 ` Hans Verkuil
2016-08-24 13:49 ` Alban Bedel
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2016-07-01 11:55 UTC (permalink / raw)
To: Alban Bedel, Javier Martinez Canillas
Cc: linux-media, Mauro Carvalho Chehab, Sakari Ailus, Bryan Wu, linux-kernel
On 05/11/2016 06:32 PM, Alban Bedel wrote:
> On Wed, 11 May 2016 12:22:44 -0400
> Javier Martinez Canillas <javier@osg.samsung.com> wrote:
>
>> Hello Alban,
>>
>> On 05/11/2016 11:40 AM, Alban Bedel wrote:
>>> In v4l2_async_test_notify() if the registered_async callback or the
>>> complete notifier returns an error the subdev is not unregistered.
>>> This leave paths where v4l2_async_register_subdev() can fail but
>>> leave the subdev still registered.
>>>
>>> Add the required calls to v4l2_device_unregister_subdev() to plug
>>> these holes.
>>>
>>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
>>> ---
>>> drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index ceb28d4..43393f8 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -121,13 +121,19 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>>>
>>> ret = v4l2_subdev_call(sd, core, registered_async);
>>> if (ret < 0 && ret != -ENOIOCTLCMD) {
>>> + v4l2_device_unregister_subdev(sd);
>>> if (notifier->unbind)
>>> notifier->unbind(notifier, sd, asd);
>>> return ret;
>>> }
>>>
>>> - if (list_empty(¬ifier->waiting) && notifier->complete)
>>> - return notifier->complete(notifier);
>>> + if (list_empty(¬ifier->waiting) && notifier->complete) {
>>> + ret = notifier->complete(notifier);
>>> + if (ret < 0) {
>>> + v4l2_device_unregister_subdev(sd);
>>
>> Isn't a call to notifier->unbind() missing here as well?
>>
>> Also, I think the error path is becoming too duplicated and complex, so
>> maybe we can have a single error path and use goto labels as is common
>> in Linux? For example something like the following (not tested) can be
>> squashed on top of your change:
>
> Yes, that look better. I'll test it and report tomorrow.
I haven't heard anything back about this. Did you manage to test it?
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
2016-07-01 11:55 ` Hans Verkuil
@ 2016-08-24 13:49 ` Alban Bedel
0 siblings, 0 replies; 5+ messages in thread
From: Alban Bedel @ 2016-08-24 13:49 UTC (permalink / raw)
To: Hans Verkuil
Cc: Alban Bedel, Javier Martinez Canillas, linux-media,
Mauro Carvalho Chehab, Sakari Ailus, Bryan Wu, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2382 bytes --]
On Fri, 1 Jul 2016 13:55:44 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 05/11/2016 06:32 PM, Alban Bedel wrote:
> > On Wed, 11 May 2016 12:22:44 -0400
> > Javier Martinez Canillas <javier@osg.samsung.com> wrote:
> >
> >> Hello Alban,
> >>
> >> On 05/11/2016 11:40 AM, Alban Bedel wrote:
> >>> In v4l2_async_test_notify() if the registered_async callback or the
> >>> complete notifier returns an error the subdev is not unregistered.
> >>> This leave paths where v4l2_async_register_subdev() can fail but
> >>> leave the subdev still registered.
> >>>
> >>> Add the required calls to v4l2_device_unregister_subdev() to plug
> >>> these holes.
> >>>
> >>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> >>> ---
> >>> drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++--
> >>> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>> index ceb28d4..43393f8 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>> @@ -121,13 +121,19 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> >>>
> >>> ret = v4l2_subdev_call(sd, core, registered_async);
> >>> if (ret < 0 && ret != -ENOIOCTLCMD) {
> >>> + v4l2_device_unregister_subdev(sd);
> >>> if (notifier->unbind)
> >>> notifier->unbind(notifier, sd, asd);
> >>> return ret;
> >>> }
> >>>
> >>> - if (list_empty(¬ifier->waiting) && notifier->complete)
> >>> - return notifier->complete(notifier);
> >>> + if (list_empty(¬ifier->waiting) && notifier->complete) {
> >>> + ret = notifier->complete(notifier);
> >>> + if (ret < 0) {
> >>> + v4l2_device_unregister_subdev(sd);
> >>
> >> Isn't a call to notifier->unbind() missing here as well?
> >>
> >> Also, I think the error path is becoming too duplicated and complex, so
> >> maybe we can have a single error path and use goto labels as is common
> >> in Linux? For example something like the following (not tested) can be
> >> squashed on top of your change:
> >
> > Yes, that look better. I'll test it and report tomorrow.
>
> I haven't heard anything back about this. Did you manage to test it?
Yes, that's working fine. Sorry for the delay, I'm sending the v2 patch.
Alban
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-24 13:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-11 15:40 [PATCH] [media] v4l2-async: Always unregister the subdev on failure Alban Bedel
2016-05-11 16:22 ` Javier Martinez Canillas
2016-05-11 16:32 ` Alban Bedel
2016-07-01 11:55 ` Hans Verkuil
2016-08-24 13:49 ` Alban Bedel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).