Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1371165 > unrolled thread

[RFC PATCH v2 0/5] Media Device Allocator API

Started byShuah Khan <shuahkh@osg.samsung.com>
First post2016-04-05 05:40 +0200
Last post2016-04-05 19:00 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v2 0/5] Media Device Allocator API Shuah Khan <shuahkh@osg.samsung.com> - 2016-04-05 05:40 +0200
    [RFC PATCH v2 2/5] media: Add driver count to keep track of media device registrations Shuah Khan <shuahkh@osg.samsung.com> - 2016-04-05 05:40 +0200
    [RFC PATCH v2 3/5] media: uvcvideo change to use Media Device Allocator API Shuah Khan <shuahkh@osg.samsung.com> - 2016-04-05 05:40 +0200
    [RFC PATCH v2 4/5] media: au0828 change to use Media Device Allocator API Shuah Khan <shuahkh@osg.samsung.com> - 2016-04-05 05:40 +0200
    Re: [RFC PATCH v2 0/5] Media Device Allocator API Takashi Iwai <tiwai@suse.de> - 2016-04-05 08:20 +0200
      Re: [RFC PATCH v2 0/5] Media Device Allocator API Shuah Khan <shuahkh@osg.samsung.com> - 2016-04-05 15:30 +0200
      Re: [RFC PATCH v2 0/5] Media Device Allocator API Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-04-05 19:00 +0200

#1371165 — [RFC PATCH v2 0/5] Media Device Allocator API

FromShuah Khan <shuahkh@osg.samsung.com>
Date2016-04-05 05:40 +0200
Subject[RFC PATCH v2 0/5] Media Device Allocator API
Message-ID<rkq0p-59v-3@gated-at.bofh.it>
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 Device can be in any the following states:

- Allocated
- Registered (could be tied to more than one driver)
- Unregistered, not in use (media device file is not open)
- Unregistered, in use (media device file is not open)
- Released

When media device belongs to  more than one driver, registrations should be
tracked to avoid unregistering when one of the drivers does unregister. A new
num_drivers field in the struct media_device covers this case. The media device
should be unregistered only when the last unregister occurs with num_drivers
count zero.

When a media device is in use when it is unregistered, it should not be
released until the application exits when it detects the unregistered
status. Media device that is in use when it is unregistered is moved to
to_delete_list. When the last unregister occurs, media device is unregistered
and becomes an unregistered, still allocated device. Unregister marks the
device to be deleted.

When media device belongs to more than one driver, as both drivers could be
unbound/bound, driver should not end up getting stale media device that is
on its way out. Moving the unregistered media device to to_delete_list helps
this case as well.

I ran bind/unbind loop tests on uvcvideo, au0828, and snd-usb-audio while
running application that does ioctls. Didn't see any use-after-free errors
on media device. A couple of known issues seen:

1. When application exits, cdev_put() gets called after media device is
   released. This is a known issue to resolve and Media Device Allocator
   can't solve this one.
2. When au0828 module is removed and then ioctls fail when cdev_get() looks
   for the owning module as au0828 is very often the module that owns the
   media devnode. This is a cdev related issue that needs to be resolved and
   Media Device Allocator can't solve this one.

Shuah Khan (5):
  media: Add Media Device Allocator API
  media: Add driver count to keep track of media device registrations
  media: uvcvideo change to use Media Device Allocator API
  media: au0828 change to use Media Device Allocator API
  sound/usb: Use Media Controller API to share media resources

 drivers/media/Makefile                 |   3 +-
 drivers/media/media-dev-allocator.c    | 154 ++++++++++++++++
 drivers/media/media-device.c           |  49 ++++-
 drivers/media/media-devnode.c          |  10 +-
 drivers/media/usb/au0828/au0828-core.c |  40 +++--
 drivers/media/usb/au0828/au0828.h      |   1 +
 drivers/media/usb/uvc/uvc_driver.c     |  36 ++--
 drivers/media/usb/uvc/uvcvideo.h       |   3 +-
 include/media/media-dev-allocator.h    | 116 ++++++++++++
 include/media/media-device.h           |  31 ++++
 sound/usb/Kconfig                      |   4 +
 sound/usb/Makefile                     |   2 +
 sound/usb/card.c                       |  14 ++
 sound/usb/card.h                       |   3 +
 sound/usb/media.c                      | 320 +++++++++++++++++++++++++++++++++
 sound/usb/media.h                      |  73 ++++++++
 sound/usb/mixer.h                      |   3 +
 sound/usb/pcm.c                        |  28 ++-
 sound/usb/quirks-table.h               |   1 +
 sound/usb/stream.c                     |   2 +
 sound/usb/usbaudio.h                   |   6 +
 21 files changed, 862 insertions(+), 37 deletions(-)
 create mode 100644 drivers/media/media-dev-allocator.c
 create mode 100644 include/media/media-dev-allocator.h
 create mode 100644 sound/usb/media.c
 create mode 100644 sound/usb/media.h

