* [RFC v4 02/21] Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind"
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 03/21] Revert "[media] media-device: dynamically allocate struct media_devnode" Sakari Ailus
` (20 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
This reverts commit 5b28dde51d0c ("[media] media: fix use-after-free in
cdev_put() when app exits after driver unbind"). The commit was part of an
original patchset to avoid crashes when an unregistering device is in use.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 6 ++----
drivers/media/media-devnode.c | 48 +++++++++++++++++--------------------------
2 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index f2525eb..6f5ed09 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -721,16 +721,16 @@ int __must_check __media_device_register(struct media_device *mdev,
ret = media_devnode_register(mdev, devnode, owner);
if (ret < 0) {
- /* devnode free is handled in media_devnode_*() */
mdev->devnode = NULL;
+ kfree(devnode);
return ret;
}
ret = device_create_file(&devnode->dev, &dev_attr_model);
if (ret < 0) {
- /* devnode free is handled in media_devnode_*() */
mdev->devnode = NULL;
media_devnode_unregister(devnode);
+ kfree(devnode);
return ret;
}
@@ -810,8 +810,6 @@ void media_device_unregister(struct media_device *mdev)
if (media_devnode_is_registered(mdev->devnode)) {
device_remove_file(&mdev->devnode->dev, &dev_attr_model);
media_devnode_unregister(mdev->devnode);
- /* devnode free is handled in media_devnode_*() */
- mdev->devnode = NULL;
}
}
EXPORT_SYMBOL_GPL(media_device_unregister);
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 5b605ff..ecdc02d 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -63,8 +63,13 @@ static void media_devnode_release(struct device *cd)
struct media_devnode *devnode = to_media_devnode(cd);
mutex_lock(&media_devnode_lock);
+
+ /* Delete the cdev on this minor as well */
+ cdev_del(&devnode->cdev);
+
/* Mark device node number as free */
clear_bit(devnode->minor, media_devnode_nums);
+
mutex_unlock(&media_devnode_lock);
/* Release media_devnode and perform other cleanups as needed. */
@@ -72,7 +77,6 @@ static void media_devnode_release(struct device *cd)
devnode->release(devnode);
kfree(devnode);
- pr_debug("%s: Media Devnode Deallocated\n", __func__);
}
static struct bus_type media_bus_type = {
@@ -201,8 +205,6 @@ static int media_release(struct inode *inode, struct file *filp)
/* decrease the refcount unconditionally since the release()
return value is ignored. */
put_device(&devnode->dev);
-
- pr_debug("%s: Media Release\n", __func__);
return 0;
}
@@ -233,7 +235,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
if (minor == MEDIA_NUM_DEVICES) {
mutex_unlock(&media_devnode_lock);
pr_err("could not get a free minor\n");
- kfree(devnode);
return -ENFILE;
}
@@ -243,31 +244,27 @@ int __must_check media_devnode_register(struct media_device *mdev,
devnode->minor = minor;
devnode->media_dev = mdev;
- /* Part 1: Initialize dev now to use dev.kobj for cdev.kobj.parent */
- devnode->dev.bus = &media_bus_type;
- devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
- devnode->dev.release = media_devnode_release;
- if (devnode->parent)
- devnode->dev.parent = devnode->parent;
- dev_set_name(&devnode->dev, "media%d", devnode->minor);
- device_initialize(&devnode->dev);
-
/* Part 2: Initialize and register the character device */
cdev_init(&devnode->cdev, &media_devnode_fops);
devnode->cdev.owner = owner;
- devnode->cdev.kobj.parent = &devnode->dev.kobj;
ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
if (ret < 0) {
pr_err("%s: cdev_add failed\n", __func__);
- goto cdev_add_error;
+ goto error;
}
- /* Part 3: Add the media device */
- ret = device_add(&devnode->dev);
+ /* Part 3: Register the media device */
+ devnode->dev.bus = &media_bus_type;
+ devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
+ devnode->dev.release = media_devnode_release;
+ if (devnode->parent)
+ devnode->dev.parent = devnode->parent;
+ dev_set_name(&devnode->dev, "media%d", devnode->minor);
+ ret = device_register(&devnode->dev);
if (ret < 0) {
- pr_err("%s: device_add failed\n", __func__);
- goto device_add_error;
+ pr_err("%s: device_register failed\n", __func__);
+ goto error;
}
/* Part 4: Activate this minor. The char device can now be used. */
@@ -275,15 +272,12 @@ int __must_check media_devnode_register(struct media_device *mdev,
return 0;
-device_add_error:
- cdev_del(&devnode->cdev);
-cdev_add_error:
+error:
mutex_lock(&media_devnode_lock);
+ cdev_del(&devnode->cdev);
clear_bit(devnode->minor, media_devnode_nums);
- devnode->media_dev = NULL;
mutex_unlock(&media_devnode_lock);
- put_device(&devnode->dev);
return ret;
}
@@ -295,12 +289,8 @@ void media_devnode_unregister(struct media_devnode *devnode)
mutex_lock(&media_devnode_lock);
clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
- /* Delete the cdev on this minor as well */
- cdev_del(&devnode->cdev);
mutex_unlock(&media_devnode_lock);
- device_del(&devnode->dev);
- devnode->media_dev = NULL;
- put_device(&devnode->dev);
+ device_unregister(&devnode->dev);
}
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 03/21] Revert "[media] media-device: dynamically allocate struct media_devnode"
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
2016-11-08 13:55 ` [RFC v4 02/21] Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind" Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 04/21] media: Remove useless curly braces and parentheses Sakari Ailus
` (19 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
This reverts commit a087ce704b80 ("[media] media-device: dynamically
allocate struct media_devnode"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 44 +++++++++++-----------------------
drivers/media/media-devnode.c | 7 +-----
drivers/media/usb/au0828/au0828-core.c | 4 ++--
drivers/media/usb/uvc/uvc_driver.c | 2 +-
include/media/media-device.h | 5 +++-
include/media/media-devnode.h | 15 ++----------
6 files changed, 24 insertions(+), 53 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 6f5ed09..0e07300 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -417,7 +417,7 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
unsigned long __arg)
{
struct media_devnode *devnode = media_devnode_data(filp);
- struct media_device *dev = devnode->media_dev;
+ struct media_device *dev = to_media_device(devnode);
const struct media_ioctl_info *info;
void __user *arg = (void __user *)__arg;
char __karg[256], *karg = __karg;
@@ -493,7 +493,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
struct media_devnode *devnode = media_devnode_data(filp);
- struct media_device *dev = devnode->media_dev;
+ struct media_device *dev = to_media_device(devnode);
long ret;
switch (cmd) {
@@ -529,8 +529,7 @@ static const struct media_file_operations media_device_fops = {
static ssize_t show_model(struct device *cd,
struct device_attribute *attr, char *buf)
{
- struct media_devnode *devnode = to_media_devnode(cd);
- struct media_device *mdev = devnode->media_dev;
+ struct media_device *mdev = to_media_device(to_media_devnode(cd));
return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
}
@@ -703,34 +702,23 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
int __must_check __media_device_register(struct media_device *mdev,
struct module *owner)
{
- struct media_devnode *devnode;
int ret;
- devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
- if (!devnode)
- return -ENOMEM;
-
/* Register the device node. */
- mdev->devnode = devnode;
- devnode->fops = &media_device_fops;
- devnode->parent = mdev->dev;
- devnode->release = media_device_release;
+ mdev->devnode.fops = &media_device_fops;
+ mdev->devnode.parent = mdev->dev;
+ mdev->devnode.release = media_device_release;
/* Set version 0 to indicate user-space that the graph is static */
mdev->topology_version = 0;
- ret = media_devnode_register(mdev, devnode, owner);
- if (ret < 0) {
- mdev->devnode = NULL;
- kfree(devnode);
+ ret = media_devnode_register(&mdev->devnode, owner);
+ if (ret < 0)
return ret;
- }
- ret = device_create_file(&devnode->dev, &dev_attr_model);
+ ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
if (ret < 0) {
- mdev->devnode = NULL;
- media_devnode_unregister(devnode);
- kfree(devnode);
+ media_devnode_unregister(&mdev->devnode);
return ret;
}
@@ -781,7 +769,7 @@ void media_device_unregister(struct media_device *mdev)
mutex_lock(&mdev->graph_mutex);
/* Check if mdev was ever registered at all */
- if (!media_devnode_is_registered(mdev->devnode)) {
+ if (!media_devnode_is_registered(&mdev->devnode)) {
mutex_unlock(&mdev->graph_mutex);
return;
}
@@ -804,13 +792,9 @@ void media_device_unregister(struct media_device *mdev)
mutex_unlock(&mdev->graph_mutex);
- dev_dbg(mdev->dev, "Media device unregistered\n");
-
- /* 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);
- }
+ device_remove_file(&mdev->devnode.dev, &dev_attr_model);
+ dev_dbg(mdev->dev, "Media device unregistering\n");
+ media_devnode_unregister(&mdev->devnode);
}
EXPORT_SYMBOL_GPL(media_device_unregister);
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index ecdc02d..7481c96 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -44,7 +44,6 @@
#include <linux/uaccess.h>
#include <media/media-devnode.h>
-#include <media/media-device.h>
#define MEDIA_NUM_DEVICES 256
#define MEDIA_NAME "media"
@@ -75,8 +74,6 @@ static void media_devnode_release(struct device *cd)
/* Release media_devnode and perform other cleanups as needed. */
if (devnode->release)
devnode->release(devnode);
-
- kfree(devnode);
}
static struct bus_type media_bus_type = {
@@ -222,8 +219,7 @@ static const struct file_operations media_devnode_fops = {
.llseek = no_llseek,
};
-int __must_check media_devnode_register(struct media_device *mdev,
- struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
struct module *owner)
{
int minor;
@@ -242,7 +238,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
mutex_unlock(&media_devnode_lock);
devnode->minor = minor;
- devnode->media_dev = mdev;
/* Part 2: Initialize and register the character device */
cdev_init(&devnode->cdev, &media_devnode_fops);
diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index bf53553..321ea5c 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
struct media_device *mdev = dev->media_dev;
struct media_entity_notify *notify, *nextp;
- if (!mdev || !media_devnode_is_registered(mdev->devnode))
+ if (!mdev || !media_devnode_is_registered(&mdev->devnode))
return;
/* Remove au0828 entity_notify callbacks */
@@ -482,7 +482,7 @@ static int au0828_media_device_register(struct au0828_dev *dev,
if (!dev->media_dev)
return 0;
- if (!media_devnode_is_registered(dev->media_dev->devnode)) {
+ if (!media_devnode_is_registered(&dev->media_dev->devnode)) {
/* register media device */
ret = media_device_register(dev->media_dev);
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 87b2fc3b..52cdef6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1780,7 +1780,7 @@ 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))
+ if (media_devnode_is_registered(&dev->mdev.devnode))
media_device_unregister(&dev->mdev);
media_device_cleanup(&dev->mdev);
#endif
diff --git a/include/media/media-device.h b/include/media/media-device.h
index ef93e21..96de915 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -127,7 +127,7 @@ struct media_device_ops {
struct media_device {
/* dev->driver_data points to this struct. */
struct device *dev;
- struct media_devnode *devnode;
+ struct media_devnode devnode;
char model[32];
char driver_name[32];
@@ -172,6 +172,9 @@ struct usb_device;
#define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0
#define MEDIA_DEV_NOTIFY_POST_LINK_CH 1
+/* media_devnode to media_device */
+#define to_media_device(node) container_of(node, struct media_device, devnode)
+
/**
* media_entity_enum_init - Initialise an entity enumeration
*
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index d55ec2b..a68b4b6 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -33,8 +33,6 @@
#include <linux/device.h>
#include <linux/cdev.h>
-struct media_device;
-
/*
* Flag to mark the media_devnode struct as registered. Drivers must not touch
* this flag directly, it will be set and cleared by media_devnode_register and
@@ -85,8 +83,6 @@ struct media_file_operations {
* before registering the node.
*/
struct media_devnode {
- struct media_device *media_dev;
-
/* device ops */
const struct media_file_operations *fops;
@@ -109,8 +105,7 @@ struct media_devnode {
/**
* media_devnode_register - register a media device node
*
- * @mdev: struct media_device we want to register a device node
- * @devnode: media device node structure we want to register
+ * @devnode: struct media_devnode we want to register a device node
* @owner: should be filled with %THIS_MODULE
*
* The registration code assigns minor numbers and registers the new device node
@@ -123,8 +118,7 @@ struct media_devnode {
* the media_devnode structure is *not* called, so the caller is responsible for
* freeing any data.
*/
-int __must_check media_devnode_register(struct media_device *mdev,
- struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
struct module *owner);
/**
@@ -154,14 +148,9 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
* false otherwise.
*
* @devnode: pointer to struct &media_devnode.
- *
- * Note: If mdev is NULL, it also returns false.
*/
static inline int media_devnode_is_registered(struct media_devnode *devnode)
{
- if (!devnode)
- return false;
-
return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 04/21] media: Remove useless curly braces and parentheses
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
2016-11-08 13:55 ` [RFC v4 02/21] Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind" Sakari Ailus
2016-11-08 13:55 ` [RFC v4 03/21] Revert "[media] media-device: dynamically allocate struct media_devnode" Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-22 9:59 ` Laurent Pinchart
2016-11-08 13:55 ` [RFC v4 05/21] media: devnode: Rename mdev argument as devnode Sakari Ailus
` (18 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 0e07300..bb19c04 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -594,9 +594,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
&entity->pads[i].graph_obj);
/* invoke entity_notify callbacks */
- list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
- (notify)->notify(entity, notify->notify_data);
- }
+ list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
+ notify->notify(entity, notify->notify_data);
if (mdev->entity_internal_idx_max
>= mdev->pm_count_walk.ent_enum.idx_max) {
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [RFC v4 04/21] media: Remove useless curly braces and parentheses
2016-11-08 13:55 ` [RFC v4 04/21] media: Remove useless curly braces and parentheses Sakari Ailus
@ 2016-11-22 9:59 ` Laurent Pinchart
0 siblings, 0 replies; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-22 9:59 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, mchehab, shuahkh
Hi Sakari,
Thank you for the patch.
On Tuesday 08 Nov 2016 15:55:13 Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/media-device.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 0e07300..bb19c04 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -594,9 +594,8 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, &entity->pads[i].graph_obj);
>
> /* invoke entity_notify callbacks */
> - list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
> - (notify)->notify(entity, notify->notify_data);
> - }
> + list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
> + notify->notify(entity, notify->notify_data);
>
> if (mdev->entity_internal_idx_max
>
> >= mdev->pm_count_walk.ent_enum.idx_max) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC v4 05/21] media: devnode: Rename mdev argument as devnode
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (2 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 04/21] media: Remove useless curly braces and parentheses Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-22 10:00 ` Laurent Pinchart
2016-11-08 13:55 ` [RFC v4 06/21] media device: Drop nop release callback Sakari Ailus
` (17 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
Historically, mdev argument name was being used on both struct
media_device and struct media_devnode. Recently most occurrences of mdev
referring to struct media_devnode were replaced by devnode, which makes
more sense. Fix the last remaining occurrence.
Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index bb19c04..a9d543f 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -540,9 +540,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
* Registration/unregistration
*/
-static void media_device_release(struct media_devnode *mdev)
+static void media_device_release(struct media_devnode *devnode)
{
- dev_dbg(mdev->parent, "Media device released\n");
+ dev_dbg(devnode->parent, "Media device released\n");
}
/**
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [RFC v4 05/21] media: devnode: Rename mdev argument as devnode
2016-11-08 13:55 ` [RFC v4 05/21] media: devnode: Rename mdev argument as devnode Sakari Ailus
@ 2016-11-22 10:00 ` Laurent Pinchart
0 siblings, 0 replies; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-22 10:00 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, mchehab, shuahkh
Hi Sakari,
Thank you for the patch.
On Tuesday 08 Nov 2016 15:55:14 Sakari Ailus wrote:
> Historically, mdev argument name was being used on both struct
> media_device and struct media_devnode. Recently most occurrences of mdev
> referring to struct media_devnode were replaced by devnode, which makes
> more sense. Fix the last remaining occurrence.
>
> Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/media-device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index bb19c04..a9d543f 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -540,9 +540,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
> * Registration/unregistration
> */
>
> -static void media_device_release(struct media_devnode *mdev)
> +static void media_device_release(struct media_devnode *devnode)
> {
> - dev_dbg(mdev->parent, "Media device released\n");
> + dev_dbg(devnode->parent, "Media device released\n");
> }
>
> /**
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC v4 06/21] media device: Drop nop release callback
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (3 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 05/21] media: devnode: Rename mdev argument as devnode Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-22 10:01 ` Laurent Pinchart
2016-11-08 13:55 ` [RFC v4 07/21] media-device: Make devnode.dev->kobj parent of devnode.cdev Sakari Ailus
` (16 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
The release callback is only used to print a debug message. Drop it. (It
will be re-introduced later in a different form.)
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index a9d543f..a31329d 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -540,11 +540,6 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
* Registration/unregistration
*/
-static void media_device_release(struct media_devnode *devnode)
-{
- dev_dbg(devnode->parent, "Media device released\n");
-}
-
/**
* media_device_register_entity - Register an entity with a media device
* @mdev: The media device
@@ -706,7 +701,6 @@ int __must_check __media_device_register(struct media_device *mdev,
/* Register the device node. */
mdev->devnode.fops = &media_device_fops;
mdev->devnode.parent = mdev->dev;
- mdev->devnode.release = media_device_release;
/* Set version 0 to indicate user-space that the graph is static */
mdev->topology_version = 0;
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [RFC v4 06/21] media device: Drop nop release callback
2016-11-08 13:55 ` [RFC v4 06/21] media device: Drop nop release callback Sakari Ailus
@ 2016-11-22 10:01 ` Laurent Pinchart
0 siblings, 0 replies; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-22 10:01 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, mchehab, shuahkh
Hi Sakari,
Thank you for the patch.
On Tuesday 08 Nov 2016 15:55:15 Sakari Ailus wrote:
> The release callback is only used to print a debug message. Drop it. (It
> will be re-introduced later in a different form.)
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/media-device.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index a9d543f..a31329d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -540,11 +540,6 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
> * Registration/unregistration
> */
>
> -static void media_device_release(struct media_devnode *devnode)
> -{
> - dev_dbg(devnode->parent, "Media device released\n");
> -}
> -
> /**
> * media_device_register_entity - Register an entity with a media device
> * @mdev: The media device
> @@ -706,7 +701,6 @@ int __must_check __media_device_register(struct
> media_device *mdev, /* Register the device node. */
> mdev->devnode.fops = &media_device_fops;
> mdev->devnode.parent = mdev->dev;
> - mdev->devnode.release = media_device_release;
>
> /* Set version 0 to indicate user-space that the graph is static */
> mdev->topology_version = 0;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC v4 07/21] media-device: Make devnode.dev->kobj parent of devnode.cdev
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (4 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 06/21] media device: Drop nop release callback Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 08/21] media: Enable allocating the media device dynamically Sakari Ailus
` (15 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
The struct cdev embedded in struct media_devnode contains its own kobj.
Instead of trying to manage its lifetime separately from struct
media_devnode, make the cdev kobj a parent of the struct media_device.dev
kobj.
The cdev will thus be released during unregistering the media_devnode, not
in media_devnode.dev kobj's release callback.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-devnode.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 7481c96..a8302fc 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -63,9 +63,6 @@ static void media_devnode_release(struct device *cd)
mutex_lock(&media_devnode_lock);
- /* Delete the cdev on this minor as well */
- cdev_del(&devnode->cdev);
-
/* Mark device node number as free */
clear_bit(devnode->minor, media_devnode_nums);
@@ -241,6 +238,7 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
/* Part 2: Initialize and register the character device */
cdev_init(&devnode->cdev, &media_devnode_fops);
+ devnode->cdev.kobj.parent = &devnode->dev.kobj;
devnode->cdev.owner = owner;
ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
@@ -285,6 +283,7 @@ void media_devnode_unregister(struct media_devnode *devnode)
mutex_lock(&media_devnode_lock);
clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
mutex_unlock(&media_devnode_lock);
+ cdev_del(&devnode->cdev);
device_unregister(&devnode->dev);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (5 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 07/21] media-device: Make devnode.dev->kobj parent of devnode.cdev Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 19:20 ` Shuah Khan
2016-11-08 13:55 ` [RFC v4 09/21] media: Split initialising and adding media devnode Sakari Ailus
` (14 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart, Sakari Ailus
From: Sakari Ailus <sakari.ailus@iki.fi>
Allow allocating the media device dynamically. As the struct media_device
embeds struct media_devnode, the lifetime of that object is that same than
that of the media_device.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 15 +++++++++++++++
include/media/media-device.h | 13 +++++++++++++
2 files changed, 28 insertions(+)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index a31329d..496195e 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
}
EXPORT_SYMBOL_GPL(media_device_init);
+struct media_device *media_device_alloc(struct device *dev)
+{
+ struct media_device *mdev;
+
+ mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+ if (!mdev)
+ return NULL;
+
+ mdev->dev = dev;
+ media_device_init(mdev);
+
+ return mdev;
+}
+EXPORT_SYMBOL_GPL(media_device_alloc);
+
void media_device_cleanup(struct media_device *mdev)
{
ida_destroy(&mdev->entity_internal_idx);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 96de915..c9b5798 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -207,6 +207,15 @@ static inline __must_check int media_entity_enum_init(
void media_device_init(struct media_device *mdev);
/**
+ * media_device_alloc() - Allocate and initialise a media device
+ *
+ * @dev: The associated struct device pointer
+ *
+ * Allocate and initialise a media device. Returns a media device.
+ */
+struct media_device *media_device_alloc(struct device *dev);
+
+/**
* media_device_cleanup() - Cleanups a media device element
*
* @mdev: pointer to struct &media_device
@@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device *mdev,
const char *driver_name);
#else
+static inline struct media_device *media_device_alloc(struct device *dev)
+{
+ return NULL;
+}
static inline int media_device_register(struct media_device *mdev)
{
return 0;
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-08 13:55 ` [RFC v4 08/21] media: Enable allocating the media device dynamically Sakari Ailus
@ 2016-11-08 19:20 ` Shuah Khan
2016-11-10 23:53 ` Laurent Pinchart
2016-11-14 13:40 ` Sakari Ailus
0 siblings, 2 replies; 43+ messages in thread
From: Shuah Khan @ 2016-11-08 19:20 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, hverkuil, mchehab, shuahkh, laurent.pinchart, Sakari Ailus
On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
>
> Allow allocating the media device dynamically. As the struct media_device
> embeds struct media_devnode, the lifetime of that object is that same than
> that of the media_device.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/media-device.c | 15 +++++++++++++++
> include/media/media-device.h | 13 +++++++++++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index a31329d..496195e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> }
> EXPORT_SYMBOL_GPL(media_device_init);
>
> +struct media_device *media_device_alloc(struct device *dev)
> +{
> + struct media_device *mdev;
> +
> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> + if (!mdev)
> + return NULL;
> +
> + mdev->dev = dev;
> + media_device_init(mdev);
> +
> + return mdev;
> +}
> +EXPORT_SYMBOL_GPL(media_device_alloc);
> +
One problem with this allocation is, this media device can't be shared across
drivers. For au0828 and snd-usb-audio should be able to share the
media_device. That is what the Media Allocator API patch series does.
This a quick review and I will review the patch series and get back to
you.
thanks,
-- Shuah
> void media_device_cleanup(struct media_device *mdev)
> {
> ida_destroy(&mdev->entity_internal_idx);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 96de915..c9b5798 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -207,6 +207,15 @@ static inline __must_check int media_entity_enum_init(
> void media_device_init(struct media_device *mdev);
>
> /**
> + * media_device_alloc() - Allocate and initialise a media device
> + *
> + * @dev: The associated struct device pointer
> + *
> + * Allocate and initialise a media device. Returns a media device.
> + */
> +struct media_device *media_device_alloc(struct device *dev);
> +
> +/**
> * media_device_cleanup() - Cleanups a media device element
> *
> * @mdev: pointer to struct &media_device
> @@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device *mdev,
> const char *driver_name);
>
> #else
> +static inline struct media_device *media_device_alloc(struct device *dev)
> +{
> + return NULL;
> +}
> static inline int media_device_register(struct media_device *mdev)
> {
> return 0;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-08 19:20 ` Shuah Khan
@ 2016-11-10 23:53 ` Laurent Pinchart
2016-11-11 0:00 ` Shuah Khan
2016-11-14 13:40 ` Sakari Ailus
1 sibling, 1 reply; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-10 23:53 UTC (permalink / raw)
To: Shuah Khan
Cc: Sakari Ailus, linux-media, hverkuil, mchehab, shuahkh, Sakari Ailus
Hi Shuah,
On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> >
> > Allow allocating the media device dynamically. As the struct media_device
> > embeds struct media_devnode, the lifetime of that object is that same than
> > that of the media_device.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >
> > drivers/media/media-device.c | 15 +++++++++++++++
> > include/media/media-device.h | 13 +++++++++++++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index a31329d..496195e 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> > }
> > EXPORT_SYMBOL_GPL(media_device_init);
> >
> > +struct media_device *media_device_alloc(struct device *dev)
> > +{
> > + struct media_device *mdev;
> > +
> > + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > + if (!mdev)
> > + return NULL;
> > +
> > + mdev->dev = dev;
> > + media_device_init(mdev);
> > +
> > + return mdev;
> > +}
> > +EXPORT_SYMBOL_GPL(media_device_alloc);
> > +
>
> One problem with this allocation is, this media device can't be shared
> across drivers. For au0828 and snd-usb-audio should be able to share the
> media_device. That is what the Media Allocator API patch series does.
No disagreement here, Sakari's patches don't address the issues that the media
allocator API fixes. The media allocator API, when ready, should replace (or
at least complement, if we decide to keep a simpler API for drivers that don't
need to share a media device, but I have no opinion on this at this time) this
allocation function.
> This a quick review and I will review the patch series and get back to
> you.
>
> > void media_device_cleanup(struct media_device *mdev)
> > {
> > ida_destroy(&mdev->entity_internal_idx);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index 96de915..c9b5798 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -207,6 +207,15 @@ static inline __must_check int
> > media_entity_enum_init(
> > void media_device_init(struct media_device *mdev);
> >
> > /**
> > + * media_device_alloc() - Allocate and initialise a media device
> > + *
> > + * @dev: The associated struct device pointer
> > + *
> > + * Allocate and initialise a media device. Returns a media device.
> > + */
> > +struct media_device *media_device_alloc(struct device *dev);
> > +
> > +/**
> > * media_device_cleanup() - Cleanups a media device element
> > *
> > * @mdev: pointer to struct &media_device
> > @@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device
> > *mdev,
> > const char *driver_name);
> > #else
> > +static inline struct media_device *media_device_alloc(struct device *dev)
> > +{
> > + return NULL;
> > +}
> > static inline int media_device_register(struct media_device *mdev)
> > {
> > return 0;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-10 23:53 ` Laurent Pinchart
@ 2016-11-11 0:00 ` Shuah Khan
2016-11-11 0:11 ` Laurent Pinchart
0 siblings, 1 reply; 43+ messages in thread
From: Shuah Khan @ 2016-11-11 0:00 UTC (permalink / raw)
To: Laurent Pinchart, Shuah Khan
Cc: Sakari Ailus, linux-media, hverkuil, mchehab, Sakari Ailus, Shuah Khan
On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
> Hi Shuah,
>
> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>
>>> Allow allocating the media device dynamically. As the struct media_device
>>> embeds struct media_devnode, the lifetime of that object is that same than
>>> that of the media_device.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>
>>> drivers/media/media-device.c | 15 +++++++++++++++
>>> include/media/media-device.h | 13 +++++++++++++
>>> 2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index a31329d..496195e 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>> }
>>> EXPORT_SYMBOL_GPL(media_device_init);
>>>
>>> +struct media_device *media_device_alloc(struct device *dev)
>>> +{
>>> + struct media_device *mdev;
>>> +
>>> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>> + if (!mdev)
>>> + return NULL;
>>> +
>>> + mdev->dev = dev;
>>> + media_device_init(mdev);
>>> +
>>> + return mdev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>> +
>>
>> One problem with this allocation is, this media device can't be shared
>> across drivers. For au0828 and snd-usb-audio should be able to share the
>> media_device. That is what the Media Allocator API patch series does.
>
> No disagreement here, Sakari's patches don't address the issues that the media
> allocator API fixes. The media allocator API, when ready, should replace (or
> at least complement, if we decide to keep a simpler API for drivers that don't
> need to share a media device, but I have no opinion on this at this time) this
> allocation function.
Media Device Allocator API is ready and reviewed. au0828 uses it as the first
driver using it. I will be sending out snd-usb-audio patch soon that makes use
of the shared media device.
thanks,
-- Shuah
>
>> This a quick review and I will review the patch series and get back to
>> you.
>>
>>> void media_device_cleanup(struct media_device *mdev)
>>> {
>>> ida_destroy(&mdev->entity_internal_idx);
>>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>>> index 96de915..c9b5798 100644
>>> --- a/include/media/media-device.h
>>> +++ b/include/media/media-device.h
>>> @@ -207,6 +207,15 @@ static inline __must_check int
>>> media_entity_enum_init(
>>> void media_device_init(struct media_device *mdev);
>>>
>>> /**
>>> + * media_device_alloc() - Allocate and initialise a media device
>>> + *
>>> + * @dev: The associated struct device pointer
>>> + *
>>> + * Allocate and initialise a media device. Returns a media device.
>>> + */
>>> +struct media_device *media_device_alloc(struct device *dev);
>>> +
>>> +/**
>>> * media_device_cleanup() - Cleanups a media device element
>>> *
>>> * @mdev: pointer to struct &media_device
>>> @@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device
>>> *mdev,
>>> const char *driver_name);
>>> #else
>>> +static inline struct media_device *media_device_alloc(struct device *dev)
>>> +{
>>> + return NULL;
>>> +}
>>> static inline int media_device_register(struct media_device *mdev)
>>> {
>>> return 0;
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-11 0:00 ` Shuah Khan
@ 2016-11-11 0:11 ` Laurent Pinchart
2016-11-11 0:16 ` Shuah Khan
0 siblings, 1 reply; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-11 0:11 UTC (permalink / raw)
To: Shuah Khan
Cc: Shuah Khan, Sakari Ailus, linux-media, hverkuil, mchehab, Sakari Ailus
Hi Shuah,
On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
> > On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
> >> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
> >>> From: Sakari Ailus <sakari.ailus@iki.fi>
> >>>
> >>> Allow allocating the media device dynamically. As the struct
> >>> media_device embeds struct media_devnode, the lifetime of that object is
> >>> that same than that of the media_device.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>>
> >>> drivers/media/media-device.c | 15 +++++++++++++++
> >>> include/media/media-device.h | 13 +++++++++++++
> >>> 2 files changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>> index a31329d..496195e 100644
> >>> --- a/drivers/media/media-device.c
> >>> +++ b/drivers/media/media-device.c
> >>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> >>> }
> >>> EXPORT_SYMBOL_GPL(media_device_init);
> >>>
> >>> +struct media_device *media_device_alloc(struct device *dev)
> >>> +{
> >>> + struct media_device *mdev;
> >>> +
> >>> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> >>> + if (!mdev)
> >>> + return NULL;
> >>> +
> >>> + mdev->dev = dev;
> >>> + media_device_init(mdev);
> >>> +
> >>> + return mdev;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(media_device_alloc);
> >>> +
> >>
> >> One problem with this allocation is, this media device can't be shared
> >> across drivers. For au0828 and snd-usb-audio should be able to share the
> >> media_device. That is what the Media Allocator API patch series does.
> >
> > No disagreement here, Sakari's patches don't address the issues that the
> > media allocator API fixes. The media allocator API, when ready, should
> > replace (or at least complement, if we decide to keep a simpler API for
> > drivers that don't need to share a media device, but I have no opinion on
> > this at this time) this allocation function.
>
> Media Device Allocator API is ready and reviewed. au0828 uses it as the
> first driver using it. I will be sending out snd-usb-audio patch soon that
> makes use of the shared media device.
I don't think it would be too difficult to rebase this series on top of the
media allocator API, as all that is needed here is a way to dynamically
allocate the media device in a clean fashion. I don't think Sakari's patches
depend on a specific implementation of media_device_alloc(). Sakari, please
let me know if I got this wrong.
> >> This a quick review and I will review the patch series and get back to
> >> you.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-11 0:11 ` Laurent Pinchart
@ 2016-11-11 0:16 ` Shuah Khan
2016-11-11 0:19 ` Laurent Pinchart
0 siblings, 1 reply; 43+ messages in thread
From: Shuah Khan @ 2016-11-11 0:16 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Shuah Khan, Sakari Ailus, linux-media, hverkuil, mchehab,
Sakari Ailus, Shuah Khan
On 11/10/2016 05:11 PM, Laurent Pinchart wrote:
> Hi Shuah,
>
> On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
>> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
>>> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
>>>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
>>>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>>>
>>>>> Allow allocating the media device dynamically. As the struct
>>>>> media_device embeds struct media_devnode, the lifetime of that object is
>>>>> that same than that of the media_device.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> ---
>>>>>
>>>>> drivers/media/media-device.c | 15 +++++++++++++++
>>>>> include/media/media-device.h | 13 +++++++++++++
>>>>> 2 files changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>>> index a31329d..496195e 100644
>>>>> --- a/drivers/media/media-device.c
>>>>> +++ b/drivers/media/media-device.c
>>>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(media_device_init);
>>>>>
>>>>> +struct media_device *media_device_alloc(struct device *dev)
>>>>> +{
>>>>> + struct media_device *mdev;
>>>>> +
>>>>> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>>>> + if (!mdev)
>>>>> + return NULL;
>>>>> +
>>>>> + mdev->dev = dev;
>>>>> + media_device_init(mdev);
>>>>> +
>>>>> + return mdev;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>>>> +
>>>>
>>>> One problem with this allocation is, this media device can't be shared
>>>> across drivers. For au0828 and snd-usb-audio should be able to share the
>>>> media_device. That is what the Media Allocator API patch series does.
>>>
>>> No disagreement here, Sakari's patches don't address the issues that the
>>> media allocator API fixes. The media allocator API, when ready, should
>>> replace (or at least complement, if we decide to keep a simpler API for
>>> drivers that don't need to share a media device, but I have no opinion on
>>> this at this time) this allocation function.
>>
>> Media Device Allocator API is ready and reviewed. au0828 uses it as the
>> first driver using it. I will be sending out snd-usb-audio patch soon that
>> makes use of the shared media device.
>
> I don't think it would be too difficult to rebase this series on top of the
> media allocator API, as all that is needed here is a way to dynamically
> allocate the media device in a clean fashion. I don't think Sakari's patches
> depend on a specific implementation of media_device_alloc(). Sakari, please
> let me know if I got this wrong.
Media Device Allocator API is independent as well. It doesn't make any
assumptions on media_device register and doesn't care really whether it
is shared or not. So I think Sakari's work can use the Media Device Allocator
API instead of the allocator routine it is adding.
thanks,
-- Shuah
>
>>>> This a quick review and I will review the patch series and get back to
>>>> you.
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-11 0:16 ` Shuah Khan
@ 2016-11-11 0:19 ` Laurent Pinchart
2016-11-11 0:35 ` Shuah Khan
0 siblings, 1 reply; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-11 0:19 UTC (permalink / raw)
To: Shuah Khan
Cc: Shuah Khan, Sakari Ailus, linux-media, hverkuil, mchehab, Sakari Ailus
Hi Shuah,
On Thursday 10 Nov 2016 17:16:31 Shuah Khan wrote:
> On 11/10/2016 05:11 PM, Laurent Pinchart wrote:
> > On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
> >> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
> >>> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
> >>>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
> >>>>> From: Sakari Ailus <sakari.ailus@iki.fi>
> >>>>>
> >>>>> Allow allocating the media device dynamically. As the struct
> >>>>> media_device embeds struct media_devnode, the lifetime of that object
> >>>>> is that same than that of the media_device.
> >>>>>
> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>> ---
> >>>>>
> >>>>> drivers/media/media-device.c | 15 +++++++++++++++
> >>>>> include/media/media-device.h | 13 +++++++++++++
> >>>>> 2 files changed, 28 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/media-device.c
> >>>>> b/drivers/media/media-device.c
> >>>>> index a31329d..496195e 100644
> >>>>> --- a/drivers/media/media-device.c
> >>>>> +++ b/drivers/media/media-device.c
> >>>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> >>>>> }
> >>>>> EXPORT_SYMBOL_GPL(media_device_init);
> >>>>>
> >>>>> +struct media_device *media_device_alloc(struct device *dev)
> >>>>> +{
> >>>>> + struct media_device *mdev;
> >>>>> +
> >>>>> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> >>>>> + if (!mdev)
> >>>>> + return NULL;
> >>>>> +
> >>>>> + mdev->dev = dev;
> >>>>> + media_device_init(mdev);
> >>>>> +
> >>>>> + return mdev;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
> >>>>> +
> >>>>
> >>>> One problem with this allocation is, this media device can't be shared
> >>>> across drivers. For au0828 and snd-usb-audio should be able to share
> >>>> the media_device. That is what the Media Allocator API patch series
> >>>> does.
> >>>
> >>> No disagreement here, Sakari's patches don't address the issues that the
> >>> media allocator API fixes. The media allocator API, when ready, should
> >>> replace (or at least complement, if we decide to keep a simpler API for
> >>> drivers that don't need to share a media device, but I have no opinion
> >>> on this at this time) this allocation function.
> >>
> >> Media Device Allocator API is ready and reviewed. au0828 uses it as the
> >> first driver using it. I will be sending out snd-usb-audio patch soon
> >> that makes use of the shared media device.
> >
> > I don't think it would be too difficult to rebase this series on top of
> > the media allocator API, as all that is needed here is a way to
> > dynamically allocate the media device in a clean fashion. I don't think
> > Sakari's patches depend on a specific implementation of
> > media_device_alloc(). Sakari, please let me know if I got this wrong.
>
> Media Device Allocator API is independent as well. It doesn't make any
> assumptions on media_device register and doesn't care really whether it
> is shared or not. So I think Sakari's work can use the Media Device
> Allocator API instead of the allocator routine it is adding.
I think you're right, and the goal is certainly to merge both. The question is
just which series should go in first. I have no strong opinion on that,
whichever series is ready first can probably get merged first. I'd just like
to get a confirmation from Sakari that both orders would be equally fine.
This being said, I'd of course like to test the combination of both series to
make sure neither introduce an issue for the other one.
> >>>> This a quick review and I will review the patch series and get back to
> >>>> you.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-11 0:19 ` Laurent Pinchart
@ 2016-11-11 0:35 ` Shuah Khan
0 siblings, 0 replies; 43+ messages in thread
From: Shuah Khan @ 2016-11-11 0:35 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Shuah Khan, Sakari Ailus, linux-media, hverkuil, mchehab,
Sakari Ailus, Shuah Khan
On 11/10/2016 05:19 PM, Laurent Pinchart wrote:
> Hi Shuah,
>
> On Thursday 10 Nov 2016 17:16:31 Shuah Khan wrote:
>> On 11/10/2016 05:11 PM, Laurent Pinchart wrote:
>>> On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
>>>> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
>>>>> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
>>>>>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
>>>>>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>>>>>
>>>>>>> Allow allocating the media device dynamically. As the struct
>>>>>>> media_device embeds struct media_devnode, the lifetime of that object
>>>>>>> is that same than that of the media_device.
>>>>>>>
>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>> ---
>>>>>>>
>>>>>>> drivers/media/media-device.c | 15 +++++++++++++++
>>>>>>> include/media/media-device.h | 13 +++++++++++++
>>>>>>> 2 files changed, 28 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/media/media-device.c
>>>>>>> b/drivers/media/media-device.c
>>>>>>> index a31329d..496195e 100644
>>>>>>> --- a/drivers/media/media-device.c
>>>>>>> +++ b/drivers/media/media-device.c
>>>>>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>>>>>> }
>>>>>>> EXPORT_SYMBOL_GPL(media_device_init);
>>>>>>>
>>>>>>> +struct media_device *media_device_alloc(struct device *dev)
>>>>>>> +{
>>>>>>> + struct media_device *mdev;
>>>>>>> +
>>>>>>> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>>>>>> + if (!mdev)
>>>>>>> + return NULL;
>>>>>>> +
>>>>>>> + mdev->dev = dev;
>>>>>>> + media_device_init(mdev);
>>>>>>> +
>>>>>>> + return mdev;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>>>>>> +
>>>>>>
>>>>>> One problem with this allocation is, this media device can't be shared
>>>>>> across drivers. For au0828 and snd-usb-audio should be able to share
>>>>>> the media_device. That is what the Media Allocator API patch series
>>>>>> does.
>>>>>
>>>>> No disagreement here, Sakari's patches don't address the issues that the
>>>>> media allocator API fixes. The media allocator API, when ready, should
>>>>> replace (or at least complement, if we decide to keep a simpler API for
>>>>> drivers that don't need to share a media device, but I have no opinion
>>>>> on this at this time) this allocation function.
>>>>
>>>> Media Device Allocator API is ready and reviewed. au0828 uses it as the
>>>> first driver using it. I will be sending out snd-usb-audio patch soon
>>>> that makes use of the shared media device.
>>>
>>> I don't think it would be too difficult to rebase this series on top of
>>> the media allocator API, as all that is needed here is a way to
>>> dynamically allocate the media device in a clean fashion. I don't think
>>> Sakari's patches depend on a specific implementation of
>>> media_device_alloc(). Sakari, please let me know if I got this wrong.
>>
>> Media Device Allocator API is independent as well. It doesn't make any
>> assumptions on media_device register and doesn't care really whether it
>> is shared or not. So I think Sakari's work can use the Media Device
>> Allocator API instead of the allocator routine it is adding.
>
> I think you're right, and the goal is certainly to merge both. The question is
> just which series should go in first. I have no strong opinion on that,
> whichever series is ready first can probably get merged first. I'd just like
> to get a confirmation from Sakari that both orders would be equally fine.
>
> This being said, I'd of course like to test the combination of both series to
> make sure neither introduce an issue for the other one.
Audio maintainer has been waiting for the snd-usb-audio media controller api
work to use it as a reference for changing other drivers to use media controller
api.
I have an urgency here to get the snd-usb-audio. So I am going to work
towards getting Media Device Allocator API, au0828, and snd-usb-audio patches
that uses Media Device Allocator API into 4.10
Sakari's work could take longer as it requires driver changes. So I am going
to assume, it will happen after the work I am doing.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-08 19:20 ` Shuah Khan
2016-11-10 23:53 ` Laurent Pinchart
@ 2016-11-14 13:40 ` Sakari Ailus
2016-11-15 0:13 ` Shuah Khan
1 sibling, 1 reply; 43+ messages in thread
From: Sakari Ailus @ 2016-11-14 13:40 UTC (permalink / raw)
To: Shuah Khan
Cc: Sakari Ailus, linux-media, hverkuil, mchehab, shuahkh, laurent.pinchart
Hi Shuah,
On Tue, Nov 08, 2016 at 12:20:29PM -0700, Shuah Khan wrote:
> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> >
> > Allow allocating the media device dynamically. As the struct media_device
> > embeds struct media_devnode, the lifetime of that object is that same than
> > that of the media_device.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/media-device.c | 15 +++++++++++++++
> > include/media/media-device.h | 13 +++++++++++++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index a31329d..496195e 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> > }
> > EXPORT_SYMBOL_GPL(media_device_init);
> >
> > +struct media_device *media_device_alloc(struct device *dev)
> > +{
> > + struct media_device *mdev;
> > +
> > + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > + if (!mdev)
> > + return NULL;
> > +
> > + mdev->dev = dev;
> > + media_device_init(mdev);
> > +
> > + return mdev;
> > +}
> > +EXPORT_SYMBOL_GPL(media_device_alloc);
> > +
>
> One problem with this allocation is, this media device can't be shared across
> drivers. For au0828 and snd-usb-audio should be able to share the
> media_device. That is what the Media Allocator API patch series does.
> This a quick review and I will review the patch series and get back to
> you.
The assumption has always been there that a media device has a single struct
device related to it. It hasn't been visible in the function call API
though, just in the data structures.
I have to admit I may have forgotten something that was discussed back then,
but do you need to share the same media device over multiple devices in the
system? I don't see that at least in the allocator patch itself. It's
"[PATCH v3] media: Media Device Allocator API", isn't it?
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 08/21] media: Enable allocating the media device dynamically
2016-11-14 13:40 ` Sakari Ailus
@ 2016-11-15 0:13 ` Shuah Khan
0 siblings, 0 replies; 43+ messages in thread
From: Shuah Khan @ 2016-11-15 0:13 UTC (permalink / raw)
To: Sakari Ailus, Shuah Khan
Cc: Sakari Ailus, linux-media, hverkuil, mchehab, laurent.pinchart,
Shuah Khan
On 11/14/2016 06:40 AM, Sakari Ailus wrote:
> Hi Shuah,
>
> On Tue, Nov 08, 2016 at 12:20:29PM -0700, Shuah Khan wrote:
>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus
>> <sakari.ailus@linux.intel.com> wrote:
>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>
>>> Allow allocating the media device dynamically. As the struct media_device
>>> embeds struct media_devnode, the lifetime of that object is that same than
>>> that of the media_device.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>> drivers/media/media-device.c | 15 +++++++++++++++
>>> include/media/media-device.h | 13 +++++++++++++
>>> 2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index a31329d..496195e 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>> }
>>> EXPORT_SYMBOL_GPL(media_device_init);
>>>
>>> +struct media_device *media_device_alloc(struct device *dev)
>>> +{
>>> + struct media_device *mdev;
>>> +
>>> + mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>> + if (!mdev)
>>> + return NULL;
>>> +
>>> + mdev->dev = dev;
>>> + media_device_init(mdev);
>>> +
>>> + return mdev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>> +
>>
>> One problem with this allocation is, this media device can't be shared across
>> drivers. For au0828 and snd-usb-audio should be able to share the
>> media_device. That is what the Media Allocator API patch series does.
>> This a quick review and I will review the patch series and get back to
>> you.
>
> The assumption has always been there that a media device has a single struct
> device related to it. It hasn't been visible in the function call API
> though, just in the data structures.
>
> I have to admit I may have forgotten something that was discussed back then,
> but do you need to share the same media device over multiple devices in the
> system? I don't see that at least in the allocator patch itself. It's
> "[PATCH v3] media: Media Device Allocator API", isn't it?
>
Hi Sakari,
Remember the work I am doing that adds Media Controller API to snd-usb-audio
and au0828 so they can share the media resources. That is where we need the
media device sharable. Please see the following: this patch series includes
the API and au0828 change to use it. I tested it with snd-usb-audio change,
didn't include it at that time.
https://www.mail-archive.com/linux-media@vger.kernel.org/msg98793.html
https://www.mail-archive.com/linux-media@vger.kernel.org/msg97779.html
https://www.mail-archive.com/linux-media@vger.kernel.org/msg97704.html
I am going to be sending the rebased Media Device Allocator patches with both
au0828 and snd-usb-audio using it in a couple of days. No code changes, just
rebased to Linux 4.9-rc4
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC v4 09/21] media: Split initialising and adding media devnode
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (6 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 08/21] media: Enable allocating the media device dynamically Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 10/21] media: Shuffle functions around Sakari Ailus
` (13 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
As registering a device node of an entity belonging to a media device
will require a reference to the struct device. Taking that reference is
only possible once the device has been initialised, which took place only
when it was registered. Split this in two, and initialise the device when
the media device is allocated.
Don't distribute the effects of these changes yet. Add media_device_get()
and media_device_put() first.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 18 +++++++++++++-----
drivers/media/media-devnode.c | 17 +++++++++++------
include/media/media-devnode.h | 19 ++++++++++++++-----
3 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 496195e..c79a9f5 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -720,19 +720,26 @@ int __must_check __media_device_register(struct media_device *mdev,
/* Set version 0 to indicate user-space that the graph is static */
mdev->topology_version = 0;
+ media_devnode_init(&mdev->devnode);
+
ret = media_devnode_register(&mdev->devnode, owner);
if (ret < 0)
- return ret;
+ goto out_put;
ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
- if (ret < 0) {
- media_devnode_unregister(&mdev->devnode);
- return ret;
- }
+ if (ret < 0)
+ goto out_unregister;
dev_dbg(mdev->dev, "Media device registered\n");
return 0;
+
+out_unregister:
+ media_devnode_unregister(&mdev->devnode);
+out_put:
+ put_device(&mdev->devnode.dev);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(__media_device_register);
@@ -803,6 +810,7 @@ void media_device_unregister(struct media_device *mdev)
device_remove_file(&mdev->devnode.dev, &dev_attr_model);
dev_dbg(mdev->dev, "Media device unregistering\n");
media_devnode_unregister(&mdev->devnode);
+ put_device(&mdev->devnode.dev);
}
EXPORT_SYMBOL_GPL(media_device_unregister);
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index a8302fc..178d692 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -216,6 +216,11 @@ static const struct file_operations media_devnode_fops = {
.llseek = no_llseek,
};
+void media_devnode_init(struct media_devnode *devnode)
+{
+ device_initialize(&devnode->dev);
+}
+
int __must_check media_devnode_register(struct media_devnode *devnode,
struct module *owner)
{
@@ -254,7 +259,7 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
if (devnode->parent)
devnode->dev.parent = devnode->parent;
dev_set_name(&devnode->dev, "media%d", devnode->minor);
- ret = device_register(&devnode->dev);
+ ret = device_add(&devnode->dev);
if (ret < 0) {
pr_err("%s: device_register failed\n", __func__);
goto error;
@@ -284,13 +289,13 @@ void media_devnode_unregister(struct media_devnode *devnode)
clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
mutex_unlock(&media_devnode_lock);
cdev_del(&devnode->cdev);
- device_unregister(&devnode->dev);
+ device_del(&devnode->dev);
}
/*
* Initialise media for linux
*/
-static int __init media_devnode_init(void)
+static int __init media_devnode_module_init(void)
{
int ret;
@@ -312,14 +317,14 @@ static int __init media_devnode_init(void)
return 0;
}
-static void __exit media_devnode_exit(void)
+static void __exit media_devnode_module_exit(void)
{
bus_unregister(&media_bus_type);
unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
}
-subsys_initcall(media_devnode_init);
-module_exit(media_devnode_exit)
+subsys_initcall(media_devnode_module_init);
+module_exit(media_devnode_module_exit)
MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
MODULE_DESCRIPTION("Device node registration for media drivers");
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index a68b4b6..ffa2d18 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -103,6 +103,17 @@ struct media_devnode {
#define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
/**
+ * media_devnode_init - initialise a media devnode
+ *
+ * @devnode: struct media_devnode we want to initialise
+ *
+ * Initialise a media devnode. Note that after initialising the media
+ * devnode is refcounted. Releasing references to it may be done using
+ * put_device().
+ */
+void media_devnode_init(struct media_devnode *devnode);
+
+/**
* media_devnode_register - register a media device node
*
* @devnode: struct media_devnode we want to register a device node
@@ -112,11 +123,9 @@ struct media_devnode {
* with the kernel. An error is returned if no free minor number can be found,
* or if the registration of the device node fails.
*
- * Zero is returned on success.
- *
- * Note that if the media_devnode_register call fails, the release() callback of
- * the media_devnode structure is *not* called, so the caller is responsible for
- * freeing any data.
+ * Zero is returned on success. Note that in case
+ * media_devnode_register() fails, the caller is responsible for
+ * releasing the reference to the device using put_device().
*/
int __must_check media_devnode_register(struct media_devnode *devnode,
struct module *owner);
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 10/21] media: Shuffle functions around
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (7 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 09/21] media: Split initialising and adding media devnode Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 11/21] media device: Refcount the media device Sakari Ailus
` (12 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
As the call paths of the functions in question will change, move them
around in anticipation of that. No other changes.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 56 ++++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c79a9f5..0920c02 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -660,6 +660,34 @@ void media_device_unregister_entity(struct media_entity *entity)
}
EXPORT_SYMBOL_GPL(media_device_unregister_entity);
+int __must_check media_device_register_entity_notify(struct media_device *mdev,
+ struct media_entity_notify *nptr)
+{
+ mutex_lock(&mdev->graph_mutex);
+ list_add_tail(&nptr->list, &mdev->entity_notify);
+ mutex_unlock(&mdev->graph_mutex);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
+
+/*
+ * Note: Should be called with mdev->lock held.
+ */
+static void __media_device_unregister_entity_notify(struct media_device *mdev,
+ struct media_entity_notify *nptr)
+{
+ list_del(&nptr->list);
+}
+
+void media_device_unregister_entity_notify(struct media_device *mdev,
+ struct media_entity_notify *nptr)
+{
+ mutex_lock(&mdev->graph_mutex);
+ __media_device_unregister_entity_notify(mdev, nptr);
+ mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
+
/**
* media_device_init() - initialize a media device
* @mdev: The media device
@@ -743,34 +771,6 @@ int __must_check __media_device_register(struct media_device *mdev,
}
EXPORT_SYMBOL_GPL(__media_device_register);
-int __must_check media_device_register_entity_notify(struct media_device *mdev,
- struct media_entity_notify *nptr)
-{
- mutex_lock(&mdev->graph_mutex);
- list_add_tail(&nptr->list, &mdev->entity_notify);
- mutex_unlock(&mdev->graph_mutex);
- return 0;
-}
-EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
-
-/*
- * Note: Should be called with mdev->lock held.
- */
-static void __media_device_unregister_entity_notify(struct media_device *mdev,
- struct media_entity_notify *nptr)
-{
- list_del(&nptr->list);
-}
-
-void media_device_unregister_entity_notify(struct media_device *mdev,
- struct media_entity_notify *nptr)
-{
- mutex_lock(&mdev->graph_mutex);
- __media_device_unregister_entity_notify(mdev, nptr);
- mutex_unlock(&mdev->graph_mutex);
-}
-EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
-
void media_device_unregister(struct media_device *mdev)
{
struct media_entity *entity;
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 11/21] media device: Refcount the media device
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (8 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 10/21] media: Shuffle functions around Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 12/21] media device: Initialise media devnode in media_device_init() Sakari Ailus
` (11 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
As the struct media_device embeds struct media_devnode, the lifetime of
that object must be that same than that of the media_device.
References are obtained by media_entity_get() and released by
media_entity_put(). In case a driver uses media_device_alloc() to allocate
its media device, it must release the media device by calling
media_device_put() rather than media_device_cleanup().
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 13 +++++++++++++
include/media/media-device.h | 31 +++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 0920c02..e9f6e76 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -712,6 +712,17 @@ void media_device_init(struct media_device *mdev)
}
EXPORT_SYMBOL_GPL(media_device_init);
+static void media_device_release(struct media_devnode *devnode)
+{
+ struct media_device *mdev = to_media_device(devnode);
+
+ dev_dbg(devnode->parent, "Media device released\n");
+
+ media_device_cleanup(mdev);
+
+ kfree(mdev);
+}
+
struct media_device *media_device_alloc(struct device *dev)
{
struct media_device *mdev;
@@ -723,6 +734,8 @@ struct media_device *media_device_alloc(struct device *dev)
mdev->dev = dev;
media_device_init(mdev);
+ mdev->devnode.release = media_device_release;
+
return mdev;
}
EXPORT_SYMBOL_GPL(media_device_alloc);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index c9b5798..fc0d82a 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -212,10 +212,39 @@ void media_device_init(struct media_device *mdev);
* @dev: The associated struct device pointer
*
* Allocate and initialise a media device. Returns a media device.
+ * The media device is refcounted, and this function returns a media
+ * device the refcount of which is one (1).
+ *
+ * References are taken and given using media_device_get() and
+ * media_device_put().
*/
struct media_device *media_device_alloc(struct device *dev);
/**
+ * media_device_get() - Get a reference to a media device
+ *
+ * mdev: media device
+ */
+#define media_device_get(mdev) \
+ do { \
+ dev_dbg((mdev)->dev, "%s: get media device %s\n", \
+ __func__, (mdev)->bus_info); \
+ get_device(&(mdev)->devnode.dev); \
+ } while (0)
+
+/**
+ * media_device_put() - Put a reference to a media device
+ *
+ * mdev: media device
+ */
+#define media_device_put(mdev) \
+ do { \
+ dev_dbg((mdev)->dev, "%s: put media device %s\n", \
+ __func__, (mdev)->bus_info); \
+ put_device(&(mdev)->devnode.dev); \
+ } while (0)
+
+/**
* media_device_cleanup() - Cleanups a media device element
*
* @mdev: pointer to struct &media_device
@@ -464,6 +493,8 @@ static inline struct media_device *media_device_alloc(struct device *dev)
{
return NULL;
}
+#define media_device_get(mdev) do { } while (0)
+#define media_device_put(mdev) do { } while (0)
static inline int media_device_register(struct media_device *mdev)
{
return 0;
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 12/21] media device: Initialise media devnode in media_device_init()
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (9 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 11/21] media device: Refcount the media device Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 13/21] media device: Deprecate media_device_{init,cleanup}() for drivers Sakari Ailus
` (10 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
Call media_devnode_init() from media_device_init(). This has the effect of
creating a struct device for the media_devnode before it is registered,
making it possible to obtain a reference to it for e.g. video devices.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index e9f6e76..2e52e44 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -708,6 +708,8 @@ void media_device_init(struct media_device *mdev)
mutex_init(&mdev->graph_mutex);
ida_init(&mdev->entity_internal_idx);
+ media_devnode_init(&mdev->devnode);
+
dev_dbg(mdev->dev, "Media device initialized\n");
}
EXPORT_SYMBOL_GPL(media_device_init);
@@ -718,7 +720,10 @@ static void media_device_release(struct media_devnode *devnode)
dev_dbg(devnode->parent, "Media device released\n");
- media_device_cleanup(mdev);
+ ida_destroy(&mdev->entity_internal_idx);
+ mdev->entity_internal_idx_max = 0;
+ media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
+ mutex_destroy(&mdev->graph_mutex);
kfree(mdev);
}
@@ -746,6 +751,7 @@ void media_device_cleanup(struct media_device *mdev)
mdev->entity_internal_idx_max = 0;
media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
mutex_destroy(&mdev->graph_mutex);
+ media_device_put(mdev);
}
EXPORT_SYMBOL_GPL(media_device_cleanup);
@@ -761,26 +767,19 @@ int __must_check __media_device_register(struct media_device *mdev,
/* Set version 0 to indicate user-space that the graph is static */
mdev->topology_version = 0;
- media_devnode_init(&mdev->devnode);
-
ret = media_devnode_register(&mdev->devnode, owner);
if (ret < 0)
- goto out_put;
+ return ret;
ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
- if (ret < 0)
- goto out_unregister;
+ if (ret < 0) {
+ media_devnode_unregister(&mdev->devnode);
+ return ret;
+ }
dev_dbg(mdev->dev, "Media device registered\n");
return 0;
-
-out_unregister:
- media_devnode_unregister(&mdev->devnode);
-out_put:
- put_device(&mdev->devnode.dev);
-
- return ret;
}
EXPORT_SYMBOL_GPL(__media_device_register);
@@ -823,7 +822,6 @@ void media_device_unregister(struct media_device *mdev)
device_remove_file(&mdev->devnode.dev, &dev_attr_model);
dev_dbg(mdev->dev, "Media device unregistering\n");
media_devnode_unregister(&mdev->devnode);
- put_device(&mdev->devnode.dev);
}
EXPORT_SYMBOL_GPL(media_device_unregister);
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 13/21] media device: Deprecate media_device_{init,cleanup}() for drivers
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (10 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 12/21] media device: Initialise media devnode in media_device_init() Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 14/21] media device: Get the media device driver's device Sakari Ailus
` (9 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
Drivers should no longer directly allocate media_device but rely on
media_device_alloc(), media_device_get() and media_device_put() instead.
Deprecate media_device_init() and media_device_cleanup().
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
include/media/media-device.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/media/media-device.h b/include/media/media-device.h
index fc0d82a..ae2bc08 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -203,6 +203,10 @@ static inline __must_check int media_entity_enum_init(
* So drivers need to first initialize the media device, register any entity
* within the media device, create pad to pad links and then finally register
* the media device by calling media_device_register() as a final step.
+ *
+ * Note that using this function in drivers is DEPRECATED. New drivers
+ * must use media_device_alloc() and manage references using
+ * media_device_get() and media_device_put() instead.
*/
void media_device_init(struct media_device *mdev);
@@ -251,6 +255,10 @@ struct media_device *media_device_alloc(struct device *dev);
*
* This function that will destroy the graph_mutex that is
* initialized in media_device_init().
+ *
+ * Note that using this function in drivers is DEPRECATED. New drivers
+ * must use media_device_alloc() and manage references using
+ * media_device_get() and media_device_put() instead.
*/
void media_device_cleanup(struct media_device *mdev);
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 14/21] media device: Get the media device driver's device
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (11 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 13/21] media device: Deprecate media_device_{init,cleanup}() for drivers Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-22 9:46 ` Hans Verkuil
2016-11-08 13:55 ` [RFC v4 15/21] media: Provide a way to the driver to set a private pointer Sakari Ailus
` (8 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
The struct device of the media device driver (i.e. not that of the media
devnode) is pointed to by the media device. The struct device pointer is
mostly used for debug prints.
Ensure it will stay around as long as the media device does.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 2e52e44..648c64c 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode *devnode)
mdev->entity_internal_idx_max = 0;
media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
mutex_destroy(&mdev->graph_mutex);
+ put_device(mdev->dev);
kfree(mdev);
}
@@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device *dev)
{
struct media_device *mdev;
+ dev = get_device(dev);
+ if (!dev)
+ return NULL;
+
mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
- if (!mdev)
+ if (!mdev) {
+ put_device(dev);
return NULL;
+ }
mdev->dev = dev;
media_device_init(mdev);
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [RFC v4 14/21] media device: Get the media device driver's device
2016-11-08 13:55 ` [RFC v4 14/21] media device: Get the media device driver's device Sakari Ailus
@ 2016-11-22 9:46 ` Hans Verkuil
2016-11-22 9:58 ` Laurent Pinchart
0 siblings, 1 reply; 43+ messages in thread
From: Hans Verkuil @ 2016-11-22 9:46 UTC (permalink / raw)
To: Sakari Ailus, linux-media; +Cc: mchehab, shuahkh, laurent.pinchart
On 08/11/16 14:55, Sakari Ailus wrote:
> The struct device of the media device driver (i.e. not that of the media
> devnode) is pointed to by the media device. The struct device pointer is
> mostly used for debug prints.
>
> Ensure it will stay around as long as the media device does.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/media-device.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 2e52e44..648c64c 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode *devnode)
> mdev->entity_internal_idx_max = 0;
> media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> mutex_destroy(&mdev->graph_mutex);
> + put_device(mdev->dev);
>
> kfree(mdev);
> }
> @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device *dev)
> {
> struct media_device *mdev;
>
> + dev = get_device(dev);
> + if (!dev)
> + return NULL;
I don't think this is right. When you allocate the media_device struct
it should
just be initialized, but not have any side effects until it is actually
registered.
When the device is registered the device_add call will increase the parent's
refcount as it should, thus ensuring it stays around for as long as is
needed.
Regards,
Hans
> +
> mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> - if (!mdev)
> + if (!mdev) {
> + put_device(dev);
> return NULL;
> + }
>
> mdev->dev = dev;
> media_device_init(mdev);
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 14/21] media device: Get the media device driver's device
2016-11-22 9:46 ` Hans Verkuil
@ 2016-11-22 9:58 ` Laurent Pinchart
2016-11-22 10:58 ` Hans Verkuil
0 siblings, 1 reply; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-22 9:58 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Sakari Ailus, linux-media, mchehab, shuahkh
Hi Hans,
On Tuesday 22 Nov 2016 10:46:31 Hans Verkuil wrote:
> On 08/11/16 14:55, Sakari Ailus wrote:
> > The struct device of the media device driver (i.e. not that of the media
> > devnode) is pointed to by the media device. The struct device pointer is
> > mostly used for debug prints.
> >
> > Ensure it will stay around as long as the media device does.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >
> > drivers/media/media-device.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 2e52e44..648c64c 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode
> > *devnode)
> > mdev->entity_internal_idx_max = 0;
> > media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> > mutex_destroy(&mdev->graph_mutex);
> > + put_device(mdev->dev);
> > kfree(mdev);
> > }
> >
> > @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device
> > *dev)
> > {
> > struct media_device *mdev;
> >
> > + dev = get_device(dev);
> > + if (!dev)
> > + return NULL;
>
> I don't think this is right. When you allocate the media_device struct
> it should just be initialized, but not have any side effects until it is
> actually registered.
>
> When the device is registered the device_add call will increase the parent's
> refcount as it should, thus ensuring it stays around for as long as is
> needed.
We're storing a pointer to dev in mdev a few lines below. As dev is
refcounted, we need to ensure that we take a reference appropriately. We can
either borrow a reference taken elsewhere or take our own reference.
Borrowing a reference is only valid if we know it will exist for at least as
long as we need to borrow it. That might be the case when creating the media
device as the driver performing the operation should hold a reference to the
struct device instance (especially given that allocation and registration are
usually - but not always - performed at probe time for that driver), but it's
harder to guarantee at unregistration time, especially when userspace can keep
device nodes open across unregistration. This patch ensures that the pointer
always remains valid until we stop needing it.
> > +
> >
> > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > - if (!mdev)
> > + if (!mdev) {
> > + put_device(dev);
> > return NULL;
> > + }
> >
> > mdev->dev = dev;
I would move the get_device() call here:
mdev->dev = get_device(dev);
if (!mdev->dev) {
kfree(mdev);
return NULL;
}
I believe it makes the code more readable.
In theory we could even remove the error check, as we have a guarantee that
the caller gives us a valid struct device reference, but I don't mind keeping
it.
> > media_device_init(mdev);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 14/21] media device: Get the media device driver's device
2016-11-22 9:58 ` Laurent Pinchart
@ 2016-11-22 10:58 ` Hans Verkuil
2016-11-22 22:16 ` Laurent Pinchart
0 siblings, 1 reply; 43+ messages in thread
From: Hans Verkuil @ 2016-11-22 10:58 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, mchehab, shuahkh
On 22/11/16 10:58, Laurent Pinchart wrote:
> Hi Hans,
>
> On Tuesday 22 Nov 2016 10:46:31 Hans Verkuil wrote:
>> On 08/11/16 14:55, Sakari Ailus wrote:
>>> The struct device of the media device driver (i.e. not that of the media
>>> devnode) is pointed to by the media device. The struct device pointer is
>>> mostly used for debug prints.
>>>
>>> Ensure it will stay around as long as the media device does.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>
>>> drivers/media/media-device.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index 2e52e44..648c64c 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode
>>> *devnode)
>>> mdev->entity_internal_idx_max = 0;
>>> media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
>>> mutex_destroy(&mdev->graph_mutex);
>>> + put_device(mdev->dev);
>>> kfree(mdev);
>>> }
>>>
>>> @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device
>>> *dev)
>>> {
>>> struct media_device *mdev;
>>>
>>> + dev = get_device(dev);
>>> + if (!dev)
>>> + return NULL;
>>
>> I don't think this is right. When you allocate the media_device struct
>> it should just be initialized, but not have any side effects until it is
>> actually registered.
>>
>> When the device is registered the device_add call will increase the parent's
>> refcount as it should, thus ensuring it stays around for as long as is
>> needed.
>
> We're storing a pointer to dev in mdev a few lines below. As dev is
> refcounted, we need to ensure that we take a reference appropriately. We can
> either borrow a reference taken elsewhere or take our own reference.
>
> Borrowing a reference is only valid if we know it will exist for at least as
> long as we need to borrow it. That might be the case when creating the media
> device as the driver performing the operation should hold a reference to the
> struct device instance (especially given that allocation and registration are
> usually - but not always - performed at probe time for that driver), but it's
> harder to guarantee at unregistration time, especially when userspace can keep
> device nodes open across unregistration. This patch ensures that the pointer
> always remains valid until we stop needing it.
I disagree. There is no reason to keep the parent device in memory once the
media devnode is unregistered.
It seems to be pretty much only used for some debugging. I suspect that in
almost all cases the debugging happens when the devnode is registered, and
not when it is unregistered. But in that case you can also use &devnode.dev
as the device pointer for dev_dbg, or use pr_debug.
Looking at what the CEC framework does I see that I pass a device pointer
to the allocate function, but I really don't need to do that. It is not
used anywhere until the register function, so the parent device pointer
should be passed as an argument to the register function, not to the
allocate function.
BTW, I would very much prefer it if mdev->dev is renamed to mdev->parent.
Or better yet, just dropped completely since it is also available as
mdev->devnode.parent. And even devnode.parent can be dropped and just
use mdev->devnode.dev.parent.
I plan on posting such a patch for the cec framework as well, since
it avoids having duplicates of the same device parent pointer in the
data structures.
Regards,
Hans
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 14/21] media device: Get the media device driver's device
2016-11-22 10:58 ` Hans Verkuil
@ 2016-11-22 22:16 ` Laurent Pinchart
0 siblings, 0 replies; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-22 22:16 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Sakari Ailus, linux-media, mchehab, shuahkh
On Tuesday 22 Nov 2016 11:58:43 Hans Verkuil wrote:
> On 22/11/16 10:58, Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Tuesday 22 Nov 2016 10:46:31 Hans Verkuil wrote:
> >> On 08/11/16 14:55, Sakari Ailus wrote:
> >>> The struct device of the media device driver (i.e. not that of the media
> >>> devnode) is pointed to by the media device. The struct device pointer is
> >>> mostly used for debug prints.
> >>>
> >>> Ensure it will stay around as long as the media device does.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>>
> >>> drivers/media/media-device.c | 9 ++++++++-
> >>> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>> index 2e52e44..648c64c 100644
> >>> --- a/drivers/media/media-device.c
> >>> +++ b/drivers/media/media-device.c
> >>> @@ -724,6 +724,7 @@ static void media_device_release(struct
> >>> media_devnode
> >>> *devnode)
> >>>
> >>> mdev->entity_internal_idx_max = 0;
> >>> media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> >>> mutex_destroy(&mdev->graph_mutex);
> >>>
> >>> + put_device(mdev->dev);
> >>>
> >>> kfree(mdev);
> >>>
> >>> }
> >>>
> >>> @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct
> >>> device
> >>> *dev)
> >>>
> >>> {
> >>>
> >>> struct media_device *mdev;
> >>>
> >>> + dev = get_device(dev);
> >>> + if (!dev)
> >>> + return NULL;
> >>
> >> I don't think this is right. When you allocate the media_device struct
> >> it should just be initialized, but not have any side effects until it is
> >> actually registered.
> >>
> >> When the device is registered the device_add call will increase the
> >> parent's refcount as it should, thus ensuring it stays around for as
> >> long as is needed.
> >
> > We're storing a pointer to dev in mdev a few lines below. As dev is
> > refcounted, we need to ensure that we take a reference appropriately. We
> > can either borrow a reference taken elsewhere or take our own reference.
> >
> > Borrowing a reference is only valid if we know it will exist for at least
> > as long as we need to borrow it. That might be the case when creating the
> > media device as the driver performing the operation should hold a
> > reference to the struct device instance (especially given that allocation
> > and registration are usually - but not always - performed at probe time
> > for that driver), but it's harder to guarantee at unregistration time,
> > especially when userspace can keep device nodes open across
> > unregistration. This patch ensures that the pointer always remains valid
> > until we stop needing it.
>
> I disagree. There is no reason to keep the parent device in memory once the
> media devnode is unregistered.
There's a very big one: the media device is accessed through a large number of
APIs, not only through its own devnode. It can for instance be accessed
through V4L2 devnodes, and thus has to live as long as *anything* can access
it.
struct media_devnode was a very very bad idea. The original goal was to share
the implementation with the V4L2 devnodes, but when that got rejected I really
should have merged struct media_device and struct media_devnode into a single
structure. We can keep media_devnode separate if that is believed to improve
readability of the code, but there is absolutely no reason for allocating
media_devnode separately from media_device.
> It seems to be pretty much only used for some debugging. I suspect that in
> almost all cases the debugging happens when the devnode is registered, and
> not when it is unregistered. But in that case you can also use &devnode.dev
> as the device pointer for dev_dbg, or use pr_debug.
>
> Looking at what the CEC framework does I see that I pass a device pointer
> to the allocate function, but I really don't need to do that. It is not
> used anywhere until the register function, so the parent device pointer
> should be passed as an argument to the register function, not to the
> allocate function.
>
> BTW, I would very much prefer it if mdev->dev is renamed to mdev->parent.
> Or better yet, just dropped completely since it is also available as
> mdev->devnode.parent. And even devnode.parent can be dropped and just
> use mdev->devnode.dev.parent.
>
> I plan on posting such a patch for the cec framework as well, since
> it avoids having duplicates of the same device parent pointer in the
> data structures.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC v4 15/21] media: Provide a way to the driver to set a private pointer
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (12 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 14/21] media device: Get the media device driver's device Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 16/21] media: Add release callback for media device Sakari Ailus
` (7 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
Now that the media device can be allocated dynamically, drivers have no
longer a way to conveniently obtain the driver private data structure.
Provide one again in the form of a private pointer passed to the
media_device_alloc() function.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 3 ++-
include/media/media-device.h | 15 ++++++++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 648c64c..7d9f76d 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -729,7 +729,7 @@ static void media_device_release(struct media_devnode *devnode)
kfree(mdev);
}
-struct media_device *media_device_alloc(struct device *dev)
+struct media_device *media_device_alloc(struct device *dev, void *priv)
{
struct media_device *mdev;
@@ -745,6 +745,7 @@ struct media_device *media_device_alloc(struct device *dev)
mdev->dev = dev;
media_device_init(mdev);
+ mdev->priv = priv;
mdev->devnode.release = media_device_release;
diff --git a/include/media/media-device.h b/include/media/media-device.h
index ae2bc08..94e96ef 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -62,6 +62,7 @@ struct media_device_ops {
* struct media_device - Media device
* @dev: Parent device
* @devnode: Media device node
+ * @priv: A pointer to driver private data
* @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
@@ -128,6 +129,7 @@ struct media_device {
/* dev->driver_data points to this struct. */
struct device *dev;
struct media_devnode devnode;
+ void *priv;
char model[32];
char driver_name[32];
@@ -214,6 +216,7 @@ void media_device_init(struct media_device *mdev);
* media_device_alloc() - Allocate and initialise a media device
*
* @dev: The associated struct device pointer
+ * @priv: pointer to a driver private data structure
*
* Allocate and initialise a media device. Returns a media device.
* The media device is refcounted, and this function returns a media
@@ -222,7 +225,7 @@ void media_device_init(struct media_device *mdev);
* References are taken and given using media_device_get() and
* media_device_put().
*/
-struct media_device *media_device_alloc(struct device *dev);
+struct media_device *media_device_alloc(struct device *dev, void *priv);
/**
* media_device_get() - Get a reference to a media device
@@ -249,6 +252,16 @@ struct media_device *media_device_alloc(struct device *dev);
} while (0)
/**
+ * media_device_priv() - Obtain the driver private pointer
+ *
+ * Returns a pointer passed to the media_device_alloc() function.
+ */
+static inline void *media_device_priv(struct media_device *mdev)
+{
+ return mdev->priv;
+}
+
+/**
* media_device_cleanup() - Cleanups a media device element
*
* @mdev: pointer to struct &media_device
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 16/21] media: Add release callback for media device
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (13 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 15/21] media: Provide a way to the driver to set a private pointer Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 17/21] v4l: Acquire a reference to the media device for every video device Sakari Ailus
` (6 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
The release callback may be used by the driver to signal the release of
the media device. This way the lifetime of the driver's own memory
allocations may be made dependent on that of the media device.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/media-device.c | 4 ++++
include/media/media-device.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7d9f76d..e9dfc87 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -724,6 +724,10 @@ static void media_device_release(struct media_devnode *devnode)
mdev->entity_internal_idx_max = 0;
media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
mutex_destroy(&mdev->graph_mutex);
+
+ if (mdev->ops && mdev->ops->release)
+ mdev->ops->release(mdev);
+
put_device(mdev->dev);
kfree(mdev);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 94e96ef..81f09ed 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -56,6 +56,7 @@ struct media_entity_notify {
struct media_device_ops {
int (*link_notify)(struct media_link *link, u32 flags,
unsigned int notification);
+ void (*release)(struct media_device *mdev);
};
/**
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 17/21] v4l: Acquire a reference to the media device for every video device
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (14 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 16/21] media: Add release callback for media device Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 18/21] media-device: Postpone graph object removal until free Sakari Ailus
` (5 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
The video device depends on the existence of its media device --- if there
is one. Acquire a reference to it.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-dev.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 8be561a..530d53e 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -171,6 +171,9 @@ static void v4l2_device_release(struct device *cd)
{
struct video_device *vdev = to_video_device(cd);
struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
+#ifdef CONFIG_MEDIA_CONTROLLER
+ struct media_device *mdev = v4l2_dev->mdev;
+#endif
mutex_lock(&videodev_lock);
if (WARN_ON(video_device[vdev->minor] != vdev)) {
@@ -193,8 +196,8 @@ static void v4l2_device_release(struct device *cd)
mutex_unlock(&videodev_lock);
-#if defined(CONFIG_MEDIA_CONTROLLER)
- if (v4l2_dev->mdev) {
+#ifdef CONFIG_MEDIA_CONTROLLER
+ if (mdev) {
/* Remove interfaces and interface links */
media_devnode_remove(vdev->intf_devnode);
if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
@@ -220,6 +223,11 @@ static void v4l2_device_release(struct device *cd)
/* Decrease v4l2_device refcount */
if (v4l2_dev)
v4l2_device_put(v4l2_dev);
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+ if (mdev)
+ media_device_put(mdev);
+#endif
}
static struct class video_class = {
@@ -813,6 +821,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
/* FIXME: how to create the other interface links? */
+ media_device_get(vdev->v4l2_dev->mdev);
#endif
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 18/21] media-device: Postpone graph object removal until free
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (15 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 17/21] v4l: Acquire a reference to the media device for every video device Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 19/21] omap3isp: Allocate the media device dynamically Sakari Ailus
` (4 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
The media device itself will be unregistered based on it being unbound and
driver's remove callback being called. The graph objects themselves may
still be in use; rely on the media device release callback to release
them.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 44 ++++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index e9dfc87..7a5884e 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -717,6 +717,26 @@ EXPORT_SYMBOL_GPL(media_device_init);
static void media_device_release(struct media_devnode *devnode)
{
struct media_device *mdev = to_media_device(devnode);
+ struct media_entity *entity;
+ struct media_entity *next;
+ struct media_interface *intf, *tmp_intf;
+ struct media_entity_notify *notify, *nextp;
+
+ /* Remove all entities from the media device */
+ list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
+ __media_device_unregister_entity(entity);
+
+ /* Remove all entity_notify callbacks from the media device */
+ list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
+ __media_device_unregister_entity_notify(mdev, notify);
+
+ /* Remove all interfaces from the media device */
+ list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
+ graph_obj.list) {
+ __media_remove_intf_links(intf);
+ media_gobj_destroy(&intf->graph_obj);
+ kfree(intf);
+ }
dev_dbg(devnode->parent, "Media device released\n");
@@ -797,38 +817,14 @@ EXPORT_SYMBOL_GPL(__media_device_register);
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);
-
- /* Remove all entity_notify callbacks from the media device */
- list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
- __media_device_unregister_entity_notify(mdev, notify);
-
- /* Remove all interfaces from the media device */
- list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
- graph_obj.list) {
- __media_remove_intf_links(intf);
- media_gobj_destroy(&intf->graph_obj);
- kfree(intf);
- }
-
mutex_unlock(&mdev->graph_mutex);
device_remove_file(&mdev->devnode.dev, &dev_attr_model);
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 19/21] omap3isp: Allocate the media device dynamically
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (16 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 18/21] media-device: Postpone graph object removal until free Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-22 10:05 ` Hans Verkuil
2016-11-08 13:55 ` [RFC v4 20/21] omap3isp: Release the isp device struct by media device callback Sakari Ailus
` (3 subsequent siblings)
21 siblings, 1 reply; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
Use the new media_device_alloc() API to allocate and release the media
device.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 24 +++++++++++++-----------
drivers/media/platform/omap3isp/isp.h | 2 +-
drivers/media/platform/omap3isp/ispvideo.c | 2 +-
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 2e1b17e..8bc7a7c 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1601,8 +1601,8 @@ static void isp_unregister_entities(struct isp_device *isp)
omap3isp_stat_unregister_entities(&isp->isp_hist);
v4l2_device_unregister(&isp->v4l2_dev);
- media_device_unregister(&isp->media_dev);
- media_device_cleanup(&isp->media_dev);
+ media_device_unregister(isp->media_dev);
+ media_device_put(isp->media_dev);
}
static int isp_link_entity(
@@ -1680,14 +1680,16 @@ static int isp_register_entities(struct isp_device *isp)
{
int ret;
- isp->media_dev.dev = isp->dev;
- strlcpy(isp->media_dev.model, "TI OMAP3 ISP",
- sizeof(isp->media_dev.model));
- isp->media_dev.hw_revision = isp->revision;
- isp->media_dev.ops = &isp_media_ops;
- media_device_init(&isp->media_dev);
+ isp->media_dev = media_device_alloc(isp->dev, isp);
+ if (!isp->media_dev)
+ return -ENOMEM;
+
+ strlcpy(isp->media_dev->model, "TI OMAP3 ISP",
+ sizeof(isp->media_dev->model));
+ isp->media_dev->hw_revision = isp->revision;
+ isp->media_dev->ops = &isp_media_ops;
- isp->v4l2_dev.mdev = &isp->media_dev;
+ isp->v4l2_dev.mdev = isp->media_dev;
ret = v4l2_device_register(isp->dev, &isp->v4l2_dev);
if (ret < 0) {
dev_err(isp->dev, "%s: V4L2 device registration failed (%d)\n",
@@ -2165,7 +2167,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
struct isp_bus_cfg *bus;
int ret;
- ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
+ ret = media_entity_enum_init(&isp->crashed, isp->media_dev);
if (ret)
return ret;
@@ -2183,7 +2185,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
if (ret < 0)
return ret;
- return media_device_register(&isp->media_dev);
+ return media_device_register(isp->media_dev);
}
/*
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 7e6f663..7378279 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -176,7 +176,7 @@ struct isp_xclk {
struct isp_device {
struct v4l2_device v4l2_dev;
struct v4l2_async_notifier notifier;
- struct media_device media_dev;
+ struct media_device *media_dev;
struct device *dev;
u32 revision;
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 7354469..33e74b9 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1104,7 +1104,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
pipe = video->video.entity.pipe
? to_isp_pipeline(&video->video.entity) : &video->pipe;
- ret = media_entity_enum_init(&pipe->ent_enum, &video->isp->media_dev);
+ ret = media_entity_enum_init(&pipe->ent_enum, video->isp->media_dev);
if (ret)
goto err_enum_init;
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [RFC v4 19/21] omap3isp: Allocate the media device dynamically
2016-11-08 13:55 ` [RFC v4 19/21] omap3isp: Allocate the media device dynamically Sakari Ailus
@ 2016-11-22 10:05 ` Hans Verkuil
2016-12-02 14:52 ` Sakari Ailus
0 siblings, 1 reply; 43+ messages in thread
From: Hans Verkuil @ 2016-11-22 10:05 UTC (permalink / raw)
To: Sakari Ailus, linux-media; +Cc: mchehab, shuahkh, laurent.pinchart
On 08/11/16 14:55, Sakari Ailus wrote:
> Use the new media_device_alloc() API to allocate and release the media
> device.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/omap3isp/isp.c | 24 +++++++++++++-----------
> drivers/media/platform/omap3isp/isp.h | 2 +-
> drivers/media/platform/omap3isp/ispvideo.c | 2 +-
> 3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 2e1b17e..8bc7a7c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1601,8 +1601,8 @@ static void isp_unregister_entities(struct isp_device *isp)
> omap3isp_stat_unregister_entities(&isp->isp_hist);
>
> v4l2_device_unregister(&isp->v4l2_dev);
> - media_device_unregister(&isp->media_dev);
> - media_device_cleanup(&isp->media_dev);
> + media_device_unregister(isp->media_dev);
> + media_device_put(isp->media_dev);
> }
>
> static int isp_link_entity(
> @@ -1680,14 +1680,16 @@ static int isp_register_entities(struct isp_device *isp)
> {
> int ret;
>
> - isp->media_dev.dev = isp->dev;
> - strlcpy(isp->media_dev.model, "TI OMAP3 ISP",
> - sizeof(isp->media_dev.model));
> - isp->media_dev.hw_revision = isp->revision;
> - isp->media_dev.ops = &isp_media_ops;
> - media_device_init(&isp->media_dev);
> + isp->media_dev = media_device_alloc(isp->dev, isp);
> + if (!isp->media_dev)
> + return -ENOMEM;
> +
> + strlcpy(isp->media_dev->model, "TI OMAP3 ISP",
> + sizeof(isp->media_dev->model));
> + isp->media_dev->hw_revision = isp->revision;
> + isp->media_dev->ops = &isp_media_ops;
>
> - isp->v4l2_dev.mdev = &isp->media_dev;
> + isp->v4l2_dev.mdev = isp->media_dev;
> ret = v4l2_device_register(isp->dev, &isp->v4l2_dev);
> if (ret < 0) {
> dev_err(isp->dev, "%s: V4L2 device registration failed (%d)\n",
> @@ -2165,7 +2167,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> struct isp_bus_cfg *bus;
> int ret;
>
> - ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
> + ret = media_entity_enum_init(&isp->crashed, isp->media_dev);
> if (ret)
> return ret;
>
> @@ -2183,7 +2185,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> if (ret < 0)
> return ret;
>
> - return media_device_register(&isp->media_dev);
> + return media_device_register(isp->media_dev);
I wonder if this is correct. Usually if the register fails, then the
release/delete function
has to be called explicitly. That doesn't happen here.
E.g. from adv7604.c:
static int adv76xx_registered(struct v4l2_subdev *sd)
{
struct adv76xx_state *state = to_state(sd);
int err;
err = cec_register_adapter(state->cec_adap);
if (err)
cec_delete_adapter(state->cec_adap);
return err;
}
> }
>
> /*
> diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
> index 7e6f663..7378279 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -176,7 +176,7 @@ struct isp_xclk {
> struct isp_device {
> struct v4l2_device v4l2_dev;
> struct v4l2_async_notifier notifier;
> - struct media_device media_dev;
> + struct media_device *media_dev;
> struct device *dev;
> u32 revision;
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> index 7354469..33e74b9 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1104,7 +1104,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
> pipe = video->video.entity.pipe
> ? to_isp_pipeline(&video->video.entity) : &video->pipe;
>
> - ret = media_entity_enum_init(&pipe->ent_enum, &video->isp->media_dev);
> + ret = media_entity_enum_init(&pipe->ent_enum, video->isp->media_dev);
> if (ret)
> goto err_enum_init;
>
>
Regards,
Hans
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 19/21] omap3isp: Allocate the media device dynamically
2016-11-22 10:05 ` Hans Verkuil
@ 2016-12-02 14:52 ` Sakari Ailus
0 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-12-02 14:52 UTC (permalink / raw)
To: Hans Verkuil
Cc: Sakari Ailus, linux-media, mchehab, shuahkh, laurent.pinchart
Hi Hans,
On Tue, Nov 22, 2016 at 11:05:49AM +0100, Hans Verkuil wrote:
...
> >@@ -2183,7 +2185,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> > if (ret < 0)
> > return ret;
> >
> >- return media_device_register(&isp->media_dev);
> >+ return media_device_register(isp->media_dev);
>
> I wonder if this is correct. Usually if the register fails, then the
> release/delete function
> has to be called explicitly. That doesn't happen here.
This patch is really about making the media_dev a pointer in struct
omap3isp_device. Currently the cleanup takes place when the device is
unbound. That's perhaps not ideal but on the other hand optimising error
handling is often just not worth it.
Improvements could be done how the async framework handles errors but that
shouldn't be in the scope of this patchset.
>
> E.g. from adv7604.c:
>
> static int adv76xx_registered(struct v4l2_subdev *sd)
> {
> struct adv76xx_state *state = to_state(sd);
> int err;
>
> err = cec_register_adapter(state->cec_adap);
> if (err)
> cec_delete_adapter(state->cec_adap);
> return err;
> }
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 43+ messages in thread
* [RFC v4 20/21] omap3isp: Release the isp device struct by media device callback
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (17 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 19/21] omap3isp: Allocate the media device dynamically Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 13:55 ` [RFC v4 21/21] omap3isp: Don't rely on devm for memory resource management Sakari Ailus
` (2 subsequent siblings)
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
Use the media device release callback to release the isp device's data
structure. This approach has the benefit of not releasing memory which may
still be accessed through open file handles whilst the isp driver is being
unbound.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 8bc7a7c..54b84a0 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -657,8 +657,11 @@ static irqreturn_t isp_isr(int irq, void *_isp)
return IRQ_HANDLED;
}
+static void isp_release(struct media_device *mdev);
+
static const struct media_device_ops isp_media_ops = {
.link_notify = v4l2_pipeline_link_notify,
+ .release = isp_release,
};
/* -----------------------------------------------------------------------------
@@ -1948,6 +1951,20 @@ static void isp_detach_iommu(struct isp_device *isp)
iommu_group_remove_device(isp->dev);
}
+static void isp_release(struct media_device *mdev)
+{
+ struct isp_device *isp = media_device_priv(mdev);
+
+ isp_cleanup_modules(isp);
+ isp_xclk_cleanup(isp);
+
+ __omap3isp_get(isp, false);
+ isp_detach_iommu(isp);
+ __omap3isp_put(isp, false);
+
+ media_entity_enum_cleanup(&isp->crashed);
+}
+
static int isp_attach_iommu(struct isp_device *isp)
{
struct dma_iommu_mapping *mapping;
@@ -2008,14 +2025,6 @@ static int isp_remove(struct platform_device *pdev)
v4l2_async_notifier_unregister(&isp->notifier);
isp_unregister_entities(isp);
- isp_cleanup_modules(isp);
- isp_xclk_cleanup(isp);
-
- __omap3isp_get(isp, false);
- isp_detach_iommu(isp);
- __omap3isp_put(isp, false);
-
- media_entity_enum_cleanup(&isp->crashed);
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* [RFC v4 21/21] omap3isp: Don't rely on devm for memory resource management
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (18 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 20/21] omap3isp: Release the isp device struct by media device callback Sakari Ailus
@ 2016-11-08 13:55 ` Sakari Ailus
2016-11-08 17:00 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Mauro Carvalho Chehab
2016-11-22 10:01 ` Laurent Pinchart
21 siblings, 0 replies; 43+ messages in thread
From: Sakari Ailus @ 2016-11-08 13:55 UTC (permalink / raw)
To: linux-media, hverkuil; +Cc: mchehab, shuahkh, laurent.pinchart
devm functions are fine for managing resources that are directly related
to the device at hand and that have no other dependencies. However, a
process holding a file handle to a device created by a driver for a device
may result in the file handle left behind after the device is long gone.
This will result in accessing released (and potentially reallocated)
memory.
Instead, rely on the media device which will stick around until all users
are gone.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 38 ++++++++++++++++++++-------
drivers/media/platform/omap3isp/ispccp2.c | 3 ++-
drivers/media/platform/omap3isp/isph3a_aewb.c | 20 +++++++++-----
drivers/media/platform/omap3isp/isph3a_af.c | 19 ++++++++++----
drivers/media/platform/omap3isp/isphist.c | 5 ++--
drivers/media/platform/omap3isp/ispstat.c | 2 ++
6 files changed, 63 insertions(+), 24 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 54b84a0..5777c55 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1377,7 +1377,7 @@ static int isp_get_clocks(struct isp_device *isp)
unsigned int i;
for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) {
- clk = devm_clk_get(isp->dev, isp_clocks[i]);
+ clk = clk_get(isp->dev, isp_clocks[i]);
if (IS_ERR(clk)) {
dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]);
return PTR_ERR(clk);
@@ -1389,6 +1389,14 @@ static int isp_get_clocks(struct isp_device *isp)
return 0;
}
+static void isp_put_clocks(struct isp_device *isp)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i)
+ clk_put(isp->clock[i]);
+}
+
/*
* omap3isp_get - Acquire the ISP resource.
*
@@ -1603,7 +1611,6 @@ static void isp_unregister_entities(struct isp_device *isp)
omap3isp_stat_unregister_entities(&isp->isp_af);
omap3isp_stat_unregister_entities(&isp->isp_hist);
- v4l2_device_unregister(&isp->v4l2_dev);
media_device_unregister(isp->media_dev);
media_device_put(isp->media_dev);
}
@@ -1955,6 +1962,8 @@ static void isp_release(struct media_device *mdev)
{
struct isp_device *isp = media_device_priv(mdev);
+ v4l2_device_unregister(&isp->v4l2_dev);
+
isp_cleanup_modules(isp);
isp_xclk_cleanup(isp);
@@ -1963,6 +1972,10 @@ static void isp_release(struct media_device *mdev)
__omap3isp_put(isp, false);
media_entity_enum_cleanup(&isp->crashed);
+
+ isp_put_clocks(isp);
+
+ kfree(isp);
}
static int isp_attach_iommu(struct isp_device *isp)
@@ -2215,7 +2228,7 @@ static int isp_probe(struct platform_device *pdev)
int ret;
int i, m;
- isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
+ isp = kzalloc(sizeof(*isp), GFP_KERNEL);
if (!isp) {
dev_err(&pdev->dev, "could not allocate memory\n");
return -ENOMEM;
@@ -2224,21 +2237,23 @@ static int isp_probe(struct platform_device *pdev)
ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
&isp->phy_type);
if (ret)
- return ret;
+ goto error_release_isp;
isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
"syscon");
- if (IS_ERR(isp->syscon))
- return PTR_ERR(isp->syscon);
+ if (IS_ERR(isp->syscon)) {
+ ret = PTR_ERR(isp->syscon);
+ goto error_release_isp;
+ }
ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
&isp->syscon_offset);
if (ret)
- return ret;
+ goto error_release_isp;
ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
if (ret < 0)
- return ret;
+ goto error_release_isp;
isp->autoidle = autoidle;
@@ -2255,8 +2270,8 @@ static int isp_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, isp);
/* Regulators */
- isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1");
- isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2");
+ isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "vdd-csiphy1");
+ isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "vdd-csiphy2");
/* Clocks
*
@@ -2388,6 +2403,9 @@ static int isp_probe(struct platform_device *pdev)
__omap3isp_put(isp, false);
error:
mutex_destroy(&isp->isp_mutex);
+ isp_put_clocks(isp);
+error_release_isp:
+ kfree(isp);
return ret;
}
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..d49ce8a 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1135,7 +1135,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
* TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
*/
if (isp->revision == ISP_REVISION_2_0) {
- ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
+ ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
if (IS_ERR(ccp2->vdds_csib)) {
dev_dbg(isp->dev,
"Could not get regulator vdds_csib\n");
@@ -1163,4 +1163,5 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
omap3isp_video_cleanup(&ccp2->video_in);
media_entity_cleanup(&ccp2->subdev.entity);
+ regulator_put(ccp2->vdds_csib);
}
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index d44626f2..38eaa92 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
{
struct ispstat *aewb = &isp->isp_aewb;
struct omap3isp_h3a_aewb_config *aewb_cfg;
- struct omap3isp_h3a_aewb_config *aewb_recover_cfg;
+ struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL;
+ int ret;
- aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL);
+ aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL);
if (!aewb_cfg)
return -ENOMEM;
@@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
aewb->isp = isp;
/* Set recover state configuration */
- aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg),
- GFP_KERNEL);
+ aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL);
if (!aewb_recover_cfg) {
dev_err(aewb->isp->dev,
"AEWB: cannot allocate memory for recover configuration.\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err;
}
aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
@@ -323,13 +324,20 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) {
dev_err(aewb->isp->dev,
"AEWB: recover configuration is invalid.\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg);
aewb->recover_priv = aewb_recover_cfg;
return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
+
+err:
+ kfree(aewb_cfg);
+ kfree(aewb_recover_cfg);
+
+ return ret;
}
/*
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 99bd6cc..39e1a0c 100644
--- a/drivers/media/platform/omap3isp/isph3a_af.c
+++ b/drivers/media/platform/omap3isp/isph3a_af.c
@@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
{
struct ispstat *af = &isp->isp_af;
struct omap3isp_h3a_af_config *af_cfg;
- struct omap3isp_h3a_af_config *af_recover_cfg;
+ struct omap3isp_h3a_af_config *af_recover_cfg = NULL;
+ int ret;
- af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL);
+ af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL);
if (af_cfg == NULL)
return -ENOMEM;
@@ -364,12 +365,13 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
af->isp = isp;
/* Set recover state configuration */
- af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg),
- GFP_KERNEL);
+ af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL);
if (!af_recover_cfg) {
dev_err(af->isp->dev,
"AF: cannot allocate memory for recover configuration.\n");
return -ENOMEM;
+ ret = -ENOMEM;
+ goto err;
}
af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN;
@@ -381,13 +383,20 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
if (h3a_af_validate_params(af, af_recover_cfg)) {
dev_err(af->isp->dev,
"AF: recover configuration is invalid.\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg);
af->recover_priv = af_recover_cfg;
return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
+
+err:
+ kfree(af_cfg);
+ kfree(af_recover_cfg);
+
+ return ret;
}
void omap3isp_h3a_af_cleanup(struct isp_device *isp)
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
index 7138b04..976cab0 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -477,9 +477,9 @@ int omap3isp_hist_init(struct isp_device *isp)
{
struct ispstat *hist = &isp->isp_hist;
struct omap3isp_hist_config *hist_cfg;
- int ret = -1;
+ int ret;
- hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL);
+ hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL);
if (hist_cfg == NULL)
return -ENOMEM;
@@ -517,6 +517,7 @@ int omap3isp_hist_init(struct isp_device *isp)
if (ret) {
if (hist->dma_ch)
dma_release_channel(hist->dma_ch);
+ kfree(hist_cfg);
}
return ret;
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 47cbc7e..aad0bc3 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -1067,4 +1067,6 @@ void omap3isp_stat_cleanup(struct ispstat *stat)
mutex_destroy(&stat->ioctl_lock);
isp_stat_bufs_free(stat);
kfree(stat->buf);
+ kfree(stat->priv);
+ kfree(stat->recover_priv);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (19 preceding siblings ...)
2016-11-08 13:55 ` [RFC v4 21/21] omap3isp: Don't rely on devm for memory resource management Sakari Ailus
@ 2016-11-08 17:00 ` Mauro Carvalho Chehab
2016-11-10 23:49 ` Laurent Pinchart
2016-11-22 10:01 ` Laurent Pinchart
21 siblings, 1 reply; 43+ messages in thread
From: Mauro Carvalho Chehab @ 2016-11-08 17:00 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, shuahkh, laurent.pinchart
Em Tue, 8 Nov 2016 15:55:10 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> ioctl/syscall and unregister race"). The commit was part of an original
> patchset to avoid crashes when an unregistering device is in use.
As I said before: I will only look on such patch series if you don't
randomly revert patches that aren't broken.
Also, if this series require changes on drivers, it is up to you
to do such changes in a way that won't break patch bisectability,
so, each change should be self-contained and touch all drivers that
are affected by the kAPI change.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"
2016-11-08 17:00 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Mauro Carvalho Chehab
@ 2016-11-10 23:49 ` Laurent Pinchart
0 siblings, 0 replies; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-10 23:49 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Sakari Ailus, linux-media, hverkuil, shuahkh
Hi Mauro,
On Tuesday 08 Nov 2016 15:00:27 Mauro Carvalho Chehab wrote:
> Em Tue, 8 Nov 2016 15:55:10 +0200 Sakari Ailus escreveu:
> > This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> > ioctl/syscall and unregister race"). The commit was part of an original
> > patchset to avoid crashes when an unregistering device is in use.
>
> As I said before: I will only look on such patch series if you don't
> randomly revert patches that aren't broken.
That's not always an option. What you're essentially saying is that if taking
a step forward improves the situation, we can only improve it more by taking
more steps forward. Sometimes the step forward leads to a dead end, making a U
turn unavoidable to avoid running into a wall :-)
Sure, we could squash the reverts as part of a large patch to would be too big
to review, but I hardly see that as an improvement.
This being said, I haven't checked whether it would be possible to shuffle
patches around and revert the first three on top of the rest of the series,
but at first glance it seems very difficult. I don't think we should use the
fact that the three patches were merged despite known issues a strong argument
to prevent them from being reverted. Fixes are being reverted all the time in
the kernel when they cause more harm than good, there's nothing special here.
> Also, if this series require changes on drivers, it is up to you
> to do such changes in a way that won't break patch bisectability,
> so, each change should be self-contained and touch all drivers that
> are affected by the kAPI change.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"
2016-11-08 13:55 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Sakari Ailus
` (20 preceding siblings ...)
2016-11-08 17:00 ` [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race" Mauro Carvalho Chehab
@ 2016-11-22 10:01 ` Laurent Pinchart
21 siblings, 0 replies; 43+ messages in thread
From: Laurent Pinchart @ 2016-11-22 10:01 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, mchehab, shuahkh
Hi Sakari,
Thank you for the patch.
On Tuesday 08 Nov 2016 15:55:10 Sakari Ailus wrote:
> This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> ioctl/syscall and unregister race"). The commit was part of an original
> patchset to avoid crashes when an unregistering device is in use.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
For 01/21 to 03/21,
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/media-device.c | 15 +++++++--------
> drivers/media/media-devnode.c | 8 +-------
> include/media/media-devnode.h | 16 ++--------------
> 3 files changed, 10 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 2783531..f2525eb 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -730,7 +730,6 @@ int __must_check __media_device_register(struct
> media_device *mdev, if (ret < 0) {
> /* devnode free is handled in media_devnode_*() */
> mdev->devnode = NULL;
> - media_devnode_unregister_prepare(devnode);
> media_devnode_unregister(devnode);
> return ret;
> }
> @@ -787,9 +786,6 @@ void media_device_unregister(struct media_device *mdev)
> return;
> }
>
> - /* Clear the devnode register bit to avoid races with media dev open
*/
> - media_devnode_unregister_prepare(mdev->devnode);
> -
> /* Remove all entities from the media device */
> list_for_each_entry_safe(entity, next, &mdev->entities,
graph_obj.list)
> __media_device_unregister_entity(entity);
> @@ -810,10 +806,13 @@ void media_device_unregister(struct media_device
> *mdev)
>
> dev_dbg(mdev->dev, "Media device unregistered\n");
>
> - device_remove_file(&mdev->devnode->dev, &dev_attr_model);
> - media_devnode_unregister(mdev->devnode);
> - /* devnode free is handled in media_devnode_*() */
> - mdev->devnode = NULL;
> + /* 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);
> + /* devnode free is handled in media_devnode_*() */
> + mdev->devnode = NULL;
> + }
> }
> EXPORT_SYMBOL_GPL(media_device_unregister);
>
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index f2772ba..5b605ff 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -287,7 +287,7 @@ int __must_check media_devnode_register(struct
> media_device *mdev, return ret;
> }
>
> -void media_devnode_unregister_prepare(struct media_devnode *devnode)
> +void media_devnode_unregister(struct media_devnode *devnode)
> {
> /* Check if devnode was ever registered at all */
> if (!media_devnode_is_registered(devnode))
> @@ -295,12 +295,6 @@ void media_devnode_unregister_prepare(struct
> media_devnode *devnode)
>
> mutex_lock(&media_devnode_lock);
> clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
> - mutex_unlock(&media_devnode_lock);
> -}
> -
> -void media_devnode_unregister(struct media_devnode *devnode)
> -{
> - mutex_lock(&media_devnode_lock);
> /* Delete the cdev on this minor as well */
> cdev_del(&devnode->cdev);
> mutex_unlock(&media_devnode_lock);
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index cd23e91..d55ec2b 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -128,26 +128,14 @@ int __must_check media_devnode_register(struct
> media_device *mdev, struct module *owner);
>
> /**
> - * media_devnode_unregister_prepare - clear the media device node register
> bit - * @devnode: the device node to prepare for unregister
> - *
> - * This clears the passed device register bit. Future open calls will be
> met - * with errors. Should be called before media_devnode_unregister() to
> avoid - * races with unregister and device file open calls.
> - *
> - * This function can safely be called if the device node has never been
> - * registered or has already been unregistered.
> - */
> -void media_devnode_unregister_prepare(struct media_devnode *devnode);
> -
> -/**
> * media_devnode_unregister - unregister a media device node
> * @devnode: the device node to unregister
> *
> * This unregisters the passed device. Future open calls will be met with
> * errors.
> *
> - * Should be called after media_devnode_unregister_prepare()
> + * This function can safely be called if the device node has never been
> + * registered or has already been unregistered.
> */
> void media_devnode_unregister(struct media_devnode *devnode);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 43+ messages in thread