[v2,media] media-device: use kref for media_device instance

  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>
---

v2: The kref is now used only when media_device is allocated via 
    the media_device*_devress. This warrants that other drivers won't be
    affected, and that we can keep media_device_cleanup() balanced with
    media_device_init().

 drivers/media/media-device.c           | 117 +++++++++++++++++++++++++--------
 drivers/media/usb/au0828/au0828-core.c |   3 +-
 include/media/media-device.h           |  28 ++++++++
 sound/usb/media.c                      |   3 +-
 4 files changed, 118 insertions(+), 33 deletions(-)
  

On 03/18/2016 06:42 PM, 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>

Looks good. Tested it doing bind/unbind, rmmod/modprobe on both
au0828 and snd-usb-audio drivers. Also generated graphs when after
bind/unbind, rmmod/modprobe. Graphs look good.

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

thanks,
-- Shuah

> ---
> 
> v2: The kref is now used only when media_device is allocated via 
>     the media_device*_devress. This warrants that other drivers won't be
>     affected, and that we can keep media_device_cleanup() balanced with
>     media_device_init().
> 
>  drivers/media/media-device.c           | 117 +++++++++++++++++++++++++--------
>  drivers/media/usb/au0828/au0828-core.c |   3 +-
>  include/media/media-device.h           |  28 ++++++++
>  sound/usb/media.c                      |   3 +-
>  4 files changed, 118 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..4a97d92a7e7d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev)
>  }
>  EXPORT_SYMBOL_GPL(media_device_init);
>  
> -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);
> +}
> +
> +void media_device_cleanup(struct media_device *mdev)
> +{
> +	__media_device_cleanup(mdev);
>  	mutex_destroy(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_cleanup);
> @@ -721,6 +726,9 @@ 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);
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct media_device *mdev,
>  
>  	ret = media_devnode_register(&mdev->devnode, owner);
>  	if (ret < 0)
> -		return ret;
> +		goto err;
>  
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
>  		media_devnode_unregister(&mdev->devnode);
> -		return ret;
> +		goto err;
>  	}
>  
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
> -	return 0;
> +err:
> +	mutex_unlock(&mdev->graph_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
>  
> @@ -773,24 +783,13 @@ 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 __media_device_unregister(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;
> -	}
> -
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>  		__media_device_unregister_entity(entity);
> @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device *mdev)
>  		kfree(intf);
>  	}
>  
> -	mutex_unlock(&mdev->graph_mutex);
> -
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> -	media_devnode_unregister(&mdev->devnode);
> +	/* Check if mdev devnode was registered */
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		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);
> +	__media_device_unregister(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
>  {
>  }
>  
> -struct media_device *media_device_get_devres(struct device *dev)
> +static void do_media_device_unregister_devres(struct kref *kref)
>  {
> +	struct media_device_devres *mdev_devres;
>  	struct media_device *mdev;
> +	int ret;
>  
> -	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> -		return mdev;
> +	mdev_devres = container_of(kref, struct media_device_devres, kref);
>  
> -	mdev = devres_alloc(media_device_release_devres,
> -				sizeof(struct media_device), GFP_KERNEL);
> -	if (!mdev)
> +	if (!mdev_devres)
> +		return;
> +
> +	mdev = &mdev_devres->mdev;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	__media_device_unregister(mdev);
> +	__media_device_cleanup(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +	mutex_destroy(&mdev->graph_mutex);
> +
> +	ret = devres_destroy(mdev->dev, media_device_release_devres,
> +			     NULL, NULL);
> +	pr_debug("%s: devres_destroy() returned %d\n", __func__, ret);
> +}
> +
> +void media_device_unregister_devres(struct media_device *mdev)
> +{
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = container_of(mdev, struct media_device_devres, mdev);
> +	kref_put(&mdev_devres->kref, do_media_device_unregister_devres);
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister_devres);
> +
> +struct media_device *media_device_get_devres(struct device *dev)
> +{
> +	struct media_device_devres *mdev_devres, *ptr;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (mdev_devres) {
> +		kref_get(&mdev_devres->kref);
> +		return &mdev_devres->mdev;
> +	}
> +
> +	mdev_devres = devres_alloc(media_device_release_devres,
> +				   sizeof(struct media_device_devres),
> +				   GFP_KERNEL);
> +	if (!mdev_devres)
>  		return NULL;
> -	return devres_get(dev, mdev, NULL, NULL);
> +
> +	ptr = devres_get(dev, mdev_devres, NULL, NULL);
> +	if (ptr)
> +		kref_init(&ptr->kref);
> +	else
> +		devres_free(mdev_devres);
> +
> +	return &ptr->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
>  
>  struct media_device *media_device_find_devres(struct device *dev)
>  {
> -	return devres_find(dev, media_device_release_devres, NULL, NULL);
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (!mdev_devres)
> +		return NULL;
> +
> +	return &mdev_devres->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_find_devres);
>  
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 06da73f1ff22..060904ed8f20 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
>  	dev->media_dev->enable_source = NULL;
>  	dev->media_dev->disable_source = NULL;
>  
> -	media_device_unregister(dev->media_dev);
> -	media_device_cleanup(dev->media_dev);
> +	media_device_unregister_devres(dev->media_dev);
>  	dev->media_dev = NULL;
>  #endif
>  }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index b21ef244ad3e..e59772ed8494 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>
>  
> @@ -382,6 +383,16 @@ struct media_device {
>  			   unsigned int notification);
>  };
>  
> +/**
> + * struct media_device_devres - Media device device resource
> + * @mdev:	pointer to struct media_device
> + * @kref:	Object refcount
> + */
> +struct media_device_devres {
> +	struct media_device mdev;
> +	struct kref kref;
> +};
> +
>  /* We don't need to include pci.h or usb.h here */
>  struct pci_dev;
>  struct usb_device;
> @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct device *dev);
>   */
>  struct media_device *media_device_find_devres(struct device *dev);
>  
> +/**
> + * media_device_unregister_devres) - Unregister media device allocated as
> + *				     as device resource
> + *
> + * @dev: pointer to struct &device.
> + *
> + * Devices allocated via media_device_get_devres should be de-alocalted
> + * and freed via this function. Callers should not call
> + * media_device_unregister() nor media_device_cleanup() on devices
> + * allocated via media_device_get_devres().
> + */
> +void media_device_unregister_devres(struct media_device *mdev);
> +
>  /* Iterate over all entities. */
>  #define media_device_for_each_entity(entity, mdev)			\
>  	list_for_each_entry(entity, &(mdev)->entities, graph_obj.list)
> @@ -688,6 +712,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev)
>  	return NULL;
>  }
>  
> +static inline void media_device_unregister_devres(struct media_device *mdev)
> +{
> +}
> +
>  static inline void media_device_pci_init(struct media_device *mdev,
>  					 struct pci_dev *pci_dev,
>  					 char *name)
> diff --git a/sound/usb/media.c b/sound/usb/media.c
> index 93a50d01490c..f78955fd0d6e 100644
> --- a/sound/usb/media.c
> +++ b/sound/usb/media.c
> @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip)
>  	media_snd_mixer_delete(chip);
>  
>  	if (mdev) {
> -		if (media_devnode_is_registered(&mdev->devnode))
> -			media_device_unregister(mdev);
> +		media_device_unregister_devres(mdev);
>  		chip->media_dev = NULL;
>  	}
>  }
>
  

