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

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

}