-- 
2.5.0

[toc] | [next] | [standalone]

#1371166 — [RFC PATCH v2 2/5] media: Add driver count to keep track of media device registrations

FromShuah Khan <shuahkh@osg.samsung.com>
Date2016-04-05 05:40 +0200
Subject[RFC PATCH v2 2/5] media: Add driver count to keep track of media device registrations
Message-ID<rkq0p-59v-9@gated-at.bofh.it>
In reply to#1371165
Add driver count to keep track of media device registrations to avoid
releasing the media device, when one of the drivers does unregister when
media device belongs to more than one driver. Also add a new interfaces
to unregister a media device allocated using Media Device Allocator and
a increment register count.

Change media_open() to get media device reference and put the reference
in media_release().

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/media-device.c  | 49 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/media/media-devnode.c | 10 ++++++---
 include/media/media-device.h  | 31 +++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 6e43c95..f22cf0f 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -36,6 +36,7 @@
 #include <media/media-device.h>
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
+#include <media/media-dev-allocator.h>
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 
@@ -743,12 +744,31 @@ int __must_check __media_device_register(struct media_device *mdev,
 		return ret;
 	}
 
-	dev_dbg(mdev->dev, "Media device registered\n");
+	mdev->num_drivers++;
+	dev_dbg(mdev->dev, "Media device registered num_drivers %d\n",
+		mdev->num_drivers);
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
+void media_device_register_ref(struct media_device *mdev)
+{
+	if (!mdev)
+		return;
+
+	mutex_lock(&mdev->graph_mutex);
+
+	/* Check if mdev is registered - bump registered driver count */
+	if (media_devnode_is_registered(&mdev->devnode))
+		mdev->num_drivers++;
+
+	dev_dbg(mdev->dev, "%s: mdev %p num_drivers %d\n", __func__, mdev,
+		mdev->num_drivers);
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_register_ref);
+
 int __must_check media_device_register_entity_notify(struct media_device *mdev,
 					struct media_entity_notify *nptr)
 {
@@ -820,6 +840,33 @@ void media_device_unregister(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
+void media_device_unregister_put(struct media_device *mdev)
+{
+	if (mdev == NULL)
+		return;
+
+	dev_dbg(mdev->dev, "%s: mdev %p num_drivers %d\n", __func__, mdev,
+		mdev->num_drivers);
+
+	mutex_lock(&mdev->graph_mutex);
+	mdev->num_drivers--;
+	if (mdev->num_drivers == 0) {
+		mutex_unlock(&mdev->graph_mutex);
+
+		/* unregister media device and cleanup */
+		media_device_unregister(mdev);
+		media_device_cleanup(mdev);
+
+		/* mark the media device for deletion */
+		media_device_set_to_delete_state(mdev->dev);
+	} else
+		mutex_unlock(&mdev->graph_mutex);
+
+	dev_dbg(mdev->dev, "%s: end mdev %p num_drivers %d\n", __func__, mdev,
+		mdev->num_drivers);
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_put);
+
 static void media_device_release_devres(struct device *dev, void *res)
 {
 }
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 29409f4..ec18815 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -44,6 +44,7 @@
 #include <linux/uaccess.h>
 
 #include <media/media-devnode.h>
+#include <media/media-dev-allocator.h>
 
 #define MEDIA_NUM_DEVICES	256
 #define MEDIA_NAME		"media"
@@ -173,7 +174,6 @@ static int media_open(struct inode *inode, struct file *filp)
 	}
 	/* and increase the device refcount */
 	get_device(&mdev->dev);
-	mutex_unlock(&media_devnode_lock);
 
 	filp->private_data = mdev;
 
@@ -182,11 +182,14 @@ static int media_open(struct inode *inode, struct file *filp)
 		if (ret) {
 			put_device(&mdev->dev);
 			filp->private_data = NULL;
-			return ret;
+			goto done;
 		}
 	}
 