Hi Mauro,

Thank you for the patch.

On Friday 18 Mar 2016 21:42:16 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.

I've discussed this with Shuah previously and I'm surprised to see that the 
problem hasn't been fixed : using devres for this purpose is just plain wrong. 
The empty media_device_release_devres() function should have given you a hint.

What we need instead is a list of media devices indexed by struct device (for 
this use case) or by struct device_node (for DT use cases). It will both 
simplify the code and get rid of the devres abuse.

Shuah, if I recall correctly you worked on implementing such a solution after 
our last discussion on the topic. Could you please update us on the status ?

In the mean time, let's hold off on this patch, and merge a proper solution 
instead.

> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
> 
> v2: The kref is now used only when media_device is allocated via
>     the media_device*_devress. This warrants that other drivers won't be
>     affected, and that we can keep media_device_cleanup() balanced with
>     media_device_init().
> 
>  drivers/media/media-device.c           | 117 ++++++++++++++++++++++--------
>  drivers/media/usb/au0828/au0828-core.c |   3 +-
>  include/media/media-device.h           |  28 ++++++++
>  sound/usb/media.c                      |   3 +-
>  4 files changed, 118 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..4a97d92a7e7d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev)
>  }
>  EXPORT_SYMBOL_GPL(media_device_init);
> 
> -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);
> +}
> +
> +void media_device_cleanup(struct media_device *mdev)
> +{
> +	__media_device_cleanup(mdev);
>  	mutex_destroy(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_cleanup);
> @@ -721,6 +726,9 @@ 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);
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct
> media_device *mdev,
> 
>  	ret = media_devnode_register(&mdev->devnode, owner);
>  	if (ret < 0)
> -		return ret;
> +		goto err;
> 
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
>  		media_devnode_unregister(&mdev->devnode);
> -		return ret;
> +		goto err;
>  	}
> 
>  	dev_dbg(mdev->dev, "Media device registered\n");
> 
> -	return 0;
> +err:
> +	mutex_unlock(&mdev->graph_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
> 
> @@ -773,24 +783,13 @@ 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 __media_device_unregister(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;
> -	}
> -
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>  		__media_device_unregister_entity(entity);
> @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device
> *mdev) kfree(intf);
>  	}
> 
> -	mutex_unlock(&mdev->graph_mutex);
> -
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> -	media_devnode_unregister(&mdev->devnode);
> +	/* Check if mdev devnode was registered */
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		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);
> +	__media_device_unregister(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
> 
>  static void media_device_release_devres(struct device *dev, void *res)
>  {
>  }
> 
> -struct media_device *media_device_get_devres(struct device *dev)
> +static void do_media_device_unregister_devres(struct kref *kref)
>  {
> +	struct media_device_devres *mdev_devres;
>  	struct media_device *mdev;
> +	int ret;
> 
> -	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> -		return mdev;
> +	mdev_devres = container_of(kref, struct media_device_devres, kref);
> 
> -	mdev = devres_alloc(media_device_release_devres,
> -				sizeof(struct media_device), GFP_KERNEL);
> -	if (!mdev)
> +	if (!mdev_devres)
> +		return;
> +
> +	mdev = &mdev_devres->mdev;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	__media_device_unregister(mdev);
> +	__media_device_cleanup(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +	mutex_destroy(&mdev->graph_mutex);
> +
> +	ret = devres_destroy(mdev->dev, media_device_release_devres,
> +			     NULL, NULL);
> +	pr_debug("%s: devres_destroy() returned %d\n", __func__, ret);
> +}
> +
> +void media_device_unregister_devres(struct media_device *mdev)
> +{
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = container_of(mdev, struct media_device_devres, mdev);
> +	kref_put(&mdev_devres->kref, do_media_device_unregister_devres);
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister_devres);
> +
> +struct media_device *media_device_get_devres(struct device *dev)
> +{
> +	struct media_device_devres *mdev_devres, *ptr;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (mdev_devres) {
> +		kref_get(&mdev_devres->kref);
> +		return &mdev_devres->mdev;
> +	}
> +
> +	mdev_devres = devres_alloc(media_device_release_devres,
> +				   sizeof(struct media_device_devres),
> +				   GFP_KERNEL);
> +	if (!mdev_devres)
>  		return NULL;
> -	return devres_get(dev, mdev, NULL, NULL);
> +
> +	ptr = devres_get(dev, mdev_devres, NULL, NULL);
> +	if (ptr)
> +		kref_init(&ptr->kref);
> +	else
> +		devres_free(mdev_devres);
> +
> +	return &ptr->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> 
>  struct media_device *media_device_find_devres(struct device *dev)
>  {
> -	return devres_find(dev, media_device_release_devres, NULL, NULL);
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (!mdev_devres)
> +		return NULL;
> +
> +	return &mdev_devres->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_find_devres);
> 
> diff --git a/drivers/media/usb/au0828/au0828-core.c
> b/drivers/media/usb/au0828/au0828-core.c index 06da73f1ff22..060904ed8f20
> 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct
> au0828_dev *dev) dev->media_dev->enable_source = NULL;
>  	dev->media_dev->disable_source = NULL;
> 
> -	media_device_unregister(dev->media_dev);
> -	media_device_cleanup(dev->media_dev);
> +	media_device_unregister_devres(dev->media_dev);
>  	dev->media_dev = NULL;
>  #endif
>  }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index b21ef244ad3e..e59772ed8494 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>
> 
> @@ -382,6 +383,16 @@ struct media_device {
>  			   unsigned int notification);
>  };
> 
> +/**
> + * struct media_device_devres - Media device device resource
> + * @mdev:	pointer to struct media_device
> + * @kref:	Object refcount
> + */
> +struct media_device_devres {
> +	struct media_device mdev;
> +	struct kref kref;
> +};
> +
>  /* We don't need to include pci.h or usb.h here */
>  struct pci_dev;
>  struct usb_device;
> @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct
> device *dev); */
>  struct media_device *media_device_find_devres(struct device *dev);
> 
> +/**
> + * media_device_unregister_devres) - Unregister media device allocated as
> + *				     as device resource
> + *
> + * @dev: pointer to struct &device.
> + *
> + * Devices allocated via media_device_get_devres should be de-alocalted
> + * and freed via this function. Callers should not call
> + * media_device_unregister() nor media_device_cleanup() on devices
> + * allocated via media_device_get_devres().
> + */
> +void media_device_unregister_devres(struct media_device *mdev);
> +
>  /* Iterate over all entities. */
>  #define media_device_for_each_entity(entity, mdev)			\
>  	list_for_each_entry(entity, &(mdev)->entities, graph_obj.list)
> @@ -688,6 +712,10 @@ static inline struct media_device
> *media_device_find_devres(struct device *dev) return NULL;
>  }
> 
> +static inline void media_device_unregister_devres(struct media_device
> *mdev)
> +{
> +}
> +
>  static inline void media_device_pci_init(struct media_device *mdev,
>  					 struct pci_dev *pci_dev,
>  					 char *name)
> diff --git a/sound/usb/media.c b/sound/usb/media.c
> index 93a50d01490c..f78955fd0d6e 100644
> --- a/sound/usb/media.c
> +++ b/sound/usb/media.c
> @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip)
> media_snd_mixer_delete(chip);
> 
>  	if (mdev) {
> -		if (media_devnode_is_registered(&mdev->devnode))
> -			media_device_unregister(mdev);
> +		media_device_unregister_devres(mdev);
>  		chip->media_dev = NULL;
>  	}
>  }
  

