Hi Hans,
Thank you for the patch.
On Thu, Mar 17, 2022 at 01:56:08PM +0100, Hans Verkuil wrote:
> The v4l2_ctrl_handler_free() function must unlink all subscribed events
> of the control handler that is being freed, but it only did so for the
> controls owned by that control handler and not for the controls referred
> to by that control handler. This leaves stale data in the ev_subs list
> of those controls.
>
> The node list header is also properly initialized and list_del_init is
> called instead of list_del to ensure that list_empty() can be used
> to detect whether a v4l2_subscribed_event is part of a list or not.
>
> This makes v4l2_ctrl_del_event() more robust since it will not attempt
> to find the control if the v4l2_subscribed_event has already been unlinked
> from the control.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-ctrls-api.c | 7 +++++--
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 6 +++++-
> drivers/media/v4l2-core/v4l2-event.c | 1 +
> 3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index db9baa0bd05f..d64b22cb182c 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -1159,13 +1159,16 @@ static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev,
>
> static void v4l2_ctrl_del_event(struct v4l2_subscribed_event *sev)
> {
> - struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
> + struct v4l2_ctrl *ctrl = NULL;
> +
> + if (!list_empty(&sev->node))
> + ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
>
> if (!ctrl)
> return;
I'd go for
if (list_empty(&sdev->node))
return;
ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
if (!ctrl)
return;
and drop the initialization of ctrl to NULL, it's easier to read.
>
> v4l2_ctrl_lock(ctrl);
v4l2_ctrl_lock() simply calls
mutex_lock(ctrl->handler->lock);
Is there any case where ctrl->handler != sev->fh->ctrl_handler ? If not
this could be simplified to drop the v4l2_ctrl_find() call.
> - list_del(&sev->node);
> + list_del_init(&sev->node);
> v4l2_ctrl_unlock(ctrl);
> }
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index 8968cec8454e..1a9d60cb119c 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -10,6 +10,7 @@
> #include <linux/slab.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-event.h>
> +#include <media/v4l2-fh.h>
> #include <media/v4l2-fwnode.h>
>
> #include "v4l2-ctrls-priv.h"
> @@ -1165,13 +1166,16 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> /* Free all nodes */
> list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs, node) {
> list_del(&ref->node);
> + list_for_each_entry_safe(sev, next_sev, &ref->ctrl->ev_subs, node)
> + if (sev->fh->ctrl_handler == hdl)
> + list_del_init(&sev->node);
I have no idea how control reference works or what they're used for, so
I can't tell if this is safe without locking ref->ctrl.
> kfree(ref);
> }
> /* Free all controls owned by the handler */
> list_for_each_entry_safe(ctrl, next_ctrl, &hdl->ctrls, node) {
> list_del(&ctrl->node);
> list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
> - list_del(&sev->node);
> + list_del_init(&sev->node);
> kvfree(ctrl);
> }
> kvfree(hdl->buckets);
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index c5ce9f11ad7b..2fd9f7187c04 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -246,6 +246,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->fh = fh;
> sev->ops = ops;
> sev->elems = elems;
> + INIT_LIST_HEAD(&sev->node);
>
> mutex_lock(&fh->subscribe_lock);
--
Regards,
Laurent Pinchart
