On Fri 17 May 2013 00:34:51 Sakari Ailus wrote:
> Hi Andrzej,
>
> Thanks for the patchset!
>
> On Thu, May 16, 2013 at 10:14:33AM +0200, Andrzej Hajda wrote:
> > This patch adds managed version of initialization
> > function for v4l2 control handler.
> >
> > Signed-off-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > Reviewed-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > ---
> > v3:
> > - removed managed cleanup
> > v2:
> > - added missing struct device forward declaration,
> > - corrected few comments
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls.c | 32 ++++++++++++++++++++++++++++++++
> > include/media/v4l2-ctrls.h | 16 ++++++++++++++++
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index ebb8e48..f47ccfa 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1421,6 +1421,38 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> > }
> > EXPORT_SYMBOL(v4l2_ctrl_handler_free);
> >
> > +static void devm_v4l2_ctrl_handler_release(struct device *dev, void *res)
> > +{
> > + struct v4l2_ctrl_handler **hdl = res;
> > +
> > + v4l2_ctrl_handler_free(*hdl);
>
> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
> existence of hdl. By default hdl->lock is in the handler, but it may also be
> elsewhere, e.g. in a driver-specific device struct such as struct
> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
> anything guarantees that hdl->mutex still exists at the time the device is
> removed.
If it is a driver-managed lock, then the driver should also be responsible for
that lock during the life-time of the control handler. I think that is a fair
assumption.
> I have to say I don't think it's neither meaningful to acquire that mutex in
> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
> anyway: reference counting would be needed to prevent bad things from
> happening, in case the drivers wouldn't take care of that.
It's probably not meaningful today, but it might become meaningful in the
future. And in any case, not taking the lock when manipulating internal
lists is very unexpected even though it might work with today's use cases.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