Em Mon, 21 Mar 2016 13:10:33 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Friday 18 Mar 2016 21:42:16 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.  
> 
> I've discussed this with Shuah previously and I'm surprised to see that the 
> problem hasn't been fixed : using devres for this purpose is just plain wrong. 

I didn't follow your discussions with Shuah. I'm pretty sure I didn't
see any such reply to the /22 patch series. 

For sure there are other approaches, although I wouldn't say that this
approach is plain wrong. It was actually suggested by Greg KH at the
USB summit, back in 2011:
	https://lkml.org/lkml/2011/8/21/61

It works fine in the cases like the ones Shuah is currently addressing: 
USB devices that have multiple interfaces handled by independent drivers.

Going further, right now, as far as I'm aware of, there are only two use
cases for a driver-independent media_device struct in the media subsystem
(on the upstream Kernel):

- USB devices with USB Audio Class: au0828 and em28xx drivers,
  plus snd-usb-audio;

- bt878/bt879 PCI devices, where the DVB driver is independent
  from the V4L2 one (affects bt87x and bttv drivers).

The devres approach fits well for both use cases.

Ok, there are a plenty of OOT SoC drivers that might benefit of some
other solution, but we should care about them only if/when they got
upstreamed.

> The empty media_device_release_devres() function should have given you a hint.
> 
> What we need instead is a list of media devices indexed by struct device (for 
> this use case) or by struct device_node (for DT use cases). It will both 
> simplify the code and get rid of the devres abuse.

