* [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