Hi Shuah,
I reviewed the entire patch series, but I'm adding the comments only here,
as the other patches are coherent with this one.
Em Fri, 25 Mar 2016 22:38:42 -0600
Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
> Add Media Device Allocator API to manage Media Device life time problems.
> There are known problems with media device life time management. When media
> device is released while an media ioctl is in progress, ioctls fail with
> use-after-free errors and kernel hangs in some cases.
>
> Media Allocator API provides interfaces to allocate a refcounted media
> device from system wide global list and maintains the state until the
> last user of the media device releases it.
>
> Signed-off-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
> ---
> drivers/media/Makefile | 3 +-
> drivers/media/media-dev-allocator.c | 153 ++++++++++++++++++++++++++++++++++++
> include/media/media-dev-allocator.h | 81 +++++++++++++++++++
> 3 files changed, 236 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/media-dev-allocator.c
> create mode 100644 include/media/media-dev-allocator.h
>
> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> index e608bbc..b08f091 100644
> --- a/drivers/media/Makefile
> +++ b/drivers/media/Makefile
> @@ -2,7 +2,8 @@
> # Makefile for the kernel multimedia device drivers.
> #
>
> -media-objs := media-device.o media-devnode.o media-entity.o
> +media-objs := media-device.o media-devnode.o media-entity.o \
> + media-dev-allocator.o
>
> #
> # I2C drivers should come before other drivers, otherwise they'll fail
> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
> new file mode 100644
> index 0000000..51edc49
> --- /dev/null
> +++ b/drivers/media/media-dev-allocator.c
> @@ -0,0 +1,153 @@
> +/*
> + * media-devkref.c - Media Controller Device Allocator API
> + *
> + * Copyright (c) 2016 Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> + */
> +
> +/*
> + * This file adds Media Controller Device Instance with
> + * Kref support. A system wide global media device list
> + * is managed and each media device is refcounted. The
> + * last put on the media device releases the media device
> + * instance.
> +*/
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
No need for an ifdef here. This file will be compiled only
if CONFIG_MEDIA_CONTROLLER, as you added it for media.ko
dependencies at the Makefile.
> +
> +#include <linux/slab.h>
> +#include <linux/kref.h>
> +#include <media/media-device.h>
> +
> +static LIST_HEAD(media_device_list);
> +static LIST_HEAD(media_device_to_delete_list);
> +static DEFINE_MUTEX(media_device_lock);
> +
> +struct media_device_instance {
> + struct media_device mdev;
> + struct list_head list;
> + struct list_head to_delete_list;
> + struct device *dev;
> + struct kref refcount;
> + bool to_delete; /* should be set when devnode is deleted */
I don't think this is needed.
> +};
> +
> +static struct media_device *__media_device_get(struct device *dev,
> + bool alloc, bool kref)
> +{
Please don't use "kref" for a boolean here. Makes it confusing, as reviewers
would expect "kref" to be of "struct kref" type.
Also, alloc seems to be a bad name for the other bool.
Maybe you could call them as "do_alloc" and "create_kref".
> + struct media_device_instance *mdi;
> +
> + mutex_lock(&media_device_lock);
> +
> + list_for_each_entry(mdi, &media_device_list, list) {
> + if (mdi->dev == dev) {
> + if (kref) {
> + kref_get(&mdi->refcount);
> + pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> + }
> + goto done;
> + }
> + }
> +
> + if (!alloc) {
> + mdi = NULL;
> + goto done;
> + }
> +
> + mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
> + if (!mdi)
> + goto done;
> +
> + mdi->dev = dev;
> + kref_init(&mdi->refcount);
> + list_add_tail(&mdi->list, &media_device_list);
> + pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +
> +done:
> + mutex_unlock(&media_device_lock);
> +
> + return mdi ? &mdi->mdev : NULL;
> +}
> +
> +struct media_device *media_device_get(struct device *dev)
> +{
> + pr_info("%s\n", __func__);
> + return __media_device_get(dev, true, true);
> +}
> +EXPORT_SYMBOL_GPL(media_device_get);
> +
> +/* Don't increment kref - this is a search and find */
> +struct media_device *media_device_find(struct device *dev)
> +{
> + pr_info("%s\n", __func__);
> + return __media_device_get(dev, false, false);
> +}
> +EXPORT_SYMBOL_GPL(media_device_find);
> +
> +/* don't allocate - increment kref if one is found */
> +struct media_device *media_device_get_ref(struct device *dev)
> +{
> + pr_info("%s\n", __func__);
> + return __media_device_get(dev, false, true);
> +}
> +EXPORT_SYMBOL_GPL(media_device_get_ref);
> +
> +static void media_device_instance_release(struct kref *kref)
> +{
> + struct media_device_instance *mdi =
> + container_of(kref, struct media_device_instance, refcount);
> +
> + pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> +
> + list_del(&mdi->list);
> + kfree(mdi);
> +}
> +
> +void media_device_put(struct device *dev)
> +{
> + struct media_device_instance *mdi;
> +
> + mutex_lock(&media_device_lock);
> + /* search first in the media_device_list */
> + list_for_each_entry(mdi, &media_device_list, list) {
> + if (mdi->dev == dev) {
> + pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> + kref_put(&mdi->refcount, media_device_instance_release);
> + goto done;
> + }
> + }
> + /* search in the media_device_to_delete_list */
> + list_for_each_entry(mdi, &media_device_to_delete_list, to_delete_list) {
> + if (mdi->dev == dev) {
> + pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> + kref_put(&mdi->refcount, media_device_instance_release);
> + goto done;
> + }
> + }
> +done:
> + mutex_unlock(&media_device_lock);
> +}
> +EXPORT_SYMBOL_GPL(media_device_put);
You also need something to initialize the kref for places where
the struct media_device is embedded, or to convert all of them to use
dynamically-allocated media_device structs.
> +
> +void media_device_set_to_delete_state(struct device *dev)
> +{
> + struct media_device_instance *mdi;
> +
> + mutex_lock(&media_device_lock);
> + list_for_each_entry(mdi, &media_device_list, list) {
> + if (mdi->dev == dev) {
> + pr_info("%s: mdev=%p\n", __func__, &mdi->mdev);
> + mdi->to_delete = true;
> + list_move(&mdi->list, &media_device_to_delete_list);
> + goto done;
> + }
> + }
> +done:
> + mutex_unlock(&media_device_lock);
> +}
> +EXPORT_SYMBOL_GPL(media_device_set_to_delete_state);
> +
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
> new file mode 100644
> index 0000000..2932c90
> --- /dev/null
> +++ b/include/media/media-dev-allocator.h
> @@ -0,0 +1,81 @@
> +/*
> + * media-devkref.c - Media Controller Device Allocator API
> + *
> + * Copyright (c) 2016 Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> + */
> +
> +/*
> + * This file adds Media Controller Device Instance with Kref support.
> + * A system wide global media device list is managed and each media
> + * device is refcounted. The last put on the media device releases
> + * the media device instance.
> +*/
> +
> +#ifndef _MEDIA_DEV_ALLOCTOR_H
> +#define _MEDIA_DEV_ALLOCTOR_H
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +/**
> + * media_device_get() - Allocate and return global media device
> + *
> + * @mdev
> + *
> + * This interface should be called to allocate media device. A new media
> + * device instance is created and added to the system wide media device
> + * instance list. If media device instance exists, media_device_get()
> + * increments the reference count and returns the media device. When
> + * more than one driver control the media device, the first driver to
> + * probe will allocate and the second driver when it calls media_device_get()
> + * it will get a reference.
> + *
> + */
> +struct media_device *media_device_get(struct device *dev);
> +/**
> + * media_device_get_ref() - Get reference to an allocated and registered
> + * media device.
> + *
> + * @mdev
> + *
> + * This interface should be called to get a reference to an allocated media
> + * device. media_open() ioctl should call this to hold a reference to ensure
> + * the media device will not be released until the media_release() does a put
> + * on it.
> + */
> +struct media_device *media_device_get_ref(struct device *dev);
> +/**
> + * media_device_find() - Find an allocated and registered media device.
> + *
> + * @mdev
> + *
> + * This interface should be called to find a media device. This will not
> + * incremnet the reference count.
> + */
> +struct media_device *media_device_find(struct device *dev);
> +/**
> + * media_device_put() - Release refcounted media device. Calls kref_put()
> + *
> + * @mdev
> + *
> + * This interface should be called to decrement refcount.
> + */
> +void media_device_put(struct device *dev);
> +/**
> + * media_device_set_to_delete_state() - Set the state to be deleted.
> + *
> + * @mdev
> + *
> + * This interface is used to not release the media device under from
> + * an active ioctl if unregister happens.
> + */
> +void media_device_set_to_delete_state(struct device *dev);
> +#else
> +struct media_device *media_device_get(struct device *dev) { return NULL; }
> +struct media_device *media_device_find(struct device *dev) { return NULL; }
> +void media_device_put(struct media_device *mdev) { }
> +void media_device_set_to_delete_state(struct device *dev) { }
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +#endif
--
Thanks,
Mauro
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