Yeah, Shuah's approach should be changed to a different one, in order to
work for DT use cases. It would be good to have a real DT use case for us
to validate the solution, before we start implementing something in the
wild.

Still, it would very likely need a kref there, in order to destroy the
media controller struct only after all drivers stop using it.

> Shuah, if I recall correctly you worked on implementing such a solution after 
> our last discussion on the topic. Could you please update us on the status ?

I saw a Shuah's email proposing to discuss this at the media summit.

> In the mean time, let's hold off on this patch, and merge a proper solution 
> instead.

Well, we should send a fix for the current issues for Kernel 4.6.

As the number of drivers that would be using this internal API is small
(right now, only 2 drivers), replacing devres by some other strategy
in the future should be easy.

Regards,
Mauro
--
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
  

On 03/21/2016 05:10 AM, Laurent Pinchart wrote:
> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Friday 18 Mar 2016 21:42:16 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.
> 
> I've discussed this with Shuah previously and I'm surprised to see that the 
> problem hasn't been fixed : using devres for this purpose is just plain wrong. 
> The empty media_device_release_devres() function should have given you a hint.
> 
> What we need instead is a list of media devices indexed by struct device (for 
> this use case) or by struct device_node (for DT use cases). It will both 
> simplify the code and get rid of the devres abuse.
> 
> Shuah, if I recall correctly you worked on implementing such a solution after 
> our last discussion on the topic. Could you please update us on the status ?
> 

It is work in progress. I have a working prototype for au0828 which is an easier
case. I am working on resolving a couple of issues to differentiate media devices
allocated using the global media device list vs. the ones embedded in the driver
data structures. We have many of those.

I had to put this work on the back burner to get the au0882 and snd-usb-audio
wrapped up. I can get the RFC patches on top of the au0882 and snd-usb-audio.
We can discuss them at the upcoming media summit.

> In the mean time, let's hold off on this patch, and merge a proper solution 
> instead.

I think Mauro's restructure helps us with such differentiation and it will be
easy enough to change out devres to get media get API.

thanks,
-- Shuah
  

On 03/21/2016 05:58 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 13:10:33 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
>> Hi Mauro,
>>
>> Thank you for the patch.
>>
>> On Friday 18 Mar 2016 21:42:16 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.  
>>
>> I've discussed this with Shuah previously and I'm surprised to see that the 
>> problem hasn't been fixed : using devres for this purpose is just plain wrong. 
> 
> I didn't follow your discussions with Shuah. I'm pretty sure I didn't
> see any such reply to the /22 patch series. 
> 
> For sure there are other approaches, although I wouldn't say that this
> approach is plain wrong. It was actually suggested by Greg KH at the
> USB summit, back in 2011:
> 	https://lkml.org/lkml/2011/8/21/61
> 
> It works fine in the cases like the ones Shuah is currently addressing: 
> USB devices that have multiple interfaces handled by independent drivers.
> 
> Going further, right now, as far as I'm aware of, there are only two use
> cases for a driver-independent media_device struct in the media subsystem
> (on the upstream Kernel):
> 
> - USB devices with USB Audio Class: au0828 and em28xx drivers,
>   plus snd-usb-audio;
> 
> - bt878/bt879 PCI devices, where the DVB driver is independent
>   from the V4L2 one (affects bt87x and bttv drivers).
> 
> The devres approach fits well for both use cases.
> 
> Ok, there are a plenty of OOT SoC drivers that might benefit of some
> other solution, but we should care about them only if/when they got
> upstreamed.
> 
>> The empty media_device_release_devres() function should have given you a hint.
>>
>> What we need instead is a list of media devices indexed by struct device (for 
>> this use case) or by struct device_node (for DT use cases). It will both 
>> simplify the code and get rid of the devres abuse.
> 
> Yeah, Shuah's approach should be changed to a different one, in order to
> work for DT use cases. It would be good to have a real DT use case for us
> to validate the solution, before we start implementing something in the
> wild.
> 
> Still, it would very likely need a kref there, in order to destroy the
> media controller struct only after all drivers stop using it.
> 
>> Shuah, if I recall correctly you worked on implementing such a solution after 
>> our last discussion on the topic. Could you please update us on the status ?
> 
> I saw a Shuah's email proposing to discuss this at the media summit.

