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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart