[alsa-devel] [PATCH v2] [media] media-device: use kref for media_device instance
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 21 12:10:33 CET 2016
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 at 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
More information about the Alsa-devel
mailing list