Right. Now that the USB devices with USB Audio Class work is in upstream,
I have a bit of breathing room. :)

I have a working solution for the media get api, ready for RFC. I didn't want
to add that in the middle of Media Controller Next Gen and the au0828 and
snd-usb-audio work. We have had several issues we had to address and fix.

I will send RFC patches for the media get API which is close to getting done.

> 
>> In the mean time, let's hold off on this patch, and merge a proper solution 
>> instead.
> 
> Well, we should send a fix for the current issues for Kernel 4.6.

We can't hold off on this patch. Also this patch will make a good base for
the RFC series and changing our devres with media get. We still have the
same issues to address, such as how do we differentiate how the media device
is allocated. This is necessary when it is time to release the media device
in the unregister path.

> 
> As the number of drivers that would be using this internal API is small
> (right now, only 2 drivers), replacing devres by some other strategy
> in the future should be easy.
> 

Yes. Please see above. It is a very simple change out. I have it in the
prototype already.

thanks,
-- Shuah
  

On 03/18/2016 06:42 PM, 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>
> ---
> 
> v2: The kref is now used only when media_device is allocated via 
>     the media_device*_devress. This warrants that other drivers won't be
>     affected, and that we can keep media_device_cleanup() balanced with
>     media_device_init().

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           | 117 +++++++++++++++++++++++++--------
>  drivers/media/usb/au0828/au0828-core.c |   3 +-
>  include/media/media-device.h           |  28 ++++++++
>  sound/usb/media.c                      |   3 +-
>  4 files changed, 118 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..4a97d92a7e7d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev)
>  }
>  EXPORT_SYMBOL_GPL(media_device_init);
>  
> -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);
> +}
> +
> +void media_device_cleanup(struct media_device *mdev)
> +{
> +	__media_device_cleanup(mdev);
>  	mutex_destroy(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_cleanup);
> @@ -721,6 +726,9 @@ 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);
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct media_device *mdev,
>  
>  	ret = media_devnode_register(&mdev->devnode, owner);
>  	if (ret < 0)
> -		return ret;
> +		goto err;
>  
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
>  		media_devnode_unregister(&mdev->devnode);
> -		return ret;
> +		goto err;
>  	}
>  
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
> -	return 0;
> +err:
> +	mutex_unlock(&mdev->graph_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
>  
> @@ -773,24 +783,13 @@ 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 __media_device_unregister(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;
> -	}
> -
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>  		__media_device_unregister_entity(entity);
> @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device *mdev)
>  		kfree(intf);
>  	}
>  
> -	mutex_unlock(&mdev->graph_mutex);
> -
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> -	media_devnode_unregister(&mdev->devnode);
> +	/* Check if mdev devnode was registered */
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		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);
> +	__media_device_unregister(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
>  {
>  }
>  
> -struct media_device *media_device_get_devres(struct device *dev)
> +static void do_media_device_unregister_devres(struct kref *kref)
>  {
> +	struct media_device_devres *mdev_devres;
>  	struct media_device *mdev;
> +	int ret;
>  
> -	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> -		return mdev;
> +	mdev_devres = container_of(kref, struct media_device_devres, kref);
>  
> -	mdev = devres_alloc(media_device_release_devres,
> -				sizeof(struct media_device), GFP_KERNEL);
> -	if (!mdev)
> +	if (!mdev_devres)
> +		return;
> +
> +	mdev = &mdev_devres->mdev;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	__media_device_unregister(mdev);
> +	__media_device_cleanup(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +	mutex_destroy(&mdev->graph_mutex);
> +
> +	ret = devres_destroy(mdev->dev, media_device_release_devres,
> +			     NULL, NULL);
> +	pr_debug("%s: devres_destroy() returned %d\n", __func__, ret);
> +}
> +
> +void media_device_unregister_devres(struct media_device *mdev)
> +{
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = container_of(mdev, struct media_device_devres, mdev);
> +	kref_put(&mdev_devres->kref, do_media_device_unregister_devres);
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister_devres);
> +
> +struct media_device *media_device_get_devres(struct device *dev)
> +{
> +	struct media_device_devres *mdev_devres, *ptr;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (mdev_devres) {
> +		kref_get(&mdev_devres->kref);
> +		return &mdev_devres->mdev;
> +	}
> +
> +	mdev_devres = devres_alloc(media_device_release_devres,
> +				   sizeof(struct media_device_devres),
> +				   GFP_KERNEL);
> +	if (!mdev_devres)
>  		return NULL;
> -	return devres_get(dev, mdev, NULL, NULL);
> +
> +	ptr = devres_get(dev, mdev_devres, NULL, NULL);
> +	if (ptr)
> +		kref_init(&ptr->kref);
> +	else
> +		devres_free(mdev_devres);
> +
> +	return &ptr->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
>  
>  struct media_device *media_device_find_devres(struct device *dev)
>  {
> -	return devres_find(dev, media_device_release_devres, NULL, NULL);
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (!mdev_devres)
> +		return NULL;
> +
> +	return &mdev_devres->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_find_devres);
>  
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 06da73f1ff22..060904ed8f20 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
>  	dev->media_dev->enable_source = NULL;
>  	dev->media_dev->disable_source = NULL;
>  
> -	media_device_unregister(dev->media_dev);
> -	media_device_cleanup(dev->media_dev);
> +	media_device_unregister_devres(dev->media_dev);
>  	dev->media_dev = NULL;
>  #endif
>  }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index b21ef244ad3e..e59772ed8494 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>
>  
> @@ -382,6 +383,16 @@ struct media_device {
>  			   unsigned int notification);
>  };
>  
> +/**
> + * struct media_device_devres - Media device device resource
> + * @mdev:	pointer to struct media_device
> + * @kref:	Object refcount
> + */
> +struct media_device_devres {
> +	struct media_device mdev;
> +	struct kref kref;
> +};
> +
>  /* We don't need to include pci.h or usb.h here */
>  struct pci_dev;
>  struct usb_device;
> @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct device *dev);
>   */
>  struct media_device *media_device_find_devres(struct device *dev);
>  
> +/**
> + * media_device_unregister_devres) - Unregister media device allocated as
> + *				     as device resource
> + *
> + * @dev: pointer to struct &device.
> + *
> + * Devices allocated via media_device_get_devres should be de-alocalted
> + * and freed via this function. Callers should not call
> + * media_device_unregister() nor media_device_cleanup() on devices
> + * allocated via media_device_get_devres().
> + */
> +void media_device_unregister_devres(struct media_device *mdev);
> +
>  /* Iterate over all entities. */
>  #define media_device_for_each_entity(entity, mdev)			\
>  	list_for_each_entry(entity, &(mdev)->entities, graph_obj.list)
> @@ -688,6 +712,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev)
>  	return NULL;
>  }
>  
> +static inline void media_device_unregister_devres(struct media_device *mdev)
> +{
> +}
> +
>  static inline void media_device_pci_init(struct media_device *mdev,
>  					 struct pci_dev *pci_dev,
>  					 char *name)
> diff --git a/sound/usb/media.c b/sound/usb/media.c
> index 93a50d01490c..f78955fd0d6e 100644
> --- a/sound/usb/media.c
> +++ b/sound/usb/media.c
> @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip)
>  	media_snd_mixer_delete(chip);
>  
>  	if (mdev) {
> -		if (media_devnode_is_registered(&mdev->devnode))
> -			media_device_unregister(mdev);
> +		media_device_unregister_devres(mdev);
>  		chip->media_dev = NULL;
>  	}
>  }
>
  

