Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
On Fri, 1 Jul 2016 13:55:44 +0200 Hans Verkuil wrote: > On 05/11/2016 06:32 PM, Alban Bedel wrote: > > On Wed, 11 May 2016 12:22:44 -0400 > > Javier Martinez Canillas 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 > >>> --- > >>> 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 pgpigT3AWV8I_.pgp Description: OpenPGP digital signature
Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
On 05/11/2016 06:32 PM, Alban Bedel wrote: > On Wed, 11 May 2016 12:22:44 -0400 > Javier Martinez Canillas 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 >>> --- >>> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
On Wed, 11 May 2016 12:22:44 -0400 Javier Martinez Canillas 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 > > --- > > 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 signature.asc Description: PGP signature
Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure
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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] v4l2-async: Always unregister the subdev on failure
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html