| Message ID | 429cc087-85e3-7bfa-b0b6-ab9434e5d47c@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- 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
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
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)