Hi Shuah,

On Monday 21 Mar 2016 07:42:34 Shuah Khan wrote:
> On 03/21/2016 05:10 AM, Laurent Pinchart wrote:
> > On Friday 18 Mar 2016 21:42:16 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.
> > 
> > I've discussed this with Shuah previously and I'm surprised to see that
> > the problem hasn't been fixed : using devres for this purpose is just
> > plain wrong. The empty media_device_release_devres() function should have
> > given you a hint.
> > 
> > What we need instead is a list of media devices indexed by struct device
> > (for this use case) or by struct device_node (for DT use cases). It will
> > both simplify the code and get rid of the devres abuse.
> > 
> > Shuah, if I recall correctly you worked on implementing such a solution
> > after our last discussion on the topic. Could you please update us on the
> > status ?
> It is work in progress. I have a working prototype for au0828 which is an
> easier case. I am working on resolving a couple of issues to differentiate
> media devices allocated using the global media device list vs. the ones
> embedded in the driver data structures. We have many of those.
> 
> I had to put this work on the back burner to get the au0882 and
> snd-usb-audio wrapped up. I can get the RFC patches on top of the au0882
> and snd-usb-audio. We can discuss them at the upcoming media summit.
> 
> > In the mean time, let's hold off on this patch, and merge a proper
> > solution instead.
> 
> I think Mauro's restructure helps us with such differentiation and it will
> be easy enough to change out devres to get media get API.

