[PATCH] media: v4l2-async: Safely unregister an non-registered async subdev

[PATCH] media: v4l2-async: Safely unregister an non-registered async subdev

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Make the V4L2 async framework a bit more robust by allowing to
unregister a non-registered async subdev. Otherwise the
v4l2_async_cleanup() will attempt to delete the async subdev from the
subdev_list with the corresponding list_head not initialized.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 8bde33c21ce4..fc4525c4a75f 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -750,6 +750,9 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
 
 void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 {
+	if (!sd->async_list.next)
+		return;
+
 	mutex_lock(&list_lock);
 
 	__v4l2_async_notifier_unregister(sd->subdev_notifier);
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Safely unregister an non-registered async subdev

[Kieran Bingham]

Hi Laurent,

On 07/01/2021 22:54, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Make the V4L2 async framework a bit more robust by allowing to
> unregister a non-registered async subdev. Otherwise the
> v4l2_async_cleanup() will attempt to delete the async subdev from the
> subdev_list with the corresponding list_head not initialized.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce4..fc4525c4a75f 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -750,6 +750,9 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
>  
>  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  {
> +	if (!sd->async_list.next)
> +		return;

This is a bit opaque for anyone reading the code alone.

It could easily read as:

"If we don't have a following item in the async list - then don't
unregister?", which seems a bit nonsensical.

Hopefully that would make someone question what it's actually checking
but still.

I think I've seen you reference this pattern a couple of times so
perhaps having a way to check if a list is initialised would be worth
having as a helper in the list.

Otherwise, at least a comment to say that we're using the initialisation
of the list to determine if the async subdevice is already registered or
not. (perhaps a bit more briefly ;D)


Anyway, with that all in mind - I always like being able to simplify
error and clean up paths, so

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> +
>  	mutex_lock(&list_lock);
>  
>  	__v4l2_async_notifier_unregister(sd->subdev_notifier);
> 

Re: [PATCH] media: v4l2-async: Safely unregister an non-registered async subdev

[Sakari Ailus]

Hi Kieran,

On Wed, Jan 20, 2021 at 11:14:39AM +0000, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 07/01/2021 22:54, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Make the V4L2 async framework a bit more robust by allowing to
> > unregister a non-registered async subdev. Otherwise the
> > v4l2_async_cleanup() will attempt to delete the async subdev from the
> > subdev_list with the corresponding list_head not initialized.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 8bde33c21ce4..fc4525c4a75f 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -750,6 +750,9 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
> >  
> >  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
> >  {
> > +	if (!sd->async_list.next)
> > +		return;
> 
> This is a bit opaque for anyone reading the code alone.
> 
> It could easily read as:
> 
> "If we don't have a following item in the async list - then don't
> unregister?", which seems a bit nonsensical.
> 
> Hopefully that would make someone question what it's actually checking
> but still.
> 
> I think I've seen you reference this pattern a couple of times so
> perhaps having a way to check if a list is initialised would be worth
> having as a helper in the list.
> 
> Otherwise, at least a comment to say that we're using the initialisation
> of the list to determine if the async subdevice is already registered or
> not. (perhaps a bit more briefly ;D)
> 
> 
> Anyway, with that all in mind - I always like being able to simplify
> error and clean up paths, so
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks.

I think the patch is good as-is but I wouldn't mind to see such a list
helper either. V4L2-async could later on use it.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: v4l2-async: Safely unregister an non-registered async subdev

[Kieran Bingham]

On 20/01/2021 13:37, Sakari Ailus wrote:
> Hi Kieran,
> 
> On Wed, Jan 20, 2021 at 11:14:39AM +0000, Kieran Bingham wrote:
>> Hi Laurent,
>>
>> On 07/01/2021 22:54, Laurent Pinchart wrote:
>>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>
>>> Make the V4L2 async framework a bit more robust by allowing to
>>> unregister a non-registered async subdev. Otherwise the
>>> v4l2_async_cleanup() will attempt to delete the async subdev from the
>>> subdev_list with the corresponding list_head not initialized.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-async.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index 8bde33c21ce4..fc4525c4a75f 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -750,6 +750,9 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
>>>  
>>>  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>>  {
>>> +	if (!sd->async_list.next)
>>> +		return;
>>
>> This is a bit opaque for anyone reading the code alone.
>>
>> It could easily read as:
>>
>> "If we don't have a following item in the async list - then don't
>> unregister?", which seems a bit nonsensical.
>>
>> Hopefully that would make someone question what it's actually checking
>> but still.
>>
>> I think I've seen you reference this pattern a couple of times so
>> perhaps having a way to check if a list is initialised would be worth
>> having as a helper in the list.
>>
>> Otherwise, at least a comment to say that we're using the initialisation
>> of the list to determine if the async subdevice is already registered or
>> not. (perhaps a bit more briefly ;D)
>>
>>
>> Anyway, with that all in mind - I always like being able to simplify
>> error and clean up paths, so
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Thanks.
> 
> I think the patch is good as-is but I wouldn't mind to see such a list
> helper either. V4L2-async could later on use it.

Yes, I don't think a list-helper is 'required' for this patch (though a
comment would be nice otherwise as described above).

Checking an internal object's list's next pointer to determine if the
object is already registered is quite opaque. That was my only concern.

--
Kieran

Re: [PATCH] media: v4l2-async: Safely unregister an non-registered async subdev

[Sakari Ailus]

On Wed, Jan 20, 2021 at 02:02:00PM +0000, Kieran Bingham wrote:
> On 20/01/2021 13:37, Sakari Ailus wrote:
> > Hi Kieran,
> > 
> > On Wed, Jan 20, 2021 at 11:14:39AM +0000, Kieran Bingham wrote:
> >> Hi Laurent,
> >>
> >> On 07/01/2021 22:54, Laurent Pinchart wrote:
> >>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>>
> >>> Make the V4L2 async framework a bit more robust by allowing to
> >>> unregister a non-registered async subdev. Otherwise the
> >>> v4l2_async_cleanup() will attempt to delete the async subdev from the
> >>> subdev_list with the corresponding list_head not initialized.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>> index 8bde33c21ce4..fc4525c4a75f 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>> @@ -750,6 +750,9 @@ EXPORT_SYMBOL(v4l2_async_register_subdev);
> >>>  
> >>>  void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
> >>>  {
> >>> +	if (!sd->async_list.next)
> >>> +		return;
> >>
> >> This is a bit opaque for anyone reading the code alone.
> >>
> >> It could easily read as:
> >>
> >> "If we don't have a following item in the async list - then don't
> >> unregister?", which seems a bit nonsensical.
> >>
> >> Hopefully that would make someone question what it's actually checking
> >> but still.
> >>
> >> I think I've seen you reference this pattern a couple of times so
> >> perhaps having a way to check if a list is initialised would be worth
> >> having as a helper in the list.
> >>
> >> Otherwise, at least a comment to say that we're using the initialisation
> >> of the list to determine if the async subdevice is already registered or
> >> not. (perhaps a bit more briefly ;D)
> >>
> >>
> >> Anyway, with that all in mind - I always like being able to simplify
> >> error and clean up paths, so
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > Thanks.
> > 
> > I think the patch is good as-is but I wouldn't mind to see such a list
> > helper either. V4L2-async could later on use it.
> 
> Yes, I don't think a list-helper is 'required' for this patch (though a
> comment would be nice otherwise as described above).
> 
> Checking an internal object's list's next pointer to determine if the
> object is already registered is quite opaque. That was my only concern.

I also have to say I don't like poking list internal data structures. There
are just no other struct members that could tell the same bit of
information, and as we do have the list, there's no reason to add a new
field for the purpose either.

Who would like to submit the patch to add a function telling whether a list
is initialised? :-)

-- 
Sakari Ailus