-	return 0;
+	media_device_get_ref(mdev->parent);
+done:
+	mutex_unlock(&media_devnode_lock);
+	return ret;
 }
 
 /* Override for the release function */
@@ -201,6 +204,7 @@ static int media_release(struct inode *inode, struct file *filp)
 	   return value is ignored. */
 	put_device(&mdev->dev);
 	filp->private_data = NULL;
+	media_device_put(mdev->parent);
 	return 0;
 }
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index df74cfa..aaeac7a 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -284,6 +284,7 @@ struct media_entity_notify {
  * struct media_device - Media device
  * @dev:	Parent device
  * @devnode:	Media device node
+ * @num_drivers: Number of drivers that own the media device and register.
  * @driver_name: Optional device driver name. If not set, calls to
  *		%MEDIA_IOC_DEVICE_INFO will return dev->driver->name.
  *		This is needed for USB drivers for example, as otherwise
@@ -349,6 +350,7 @@ struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
 	struct media_devnode devnode;
+	int num_drivers;
 
 	char model[32];
 	char driver_name[32];
@@ -494,6 +496,17 @@ int __must_check __media_device_register(struct media_device *mdev,
 #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
 
 /**
+ * media_device_register_ref() - Increments media device register driver count
+ *
+ * @mdev:	pointer to struct &media_device
+ *
+ * When more than one driver is associated with the media device, it is
+ * necessary to maintain the number of registrations to avoid unregister
+ * when it is still in use.
+ */
+void media_device_register_ref(struct media_device *mdev);
+
+/**
  * __media_device_unregister() - Unegisters a media device element
  *
  * @mdev:	pointer to struct &media_device
@@ -505,6 +518,18 @@ int __must_check __media_device_register(struct media_device *mdev,
 void media_device_unregister(struct media_device *mdev);
 
 /**
+ * media_device_unregister_put() - Unregisters a media device element
+ *
+ * @mdev:	pointer to struct &media_device
+ *
+ * Should be called to unregister media device allocated with Media Device
+ * Allocator API media_device_get() interface.
+ * It is safe to call this function on an unregistered (but initialised)
+ * media device.
+ */
+void media_device_unregister_put(struct media_device *mdev);
+
+/**
  * media_device_register_entity() - registers a media entity inside a
  *	previously registered media device.
  *
@@ -661,9 +686,15 @@ static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
 }
+static inline void media_device_register_ref(struct media_device *mdev)
+{
+}
 static inline void media_device_unregister(struct media_device *mdev)
 {
 }
+static inline void media_device_unregister_put(struct media_device *mdev)
+{
+}
 static inline int media_device_register_entity(struct media_device *mdev,
 						struct media_entity *entity)
 {
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1371168 — [RFC PATCH v2 3/5] media: uvcvideo change to use Media Device Allocator API

FromShuah Khan <shuahkh@osg.samsung.com>
Date2016-04-05 05:40 +0200
Subject[RFC PATCH v2 3/5] media: uvcvideo change to use Media Device Allocator API
Message-ID<rkq0p-59v-11@gated-at.bofh.it>
In reply to#1371165
Change uvcvideo to use Media Device Allocator API and new Media Controller
media_device_unregister_put() interface.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 36 ++++++++++++++++++++++--------------
 drivers/media/usb/uvc/uvcvideo.h   |  3 ++-
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 451e84e9..95e30c4 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1674,9 +1674,10 @@ static void uvc_delete(struct uvc_device *dev)
 	if (dev->vdev.dev)
 		v4l2_device_unregister(&dev->vdev);
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(&dev->mdev.devnode))
-		media_device_unregister(&dev->mdev);
-	media_device_cleanup(&dev->mdev);
+	if (media_devnode_is_registered(&dev->mdev->devnode)) {
+		media_device_unregister_put(dev->mdev);
+		media_device_put(dev->mdev->dev);
+	}
 #endif
 
 	list_for_each_safe(p, n, &dev->chains) {
@@ -1929,17 +1930,20 @@ static int uvc_probe(struct usb_interface *intf,
 
 	/* Initialize the media device and register the V4L2 device. */
 #ifdef CONFIG_MEDIA_CONTROLLER
-	dev->mdev.dev = &intf->dev;
-	strlcpy(dev->mdev.model, dev->name, sizeof(dev->mdev.model));
+	dev->mdev = media_device_get(&intf->dev);
+	if (!dev->mdev)
+		goto media_device_get_error;
+	dev->mdev->dev = &intf->dev;
+	strlcpy(dev->mdev->model, dev->name, sizeof(dev->mdev->model));
 	if (udev->serial)
-		strlcpy(dev->mdev.serial, udev->serial,
-			sizeof(dev->mdev.serial));
-	strcpy(dev->mdev.bus_info, udev->devpath);
-	dev->mdev.hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
-	dev->mdev.driver_version = LINUX_VERSION_CODE;
-	media_device_init(&dev->mdev);
-
-	dev->vdev.mdev = &dev->mdev;
+		strlcpy(dev->mdev->serial, udev->serial,
+			sizeof(dev->mdev->serial));
+	strcpy(dev->mdev->bus_info, udev->devpath);
+	dev->mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
+	dev->mdev->driver_version = LINUX_VERSION_CODE;
+	media_device_init(dev->mdev);
+
+	dev->vdev.mdev = dev->mdev;
 #endif
 	if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
 		goto error;
@@ -1958,7 +1962,7 @@ static int uvc_probe(struct usb_interface *intf,
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 	/* Register the media device node */
-	if (media_device_register(&dev->mdev) < 0)
+	if (media_device_register(dev->mdev) < 0)
 		goto error;
 #endif
 	/* Save our data pointer in the interface data. */
@@ -1976,6 +1980,10 @@ static int uvc_probe(struct usb_interface *intf,
 	return 0;
 
 error:
+#ifdef CONFIG_MEDIA_CONTROLLER
+	media_device_put(&intf->dev);
+media_device_get_error:
+#endif
 	uvc_unregister_video(dev);
 	return -ENODEV;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7e4d3ee..a5ef719 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -12,6 +12,7 @@
 #include <linux/uvcvideo.h>
 #include <linux/videodev2.h>
 #include <media/media-device.h>
+#include <media/media-dev-allocator.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fh.h>
@@ -543,7 +544,7 @@ struct uvc_device {
 
 	/* Video control interface */
 #ifdef CONFIG_MEDIA_CONTROLLER
-	struct media_device mdev;
+	struct media_device *mdev;
 #endif
 	struct v4l2_device vdev;
 	__u16 uvc_version;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1371169 — [RFC PATCH v2 4/5] media: au0828 change to use Media Device Allocator API

FromShuah Khan <shuahkh@osg.samsung.com>
Date2016-04-05 05:40 +0200
Subject[RFC PATCH v2 4/5] media: au0828 change to use Media Device Allocator API
Message-ID<rkq0q-59v-15@gated-at.bofh.it>
In reply to#1371165
Change au0828 to use Media Device Allocator API and new Media Controller
media_device_unregister_put() interface. Fix to unregister entity_notify
hook.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-core.c | 40 ++++++++++++++++++++++++----------
 drivers/media/usb/au0828/au0828.h      |  1 +
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index cc22b32..c34af36 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -131,22 +131,36 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
 	return status;
 }
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+static void au0828_media_graph_notify(struct media_entity *new,
+				      void *notify_data);
+#endif
+
 static void au0828_unregister_media_device(struct au0828_dev *dev)
 {
 
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (dev->media_dev &&
-		media_devnode_is_registered(&dev->media_dev->devnode)) {
-		/* clear enable_source, disable_source */
-		dev->media_dev->source_priv = NULL;
-		dev->media_dev->enable_source = NULL;
-		dev->media_dev->disable_source = NULL;
-
-		media_device_unregister(dev->media_dev);
-		media_device_cleanup(dev->media_dev);
-		kfree(dev->media_dev);
-		dev->media_dev = NULL;
+	struct media_device *mdev = dev->media_dev;
+	struct media_entity_notify *notify, *nextp;
+
+	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
+		return;
+
+	/* Remove au0828 entity_notify callbacks */
+	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list) {
+		if (notify->notify != au0828_media_graph_notify)
+			continue;
+			media_device_unregister_entity_notify(mdev, notify);
 	}
+
+	/* clear enable_source, disable_source */
+	dev->media_dev->source_priv = NULL;
+	dev->media_dev->enable_source = NULL;
+	dev->media_dev->disable_source = NULL;
+
+	media_device_unregister_put(dev->media_dev);
+	media_device_put(dev->media_dev->dev);
+	dev->media_dev = NULL;
 #endif
 }
 
@@ -198,7 +212,7 @@ static int au0828_media_device_init(struct au0828_dev *dev,
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_device *mdev;
 
-	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+	mdev = media_device_get(&udev->dev);
 	if (!mdev)
 		return -ENOMEM;
 
@@ -473,11 +487,13 @@ static int au0828_media_device_register(struct au0828_dev *dev,
 		/* register media device */
 		ret = media_device_register(dev->media_dev);
 		if (ret) {
+			media_device_put(dev->media_dev->dev);
 			dev_err(&udev->dev,
 				"Media Device Register Error: %d\n", ret);
 			return ret;
 		}
 	} else {
+		media_device_register_ref(dev->media_dev);
 		/*
 		 * Call au0828_media_graph_notify() to connect
 		 * audio graph to our graph. In this case, audio
diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h
index 87f3284..3edd50f 100644
--- a/drivers/media/usb/au0828/au0828.h
+++ b/drivers/media/usb/au0828/au0828.h
@@ -35,6 +35,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
 #include <media/media-device.h>
+#include <media/media-dev-allocator.h>
 
 /* DVB */
 #include "demux.h"
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1371258

FromTakashi Iwai <tiwai@suse.de>
Date2016-04-05 08:20 +0200
Message-ID<rksvg-7iO-5@gated-at.bofh.it>
In reply to#1371165
On Tue, 05 Apr 2016 05:35:55 +0200,
Shuah Khan wrote:
> 
> 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 Device can be in any the following states:
> 
> - Allocated
> - Registered (could be tied to more than one driver)
> - Unregistered, not in use (media device file is not open)
> - Unregistered, in use (media device file is not open)
> - Released
> 
> When media device belongs to  more than one driver, registrations should be
> tracked to avoid unregistering when one of the drivers does unregister. A new
> num_drivers field in the struct media_device covers this case. The media device
> should be unregistered only when the last unregister occurs with num_drivers
> count zero.
> 
> When a media device is in use when it is unregistered, it should not be
> released until the application exits when it detects the unregistered
> status. Media device that is in use when it is unregistered is moved to
> to_delete_list. When the last unregister occurs, media device is unregistered
> and becomes an unregistered, still allocated device. Unregister marks the
> device to be deleted.
> 
> When media device belongs to more than one driver, as both drivers could be
> unbound/bound, driver should not end up getting stale media device that is
> on its way out. Moving the unregistered media device to to_delete_list helps
> this case as well.
> 
> I ran bind/unbind loop tests on uvcvideo, au0828, and snd-usb-audio while
> running application that does ioctls. Didn't see any use-after-free errors
> on media device. A couple of known issues seen:
> 
> 1. When application exits, cdev_put() gets called after media device is
>    released. This is a known issue to resolve and Media Device Allocator
>    can't solve this one.
> 2. When au0828 module is removed and then ioctls fail when cdev_get() looks
>    for the owning module as au0828 is very often the module that owns the
>    media devnode. This is a cdev related issue that needs to be resolved and
>    Media Device Allocator can't solve this one.
> 
> Shuah Khan (5):
>   media: Add Media Device Allocator API
>   media: Add driver count to keep track of media device registrations
>   media: uvcvideo change to use Media Device Allocator API
>   media: au0828 change to use Media Device Allocator API
>   sound/usb: Use Media Controller API to share media resources

I don't think we need to include usb-audio patch at this stage yet.
The most important thing for now is to improve / stabilize the API
itself so that other drivers can use it as is.  Once when the API is
really stabilized, we create a solid git branch that may be based for
multiple subsystems, and I'll merge usb-audio stuff through sound git
tree.

Also, the previous usb-audio MC implementation had a few serious bugs,
including quirk NULL dereference.  See the bugzilla below for some fix
patches to 4.6-rc1:
  https://bugzilla.kernel.org/show_bug.cgi?id=115561
Feel free to fold them in, if they are still valid.


thanks,

Takashi

[toc] | [prev] | [next] | [standalone]

#1371565

FromShuah Khan <shuahkh@osg.samsung.com>
Date2016-04-05 15:30 +0200
Message-ID<rkzdo-4bJ-13@gated-at.bofh.it>
In reply to#1371258
On 04/05/2016 12:10 AM, Takashi Iwai wrote:
> On Tue, 05 Apr 2016 05:35:55 +0200,
> Shuah Khan wrote:
>>
>> 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 Device can be in any the following states:
>>
>> - Allocated
>> - Registered (could be tied to more than one driver)
>> - Unregistered, not in use (media device file is not open)
>> - Unregistered, in use (media device file is not open)
>> - Released
>>
>> When media device belongs to  more than one driver, registrations should be
>> tracked to avoid unregistering when one of the drivers does unregister. A new
>> num_drivers field in the struct media_device covers this case. The media device
>> should be unregistered only when the last unregister occurs with num_drivers
>> count zero.
>>
>> When a media device is in use when it is unregistered, it should not be
>> released until the application exits when it detects the unregistered
>> status. Media device that is in use when it is unregistered is moved to
>> to_delete_list. When the last unregister occurs, media device is unregistered
>> and becomes an unregistered, still allocated device. Unregister marks the
>> device to be deleted.
>>
>> When media device belongs to more than one driver, as both drivers could be
>> unbound/bound, driver should not end up getting stale media device that is
>> on its way out. Moving the unregistered media device to to_delete_list helps
>> this case as well.
>>
>> I ran bind/unbind loop tests on uvcvideo, au0828, and snd-usb-audio while
>> running application that does ioctls. Didn't see any use-after-free errors
>> on media device. A couple of known issues seen:
>>
>> 1. When application exits, cdev_put() gets called after media device is
>>    released. This is a known issue to resolve and Media Device Allocator
>>    can't solve this one.
>> 2. When au0828 module is removed and then ioctls fail when cdev_get() looks
>>    for the owning module as au0828 is very often the module that owns the
>>    media devnode. This is a cdev related issue that needs to be resolved and
>>    Media Device Allocator can't solve this one.
>>
>> Shuah Khan (5):
>>   media: Add Media Device Allocator API
>>   media: Add driver count to keep track of media device registrations
>>   media: uvcvideo change to use Media Device Allocator API
>>   media: au0828 change to use Media Device Allocator API
>>   sound/usb: Use Media Controller API to share media resources
> 
> I don't think we need to include usb-audio patch at this stage yet.
> The most important thing for now is to improve / stabilize the API
> itself so that other drivers can use it as is.  Once when the API is
> really stabilized, we create a solid git branch that may be based for
> multiple subsystems, and I'll merge usb-audio stuff through sound git
> tree.

Agreed. I included snd-usb-audio as it provides a good test case for
multiple driver use-case. Yes it is a good idea to have a git branch
for wider testing.

> 
> Also, the previous usb-audio MC implementation had a few serious bugs,
> including quirk NULL dereference.  See the bugzilla below for some fix
> patches to 4.6-rc1:
>   https://bugzilla.kernel.org/show_bug.cgi?id=115561
> Feel free to fold them in, if they are still valid.

I folded them in. It is unfortunate that these bugs were introduced towards
the end when I was making changes to address review comments and I didn't
catch them, especially the quirk NULL dereference.

thanks,
-- Shuah

[toc] | [prev] | [next] | [standalone]

#1371786

FromMauro Carvalho Chehab <mchehab@osg.samsung.com>
Date2016-04-05 19:00 +0200
Message-ID<rkCuD-74o-11@gated-at.bofh.it>
In reply to#1371258
Em Tue, 5 Apr 2016 08:10:11 +0200
Takashi Iwai <tiwai@suse.de> escreveu:

> On Tue, 05 Apr 2016 05:35:55 +0200,
> Shuah Khan wrote:
> > 
> > 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 Device can be in any the following states:
> > 
> > - Allocated
> > - Registered (could be tied to more than one driver)
> > - Unregistered, not in use (media device file is not open)
> > - Unregistered, in use (media device file is not open)
> > - Released
> > 
> > When media device belongs to  more than one driver, registrations should be
> > tracked to avoid unregistering when one of the drivers does unregister. A new
> > num_drivers field in the struct media_device covers this case. The media device
> > should be unregistered only when the last unregister occurs with num_drivers
> > count zero.
> > 
> > When a media device is in use when it is unregistered, it should not be
> > released until the application exits when it detects the unregistered
> > status. Media device that is in use when it is unregistered is moved to
> > to_delete_list. When the last unregister occurs, media device is unregistered
> > and becomes an unregistered, still allocated device. Unregister marks the
> > device to be deleted.
> > 
> > When media device belongs to more than one driver, as both drivers could be
> > unbound/bound, driver should not end up getting stale media device that is
> > on its way out. Moving the unregistered media device to to_delete_list helps
> > this case as well.
> > 
> > I ran bind/unbind loop tests on uvcvideo, au0828, and snd-usb-audio while
> > running application that does ioctls. Didn't see any use-after-free errors
> > on media device. A couple of known issues seen:
> > 
> > 1. When application exits, cdev_put() gets called after media device is
> >    released. This is a known issue to resolve and Media Device Allocator
> >    can't solve this one.
> > 2. When au0828 module is removed and then ioctls fail when cdev_get() looks
> >    for the owning module as au0828 is very often the module that owns the
> >    media devnode. This is a cdev related issue that needs to be resolved and
> >    Media Device Allocator can't solve this one.
> > 
> > Shuah Khan (5):
> >   media: Add Media Device Allocator API
> >   media: Add driver count to keep track of media device registrations
> >   media: uvcvideo change to use Media Device Allocator API
> >   media: au0828 change to use Media Device Allocator API
> >   sound/usb: Use Media Controller API to share media resources  
> 
> I don't think we need to include usb-audio patch at this stage yet.

I agree. Let's first fix MC races first, then address the multi-driver
issues. Only after having those fixed, we should look at the sound/usb
patch.

Ok, we could keep it on some testing tree, but, IMHO,  it doesn't make any
sense to submit it to review, while the core is not fixed.

> The most important thing for now is to improve / stabilize the API
> itself so that other drivers can use it as is.  Once when the API is
> really stabilized, we create a solid git branch that may be based for
> multiple subsystems, and I'll merge usb-audio stuff through sound git
> tree.

Works for me. After we have this properly fixed and stabilized on media,
I'll pass you a stable topic branch for you. This way, you can test a
new version of the sound/usb patch and apply on your tree when it fits
well for you.

> 
> Also, the previous usb-audio MC implementation had a few serious bugs,
> including quirk NULL dereference.  See the bugzilla below for some fix
> patches to 4.6-rc1:
>   https://bugzilla.kernel.org/show_bug.cgi?id=115561
> Feel free to fold them in, if they are still valid.
> 
> 
> thanks,
> 
> Takashi


-- 

Cheers,
Mauro

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web