We have build too much technical debt already. I would really dislike seeing 
another hack being merged to fix partial problems when we know what needs to 
be done for a proper implementation. That's not the Linux upstream development 
I've known and grown to love, we don't pile up half-baked patches and hope 
that everything will be magically cleaned up later :-)
  

On Friday 18 Mar 2016 21:42:16 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>
> ---
> 
> v2: The kref is now used only when media_device is allocated via
>     the media_device*_devress. This warrants that other drivers won't be
>     affected, and that we can keep media_device_cleanup() balanced with
>     media_device_init().
> 
>  drivers/media/media-device.c           | 117 ++++++++++++++++++++++--------
>  drivers/media/usb/au0828/au0828-core.c |   3 +-
>  include/media/media-device.h           |  28 ++++++++
>  sound/usb/media.c                      |   3 +-
>  4 files changed, 118 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..4a97d92a7e7d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev)
>  }
>  EXPORT_SYMBOL_GPL(media_device_init);
> 
> -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);
> +}
> +
> +void media_device_cleanup(struct media_device *mdev)
> +{
> +	__media_device_cleanup(mdev);
>  	mutex_destroy(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_cleanup);
> @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct
> media_device *mdev, {
>  	int ret;
> 
> +	/* Check if mdev was ever registered at all */

This comment doesn't seem to apply to the next line, is it a leftover ? If so, 
please remove it.

> +	mutex_lock(&mdev->graph_mutex);
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -731,17 +739,19 @@ int __must_check __media_device_register(struct
> media_device *mdev,
> 
>  	ret = media_devnode_register(&mdev->devnode, owner);
>  	if (ret < 0)
> -		return ret;
> +		goto err;
> 
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
>  		media_devnode_unregister(&mdev->devnode);
> -		return ret;
> +		goto err;
>  	}
> 
>  	dev_dbg(mdev->dev, "Media device registered\n");
> 
> -	return 0;
> +err:
> +	mutex_unlock(&mdev->graph_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
> 
> @@ -773,24 +783,13 @@ 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 __media_device_unregister(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;
> -	}
> -
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>  		__media_device_unregister_entity(entity);
> @@ -807,38 +806,98 @@ void media_device_unregister(struct media_device
> *mdev) kfree(intf);
>  	}
> 
> -	mutex_unlock(&mdev->graph_mutex);
> -
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> -	media_devnode_unregister(&mdev->devnode);
> +	/* Check if mdev devnode was registered */
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		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);
> +	__media_device_unregister(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
> 
>  static void media_device_release_devres(struct device *dev, void *res)
>  {
>  }
> 
> -struct media_device *media_device_get_devres(struct device *dev)
> +static void do_media_device_unregister_devres(struct kref *kref)
>  {
> +	struct media_device_devres *mdev_devres;
>  	struct media_device *mdev;
> +	int ret;
> 
> -	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> -		return mdev;
> +	mdev_devres = container_of(kref, struct media_device_devres, kref);
> 
> -	mdev = devres_alloc(media_device_release_devres,
> -				sizeof(struct media_device), GFP_KERNEL);
> -	if (!mdev)
> +	if (!mdev_devres)
> +		return;
> +
> +	mdev = &mdev_devres->mdev;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	__media_device_unregister(mdev);
> +	__media_device_cleanup(mdev);
> +	mutex_unlock(&mdev->graph_mutex);
> +	mutex_destroy(&mdev->graph_mutex);
> +
> +	ret = devres_destroy(mdev->dev, media_device_release_devres,
> +			     NULL, NULL);
> +	pr_debug("%s: devres_destroy() returned %d\n", __func__, ret);
> +}
> +
> +void media_device_unregister_devres(struct media_device *mdev)
> +{
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = container_of(mdev, struct media_device_devres, mdev);
> +	kref_put(&mdev_devres->kref, do_media_device_unregister_devres);
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister_devres);
> +
> +struct media_device *media_device_get_devres(struct device *dev)
> +{
> +	struct media_device_devres *mdev_devres, *ptr;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (mdev_devres) {
> +		kref_get(&mdev_devres->kref);
> +		return &mdev_devres->mdev;
> +	}
> +
> +	mdev_devres = devres_alloc(media_device_release_devres,
> +				   sizeof(struct media_device_devres),
> +				   GFP_KERNEL);
> +	if (!mdev_devres)
>  		return NULL;
> -	return devres_get(dev, mdev, NULL, NULL);
> +
> +	ptr = devres_get(dev, mdev_devres, NULL, NULL);
> +	if (ptr)
> +		kref_init(&ptr->kref);
> +	else
> +		devres_free(mdev_devres);
> +
> +	return &ptr->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> 
>  struct media_device *media_device_find_devres(struct device *dev)
>  {
> -	return devres_find(dev, media_device_release_devres, NULL, NULL);
> +	struct media_device_devres *mdev_devres;
> +
> +	mdev_devres = devres_find(dev, media_device_release_devres, NULL, NULL);
> +	if (!mdev_devres)
> +		return NULL;
> +
> +	return &mdev_devres->mdev;
>  }
>  EXPORT_SYMBOL_GPL(media_device_find_devres);
> 
> diff --git a/drivers/media/usb/au0828/au0828-core.c
> b/drivers/media/usb/au0828/au0828-core.c index 06da73f1ff22..060904ed8f20
> 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -157,8 +157,7 @@ static void au0828_unregister_media_device(struct
> au0828_dev *dev) dev->media_dev->enable_source = NULL;
>  	dev->media_dev->disable_source = NULL;
> 
> -	media_device_unregister(dev->media_dev);
> -	media_device_cleanup(dev->media_dev);
> +	media_device_unregister_devres(dev->media_dev);
>  	dev->media_dev = NULL;
>  #endif
>  }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index b21ef244ad3e..e59772ed8494 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>
> 
> @@ -382,6 +383,16 @@ struct media_device {
>  			   unsigned int notification);
>  };
> 
> +/**
> + * struct media_device_devres - Media device device resource
> + * @mdev:	pointer to struct media_device
> + * @kref:	Object refcount
> + */
> +struct media_device_devres {
> +	struct media_device mdev;
> +	struct kref kref;
> +};
> +
>  /* We don't need to include pci.h or usb.h here */
>  struct pci_dev;
>  struct usb_device;
> @@ -604,6 +615,19 @@ struct media_device *media_device_get_devres(struct
> device *dev); */
>  struct media_device *media_device_find_devres(struct device *dev);
> 
> +/**
> + * media_device_unregister_devres) - Unregister media device allocated as
> + *				     as device resource
> + *
> + * @dev: pointer to struct &device.
> + *
> + * Devices allocated via media_device_get_devres should be de-alocalted
> + * and freed via this function. Callers should not call
> + * media_device_unregister() nor media_device_cleanup() on devices
> + * allocated via media_device_get_devres().
> + */
> +void media_device_unregister_devres(struct media_device *mdev);
> +
>  /* Iterate over all entities. */
>  #define media_device_for_each_entity(entity, mdev)			\
>  	list_for_each_entry(entity, &(mdev)->entities, graph_obj.list)
> @@ -688,6 +712,10 @@ static inline struct media_device
> *media_device_find_devres(struct device *dev) return NULL;
>  }
> 
> +static inline void media_device_unregister_devres(struct media_device
> *mdev)
> +{
> +}
> +
>  static inline void media_device_pci_init(struct media_device *mdev,
>  					 struct pci_dev *pci_dev,
>  					 char *name)
> diff --git a/sound/usb/media.c b/sound/usb/media.c
> index 93a50d01490c..f78955fd0d6e 100644
> --- a/sound/usb/media.c
> +++ b/sound/usb/media.c
> @@ -311,8 +311,7 @@ void media_snd_device_delete(struct snd_usb_audio *chip)
> media_snd_mixer_delete(chip);
> 
>  	if (mdev) {
> -		if (media_devnode_is_registered(&mdev->devnode))
> -			media_device_unregister(mdev);
> +		media_device_unregister_devres(mdev);
>  		chip->media_dev = NULL;
>  	}
>  }
  

