On 03/16/2016 08:25 AM, Mauro Carvalho Chehab wrote:
> Em Wed, 16 Mar 2016 08:05:15 -0600
> Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
>
>> On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
>>> Now that the media_device can be used by multiple drivers,
>>> via devres, we need to be sure that it will be dropped only
>>> when all drivers stop using it.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
>>> ---
>>> drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
>>> include/media/media-device.h | 3 +++
>>> 2 files changed, 37 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index c32fa15cc76e..38e6c319fe6e 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>>> {
>>> int ret;
>>>
>>> + /* Check if mdev was ever registered at all */
>>> + mutex_lock(&mdev->graph_mutex);
>>> + if (media_devnode_is_registered(&mdev->devnode)) {
>>> + kref_get(&mdev->kref);
>>> + mutex_unlock(&mdev->graph_mutex);
>>> + return 0;
>>> + }
>>> + kref_init(&mdev->kref);
>>> +
>>> /* Register the device node. */
>>> mdev->devnode.fops = &media_device_fops;
>>> mdev->devnode.parent = mdev->dev;
>>> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
>>> mdev->topology_version = 0;
>>>
>>> ret = media_devnode_register(&mdev->devnode, owner);
>>> - if (ret < 0)
>>> + if (ret < 0) {
>>> + media_devnode_unregister(&mdev->devnode);
>>> return ret;
>>> + }
>>>
>>> ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>>> if (ret < 0) {
>>> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>>> return ret;
>>> }
>>>
>>> + mutex_unlock(&mdev->graph_mutex);
>>> dev_dbg(mdev->dev, "Media device registered\n");
>>>
>>> return 0;
>>> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
>>> }
>>> EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>>>
>>> -void media_device_unregister(struct media_device *mdev)
>>> +static void struct kref *kref)
>>> {
>>> + struct media_device *mdev;
>>> struct media_entity *entity;
>>> struct media_entity *next;
>>> struct media_interface *intf, *tmp_intf;
>>> struct media_entity_notify *notify, *nextp;
>>>
>>> - if (mdev == NULL)
>>> - return;
>>> -
>>> - mutex_lock(&mdev->graph_mutex);
>>> -
>>> - /* Check if mdev was ever registered at all */
>>> - if (!media_devnode_is_registered(&mdev->devnode)) {
>>> - mutex_unlock(&mdev->graph_mutex);
>>> - return;
>>> - }
>>> + mdev = container_of(kref, struct media_device, kref);
>>>
>>> /* Remove all entities from the media device */
>>> list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>>> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
>>> kfree(intf);
>>> }
>>>
>>> - mutex_unlock(&mdev->graph_mutex);
>>> + /* Check if mdev devnode was registered */
>>> + if (!media_devnode_is_registered(&mdev->devnode))
>>> + return;
>>>
>>> device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>>> media_devnode_unregister(&mdev->devnode);
>>
>> Patch looks good.
>>
>> Reviewed-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
>>
>> Please see a few comments below that aren't related to this patch.
>>
>> The above is unprotected and could be done twice when two drivers
>> call media_device_unregister(). I think we still mark the media
>> device unregistered in media_devnode_unregister(). We have to
>> protect these two steps still.
>>
>> I attempted to do this with a unregister in progress flag which
>> gets set at the beginning in media_device_unregister(). That
>> does ensure media_device_unregister() runs only once. If that
>> approach isn't desirable, we have to find another way.
>
> Do you mean do_media_device_unregister()? This is protected, as
> this function is only called via media_device_unregister(),
> with the mutex hold. I opted to take the mutex there, as
> it makes the return code simpler.
>
The below two steps are my concern. With the mutex changes in
do_media_device_unregister() closed critical windows, however
the below code path still concerns me.
device_remove_file(&mdev->devnode.dev, &dev_attr_model);
media_devnode_unregister(&mdev->devnode);
Especially since we do the clear MEDIA_FLAG_REGISTERED in
media_devnode_unregister(). This step is done while holding
media_devnode_lock - a different mutex
We rely on media_devnode_is_registered() check to determine
whether to start unregister or not. Hence, there is a window
where, we could potentially try to do the following twice:
device_remove_file(&mdev->devnode.dev, &dev_attr_model);
media_devnode_unregister(&mdev->devnode);
You will see this only when both au0828 and snd-usb-audio
try to unregister()
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978
--
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
