[PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Javier Martinez Canillas]

Hello Mauro,

On Wed, Mar 16, 2016 at 11:36 AM, Mauro Carvalho Chehab
<mchehab at osg.samsung.com> wrote:
> Em Wed, 16 Mar 2016 11:03:38 -0300
> Javier Martinez Canillas <javier at dowhile0.org> escreveu:
>
>> Hello Mauro,
>>
>> The patch looks mostly good to me, I just have a question below:
>>
>> On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
>> <mchehab at osg.samsung.com> wrote:
>>
>> [snip]
>>
>> >
>> > -void media_device_cleanup(struct media_device *mdev)
>> > +static void media_device_cleanup(struct media_device *mdev)
>> >  {
>> >         ida_destroy(&mdev->entity_internal_idx);
>> >         mdev->entity_internal_idx_max = 0;
>> >         media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
>> > -       mutex_destroy(&mdev->graph_mutex);
>>
>> Any reason why this is removed? mutex_init() is still called in
>> media_device_init() so I believe the destroy should be kept here.
>
> This code is now called only at do_media_device_unregister, with
> is protected by the mutex. If we keep the mutex_destroy there,
> the mutex will be destroyed in lock state, causing an error if
> mutex debug is enabled.
>
> Btw, the standard implementation for the mutex is:
>         include/linux/mutex.h:static inline void mutex_destroy(struct mutex *lock) {}
>
> Only when mutex debug is enabled, it becomes something else:
>         include/linux/mutex-debug.h:extern void mutex_destroy(struct mutex *lock);
>
> With produces a warning if the mutex_destroy() is called with the
> mutex is hold. This can never happen with the current implementation, as
> the logic is:
>         mutex_lock(&mdev->graph_mutex);
>         kref_put(&mdev->kref, do_media_device_unregister);
>         mutex_unlock(&mdev->graph_mutex);
>
> We could eventually move the mutex lock to
> do_media_device_unregister() and then add there the mutex_destroy()
>

I see, thanks a lot for your explanation.

> Regards,
> Mauro
>

Best regards,
Javier