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@xxxxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxx>
>
> 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
