[4/5,media] media-device: use kref for media_device instance
diff mbox

Message ID 82ef082c4de7c0a1c546da1d9e462bc86ab423bf.1458129823.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab March 16, 2016, 12:04 p.m. UTC
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@osg.samsung.com>
---
 drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
 include/media/media-device.h |  3 +++
 2 files changed, 37 insertions(+), 14 deletions(-)

Comments

Javier Martinez Canillas March 16, 2016, 1:23 p.m. UTC | #1
Hello Mauro,

Patch looks almost good to me, I just have a question below:

On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> 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@osg.samsung.com>
> ---
>  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);

Why are you adding this? If media_devnode_register() failed then the
device node won't be registered so is not correct to call
media_devnode_unregister(). Or maybe I'm missing something.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shuah Khan March 16, 2016, 2:05 p.m. UTC | #2
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@osg.samsung.com>
> ---
>  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 do_media_device_unregister(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@osg.samsung.com>

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.

-- Shuah
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> +	if (mdev == NULL)
> +		return;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	kref_put(&mdev->kref, do_media_device_unregister);
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>  	struct media_device *mdev;
>  
>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> +	if (mdev) {
> +		kref_get(&mdev->kref);
>  		return mdev;
> +	}
>  
>  	mdev = devres_alloc(media_device_release_devres,
>  				sizeof(struct media_device), GFP_KERNEL);
>  	if (!mdev)
>  		return NULL;
> +
>  	return devres_get(dev, mdev, NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index ca3871b853ba..73c16e6e6b6b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -23,6 +23,7 @@
>  #ifndef _MEDIA_DEVICE_H
>  #define _MEDIA_DEVICE_H
>  
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> @@ -283,6 +284,7 @@ struct media_entity_notify {
>   * struct media_device - Media device
>   * @dev:	Parent device
>   * @devnode:	Media device node
> + * @kref:	Object refcount
>   * @driver_name: Optional device driver name. If not set, calls to
>   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
>   *		This is needed for USB drivers for example, as otherwise
> @@ -347,6 +349,7 @@ struct media_device {
>  	/* dev->driver_data points to this struct. */
>  	struct device *dev;
>  	struct media_devnode devnode;
> +	struct kref kref;
>  
>  	char model[32];
>  	char driver_name[32];
>
Mauro Carvalho Chehab March 16, 2016, 2:25 p.m. UTC | #3
Em Wed, 16 Mar 2016 08:05:15 -0600
Shuah Khan <shuahkh@osg.samsung.com> 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@osg.samsung.com>
> > ---
> >  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@osg.samsung.com>
> 
> 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.

> 
> -- Shuah
> >  
> >  	dev_dbg(mdev->dev, "Media device unregistered\n");
> >  }
> > +
> > +void media_device_unregister(struct media_device *mdev)
> > +{
> > +	if (mdev == NULL)
> > +		return;
> > +
> > +	mutex_lock(&mdev->graph_mutex);
> > +	kref_put(&mdev->kref, do_media_device_unregister);
> > +	mutex_unlock(&mdev->graph_mutex);
> > +
> > +}
> >  EXPORT_SYMBOL_GPL(media_device_unregister);
> >  
> >  static void media_device_release_devres(struct device *dev, void *res)
> > @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
> >  	struct media_device *mdev;
> >  
> >  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> > -	if (mdev)
> > +	if (mdev) {
> > +		kref_get(&mdev->kref);
> >  		return mdev;
> > +	}
> >  
> >  	mdev = devres_alloc(media_device_release_devres,
> >  				sizeof(struct media_device), GFP_KERNEL);
> >  	if (!mdev)
> >  		return NULL;
> > +
> >  	return devres_get(dev, mdev, NULL, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_get_devres);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index ca3871b853ba..73c16e6e6b6b 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -23,6 +23,7 @@
> >  #ifndef _MEDIA_DEVICE_H
> >  #define _MEDIA_DEVICE_H
> >  
> > +#include <linux/kref.h>
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> >  
> > @@ -283,6 +284,7 @@ struct media_entity_notify {
> >   * struct media_device - Media device
> >   * @dev:	Parent device
> >   * @devnode:	Media device node
> > + * @kref:	Object refcount
> >   * @driver_name: Optional device driver name. If not set, calls to
> >   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
> >   *		This is needed for USB drivers for example, as otherwise
> > @@ -347,6 +349,7 @@ struct media_device {
> >  	/* dev->driver_data points to this struct. */
> >  	struct device *dev;
> >  	struct media_devnode devnode;
> > +	struct kref kref;
> >  
> >  	char model[32];
> >  	char driver_name[32];
> >   
> 
>
Shuah Khan March 16, 2016, 2:32 p.m. UTC | #4
On 03/16/2016 08:25 AM, Mauro Carvalho Chehab wrote:
> Em Wed, 16 Mar 2016 08:05:15 -0600
> Shuah Khan <shuahkh@osg.samsung.com> 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@osg.samsung.com>
>>> ---
>>>  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@osg.samsung.com>
>>
>> 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
Sakari Ailus March 17, 2016, 11:50 a.m. UTC | #5
Hi Mauro,

On Wed, Mar 16, 2016 at 09:04:05AM -0300, 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.

Not long ago we split media device initialisation and registration into two.
The intent with that was to prevent users from seeing the media device until
all the initialisation is finished. I suppose that with dynamic updates, or
media device being shared with two drivers, it might be difficult to achieve
that. At least the method has to be different.

media_device_init() and media_device_cleanup() were a pair where the latter
undid the first. This patchs remove the requirement of calling cleanup
explitly, breaking that model. It's perhaps not a big problem, there is
likely no single driver which would initialise the media controller device
once but would register and unregister it multiple times. I still wonder if
we really lost something important if we removed media_device_init() and
media_device_cleanup() altogether and merged the functionality in
media_device_{,un}register().

Cc Javier who wrote the patch.

commit 9832e155f1ed3030fdfaa19e72c06472dc2ecb1d
Author: Javier Martinez Canillas <javier@osg.samsung.com>
Date:   Fri Dec 11 20:57:08 2015 -0200

    [media] media-device: split media initialization and registration

For system-wide media device, I think we'd need to introduce a new concept,
graph, that would define an interconnected set of entities. This would
mainly be needed for callbacks, e.g. the power on / off sequences of the
entities could be specific to V4L as discussed earlier. With this approach
hacks wouldn't be needed to be made in the first place to support two drivers
sharing a media device.

What was the original reason you needed to share the media device btw.?

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  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 do_media_device_unregister(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);
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> +	if (mdev == NULL)
> +		return;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	kref_put(&mdev->kref, do_media_device_unregister);
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>  	struct media_device *mdev;
>  
>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> +	if (mdev) {
> +		kref_get(&mdev->kref);
>  		return mdev;
> +	}
>  
>  	mdev = devres_alloc(media_device_release_devres,
>  				sizeof(struct media_device), GFP_KERNEL);
>  	if (!mdev)
>  		return NULL;
> +
>  	return devres_get(dev, mdev, NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index ca3871b853ba..73c16e6e6b6b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -23,6 +23,7 @@
>  #ifndef _MEDIA_DEVICE_H
>  #define _MEDIA_DEVICE_H
>  
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> @@ -283,6 +284,7 @@ struct media_entity_notify {
>   * struct media_device - Media device
>   * @dev:	Parent device
>   * @devnode:	Media device node
> + * @kref:	Object refcount
>   * @driver_name: Optional device driver name. If not set, calls to
>   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
>   *		This is needed for USB drivers for example, as otherwise
> @@ -347,6 +349,7 @@ struct media_device {
>  	/* dev->driver_data points to this struct. */
>  	struct device *dev;
>  	struct media_devnode devnode;
> +	struct kref kref;
>  
>  	char model[32];
>  	char driver_name[32];
> -- 
> 2.5.0
>
Mauro Carvalho Chehab March 18, 2016, 11:17 a.m. UTC | #6
Em Thu, 17 Mar 2016 13:50:53 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Wed, Mar 16, 2016 at 09:04:05AM -0300, 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.  
> 
> Not long ago we split media device initialisation and registration into two.
> The intent with that was to prevent users from seeing the media device until
> all the initialisation is finished. I suppose that with dynamic updates, or
> media device being shared with two drivers, it might be difficult to achieve
> that. At least the method has to be different.
> 
> media_device_init() and media_device_cleanup() were a pair where the latter
> undid the first. 

Yes, this patch breaks the balance of _init()/_cleanup(), but this is for
a greater good. It is very hard to remove the devnode and cleanup the
media_device struct when multiple drivers can use it. The best way to
warrant that we'll be doing that is via kref.

> This patchs remove the requirement of calling cleanup
> explitly, breaking that model. It's perhaps not a big problem, there is
> likely no single driver which would initialise the media controller device
> once but would register and unregister it multiple times. I still wonder if
> we really lost something important if we removed media_device_init() and
> media_device_cleanup() altogether and merged the functionality in
> media_device_{,un}register().
> 
> Cc Javier who wrote the patch.

That patch was written to avoid a race trouble of having the media
devnode to initialize too early. Merging them back would cause troubles.

Maybe what we could do, instead, is to rename media_device_init()
to media_device_register(), and call the function that creates the
devnode as media_device_create_devnode().

What do you think?

> 
> commit 9832e155f1ed3030fdfaa19e72c06472dc2ecb1d
> Author: Javier Martinez Canillas <javier@osg.samsung.com>
> Date:   Fri Dec 11 20:57:08 2015 -0200
> 
>     [media] media-device: split media initialization and registration
> 
> For system-wide media device, I think we'd need to introduce a new concept,
> graph, that would define an interconnected set of entities. This would
> mainly be needed for callbacks, e.g. the power on / off sequences of the
> entities could be specific to V4L as discussed earlier. With this approach
> hacks wouldn't be needed to be made in the first place to support two drivers
> sharing a media device.
> 
> What was the original reason you needed to share the media device btw.?
> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > ---
> >  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 do_media_device_unregister(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);
> >  
> >  	dev_dbg(mdev->dev, "Media device unregistered\n");
> >  }
> > +
> > +void media_device_unregister(struct media_device *mdev)
> > +{
> > +	if (mdev == NULL)
> > +		return;
> > +
> > +	mutex_lock(&mdev->graph_mutex);
> > +	kref_put(&mdev->kref, do_media_device_unregister);
> > +	mutex_unlock(&mdev->graph_mutex);
> > +
> > +}
> >  EXPORT_SYMBOL_GPL(media_device_unregister);
> >  
> >  static void media_device_release_devres(struct device *dev, void *res)
> > @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
> >  	struct media_device *mdev;
> >  
> >  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> > -	if (mdev)
> > +	if (mdev) {
> > +		kref_get(&mdev->kref);
> >  		return mdev;
> > +	}
> >  
> >  	mdev = devres_alloc(media_device_release_devres,
> >  				sizeof(struct media_device), GFP_KERNEL);
> >  	if (!mdev)
> >  		return NULL;
> > +
> >  	return devres_get(dev, mdev, NULL, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_get_devres);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index ca3871b853ba..73c16e6e6b6b 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -23,6 +23,7 @@
> >  #ifndef _MEDIA_DEVICE_H
> >  #define _MEDIA_DEVICE_H
> >  
> > +#include <linux/kref.h>
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> >  
> > @@ -283,6 +284,7 @@ struct media_entity_notify {
> >   * struct media_device - Media device
> >   * @dev:	Parent device
> >   * @devnode:	Media device node
> > + * @kref:	Object refcount
> >   * @driver_name: Optional device driver name. If not set, calls to
> >   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
> >   *		This is needed for USB drivers for example, as otherwise
> > @@ -347,6 +349,7 @@ struct media_device {
> >  	/* dev->driver_data points to this struct. */
> >  	struct device *dev;
> >  	struct media_devnode devnode;
> > +	struct kref kref;
> >  
> >  	char model[32];
> >  	char driver_name[32];
> > -- 
> > 2.5.0
> >   
>
Sakari Ailus March 18, 2016, 1:10 p.m. UTC | #7
Hi Mauro,

On Wed, Mar 16, 2016 at 09:04:05AM -0300, 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@osg.samsung.com>

For patches 2 (assuming fixes according to Javier's comment), 4 and 5, and
"[media] media: rename media unregister function":

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Shuah Khan March 22, 2016, 7:56 p.m. UTC | #8
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@osg.samsung.com>

Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.

Ran device removal test and rmmod and modprobe tests on both drivers.

Generated graph after the runs and the graph looks good.

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
Tested-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

> ---
>  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 do_media_device_unregister(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);
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> +	if (mdev == NULL)
> +		return;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	kref_put(&mdev->kref, do_media_device_unregister);
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>  	struct media_device *mdev;
>  
>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> +	if (mdev) {
> +		kref_get(&mdev->kref);
>  		return mdev;
> +	}
>  
>  	mdev = devres_alloc(media_device_release_devres,
>  				sizeof(struct media_device), GFP_KERNEL);
>  	if (!mdev)
>  		return NULL;
> +
>  	return devres_get(dev, mdev, NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index ca3871b853ba..73c16e6e6b6b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -23,6 +23,7 @@
>  #ifndef _MEDIA_DEVICE_H
>  #define _MEDIA_DEVICE_H
>  
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> @@ -283,6 +284,7 @@ struct media_entity_notify {
>   * struct media_device - Media device
>   * @dev:	Parent device
>   * @devnode:	Media device node
> + * @kref:	Object refcount
>   * @driver_name: Optional device driver name. If not set, calls to
>   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
>   *		This is needed for USB drivers for example, as otherwise
> @@ -347,6 +349,7 @@ struct media_device {
>  	/* dev->driver_data points to this struct. */
>  	struct device *dev;
>  	struct media_devnode devnode;
> +	struct kref kref;
>  
>  	char model[32];
>  	char driver_name[32];
>
Shuah Khan March 22, 2016, 8:07 p.m. UTC | #9
On 03/22/2016 01:56 PM, Shuah Khan wrote:
> 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@osg.samsung.com>
> 
> Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
> snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
> KASAN enabled kernel (lock testing enabled). No use-after-free errors
> during these runs.
> 
> Ran device removal test and rmmod and modprobe tests on both drivers.
> 
> Generated graph after the runs and the graph looks good.
> 
> Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
> Tested-by: Shuah Khan <shuahkh@osg.samsung.com>
> 

Acked the wrong patch. Sorry. Will ack the v2 which is the one I tested.

thanks,
-- Shuah

>> ---
>>  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 do_media_device_unregister(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);
>>  
>>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>>  }
>> +
>> +void media_device_unregister(struct media_device *mdev)
>> +{
>> +	if (mdev == NULL)
>> +		return;
>> +
>> +	mutex_lock(&mdev->graph_mutex);
>> +	kref_put(&mdev->kref, do_media_device_unregister);
>> +	mutex_unlock(&mdev->graph_mutex);
>> +
>> +}
>>  EXPORT_SYMBOL_GPL(media_device_unregister);
>>  
>>  static void media_device_release_devres(struct device *dev, void *res)
>> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>>  	struct media_device *mdev;
>>  
>>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
>> -	if (mdev)
>> +	if (mdev) {
>> +		kref_get(&mdev->kref);
>>  		return mdev;
>> +	}
>>  
>>  	mdev = devres_alloc(media_device_release_devres,
>>  				sizeof(struct media_device), GFP_KERNEL);
>>  	if (!mdev)
>>  		return NULL;
>> +
>>  	return devres_get(dev, mdev, NULL, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(media_device_get_devres);
>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>> index ca3871b853ba..73c16e6e6b6b 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -23,6 +23,7 @@
>>  #ifndef _MEDIA_DEVICE_H
>>  #define _MEDIA_DEVICE_H
>>  
>> +#include <linux/kref.h>
>>  #include <linux/list.h>
>>  #include <linux/mutex.h>
>>  
>> @@ -283,6 +284,7 @@ struct media_entity_notify {
>>   * struct media_device - Media device
>>   * @dev:	Parent device
>>   * @devnode:	Media device node
>> + * @kref:	Object refcount
>>   * @driver_name: Optional device driver name. If not set, calls to
>>   *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
>>   *		This is needed for USB drivers for example, as otherwise
>> @@ -347,6 +349,7 @@ struct media_device {
>>  	/* dev->driver_data points to this struct. */
>>  	struct device *dev;
>>  	struct media_devnode devnode;
>> +	struct kref kref;
>>  
>>  	char model[32];
>>  	char driver_name[32];
>>
> 
>

Patch
diff mbox

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 do_media_device_unregister(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);
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 }
+
+void media_device_unregister(struct media_device *mdev)
+{
+	if (mdev == NULL)
+		return;
+
+	mutex_lock(&mdev->graph_mutex);
+	kref_put(&mdev->kref, do_media_device_unregister);
+	mutex_unlock(&mdev->graph_mutex);
+
+}
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
 static void media_device_release_devres(struct device *dev, void *res)
@@ -825,13 +842,16 @@  struct media_device *media_device_get_devres(struct device *dev)
 	struct media_device *mdev;
 
 	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
-	if (mdev)
+	if (mdev) {
+		kref_get(&mdev->kref);
 		return mdev;
+	}
 
 	mdev = devres_alloc(media_device_release_devres,
 				sizeof(struct media_device), GFP_KERNEL);
 	if (!mdev)
 		return NULL;
+
 	return devres_get(dev, mdev, NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(media_device_get_devres);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index ca3871b853ba..73c16e6e6b6b 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -23,6 +23,7 @@ 
 #ifndef _MEDIA_DEVICE_H
 #define _MEDIA_DEVICE_H
 
+#include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 
@@ -283,6 +284,7 @@  struct media_entity_notify {
  * struct media_device - Media device
  * @dev:	Parent device
  * @devnode:	Media device node
+ * @kref:	Object refcount
  * @driver_name: Optional device driver name. If not set, calls to
  *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
  *		This is needed for USB drivers for example, as otherwise
@@ -347,6 +349,7 @@  struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
 	struct media_devnode devnode;
+	struct kref kref;
 
 	char model[32];
 	char driver_name[32];