Hi Laurent,

Thanks for reviewing it.

Em Wed, 23 Mar 2016 18:57:50 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> On Friday 18 Mar 2016 21:42:16 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>
> > ---
> > 
> > v2: The kref is now used only when media_device is allocated via
> >     the media_device*_devress. This warrants that other drivers won't be
> >     affected, and that we can keep media_device_cleanup() balanced with
> >     media_device_init().
> > 
> >  drivers/media/media-device.c           | 117 ++++++++++++++++++++++--------
> >  drivers/media/usb/au0828/au0828-core.c |   3 +-
> >  include/media/media-device.h           |  28 ++++++++
> >  sound/usb/media.c                      |   3 +-
> >  4 files changed, 118 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index c32fa15cc76e..4a97d92a7e7d 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -707,11 +707,16 @@ void media_device_init(struct media_device *mdev)
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_init);
> > 
> > -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);
> > +}
> > +
> > +void media_device_cleanup(struct media_device *mdev)
> > +{
> > +	__media_device_cleanup(mdev);
> >  	mutex_destroy(&mdev->graph_mutex);
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_cleanup);
> > @@ -721,6 +726,9 @@ int __must_check __media_device_register(struct
> > media_device *mdev, {
> >  	int ret;
> > 
> > +	/* Check if mdev was ever registered at all */  
> 
> This comment doesn't seem to apply to the next line, is it a leftover ? If so, 
> please remove it.

Yes, it is a left over. I'll remove it. Thanks for noticing it.

Thanks,
Mauro
--
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