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

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

}