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.
--
Regards,
Sakari Ailus
