Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure

[Alban Bedel]

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(>waiting) && notifier->complete)
> >>> - return notifier->complete(notifier);
> >>> + if (list_empty(>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

[Hans Verkuil]

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(>waiting) && notifier->complete)
>>> -   return notifier->complete(notifier);
>>> +   if (list_empty(>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

[Alban Bedel]

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(>waiting) && notifier->complete)
> > -   return notifier->complete(notifier);
> > +   if (list_empty(>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

[Javier Martinez Canillas]

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(>waiting) && notifier->complete)
> - return notifier->complete(notifier);
> + if (list_empty(>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(>async_list, >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(>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

[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 
---
 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(>waiting) && notifier->complete)
-   return notifier->complete(notifier);
+   if (list_empty(>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