Re: [PATCH v4 1/3] media: Media Device Allocator API

[Sakari Ailus]

Hi Shuah,

On Wed, Nov 16, 2016 at 07:29:09AM -0700, Shuah Khan wrote:
> Media Device Allocator API to allows multiple drivers share a media device.
> Using this API, drivers can allocate a media device with the shared struct
> device as the key. Once the media device is allocated by a driver, other
> drivers can get a reference to it. The media device is released when all
> the references are released.
> 
> Signed-off-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> Changes since v3:
> - Fixed undefined reference to `__media_device_usb_init compile error when
>   CONFIG_USB is disabled.
> - Fixed kernel paging error when accessing /dev/mediaX after rmmod of the
>   module that owns the media_device. The fix bumps the reference count for
>   the owner when second driver comes along to share the media_device. If
>   au0828 owns the media_device, then snd_usb_audio will bump the refcount
>   for au0828, so it won't get deleted and vice versa.
> 
>  drivers/media/Makefile              |   3 +-
>  drivers/media/media-dev-allocator.c | 146 ++++++++++++++++++++++++++++++++++++
>  include/media/media-dev-allocator.h |  87 +++++++++++++++++++++
>  3 files changed, 235 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 0deaa93..7c0701d 100644
> --- a/drivers/media/Makefile
> +++ b/drivers/media/Makefile
> @@ -6,7 +6,8 @@ ifeq ($(CONFIG_MEDIA_CEC_EDID),y)
>    obj-$(CONFIG_MEDIA_SUPPORT) += cec-edid.o
>  endif
>  
> -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..014a317
> --- /dev/null
> +++ b/drivers/media/media-dev-allocator.c
> @@ -0,0 +1,146 @@
> +/*
> + * media-dev-allocator.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 a global refcounted Media Controller Device Instance API.
> + * A system wide global media device list is managed and each media device
> + * includes a kref count. The last put on the media device releases the media
> + * device instance.
> + *
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kref.h>
> +#include <linux/usb.h>
> +#include <linux/module.h>
> +#include <media/media-device.h>

Alphabetical order, please.

> +
> +static LIST_HEAD(media_device_list);
> +static DEFINE_MUTEX(media_device_lock);
> +
> +struct media_device_instance {
> +	struct media_device mdev;
> +	struct module *owner;
> +	struct list_head list;
> +	struct device *dev;

We've got a struct device pointer in struct media_device. Should we have the
same here, or use the value in struct media_device?

> +	struct kref refcount;
> +};
> +
> +static inline struct media_device_instance *
> +to_media_device_instance(struct media_device *mdev)
> +{
> +	return container_of(mdev, struct media_device_instance, mdev);
> +}
> +
> +static void media_device_instance_release(struct kref *kref)
> +{
> +	struct media_device_instance *mdi =
> +		container_of(kref, struct media_device_instance, refcount);
> +
> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
> +
> +	mutex_lock(&media_device_lock);

Do you need the lock during unregister / cleanup? Isn't it just to serialise
the access to the linked list?

> +
> +	media_device_unregister(&mdi->mdev);
> +	media_device_cleanup(&mdi->mdev);
> +
> +	list_del(&mdi->list);
> +	mutex_unlock(&media_device_lock);
> +
> +	kfree(mdi);
> +}
> +
> +/* Callers should hold media_device_lock when calling this function */
> +static struct media_device *__media_device_get(struct device *dev,
> +					       char *module_name)
> +{
> +	struct media_device_instance *mdi;
> +
> +	list_for_each_entry(mdi, &media_device_list, list) {
> +		if (mdi->dev == dev) {

You could make this look nicer if you continue here if mdi->dev != dev.

> +			kref_get(&mdi->refcount);
> +			/* get module reference for the media_device owner */
> +			if (find_module(module_name) != mdi->owner &&
> +			    !try_module_get(mdi->owner))
> +				dev_err(dev, "%s: try_module_get() error\n",
> +					__func__);
> +			dev_dbg(dev, "%s: get mdev=%p module_name %s\n",
> +				__func__, &mdi->mdev, module_name);
> +			return &mdi->mdev;
> +		}
> +	}
> +
> +	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
> +	if (!mdi)
> +		return NULL;
> +
> +	mdi->dev = dev;
> +	mdi->owner = find_module(module_name);
> +	kref_init(&mdi->refcount);
> +	list_add_tail(&mdi->list, &media_device_list);
> +
> +	dev_dbg(dev, "%s: alloc mdev=%p module_name %s\n", __func__,

Does the pointer value provide useful information here? The media device is
specific to a device and there's just one per device. The device name is
already printed anyway.

> +		&mdi->mdev, module_name);
> +	return &mdi->mdev;
> +}
> +
> +#if IS_ENABLED(CONFIG_USB)
> +struct media_device *media_device_usb_allocate(struct usb_device *udev,
> +					       char *module_name)

How about adding a macro bearing this name, and using KBUILD_MODNAME there?
It'd look cleaner in drivers. That's what e.g. media_device_register() does,
for instance.

> +{
> +	struct media_device *mdev;
> +
> +	if (!module_name) {
> +		dev_err(&udev->dev, "%s Module Name is null\n", __func__);

You use colon (":") elsewhere after the function name, how about here as
well? "Name" could begin with lowercase letter, too.

> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	mutex_lock(&media_device_lock);
> +	mdev = __media_device_get(&udev->dev, module_name);
> +	if (!mdev) {
> +		mutex_unlock(&media_device_lock);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* check if media device is already initialized */
> +	if (!mdev->dev)

I think I would look up a device explicitly, and allocate one if needed.
This would mean splitting __media_device_get(), you could return the device
early if found, and then initialise without a need to check anything.

> +		__media_device_usb_init(mdev, udev, udev->product,
> +					module_name);

When is the media device registered and by which party?

I guess you can register entities once the media device is registered.
Nothing I can think of should prevent that, but I don't think it's been done
before. There could be bugs.

> +	mutex_unlock(&media_device_lock);
> +
> +	dev_dbg(&udev->dev, "%s\n", __func__);

This was probably nice to have during development time but should be removed
now.

> +	return mdev;
> +}
> +EXPORT_SYMBOL_GPL(media_device_usb_allocate);
> +#endif
> +
> +void media_device_delete(struct media_device *mdev, char *module_name)
> +{
> +	struct media_device_instance *mdi = to_media_device_instance(mdev);
> +
> +	if (!module_name) {
> +		dev_err(mdi->mdev.dev, "%s Module Name is null\n", __func__);

Can this happen? If so, why? What if the driver is linked to the kernel
instead?

> +		return;
> +	}
> +
> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p module_name %s\n",
> +		__func__, &mdi->mdev, module_name);
> +
> +	mutex_lock(&media_device_lock);
> +	/* put module reference if media_device owner is not THIS_MODULE */
> +	if (mdi->owner != find_module(module_name)) {
> +		module_put(mdi->owner);
> +		dev_dbg(mdi->mdev.dev,
> +			"%s decremented owner module reference\n", __func__);
> +	}
> +	mutex_unlock(&media_device_lock);
> +	kref_put(&mdi->refcount, media_device_instance_release);
> +}
> +EXPORT_SYMBOL_GPL(media_device_delete);
> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
> new file mode 100644
> index 0000000..45c437d
> --- /dev/null
> +++ b/include/media/media-dev-allocator.h
> @@ -0,0 +1,87 @@
> +/*
> + * media-dev-allocator.h - 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 a global ref-counted Media Controller Device Instance API.
> + * A system wide global media device list is managed and each media device
> + * includes a kref count. The last put on the media device releases the media
> + * device instance.
> + */
> +
> +#ifndef _MEDIA_DEV_ALLOCTOR_H
> +#define _MEDIA_DEV_ALLOCTOR_H
> +
> +struct usb_device;
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +/**
> + * DOC: Media Controller Device Allocator API
> + *
> + * When media device belongs to more than one driver, the shared media device
> + * is allocated with the shared struct device as the key for look ups.
> + *
> + * Shared media device should stay in registered state until the last driver
> + * unregisters it. In addition, media device should be released when all the

The last or the first?

Presumably any driver that acquires the media device this way can create
entities, and these entities would be removed when that driver releases the
media device. We don't refcount graph objects at the moment, so if these
objects are referred to elsewhere in the kernel while that driver goes away,
bad things will follow.

It's not the only place this kind of problems exist at the moment though, so
the solution could perhaps be left for later as well. The patchset I've been
pushing solves, as far as I understand, the ones that pre-exist w/o this
patchset. The solutions need some reworking if this set is applied, as
there's no longer a single driver responsible for a media device.

> + * references are released. Each driver gets a reference to the media device
> + * during probe, when it allocates the media device. If media device is already
> + * allocated, allocate API bumps up the refcount and return the existing media
> + * device. Driver puts the reference back from its disconnect routine when it
> + * calls media_device_delete().
> + *
> + * Media device is unregistered and cleaned up from the kref put handler to
> + * ensure that the media device stays in registered state until the last driver
> + * unregisters the media device.
> + *
> + * Driver Usage:
> + *
> + * Drivers should use the media-core routines to get register reference and
> + * call media_device_delete() routine to make sure the shared media device
> + * delete is handled correctly.
> + *
> + * driver probe:
> + *	Call media_device_usb_allocate() to allocate or get a reference
> + *	Call media_device_register(), if media devnode isn't registered
> + *
> + * driver disconnect:
> + *	Call media_device_delete() to free the media_device. Free'ing is
> + *	handled by the put handler.
> + *
> + */
> +
> +/**
> + * media_device_usb_allocate() - Allocate and return media device
> + *
> + * @udev - struct usb_device pointer
> + * @module_name
> + *
> + * This interface should be called to allocate a media device when multiple
> + * drivers share usb_device and the media device. This interface allocates
> + * media device and calls media_device_usb_init() to initialize it.
> + *
> + */
> +struct media_device *media_device_usb_allocate(struct usb_device *udev,
> +					       char *module_name);
> +/**
> + * media_device_delete() - Release media device. Calls kref_put().
> + *
> + * @mdev - struct media_device pointer
> + * @module_name
> + *
> + * This interface should be called to put Media Device Instance kref.
> + */
> +void media_device_delete(struct media_device *mdev, char *driver_name);
> +#else
> +static inline struct media_device *media_device_usb_allocate(
> +			struct usb_device *udev, char *driver_name)
> +			{ return NULL; }
> +static inline void media_device_delete(
> +			struct media_device *mdev, char *module_name) { }
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +#endif

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx