Hi Sakari,
Thanks for the review.
On 17.05.2013 00:34, 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.
IMO in general if somebody provides custom mutex he becomes responsible
for validity of that mutex during v4l2_ctrl_handler usage.
In the particular case of smiapp if we replace v4l2_ctrl_handler_init with
devm version and remove v4l2_ctrl_handler_free calls it seems to be OK:
- mutex is in devm allocated memory chunk acquired at the beginning of
probe,
- mutex is initialized before devm_v4l2_ctrl_handler_init,
- there is no mutex_destroy call - ie the mutex is valid until the
memory is freed,
- memory free is called after v4l2_ctrl_handler_free - devm
'destructors' are
called in order reverse to devm_* 'constructor' calls.
Anyway in cases when devm_* usage would cause errors
we can still use non devm_* versions.
>
> 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.
I do not understand what do you mean exactly. Could you please explain
it more?
What do you want to reference count?
Regards
Andrzej
>
>> +}
>> +
--
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
