| Message ID | 20210107225458.4485-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-async: Safely unregister an non-registered async subdev | expand |
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); >
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.
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
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? :-)
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);