[PATCH 1/5] [media] media-device: get rid of the spinlock

[PATCH 1/5] [media] media-device: get rid of the spinlock

[Mauro Carvalho Chehab]

Right now, the lock schema for media_device struct is messy,
since sometimes, it is protected via a spin lock, while, for
media graph traversal, it is protected by a mutex.

Solve this conflict by always using a mutex.

As a side effect, this prevents a bug where the media notifiers
were called at atomic context.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/media-device.c | 32 ++++++++++++++------------------
 drivers/media/media-entity.c | 16 ++++++++--------
 include/media/media-device.h |  6 +-----
 3 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 6e43c95629ea..c32fa15cc76e 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -90,17 +90,17 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
 
 	id &= ~MEDIA_ENT_ID_FLAG_NEXT;
 
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 
 	media_device_for_each_entity(entity, mdev) {
 		if (((media_entity_id(entity) == id) && !next) ||
 		    ((media_entity_id(entity) > id) && next)) {
-			spin_unlock(&mdev->lock);
+			mutex_unlock(&mdev->graph_mutex);
 			return entity;
 		}
 	}
 
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 
 	return NULL;
 }
@@ -590,12 +590,12 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 	if (!ida_pre_get(&mdev->entity_internal_idx, GFP_KERNEL))
 		return -ENOMEM;
 
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 
 	ret = ida_get_new_above(&mdev->entity_internal_idx, 1,
 				&entity->internal_idx);
 	if (ret < 0) {
-		spin_unlock(&mdev->lock);
+		mutex_unlock(&mdev->graph_mutex);
 		return ret;
 	}
 
@@ -615,9 +615,6 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 		(notify)->notify(entity, notify->notify_data);
 	}
 
-	spin_unlock(&mdev->lock);
-
-	mutex_lock(&mdev->graph_mutex);
 	if (mdev->entity_internal_idx_max
 	    >= mdev->pm_count_walk.ent_enum.idx_max) {
 		struct media_entity_graph new = { .top = 0 };
@@ -680,9 +677,9 @@ void media_device_unregister_entity(struct media_entity *entity)
 	if (mdev == NULL)
 		return;
 
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 	__media_device_unregister_entity(entity);
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
@@ -703,7 +700,6 @@ void media_device_init(struct media_device *mdev)
 	INIT_LIST_HEAD(&mdev->pads);
 	INIT_LIST_HEAD(&mdev->links);
 	INIT_LIST_HEAD(&mdev->entity_notify);
-	spin_lock_init(&mdev->lock);
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
 
@@ -752,9 +748,9 @@ EXPORT_SYMBOL_GPL(__media_device_register);
 int __must_check media_device_register_entity_notify(struct media_device *mdev,
 					struct media_entity_notify *nptr)
 {
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 	list_add_tail(&nptr->list, &mdev->entity_notify);
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
@@ -771,9 +767,9 @@ static void __media_device_unregister_entity_notify(struct media_device *mdev,
 void media_device_unregister_entity_notify(struct media_device *mdev,
 					struct media_entity_notify *nptr)
 {
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 	__media_device_unregister_entity_notify(mdev, nptr);
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
 
@@ -787,11 +783,11 @@ void media_device_unregister(struct media_device *mdev)
 	if (mdev == NULL)
 		return;
 
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 
 	/* Check if mdev was ever registered at all */
 	if (!media_devnode_is_registered(&mdev->devnode)) {
-		spin_unlock(&mdev->lock);
+		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
 
@@ -811,7 +807,7 @@ void media_device_unregister(struct media_device *mdev)
 		kfree(intf);
 	}
 
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	media_devnode_unregister(&mdev->devnode);
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e95070b3a3d4..c53c1d5589a0 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -219,7 +219,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
 	entity->pads = pads;
 
 	if (mdev)
-		spin_lock(&mdev->lock);
+		mutex_lock(&mdev->graph_mutex);
 
 	for (i = 0; i < num_pads; i++) {
 		pads[i].entity = entity;
@@ -230,7 +230,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
 	}
 
 	if (mdev)
-		spin_unlock(&mdev->lock);
+		mutex_unlock(&mdev->graph_mutex);
 
 	return 0;
 }
@@ -747,9 +747,9 @@ void media_entity_remove_links(struct media_entity *entity)
 	if (mdev == NULL)
 		return;
 
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 	__media_entity_remove_links(entity);
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_entity_remove_links);
 
@@ -951,9 +951,9 @@ void media_remove_intf_link(struct media_link *link)
 	if (mdev == NULL)
 		return;
 
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 	__media_remove_intf_link(link);
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_remove_intf_link);
 
@@ -975,8 +975,8 @@ void media_remove_intf_links(struct media_interface *intf)
 	if (mdev == NULL)
 		return;
 
-	spin_lock(&mdev->lock);
+	mutex_lock(&mdev->graph_mutex);
 	__media_remove_intf_links(intf);
-	spin_unlock(&mdev->lock);
+	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_remove_intf_links);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index df74cfa7da4a..0c2de97181f3 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -25,7 +25,6 @@
 
 #include <linux/list.h>
 #include <linux/mutex.h>
-#include <linux/spinlock.h>
 
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
@@ -304,8 +303,7 @@ struct media_entity_notify {
  * @pads:	List of registered pads
  * @links:	List of registered links
  * @entity_notify: List of registered entity_notify callbacks
- * @lock:	Entities list lock
- * @graph_mutex: Entities graph operation lock
+ * @graph_mutex: Protects access to struct media_device data
  * @pm_count_walk: Graph walk for power state walk. Access serialised using
  *		   graph_mutex.
  *
@@ -371,8 +369,6 @@ struct media_device {
 	/* notify callback list invoked when a new entity is registered */
 	struct list_head entity_notify;
 
-	/* Protects the graph objects creation/removal */
-	spinlock_t lock;
 	/* Serializes graph operations. */
 	struct mutex graph_mutex;
 	struct media_entity_graph pm_count_walk;
-- 
2.5.0

[PATCH 2/5] [media] media-device: Fix a comment

[Mauro Carvalho Chehab]

The comment is for the wrong function. Fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 include/media/media-device.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/media/media-device.h b/include/media/media-device.h
index 0c2de97181f3..ca3871b853ba 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -490,7 +490,7 @@ int __must_check __media_device_register(struct media_device *mdev,
 #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
 
 /**
- * __media_device_unregister() - Unegisters a media device element
+ *_media_device_unregister() - Unegisters a media device element
  *
  * @mdev:	pointer to struct &media_device
  *
-- 
2.5.0

Re: [PATCH 2/5] [media] media-device: Fix a comment

[Javier Martinez Canillas]

Hello Mauro,

On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> The comment is for the wrong function. Fix it.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  include/media/media-device.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 0c2de97181f3..ca3871b853ba 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -490,7 +490,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>  #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
>
>  /**
> - * __media_device_unregister() - Unegisters a media device element
> + *_media_device_unregister() - Unegisters a media device element
>   *

s/_media_device_unregister()/media_device_unregister()

and while being there, you can also fix the "Unegisters" typo.

After those changes:

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

[PATCH 3/5] [media] au0828: Unregister notifiers

[Mauro Carvalho Chehab]

If au0828 gets removed, we need to remove the notifiers.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-core.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 2fcd17d9b1a6..06da73f1ff22 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -131,21 +131,35 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
 	return status;
 }
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+static void au0828_media_graph_notify(struct media_entity *new,
+				      void *notify_data);
+#endif
+
 static void au0828_unregister_media_device(struct au0828_dev *dev)
 {
-
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (dev->media_dev &&
-		media_devnode_is_registered(&dev->media_dev->devnode)) {
-		/* clear enable_source, disable_source */
-		dev->media_dev->source_priv = NULL;
-		dev->media_dev->enable_source = NULL;
-		dev->media_dev->disable_source = NULL;
+	struct media_device *mdev = dev->media_dev;
+	struct media_entity_notify *notify, *nextp;
 
-		media_device_unregister(dev->media_dev);
-		media_device_cleanup(dev->media_dev);
-		dev->media_dev = NULL;
+	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
+		return;
+
+	/* Remove au0828 entity_notify callbacks */
+	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list) {
+		if (notify->notify != au0828_media_graph_notify)
+			continue;
+		media_device_unregister_entity_notify(mdev, notify);
 	}
+
+	/* clear enable_source, disable_source */
+	dev->media_dev->source_priv = NULL;
+	dev->media_dev->enable_source = NULL;
+	dev->media_dev->disable_source = NULL;
+
+	media_device_unregister(dev->media_dev);
+	media_device_cleanup(dev->media_dev);
+	dev->media_dev = NULL;
 #endif
 }
 
-- 
2.5.0

Re: [PATCH 3/5] [media] au0828: Unregister notifiers

[Javier Martinez Canillas]

Hello Mauro,

Patch looks good to me, I just have a minor comment:

On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> If au0828 gets removed, we need to remove the notifiers.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/usb/au0828/au0828-core.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 2fcd17d9b1a6..06da73f1ff22 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -131,21 +131,35 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
>         return status;
>  }
>
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static void au0828_media_graph_notify(struct media_entity *new,
> +                                     void *notify_data);
> +#endif
> +

I would rather move the au0828_media_graph_notify() function
definition before au0828_unregister_media_device() instead of adding
this function forward declaration. Specially since requires ifdef
guards which makes it even harder to read.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

Re: [PATCH 3/5] [media] au0828: Unregister notifiers

[Shuah Khan]

On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> If au0828 gets removed, we need to remove the notifiers.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.

Ran device removal test and rmmod and modprobe tests on both drivers.

Generated graph after the runs and the graph looks good.

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
Tested-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

> ---
>  drivers/media/usb/au0828/au0828-core.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 2fcd17d9b1a6..06da73f1ff22 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -131,21 +131,35 @@ static int recv_control_msg(struct au0828_dev *dev, u16 request, u32 value,
>  	return status;
>  }
>  
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static void au0828_media_graph_notify(struct media_entity *new,
> +				      void *notify_data);
> +#endif
> +
>  static void au0828_unregister_media_device(struct au0828_dev *dev)
>  {
> -
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -	if (dev->media_dev &&
> -		media_devnode_is_registered(&dev->media_dev->devnode)) {
> -		/* clear enable_source, disable_source */
> -		dev->media_dev->source_priv = NULL;
> -		dev->media_dev->enable_source = NULL;
> -		dev->media_dev->disable_source = NULL;
> +	struct media_device *mdev = dev->media_dev;
> +	struct media_entity_notify *notify, *nextp;
>  
> -		media_device_unregister(dev->media_dev);
> -		media_device_cleanup(dev->media_dev);
> -		dev->media_dev = NULL;
> +	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
> +		return;
> +
> +	/* Remove au0828 entity_notify callbacks */
> +	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list) {
> +		if (notify->notify != au0828_media_graph_notify)
> +			continue;
> +		media_device_unregister_entity_notify(mdev, notify);
>  	}
> +
> +	/* clear enable_source, disable_source */
> +	dev->media_dev->source_priv = NULL;
> +	dev->media_dev->enable_source = NULL;
> +	dev->media_dev->disable_source = NULL;
> +
> +	media_device_unregister(dev->media_dev);
> +	media_device_cleanup(dev->media_dev);
> +	dev->media_dev = NULL;
>  #endif
>  }
>  
> 


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[PATCH 4/5] [media] media-device: use kref for media_device instance

[Mauro Carvalho Chehab]

Now that the media_device can be used by multiple drivers,
via devres, we need to be sure that it will be dropped only
when all drivers stop using it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
 include/media/media-device.h |  3 +++
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c32fa15cc76e..38e6c319fe6e 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
 {
 	int ret;
 
+	/* Check if mdev was ever registered at all */
+	mutex_lock(&mdev->graph_mutex);
+	if (media_devnode_is_registered(&mdev->devnode)) {
+		kref_get(&mdev->kref);
+		mutex_unlock(&mdev->graph_mutex);
+		return 0;
+	}
+	kref_init(&mdev->kref);
+
 	/* Register the device node. */
 	mdev->devnode.fops = &media_device_fops;
 	mdev->devnode.parent = mdev->dev;
@@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
 	mdev->topology_version = 0;
 
 	ret = media_devnode_register(&mdev->devnode, owner);
-	if (ret < 0)
+	if (ret < 0) {
+		media_devnode_unregister(&mdev->devnode);
 		return ret;
+	}
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
 	if (ret < 0) {
@@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
 		return ret;
 	}
 
+	mutex_unlock(&mdev->graph_mutex);
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
@@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
 
-void media_device_unregister(struct media_device *mdev)
+static void do_media_device_unregister(struct kref *kref)
 {
+	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;
-	}
+	mdev = container_of(kref, struct media_device, kref);
 
 	/* Remove all entities from the media device */
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
@@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
 		kfree(intf);
 	}
 
-	mutex_unlock(&mdev->graph_mutex);
+	/* Check if mdev devnode was registered */
+	if (!media_devnode_is_registered(&mdev->devnode))
+		return;
 
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	media_devnode_unregister(&mdev->devnode);
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 }
+
+void media_device_unregister(struct media_device *mdev)
+{
+	if (mdev == NULL)
+		return;
+
+	mutex_lock(&mdev->graph_mutex);
+	kref_put(&mdev->kref, do_media_device_unregister);
+	mutex_unlock(&mdev->graph_mutex);
+
+}
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
 static void media_device_release_devres(struct device *dev, void *res)
@@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
 	struct media_device *mdev;
 
 	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
-	if (mdev)
+	if (mdev) {
+		kref_get(&mdev->kref);
 		return mdev;
+	}
 
 	mdev = devres_alloc(media_device_release_devres,
 				sizeof(struct media_device), GFP_KERNEL);
 	if (!mdev)
 		return NULL;
+
 	return devres_get(dev, mdev, NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(media_device_get_devres);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index ca3871b853ba..73c16e6e6b6b 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -23,6 +23,7 @@
 #ifndef _MEDIA_DEVICE_H
 #define _MEDIA_DEVICE_H
 
+#include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 
@@ -283,6 +284,7 @@ struct media_entity_notify {
  * struct media_device - Media device
  * @dev:	Parent device
  * @devnode:	Media device node
+ * @kref:	Object refcount
  * @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
@@ -347,6 +349,7 @@ struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
 	struct media_devnode devnode;
+	struct kref kref;
 
 	char model[32];
 	char driver_name[32];
-- 
2.5.0

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Javier Martinez Canillas]

Hello Mauro,

Patch looks almost good to me, I just have a question below:

On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> Now that the media_device can be used by multiple drivers,
> via devres, we need to be sure that it will be dropped only
> when all drivers stop using it.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
>  include/media/media-device.h |  3 +++
>  2 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..38e6c319fe6e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>  {
>         int ret;
>
> +       /* Check if mdev was ever registered at all */
> +       mutex_lock(&mdev->graph_mutex);
> +       if (media_devnode_is_registered(&mdev->devnode)) {
> +               kref_get(&mdev->kref);
> +               mutex_unlock(&mdev->graph_mutex);
> +               return 0;
> +       }
> +       kref_init(&mdev->kref);
> +
>         /* Register the device node. */
>         mdev->devnode.fops = &media_device_fops;
>         mdev->devnode.parent = mdev->dev;
> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
>         mdev->topology_version = 0;
>
>         ret = media_devnode_register(&mdev->devnode, owner);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               media_devnode_unregister(&mdev->devnode);

Why are you adding this? If media_devnode_register() failed then the
device node won't be registered so is not correct to call
media_devnode_unregister(). Or maybe I'm missing something.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Shuah Khan]

On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> Now that the media_device can be used by multiple drivers,
> via devres, we need to be sure that it will be dropped only
> when all drivers stop using it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
>  include/media/media-device.h |  3 +++
>  2 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..38e6c319fe6e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>  {
>  	int ret;
>  
> +	/* Check if mdev was ever registered at all */
> +	mutex_lock(&mdev->graph_mutex);
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		kref_get(&mdev->kref);
> +		mutex_unlock(&mdev->graph_mutex);
> +		return 0;
> +	}
> +	kref_init(&mdev->kref);
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
>  	mdev->topology_version = 0;
>  
>  	ret = media_devnode_register(&mdev->devnode, owner);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		media_devnode_unregister(&mdev->devnode);
>  		return ret;
> +	}
>  
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>  		return ret;
>  	}
>  
> +	mutex_unlock(&mdev->graph_mutex);
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
>  	return 0;
> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>  
> -void media_device_unregister(struct media_device *mdev)
> +static void do_media_device_unregister(struct kref *kref)
>  {
> +	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;
> -	}
> +	mdev = container_of(kref, struct media_device, kref);
>  
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
>  		kfree(intf);
>  	}
>  
> -	mutex_unlock(&mdev->graph_mutex);
> +	/* Check if mdev devnode was registered */
> +	if (!media_devnode_is_registered(&mdev->devnode))
> +		return;
>  
>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>  	media_devnode_unregister(&mdev->devnode);

Patch looks good.

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>

Please see a few comments below that aren't related to this patch.

The above is unprotected and could be done twice when two drivers
call media_device_unregister(). I think we still mark the media
device unregistered in media_devnode_unregister(). We have to
protect these two steps still.

I attempted to do this with a unregister in progress flag which
gets set at the beginning in media_device_unregister(). That
does ensure media_device_unregister() runs only once. If that
approach isn't desirable, we have to find another way.

-- Shuah
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> +	if (mdev == NULL)
> +		return;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	kref_put(&mdev->kref, do_media_device_unregister);
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>  	struct media_device *mdev;
>  
>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> +	if (mdev) {
> +		kref_get(&mdev->kref);
>  		return mdev;
> +	}
>  
>  	mdev = devres_alloc(media_device_release_devres,
>  				sizeof(struct media_device), GFP_KERNEL);
>  	if (!mdev)
>  		return NULL;
> +
>  	return devres_get(dev, mdev, NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index ca3871b853ba..73c16e6e6b6b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -23,6 +23,7 @@
>  #ifndef _MEDIA_DEVICE_H
>  #define _MEDIA_DEVICE_H
>  
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> @@ -283,6 +284,7 @@ struct media_entity_notify {
>   * struct media_device - Media device
>   * @dev:	Parent device
>   * @devnode:	Media device node
> + * @kref:	Object refcount
>   * @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
> @@ -347,6 +349,7 @@ struct media_device {
>  	/* dev->driver_data points to this struct. */
>  	struct device *dev;
>  	struct media_devnode devnode;
> +	struct kref kref;
>  
>  	char model[32];
>  	char driver_name[32];
> 


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Mauro Carvalho Chehab]

Em Wed, 16 Mar 2016 08:05:15 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> > Now that the media_device can be used by multiple drivers,
> > via devres, we need to be sure that it will be dropped only
> > when all drivers stop using it.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > ---
> >  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
> >  include/media/media-device.h |  3 +++
> >  2 files changed, 37 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index c32fa15cc76e..38e6c319fe6e 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  {
> >  	int ret;
> >  
> > +	/* Check if mdev was ever registered at all */
> > +	mutex_lock(&mdev->graph_mutex);
> > +	if (media_devnode_is_registered(&mdev->devnode)) {
> > +		kref_get(&mdev->kref);
> > +		mutex_unlock(&mdev->graph_mutex);
> > +		return 0;
> > +	}
> > +	kref_init(&mdev->kref);
> > +
> >  	/* Register the device node. */
> >  	mdev->devnode.fops = &media_device_fops;
> >  	mdev->devnode.parent = mdev->dev;
> > @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  	mdev->topology_version = 0;
> >  
> >  	ret = media_devnode_register(&mdev->devnode, owner);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		media_devnode_unregister(&mdev->devnode);
> >  		return ret;
> > +	}
> >  
> >  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
> >  	if (ret < 0) {
> > @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  		return ret;
> >  	}
> >  
> > +	mutex_unlock(&mdev->graph_mutex);
> >  	dev_dbg(mdev->dev, "Media device registered\n");
> >  
> >  	return 0;
> > @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
> >  
> > -void media_device_unregister(struct media_device *mdev)
> > +static void struct kref *kref)
> >  {
> > +	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;
> > -	}
> > +	mdev = container_of(kref, struct media_device, kref);
> >  
> >  	/* Remove all entities from the media device */
> >  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> > @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
> >  		kfree(intf);
> >  	}
> >  
> > -	mutex_unlock(&mdev->graph_mutex);
> > +	/* Check if mdev devnode was registered */
> > +	if (!media_devnode_is_registered(&mdev->devnode))
> > +		return;
> >  
> >  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> >  	media_devnode_unregister(&mdev->devnode);  
> 
> Patch looks good.
> 
> Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
> 
> Please see a few comments below that aren't related to this patch.
> 
> The above is unprotected and could be done twice when two drivers
> call media_device_unregister(). I think we still mark the media
> device unregistered in media_devnode_unregister(). We have to
> protect these two steps still.
> 
> I attempted to do this with a unregister in progress flag which
> gets set at the beginning in media_device_unregister(). That
> does ensure media_device_unregister() runs only once. If that
> approach isn't desirable, we have to find another way.

Do you mean do_media_device_unregister()? This is protected, as
this function is only called via media_device_unregister(),
with the mutex hold. I opted to take the mutex there, as
it makes the return code simpler.

> 
> -- Shuah
> >  
> >  	dev_dbg(mdev->dev, "Media device unregistered\n");
> >  }
> > +
> > +void media_device_unregister(struct media_device *mdev)
> > +{
> > +	if (mdev == NULL)
> > +		return;
> > +
> > +	mutex_lock(&mdev->graph_mutex);
> > +	kref_put(&mdev->kref, do_media_device_unregister);
> > +	mutex_unlock(&mdev->graph_mutex);
> > +
> > +}
> >  EXPORT_SYMBOL_GPL(media_device_unregister);
> >  
> >  static void media_device_release_devres(struct device *dev, void *res)
> > @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
> >  	struct media_device *mdev;
> >  
> >  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> > -	if (mdev)
> > +	if (mdev) {
> > +		kref_get(&mdev->kref);
> >  		return mdev;
> > +	}
> >  
> >  	mdev = devres_alloc(media_device_release_devres,
> >  				sizeof(struct media_device), GFP_KERNEL);
> >  	if (!mdev)
> >  		return NULL;
> > +
> >  	return devres_get(dev, mdev, NULL, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_get_devres);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index ca3871b853ba..73c16e6e6b6b 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -23,6 +23,7 @@
> >  #ifndef _MEDIA_DEVICE_H
> >  #define _MEDIA_DEVICE_H
> >  
> > +#include <linux/kref.h>
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> >  
> > @@ -283,6 +284,7 @@ struct media_entity_notify {
> >   * struct media_device - Media device
> >   * @dev:	Parent device
> >   * @devnode:	Media device node
> > + * @kref:	Object refcount
> >   * @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
> > @@ -347,6 +349,7 @@ struct media_device {
> >  	/* dev->driver_data points to this struct. */
> >  	struct device *dev;
> >  	struct media_devnode devnode;
> > +	struct kref kref;
> >  
> >  	char model[32];
> >  	char driver_name[32];
> >   
> 
> 


-- 
Thanks,
Mauro

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Shuah Khan]

On 03/16/2016 08:25 AM, Mauro Carvalho Chehab wrote:
> Em Wed, 16 Mar 2016 08:05:15 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
>>> Now that the media_device can be used by multiple drivers,
>>> via devres, we need to be sure that it will be dropped only
>>> when all drivers stop using it.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>> ---
>>>  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
>>>  include/media/media-device.h |  3 +++
>>>  2 files changed, 37 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index c32fa15cc76e..38e6c319fe6e 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>>>  {
>>>  	int ret;
>>>  
>>> +	/* Check if mdev was ever registered at all */
>>> +	mutex_lock(&mdev->graph_mutex);
>>> +	if (media_devnode_is_registered(&mdev->devnode)) {
>>> +		kref_get(&mdev->kref);
>>> +		mutex_unlock(&mdev->graph_mutex);
>>> +		return 0;
>>> +	}
>>> +	kref_init(&mdev->kref);
>>> +
>>>  	/* Register the device node. */
>>>  	mdev->devnode.fops = &media_device_fops;
>>>  	mdev->devnode.parent = mdev->dev;
>>> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
>>>  	mdev->topology_version = 0;
>>>  
>>>  	ret = media_devnode_register(&mdev->devnode, owner);
>>> -	if (ret < 0)
>>> +	if (ret < 0) {
>>> +		media_devnode_unregister(&mdev->devnode);
>>>  		return ret;
>>> +	}
>>>  
>>>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>>>  	if (ret < 0) {
>>> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>>>  		return ret;
>>>  	}
>>>  
>>> +	mutex_unlock(&mdev->graph_mutex);
>>>  	dev_dbg(mdev->dev, "Media device registered\n");
>>>  
>>>  	return 0;
>>> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
>>>  }
>>>  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>>>  
>>> -void media_device_unregister(struct media_device *mdev)
>>> +static void struct kref *kref)
>>>  {
>>> +	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;
>>> -	}
>>> +	mdev = container_of(kref, struct media_device, kref);
>>>  
>>>  	/* Remove all entities from the media device */
>>>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>>> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
>>>  		kfree(intf);
>>>  	}
>>>  
>>> -	mutex_unlock(&mdev->graph_mutex);
>>> +	/* Check if mdev devnode was registered */
>>> +	if (!media_devnode_is_registered(&mdev->devnode))
>>> +		return;
>>>  
>>>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>>>  	media_devnode_unregister(&mdev->devnode);  
>>
>> Patch looks good.
>>
>> Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
>>
>> Please see a few comments below that aren't related to this patch.
>>
>> The above is unprotected and could be done twice when two drivers
>> call media_device_unregister(). I think we still mark the media
>> device unregistered in media_devnode_unregister(). We have to
>> protect these two steps still.
>>
>> I attempted to do this with a unregister in progress flag which
>> gets set at the beginning in media_device_unregister(). That
>> does ensure media_device_unregister() runs only once. If that
>> approach isn't desirable, we have to find another way.
> 
> Do you mean do_media_device_unregister()? This is protected, as
> this function is only called via media_device_unregister(),
> with the mutex hold. I opted to take the mutex there, as
> it makes the return code simpler.
> 

The below two steps are my concern. With the mutex changes in
do_media_device_unregister() closed critical windows, however
the below code path still concerns me.

device_remove_file(&mdev->devnode.dev, &dev_attr_model);
media_devnode_unregister(&mdev->devnode); 

Especially since we do the clear MEDIA_FLAG_REGISTERED in
media_devnode_unregister(). This step is done while holding
media_devnode_lock - a different mutex

We rely on media_devnode_is_registered() check to determine
whether to start unregister or not. Hence, there is a window
where, we could potentially try to do the following twice:

device_remove_file(&mdev->devnode.dev, &dev_attr_model);
media_devnode_unregister(&mdev->devnode);

You will see this only when both au0828 and snd-usb-audio
try to unregister()

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Sakari Ailus]

Hi Mauro,

On Wed, Mar 16, 2016 at 09:04:05AM -0300, Mauro Carvalho Chehab wrote:
> Now that the media_device can be used by multiple drivers,
> via devres, we need to be sure that it will be dropped only
> when all drivers stop using it.

Not long ago we split media device initialisation and registration into two.
The intent with that was to prevent users from seeing the media device until
all the initialisation is finished. I suppose that with dynamic updates, or
media device being shared with two drivers, it might be difficult to achieve
that. At least the method has to be different.

media_device_init() and media_device_cleanup() were a pair where the latter
undid the first. This patchs remove the requirement of calling cleanup
explitly, breaking that model. It's perhaps not a big problem, there is
likely no single driver which would initialise the media controller device
once but would register and unregister it multiple times. I still wonder if
we really lost something important if we removed media_device_init() and
media_device_cleanup() altogether and merged the functionality in
media_device_{,un}register().

Cc Javier who wrote the patch.

commit 9832e155f1ed3030fdfaa19e72c06472dc2ecb1d
Author: Javier Martinez Canillas <javier@osg.samsung.com>
Date:   Fri Dec 11 20:57:08 2015 -0200

    [media] media-device: split media initialization and registration

For system-wide media device, I think we'd need to introduce a new concept,
graph, that would define an interconnected set of entities. This would
mainly be needed for callbacks, e.g. the power on / off sequences of the
entities could be specific to V4L as discussed earlier. With this approach
hacks wouldn't be needed to be made in the first place to support two drivers
sharing a media device.

What was the original reason you needed to share the media device btw.?

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
>  include/media/media-device.h |  3 +++
>  2 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..38e6c319fe6e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>  {
>  	int ret;
>  
> +	/* Check if mdev was ever registered at all */
> +	mutex_lock(&mdev->graph_mutex);
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		kref_get(&mdev->kref);
> +		mutex_unlock(&mdev->graph_mutex);
> +		return 0;
> +	}
> +	kref_init(&mdev->kref);
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
>  	mdev->topology_version = 0;
>  
>  	ret = media_devnode_register(&mdev->devnode, owner);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		media_devnode_unregister(&mdev->devnode);
>  		return ret;
> +	}
>  
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>  		return ret;
>  	}
>  
> +	mutex_unlock(&mdev->graph_mutex);
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
>  	return 0;
> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>  
> -void media_device_unregister(struct media_device *mdev)
> +static void do_media_device_unregister(struct kref *kref)
>  {
> +	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;
> -	}
> +	mdev = container_of(kref, struct media_device, kref);
>  
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
>  		kfree(intf);
>  	}
>  
> -	mutex_unlock(&mdev->graph_mutex);
> +	/* Check if mdev devnode was registered */
> +	if (!media_devnode_is_registered(&mdev->devnode))
> +		return;
>  
>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>  	media_devnode_unregister(&mdev->devnode);
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> +	if (mdev == NULL)
> +		return;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	kref_put(&mdev->kref, do_media_device_unregister);
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>  	struct media_device *mdev;
>  
>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> +	if (mdev) {
> +		kref_get(&mdev->kref);
>  		return mdev;
> +	}
>  
>  	mdev = devres_alloc(media_device_release_devres,
>  				sizeof(struct media_device), GFP_KERNEL);
>  	if (!mdev)
>  		return NULL;
> +
>  	return devres_get(dev, mdev, NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index ca3871b853ba..73c16e6e6b6b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -23,6 +23,7 @@
>  #ifndef _MEDIA_DEVICE_H
>  #define _MEDIA_DEVICE_H
>  
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> @@ -283,6 +284,7 @@ struct media_entity_notify {
>   * struct media_device - Media device
>   * @dev:	Parent device
>   * @devnode:	Media device node
> + * @kref:	Object refcount
>   * @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
> @@ -347,6 +349,7 @@ struct media_device {
>  	/* dev->driver_data points to this struct. */
>  	struct device *dev;
>  	struct media_devnode devnode;
> +	struct kref kref;
>  
>  	char model[32];
>  	char driver_name[32];
> -- 
> 2.5.0
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Mauro Carvalho Chehab]

Em Thu, 17 Mar 2016 13:50:53 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Wed, Mar 16, 2016 at 09:04:05AM -0300, Mauro Carvalho Chehab wrote:
> > Now that the media_device can be used by multiple drivers,
> > via devres, we need to be sure that it will be dropped only
> > when all drivers stop using it.  
> 
> Not long ago we split media device initialisation and registration into two.
> The intent with that was to prevent users from seeing the media device until
> all the initialisation is finished. I suppose that with dynamic updates, or
> media device being shared with two drivers, it might be difficult to achieve
> that. At least the method has to be different.
> 
> media_device_init() and media_device_cleanup() were a pair where the latter
> undid the first. 

Yes, this patch breaks the balance of _init()/_cleanup(), but this is for
a greater good. It is very hard to remove the devnode and cleanup the
media_device struct when multiple drivers can use it. The best way to
warrant that we'll be doing that is via kref.

> This patchs remove the requirement of calling cleanup
> explitly, breaking that model. It's perhaps not a big problem, there is
> likely no single driver which would initialise the media controller device
> once but would register and unregister it multiple times. I still wonder if
> we really lost something important if we removed media_device_init() and
> media_device_cleanup() altogether and merged the functionality in
> media_device_{,un}register().
> 
> Cc Javier who wrote the patch.

That patch was written to avoid a race trouble of having the media
devnode to initialize too early. Merging them back would cause troubles.

Maybe what we could do, instead, is to rename media_device_init()
to media_device_register(), and call the function that creates the
devnode as media_device_create_devnode().

What do you think?

> 
> commit 9832e155f1ed3030fdfaa19e72c06472dc2ecb1d
> Author: Javier Martinez Canillas <javier@osg.samsung.com>
> Date:   Fri Dec 11 20:57:08 2015 -0200
> 
>     [media] media-device: split media initialization and registration
> 
> For system-wide media device, I think we'd need to introduce a new concept,
> graph, that would define an interconnected set of entities. This would
> mainly be needed for callbacks, e.g. the power on / off sequences of the
> entities could be specific to V4L as discussed earlier. With this approach
> hacks wouldn't be needed to be made in the first place to support two drivers
> sharing a media device.
> 
> What was the original reason you needed to share the media device btw.?
> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > ---
> >  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
> >  include/media/media-device.h |  3 +++
> >  2 files changed, 37 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index c32fa15cc76e..38e6c319fe6e 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  {
> >  	int ret;
> >  
> > +	/* Check if mdev was ever registered at all */
> > +	mutex_lock(&mdev->graph_mutex);
> > +	if (media_devnode_is_registered(&mdev->devnode)) {
> > +		kref_get(&mdev->kref);
> > +		mutex_unlock(&mdev->graph_mutex);
> > +		return 0;
> > +	}
> > +	kref_init(&mdev->kref);
> > +
> >  	/* Register the device node. */
> >  	mdev->devnode.fops = &media_device_fops;
> >  	mdev->devnode.parent = mdev->dev;
> > @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  	mdev->topology_version = 0;
> >  
> >  	ret = media_devnode_register(&mdev->devnode, owner);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		media_devnode_unregister(&mdev->devnode);
> >  		return ret;
> > +	}
> >  
> >  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
> >  	if (ret < 0) {
> > @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
> >  		return ret;
> >  	}
> >  
> > +	mutex_unlock(&mdev->graph_mutex);
> >  	dev_dbg(mdev->dev, "Media device registered\n");
> >  
> >  	return 0;
> > @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
> >  
> > -void media_device_unregister(struct media_device *mdev)
> > +static void do_media_device_unregister(struct kref *kref)
> >  {
> > +	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;
> > -	}
> > +	mdev = container_of(kref, struct media_device, kref);
> >  
> >  	/* Remove all entities from the media device */
> >  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> > @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
> >  		kfree(intf);
> >  	}
> >  
> > -	mutex_unlock(&mdev->graph_mutex);
> > +	/* Check if mdev devnode was registered */
> > +	if (!media_devnode_is_registered(&mdev->devnode))
> > +		return;
> >  
> >  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> >  	media_devnode_unregister(&mdev->devnode);
> >  
> >  	dev_dbg(mdev->dev, "Media device unregistered\n");
> >  }
> > +
> > +void media_device_unregister(struct media_device *mdev)
> > +{
> > +	if (mdev == NULL)
> > +		return;
> > +
> > +	mutex_lock(&mdev->graph_mutex);
> > +	kref_put(&mdev->kref, do_media_device_unregister);
> > +	mutex_unlock(&mdev->graph_mutex);
> > +
> > +}
> >  EXPORT_SYMBOL_GPL(media_device_unregister);
> >  
> >  static void media_device_release_devres(struct device *dev, void *res)
> > @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
> >  	struct media_device *mdev;
> >  
> >  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> > -	if (mdev)
> > +	if (mdev) {
> > +		kref_get(&mdev->kref);
> >  		return mdev;
> > +	}
> >  
> >  	mdev = devres_alloc(media_device_release_devres,
> >  				sizeof(struct media_device), GFP_KERNEL);
> >  	if (!mdev)
> >  		return NULL;
> > +
> >  	return devres_get(dev, mdev, NULL, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_get_devres);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index ca3871b853ba..73c16e6e6b6b 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -23,6 +23,7 @@
> >  #ifndef _MEDIA_DEVICE_H
> >  #define _MEDIA_DEVICE_H
> >  
> > +#include <linux/kref.h>
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> >  
> > @@ -283,6 +284,7 @@ struct media_entity_notify {
> >   * struct media_device - Media device
> >   * @dev:	Parent device
> >   * @devnode:	Media device node
> > + * @kref:	Object refcount
> >   * @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
> > @@ -347,6 +349,7 @@ struct media_device {
> >  	/* dev->driver_data points to this struct. */
> >  	struct device *dev;
> >  	struct media_devnode devnode;
> > +	struct kref kref;
> >  
> >  	char model[32];
> >  	char driver_name[32];
> > -- 
> > 2.5.0
> >   
> 


-- 
Thanks,
Mauro

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Sakari Ailus]

Hi Mauro,

On Wed, Mar 16, 2016 at 09:04:05AM -0300, Mauro Carvalho Chehab wrote:
> Now that the media_device can be used by multiple drivers,
> via devres, we need to be sure that it will be dropped only
> when all drivers stop using it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

For patches 2 (assuming fixes according to Javier's comment), 4 and 5, and
"[media] media: rename media unregister function":

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Shuah Khan]

On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> Now that the media_device can be used by multiple drivers,
> via devres, we need to be sure that it will be dropped only
> when all drivers stop using it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.

Ran device removal test and rmmod and modprobe tests on both drivers.

Generated graph after the runs and the graph looks good.

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
Tested-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

> ---
>  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
>  include/media/media-device.h |  3 +++
>  2 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c32fa15cc76e..38e6c319fe6e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>  {
>  	int ret;
>  
> +	/* Check if mdev was ever registered at all */
> +	mutex_lock(&mdev->graph_mutex);
> +	if (media_devnode_is_registered(&mdev->devnode)) {
> +		kref_get(&mdev->kref);
> +		mutex_unlock(&mdev->graph_mutex);
> +		return 0;
> +	}
> +	kref_init(&mdev->kref);
> +
>  	/* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
>  	mdev->topology_version = 0;
>  
>  	ret = media_devnode_register(&mdev->devnode, owner);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		media_devnode_unregister(&mdev->devnode);
>  		return ret;
> +	}
>  
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>  		return ret;
>  	}
>  
> +	mutex_unlock(&mdev->graph_mutex);
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
>  	return 0;
> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>  
> -void media_device_unregister(struct media_device *mdev)
> +static void do_media_device_unregister(struct kref *kref)
>  {
> +	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;
> -	}
> +	mdev = container_of(kref, struct media_device, kref);
>  
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
>  		kfree(intf);
>  	}
>  
> -	mutex_unlock(&mdev->graph_mutex);
> +	/* Check if mdev devnode was registered */
> +	if (!media_devnode_is_registered(&mdev->devnode))
> +		return;
>  
>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>  	media_devnode_unregister(&mdev->devnode);
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> +
> +void media_device_unregister(struct media_device *mdev)
> +{
> +	if (mdev == NULL)
> +		return;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	kref_put(&mdev->kref, do_media_device_unregister);
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +}
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
>  static void media_device_release_devres(struct device *dev, void *res)
> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>  	struct media_device *mdev;
>  
>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
> -	if (mdev)
> +	if (mdev) {
> +		kref_get(&mdev->kref);
>  		return mdev;
> +	}
>  
>  	mdev = devres_alloc(media_device_release_devres,
>  				sizeof(struct media_device), GFP_KERNEL);
>  	if (!mdev)
>  		return NULL;
> +
>  	return devres_get(dev, mdev, NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(media_device_get_devres);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index ca3871b853ba..73c16e6e6b6b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -23,6 +23,7 @@
>  #ifndef _MEDIA_DEVICE_H
>  #define _MEDIA_DEVICE_H
>  
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  
> @@ -283,6 +284,7 @@ struct media_entity_notify {
>   * struct media_device - Media device
>   * @dev:	Parent device
>   * @devnode:	Media device node
> + * @kref:	Object refcount
>   * @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
> @@ -347,6 +349,7 @@ struct media_device {
>  	/* dev->driver_data points to this struct. */
>  	struct device *dev;
>  	struct media_devnode devnode;
> +	struct kref kref;
>  
>  	char model[32];
>  	char driver_name[32];
> 


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH 4/5] [media] media-device: use kref for media_device instance

[Shuah Khan]

On 03/22/2016 01:56 PM, Shuah Khan wrote:
> On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
>> Now that the media_device can be used by multiple drivers,
>> via devres, we need to be sure that it will be dropped only
>> when all drivers stop using it.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
> snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
> KASAN enabled kernel (lock testing enabled). No use-after-free errors
> during these runs.
> 
> Ran device removal test and rmmod and modprobe tests on both drivers.
> 
> Generated graph after the runs and the graph looks good.
> 
> Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
> Tested-by: Shuah Khan <shuahkh@osg.samsung.com>
> 

Acked the wrong patch. Sorry. Will ack the v2 which is the one I tested.

thanks,
-- Shuah

>> ---
>>  drivers/media/media-device.c | 48 +++++++++++++++++++++++++++++++-------------
>>  include/media/media-device.h |  3 +++
>>  2 files changed, 37 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index c32fa15cc76e..38e6c319fe6e 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -721,6 +721,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  {
>>  	int ret;
>>  
>> +	/* Check if mdev was ever registered at all */
>> +	mutex_lock(&mdev->graph_mutex);
>> +	if (media_devnode_is_registered(&mdev->devnode)) {
>> +		kref_get(&mdev->kref);
>> +		mutex_unlock(&mdev->graph_mutex);
>> +		return 0;
>> +	}
>> +	kref_init(&mdev->kref);
>> +
>>  	/* Register the device node. */
>>  	mdev->devnode.fops = &media_device_fops;
>>  	mdev->devnode.parent = mdev->dev;
>> @@ -730,8 +739,10 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  	mdev->topology_version = 0;
>>  
>>  	ret = media_devnode_register(&mdev->devnode, owner);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>> +		media_devnode_unregister(&mdev->devnode);
>>  		return ret;
>> +	}
>>  
>>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>>  	if (ret < 0) {
>> @@ -739,6 +750,7 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  		return ret;
>>  	}
>>  
>> +	mutex_unlock(&mdev->graph_mutex);
>>  	dev_dbg(mdev->dev, "Media device registered\n");
>>  
>>  	return 0;
>> @@ -773,23 +785,15 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
>>  }
>>  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>>  
>> -void media_device_unregister(struct media_device *mdev)
>> +static void do_media_device_unregister(struct kref *kref)
>>  {
>> +	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;
>> -	}
>> +	mdev = container_of(kref, struct media_device, kref);
>>  
>>  	/* Remove all entities from the media device */
>>  	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
>> @@ -807,13 +811,26 @@ void media_device_unregister(struct media_device *mdev)
>>  		kfree(intf);
>>  	}
>>  
>> -	mutex_unlock(&mdev->graph_mutex);
>> +	/* Check if mdev devnode was registered */
>> +	if (!media_devnode_is_registered(&mdev->devnode))
>> +		return;
>>  
>>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>>  	media_devnode_unregister(&mdev->devnode);
>>  
>>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>>  }
>> +
>> +void media_device_unregister(struct media_device *mdev)
>> +{
>> +	if (mdev == NULL)
>> +		return;
>> +
>> +	mutex_lock(&mdev->graph_mutex);
>> +	kref_put(&mdev->kref, do_media_device_unregister);
>> +	mutex_unlock(&mdev->graph_mutex);
>> +
>> +}
>>  EXPORT_SYMBOL_GPL(media_device_unregister);
>>  
>>  static void media_device_release_devres(struct device *dev, void *res)
>> @@ -825,13 +842,16 @@ struct media_device *media_device_get_devres(struct device *dev)
>>  	struct media_device *mdev;
>>  
>>  	mdev = devres_find(dev, media_device_release_devres, NULL, NULL);
>> -	if (mdev)
>> +	if (mdev) {
>> +		kref_get(&mdev->kref);
>>  		return mdev;
>> +	}
>>  
>>  	mdev = devres_alloc(media_device_release_devres,
>>  				sizeof(struct media_device), GFP_KERNEL);
>>  	if (!mdev)
>>  		return NULL;
>> +
>>  	return devres_get(dev, mdev, NULL, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(media_device_get_devres);
>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>> index ca3871b853ba..73c16e6e6b6b 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -23,6 +23,7 @@
>>  #ifndef _MEDIA_DEVICE_H
>>  #define _MEDIA_DEVICE_H
>>  
>> +#include <linux/kref.h>
>>  #include <linux/list.h>
>>  #include <linux/mutex.h>
>>  
>> @@ -283,6 +284,7 @@ struct media_entity_notify {
>>   * struct media_device - Media device
>>   * @dev:	Parent device
>>   * @devnode:	Media device node
>> + * @kref:	Object refcount
>>   * @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
>> @@ -347,6 +349,7 @@ struct media_device {
>>  	/* dev->driver_data points to this struct. */
>>  	struct device *dev;
>>  	struct media_devnode devnode;
>> +	struct kref kref;
>>  
>>  	char model[32];
>>  	char driver_name[32];
>>
> 
> 


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

[PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Mauro Carvalho Chehab]

When multiple drivers are sharing the media_device struct,
one driver cannot know the right moment to cleanup the
media_device struct, because it can happen only when the
struct is not used by the other drivers.

So, let's call media_device_cleanup() internally, and
ensure that media_device_unregister() will do the right thing,
if the media device is not fully initialized.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/common/siano/smsdvb-main.c      |  1 -
 drivers/media/media-device.c                  | 13 ++++++-------
 drivers/media/pci/saa7134/saa7134-core.c      |  1 -
 drivers/media/platform/exynos4-is/media-dev.c |  3 +--
 drivers/media/platform/omap3isp/isp.c         |  1 -
 drivers/media/platform/s3c-camif/camif-core.c |  2 --
 drivers/media/platform/vsp1/vsp1_drv.c        |  1 -
 drivers/media/platform/xilinx/xilinx-vipp.c   |  3 +--
 drivers/media/usb/au0828/au0828-core.c        |  1 -
 drivers/media/usb/cx231xx/cx231xx-cards.c     |  1 -
 drivers/media/usb/dvb-usb-v2/dvb_usb_core.c   |  1 -
 drivers/media/usb/dvb-usb/dvb-usb-dvb.c       |  1 -
 drivers/media/usb/em28xx/em28xx-cards.c       |  1 -
 drivers/media/usb/siano/smsusb.c              |  2 +-
 drivers/media/usb/uvc/uvc_driver.c            |  4 +---
 include/media/media-device.h                  | 10 ----------
 16 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/drivers/media/common/siano/smsdvb-main.c b/drivers/media/common/siano/smsdvb-main.c
index 9148e14c9d07..711c389c05e3 100644
--- a/drivers/media/common/siano/smsdvb-main.c
+++ b/drivers/media/common/siano/smsdvb-main.c
@@ -617,7 +617,6 @@ static void smsdvb_media_device_unregister(struct smsdvb_client_t *client)
 	if (!coredev->media_dev)
 		return;
 	media_device_unregister(coredev->media_dev);
-	media_device_cleanup(coredev->media_dev);
 	kfree(coredev->media_dev);
 	coredev->media_dev = NULL;
 #endif
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 38e6c319fe6e..0c7371eeda84 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -707,14 +707,12 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
-void media_device_cleanup(struct media_device *mdev)
+static void media_device_cleanup(struct media_device *mdev)
 {
 	ida_destroy(&mdev->entity_internal_idx);
 	mdev->entity_internal_idx_max = 0;
 	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
-	mutex_destroy(&mdev->graph_mutex);
 }
-EXPORT_SYMBOL_GPL(media_device_cleanup);
 
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner)
@@ -812,11 +810,12 @@ static void do_media_device_unregister(struct kref *kref)
 	}
 
 	/* Check if mdev devnode was registered */
-	if (!media_devnode_is_registered(&mdev->devnode))
-		return;
+	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);
-	media_devnode_unregister(&mdev->devnode);
+	media_device_cleanup(mdev);
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 }
diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
index c0e1780ec831..213dc71f09eb 100644
--- a/drivers/media/pci/saa7134/saa7134-core.c
+++ b/drivers/media/pci/saa7134/saa7134-core.c
@@ -813,7 +813,6 @@ static void saa7134_unregister_media_device(struct saa7134_dev *dev)
 	if (!dev->media_dev)
 		return;
 	media_device_unregister(dev->media_dev);
-	media_device_cleanup(dev->media_dev);
 	kfree(dev->media_dev);
 	dev->media_dev = NULL;
 #endif
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index feb521f28e14..8c106fda7b84 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1501,7 +1501,7 @@ err_clk:
 err_m_ent:
 	fimc_md_unregister_entities(fmd);
 err_md:
-	media_device_cleanup(&fmd->media_dev);
+	media_device_unregister(&fmd->media_dev);
 	v4l2_device_unregister(&fmd->v4l2_dev);
 	return ret;
 }
@@ -1521,7 +1521,6 @@ static int fimc_md_remove(struct platform_device *pdev)
 	fimc_md_unregister_entities(fmd);
 	fimc_md_pipelines_free(fmd);
 	media_device_unregister(&fmd->media_dev);
-	media_device_cleanup(&fmd->media_dev);
 	fimc_md_put_clocks(fmd);
 
 	return 0;
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 5d54e2c6c16b..cd67edcad8d3 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1598,7 +1598,6 @@ static void isp_unregister_entities(struct isp_device *isp)
 
 	v4l2_device_unregister(&isp->v4l2_dev);
 	media_device_unregister(&isp->media_dev);
-	media_device_cleanup(&isp->media_dev);
 }
 
 static int isp_link_entity(
diff --git a/drivers/media/platform/s3c-camif/camif-core.c b/drivers/media/platform/s3c-camif/camif-core.c
index 0b44b9accf50..0159f98da435 100644
--- a/drivers/media/platform/s3c-camif/camif-core.c
+++ b/drivers/media/platform/s3c-camif/camif-core.c
@@ -521,7 +521,6 @@ err_unlock:
 err_sens:
 	v4l2_device_unregister(&camif->v4l2_dev);
 	media_device_unregister(&camif->media_dev);
-	media_device_cleanup(&camif->media_dev);
 	camif_unregister_media_entities(camif);
 err_mdev:
 	vb2_dma_contig_cleanup_ctx(camif->alloc_ctx);
@@ -543,7 +542,6 @@ static int s3c_camif_remove(struct platform_device *pdev)
 	struct s3c_camif_plat_data *pdata = &camif->pdata;
 
 	media_device_unregister(&camif->media_dev);
-	media_device_cleanup(&camif->media_dev);
 	camif_unregister_media_entities(camif);
 	v4l2_device_unregister(&camif->v4l2_dev);
 
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index 25750a0e4631..2faefec89b62 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -213,7 +213,6 @@ static void vsp1_destroy_entities(struct vsp1_device *vsp1)
 
 	v4l2_device_unregister(&vsp1->v4l2_dev);
 	media_device_unregister(&vsp1->media_dev);
-	media_device_cleanup(&vsp1->media_dev);
 
 	if (!vsp1->info->uapi)
 		vsp1_drm_cleanup(vsp1);
diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index e795a4501e8b..ec4b21c64ef6 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -573,7 +573,6 @@ static void xvip_composite_v4l2_cleanup(struct xvip_composite_device *xdev)
 {
 	v4l2_device_unregister(&xdev->v4l2_dev);
 	media_device_unregister(&xdev->media_dev);
-	media_device_cleanup(&xdev->media_dev);
 }
 
 static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev)
@@ -592,7 +591,7 @@ static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev)
 	if (ret < 0) {
 		dev_err(xdev->dev, "V4L2 device registration failed (%d)\n",
 			ret);
-		media_device_cleanup(&xdev->media_dev);
+		media_device_unregister(&xdev->media_dev);
 		return ret;
 	}
 
diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 06da73f1ff22..0a26ffff6f18 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -158,7 +158,6 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	dev->media_dev->disable_source = NULL;
 
 	media_device_unregister(dev->media_dev);
-	media_device_cleanup(dev->media_dev);
 	dev->media_dev = NULL;
 #endif
 }
diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c b/drivers/media/usb/cx231xx/cx231xx-cards.c
index c63248a18823..438fe0761bda 100644
--- a/drivers/media/usb/cx231xx/cx231xx-cards.c
+++ b/drivers/media/usb/cx231xx/cx231xx-cards.c
@@ -1172,7 +1172,6 @@ static void cx231xx_unregister_media_device(struct cx231xx *dev)
 #ifdef CONFIG_MEDIA_CONTROLLER
 	if (dev->media_dev) {
 		media_device_unregister(dev->media_dev);
-		media_device_cleanup(dev->media_dev);
 		kfree(dev->media_dev);
 		dev->media_dev = NULL;
 	}
diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
index 3fbb2cd19f5e..238ecb3a510b 100644
--- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
+++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
@@ -438,7 +438,6 @@ static void dvb_usbv2_media_device_unregister(struct dvb_usb_adapter *adap)
 		return;
 
 	media_device_unregister(adap->dvb_adap.mdev);
-	media_device_cleanup(adap->dvb_adap.mdev);
 	kfree(adap->dvb_adap.mdev);
 	adap->dvb_adap.mdev = NULL;
 
diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
index 6477b04e95c7..093ca809b5f8 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
@@ -132,7 +132,6 @@ static void dvb_usb_media_device_unregister(struct dvb_usb_adapter *adap)
 		return;
 
 	media_device_unregister(adap->dvb_adap.mdev);
-	media_device_cleanup(adap->dvb_adap.mdev);
 	kfree(adap->dvb_adap.mdev);
 	adap->dvb_adap.mdev = NULL;
 #endif
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 930e3e3fc948..734ecfb890ff 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3091,7 +3091,6 @@ static void em28xx_unregister_media_device(struct em28xx *dev)
 #ifdef CONFIG_MEDIA_CONTROLLER
 	if (dev->media_dev) {
 		media_device_unregister(dev->media_dev);
-		media_device_cleanup(dev->media_dev);
 		kfree(dev->media_dev);
 		dev->media_dev = NULL;
 	}
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index c2e25876e93b..cbf9a34f2074 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -375,7 +375,7 @@ static void *siano_media_device_register(struct smsusb_device_t *dev,
 
 	ret = media_device_register(mdev);
 	if (ret) {
-		media_device_cleanup(mdev);
+		media_device_unregister(mdev);
 		kfree(mdev);
 		return NULL;
 	}
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 451e84e962e2..dcce75e1aff2 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1674,9 +1674,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))
-		media_device_unregister(&dev->mdev);
-	media_device_cleanup(&dev->mdev);
+	media_device_unregister(&dev->mdev);
 #endif
 
 	list_for_each_safe(p, n, &dev->chains) {
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 73c16e6e6b6b..9166bff8068e 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -430,16 +430,6 @@ static inline __must_check int media_entity_enum_init(
 void media_device_init(struct media_device *mdev);
 
 /**
- * media_device_cleanup() - Cleanups a media device element
- *
- * @mdev:	pointer to struct &media_device
- *
- * This function that will destroy the graph_mutex that is
- * initialized in media_device_init().
- */
-void media_device_cleanup(struct media_device *mdev);
-
-/**
  * __media_device_register() - Registers a media device element
  *
  * @mdev:	pointer to struct &media_device
-- 
2.5.0

Re: [PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Javier Martinez Canillas]

Hello Mauro,

The patch looks mostly good to me, I just have a question below:

On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:

[snip]

>
> -void media_device_cleanup(struct media_device *mdev)
> +static void media_device_cleanup(struct media_device *mdev)
>  {
>         ida_destroy(&mdev->entity_internal_idx);
>         mdev->entity_internal_idx_max = 0;
>         media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> -       mutex_destroy(&mdev->graph_mutex);

Any reason why this is removed? mutex_init() is still called in
media_device_init() so I believe the destroy should be kept here.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

Re: [PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Mauro Carvalho Chehab]

Em Wed, 16 Mar 2016 11:03:38 -0300
Javier Martinez Canillas <javier@dowhile0.org> escreveu:

> Hello Mauro,
> 
> The patch looks mostly good to me, I just have a question below:
> 
> On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
> <mchehab@osg.samsung.com> wrote:
> 
> [snip]
> 
> >
> > -void media_device_cleanup(struct media_device *mdev)
> > +static void media_device_cleanup(struct media_device *mdev)
> >  {
> >         ida_destroy(&mdev->entity_internal_idx);
> >         mdev->entity_internal_idx_max = 0;
> >         media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> > -       mutex_destroy(&mdev->graph_mutex);  
> 
> Any reason why this is removed? mutex_init() is still called in
> media_device_init() so I believe the destroy should be kept here.

This code is now called only at do_media_device_unregister, with
is protected by the mutex. If we keep the mutex_destroy there,
the mutex will be destroyed in lock state, causing an error if
mutex debug is enabled.

Btw, the standard implementation for the mutex is:
	include/linux/mutex.h:static inline void mutex_destroy(struct mutex *lock) {}

Only when mutex debug is enabled, it becomes something else:
	include/linux/mutex-debug.h:extern void mutex_destroy(struct mutex *lock);

With produces a warning if the mutex_destroy() is called with the
mutex is hold. This can never happen with the current implementation, as
the logic is:
	mutex_lock(&mdev->graph_mutex);
	kref_put(&mdev->kref, do_media_device_unregister);
	mutex_unlock(&mdev->graph_mutex);

We could eventually move the mutex lock to
do_media_device_unregister() and then add there the mutex_destroy()

Regards,
Mauro

> 
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> Best regards,
> Javier


-- 
Thanks,
Mauro

Re: [PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Javier Martinez Canillas]

Hello Mauro,

On Wed, Mar 16, 2016 at 11:36 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> Em Wed, 16 Mar 2016 11:03:38 -0300
> Javier Martinez Canillas <javier@dowhile0.org> escreveu:
>
>> Hello Mauro,
>>
>> The patch looks mostly good to me, I just have a question below:
>>
>> On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
>> <mchehab@osg.samsung.com> wrote:
>>
>> [snip]
>>
>> >
>> > -void media_device_cleanup(struct media_device *mdev)
>> > +static void media_device_cleanup(struct media_device *mdev)
>> >  {
>> >         ida_destroy(&mdev->entity_internal_idx);
>> >         mdev->entity_internal_idx_max = 0;
>> >         media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
>> > -       mutex_destroy(&mdev->graph_mutex);
>>
>> Any reason why this is removed? mutex_init() is still called in
>> media_device_init() so I believe the destroy should be kept here.
>
> This code is now called only at do_media_device_unregister, with
> is protected by the mutex. If we keep the mutex_destroy there,
> the mutex will be destroyed in lock state, causing an error if
> mutex debug is enabled.
>
> Btw, the standard implementation for the mutex is:
>         include/linux/mutex.h:static inline void mutex_destroy(struct mutex *lock) {}
>
> Only when mutex debug is enabled, it becomes something else:
>         include/linux/mutex-debug.h:extern void mutex_destroy(struct mutex *lock);
>
> With produces a warning if the mutex_destroy() is called with the
> mutex is hold. This can never happen with the current implementation, as
> the logic is:
>         mutex_lock(&mdev->graph_mutex);
>         kref_put(&mdev->kref, do_media_device_unregister);
>         mutex_unlock(&mdev->graph_mutex);
>
> We could eventually move the mutex lock to
> do_media_device_unregister() and then add there the mutex_destroy()
>

I see, thanks a lot for your explanation.

> Regards,
> Mauro
>

Best regards,
Javier

Re: [PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Shuah Khan]

On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> When multiple drivers are sharing the media_device struct,
> one driver cannot know the right moment to cleanup the
> media_device struct, because it can happen only when the
> struct is not used by the other drivers.
> 
> So, let's call media_device_cleanup() internally, and
> ensure that media_device_unregister() will do the right thing,
> if the media device is not fully initialized.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.

Ran device removal test and rmmod and modprobe tests on both drivers.

Generated graph after the runs and the graph looks good.

For the au0828 and snd_usb_audio change:

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
Tested-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

> ---
>  drivers/media/common/siano/smsdvb-main.c      |  1 -
>  drivers/media/media-device.c                  | 13 ++++++-------
>  drivers/media/pci/saa7134/saa7134-core.c      |  1 -
>  drivers/media/platform/exynos4-is/media-dev.c |  3 +--
>  drivers/media/platform/omap3isp/isp.c         |  1 -
>  drivers/media/platform/s3c-camif/camif-core.c |  2 --
>  drivers/media/platform/vsp1/vsp1_drv.c        |  1 -
>  drivers/media/platform/xilinx/xilinx-vipp.c   |  3 +--
>  drivers/media/usb/au0828/au0828-core.c        |  1 -
>  drivers/media/usb/cx231xx/cx231xx-cards.c     |  1 -
>  drivers/media/usb/dvb-usb-v2/dvb_usb_core.c   |  1 -
>  drivers/media/usb/dvb-usb/dvb-usb-dvb.c       |  1 -
>  drivers/media/usb/em28xx/em28xx-cards.c       |  1 -
>  drivers/media/usb/siano/smsusb.c              |  2 +-
>  drivers/media/usb/uvc/uvc_driver.c            |  4 +---
>  include/media/media-device.h                  | 10 ----------
>  16 files changed, 10 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/common/siano/smsdvb-main.c b/drivers/media/common/siano/smsdvb-main.c
> index 9148e14c9d07..711c389c05e3 100644
> --- a/drivers/media/common/siano/smsdvb-main.c
> +++ b/drivers/media/common/siano/smsdvb-main.c
> @@ -617,7 +617,6 @@ static void smsdvb_media_device_unregister(struct smsdvb_client_t *client)
>  	if (!coredev->media_dev)
>  		return;
>  	media_device_unregister(coredev->media_dev);
> -	media_device_cleanup(coredev->media_dev);
>  	kfree(coredev->media_dev);
>  	coredev->media_dev = NULL;
>  #endif
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 38e6c319fe6e..0c7371eeda84 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -707,14 +707,12 @@ void media_device_init(struct media_device *mdev)
>  }
>  EXPORT_SYMBOL_GPL(media_device_init);
>  
> -void media_device_cleanup(struct media_device *mdev)
> +static void media_device_cleanup(struct media_device *mdev)
>  {
>  	ida_destroy(&mdev->entity_internal_idx);
>  	mdev->entity_internal_idx_max = 0;
>  	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> -	mutex_destroy(&mdev->graph_mutex);
>  }
> -EXPORT_SYMBOL_GPL(media_device_cleanup);
>  
>  int __must_check __media_device_register(struct media_device *mdev,
>  					 struct module *owner)
> @@ -812,11 +810,12 @@ static void do_media_device_unregister(struct kref *kref)
>  	}
>  
>  	/* Check if mdev devnode was registered */
> -	if (!media_devnode_is_registered(&mdev->devnode))
> -		return;
> +	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);
> -	media_devnode_unregister(&mdev->devnode);
> +	media_device_cleanup(mdev);
>  
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
> index c0e1780ec831..213dc71f09eb 100644
> --- a/drivers/media/pci/saa7134/saa7134-core.c
> +++ b/drivers/media/pci/saa7134/saa7134-core.c
> @@ -813,7 +813,6 @@ static void saa7134_unregister_media_device(struct saa7134_dev *dev)
>  	if (!dev->media_dev)
>  		return;
>  	media_device_unregister(dev->media_dev);
> -	media_device_cleanup(dev->media_dev);
>  	kfree(dev->media_dev);
>  	dev->media_dev = NULL;
>  #endif
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index feb521f28e14..8c106fda7b84 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -1501,7 +1501,7 @@ err_clk:
>  err_m_ent:
>  	fimc_md_unregister_entities(fmd);
>  err_md:
> -	media_device_cleanup(&fmd->media_dev);
> +	media_device_unregister(&fmd->media_dev);
>  	v4l2_device_unregister(&fmd->v4l2_dev);
>  	return ret;
>  }
> @@ -1521,7 +1521,6 @@ static int fimc_md_remove(struct platform_device *pdev)
>  	fimc_md_unregister_entities(fmd);
>  	fimc_md_pipelines_free(fmd);
>  	media_device_unregister(&fmd->media_dev);
> -	media_device_cleanup(&fmd->media_dev);
>  	fimc_md_put_clocks(fmd);
>  
>  	return 0;
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 5d54e2c6c16b..cd67edcad8d3 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1598,7 +1598,6 @@ static void isp_unregister_entities(struct isp_device *isp)
>  
>  	v4l2_device_unregister(&isp->v4l2_dev);
>  	media_device_unregister(&isp->media_dev);
> -	media_device_cleanup(&isp->media_dev);
>  }
>  
>  static int isp_link_entity(
> diff --git a/drivers/media/platform/s3c-camif/camif-core.c b/drivers/media/platform/s3c-camif/camif-core.c
> index 0b44b9accf50..0159f98da435 100644
> --- a/drivers/media/platform/s3c-camif/camif-core.c
> +++ b/drivers/media/platform/s3c-camif/camif-core.c
> @@ -521,7 +521,6 @@ err_unlock:
>  err_sens:
>  	v4l2_device_unregister(&camif->v4l2_dev);
>  	media_device_unregister(&camif->media_dev);
> -	media_device_cleanup(&camif->media_dev);
>  	camif_unregister_media_entities(camif);
>  err_mdev:
>  	vb2_dma_contig_cleanup_ctx(camif->alloc_ctx);
> @@ -543,7 +542,6 @@ static int s3c_camif_remove(struct platform_device *pdev)
>  	struct s3c_camif_plat_data *pdata = &camif->pdata;
>  
>  	media_device_unregister(&camif->media_dev);
> -	media_device_cleanup(&camif->media_dev);
>  	camif_unregister_media_entities(camif);
>  	v4l2_device_unregister(&camif->v4l2_dev);
>  
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> index 25750a0e4631..2faefec89b62 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -213,7 +213,6 @@ static void vsp1_destroy_entities(struct vsp1_device *vsp1)
>  
>  	v4l2_device_unregister(&vsp1->v4l2_dev);
>  	media_device_unregister(&vsp1->media_dev);
> -	media_device_cleanup(&vsp1->media_dev);
>  
>  	if (!vsp1->info->uapi)
>  		vsp1_drm_cleanup(vsp1);
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index e795a4501e8b..ec4b21c64ef6 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -573,7 +573,6 @@ static void xvip_composite_v4l2_cleanup(struct xvip_composite_device *xdev)
>  {
>  	v4l2_device_unregister(&xdev->v4l2_dev);
>  	media_device_unregister(&xdev->media_dev);
> -	media_device_cleanup(&xdev->media_dev);
>  }
>  
>  static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev)
> @@ -592,7 +591,7 @@ static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev)
>  	if (ret < 0) {
>  		dev_err(xdev->dev, "V4L2 device registration failed (%d)\n",
>  			ret);
> -		media_device_cleanup(&xdev->media_dev);
> +		media_device_unregister(&xdev->media_dev);
>  		return ret;
>  	}
>  
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 06da73f1ff22..0a26ffff6f18 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -158,7 +158,6 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
>  	dev->media_dev->disable_source = NULL;
>  
>  	media_device_unregister(dev->media_dev);
> -	media_device_cleanup(dev->media_dev);
>  	dev->media_dev = NULL;
>  #endif
>  }
> diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c b/drivers/media/usb/cx231xx/cx231xx-cards.c
> index c63248a18823..438fe0761bda 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-cards.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-cards.c
> @@ -1172,7 +1172,6 @@ static void cx231xx_unregister_media_device(struct cx231xx *dev)
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	if (dev->media_dev) {
>  		media_device_unregister(dev->media_dev);
> -		media_device_cleanup(dev->media_dev);
>  		kfree(dev->media_dev);
>  		dev->media_dev = NULL;
>  	}
> diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> index 3fbb2cd19f5e..238ecb3a510b 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> @@ -438,7 +438,6 @@ static void dvb_usbv2_media_device_unregister(struct dvb_usb_adapter *adap)
>  		return;
>  
>  	media_device_unregister(adap->dvb_adap.mdev);
> -	media_device_cleanup(adap->dvb_adap.mdev);
>  	kfree(adap->dvb_adap.mdev);
>  	adap->dvb_adap.mdev = NULL;
>  
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> index 6477b04e95c7..093ca809b5f8 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> @@ -132,7 +132,6 @@ static void dvb_usb_media_device_unregister(struct dvb_usb_adapter *adap)
>  		return;
>  
>  	media_device_unregister(adap->dvb_adap.mdev);
> -	media_device_cleanup(adap->dvb_adap.mdev);
>  	kfree(adap->dvb_adap.mdev);
>  	adap->dvb_adap.mdev = NULL;
>  #endif
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 930e3e3fc948..734ecfb890ff 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3091,7 +3091,6 @@ static void em28xx_unregister_media_device(struct em28xx *dev)
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	if (dev->media_dev) {
>  		media_device_unregister(dev->media_dev);
> -		media_device_cleanup(dev->media_dev);
>  		kfree(dev->media_dev);
>  		dev->media_dev = NULL;
>  	}
> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index c2e25876e93b..cbf9a34f2074 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c
> @@ -375,7 +375,7 @@ static void *siano_media_device_register(struct smsusb_device_t *dev,
>  
>  	ret = media_device_register(mdev);
>  	if (ret) {
> -		media_device_cleanup(mdev);
> +		media_device_unregister(mdev);
>  		kfree(mdev);
>  		return NULL;
>  	}
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 451e84e962e2..dcce75e1aff2 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1674,9 +1674,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))
> -		media_device_unregister(&dev->mdev);
> -	media_device_cleanup(&dev->mdev);
> +	media_device_unregister(&dev->mdev);
>  #endif
>  
>  	list_for_each_safe(p, n, &dev->chains) {
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 73c16e6e6b6b..9166bff8068e 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -430,16 +430,6 @@ static inline __must_check int media_entity_enum_init(
>  void media_device_init(struct media_device *mdev);
>  
>  /**
> - * media_device_cleanup() - Cleanups a media device element
> - *
> - * @mdev:	pointer to struct &media_device
> - *
> - * This function that will destroy the graph_mutex that is
> - * initialized in media_device_init().
> - */
> -void media_device_cleanup(struct media_device *mdev);
> -
> -/**
>   * __media_device_register() - Registers a media device element
>   *
>   * @mdev:	pointer to struct &media_device
> 


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH 1/5] [media] media-device: get rid of the spinlock

[Javier Martinez Canillas]

Hello Mauro,

On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> Right now, the lock schema for media_device struct is messy,
> since sometimes, it is protected via a spin lock, while, for
> media graph traversal, it is protected by a mutex.
>
> Solve this conflict by always using a mutex.
>
> As a side effect, this prevents a bug where the media notifiers
> were called at atomic context.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---

I agree with the patch.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

Re: [PATCH 1/5] [media] media-device: get rid of the spinlock

[Mauro Carvalho Chehab]

Em Wed, 16 Mar 2016 09:53:12 -0300
Javier Martinez Canillas <javier@dowhile0.org> escreveu:

> Hello Mauro,
> 
> On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
> <mchehab@osg.samsung.com> wrote:
> > Right now, the lock schema for media_device struct is messy,
> > since sometimes, it is protected via a spin lock, while, for
> > media graph traversal, it is protected by a mutex.
> >
> > Solve this conflict by always using a mutex.
> >
> > As a side effect, this prevents a bug where the media notifiers
> > were called at atomic context.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Btw, I'm running a stress test here, doing bind/unbind on au0828,
while calling mc_nextgen_test:

Running one instance of this loop:
	$ i=0; while :; do i=$((i+1)); echo "loop $i"; sudo su -c "echo 1-3.1.2:1.0 > /sys/bus/usb/drivers/au0828/bind"; sudo su -c "echo 1-3.1.2:1.0 > /sys/bus/usb/drivers/au0828/unbind"; done


and 3 instances of this loop:
	$ while :; do clear; mc_nextgen_test; done

My test machine has 4 CPUs, so this should be enough to check
if the mutexes at ioctl and at the register/unregister functions
are ok.

Right now, the loop ran 160 times. Not a single trouble.

Ok, it is not doing any graph traversal ops, but the code seems to be
pretty much reliable with mutexes.

I'll keep it running for more time to be sure, but it seems that
the current media core works fine for dynamic
entity/interface/link addition/removal.

Regards,
Mauro


> > ---  
> 
> I agree with the patch.
> 
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> Best regards,
> Javier
> --
> 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


-- 
Thanks,
Mauro

Re: [PATCH 1/5] [media] media-device: get rid of the spinlock

[Sakari Ailus]

Hi Mauro,

On Wed, Mar 16, 2016 at 09:04:02AM -0300, Mauro Carvalho Chehab wrote:
> Right now, the lock schema for media_device struct is messy,
> since sometimes, it is protected via a spin lock, while, for
> media graph traversal, it is protected by a mutex.
> 
> Solve this conflict by always using a mutex.
> 
> As a side effect, this prevents a bug where the media notifiers
> were called at atomic context.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

The scope of mdev->lock isn't much, really. I think this is fine, no need to
create another mutex.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/5] [media] media-device: get rid of the spinlock

[Shuah Khan]

On 03/16/2016 06:04 AM, Mauro Carvalho Chehab wrote:
> Right now, the lock schema for media_device struct is messy,
> since sometimes, it is protected via a spin lock, while, for
> media graph traversal, it is protected by a mutex.
> 
> Solve this conflict by always using a mutex.
> 
> As a side effect, this prevents a bug where the media notifiers
> were called at atomic context.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Tested bind_unbind au0828 loop 1000 times, followed by bind_unbind
snd_usb_audio loop 1000 times. Didn't see any lock warnings on a
KASAN enabled kernel (lock testing enabled). No use-after-free errors
during these runs.

Ran device removal test and rmmod and modprobe tests on both drivers.

Generated graph after the runs and the graph looks good.

Reviewed-by: Shuah Khan <shuahkh@osg.samsung.com>
Tested-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

> ---
>  drivers/media/media-device.c | 32 ++++++++++++++------------------
>  drivers/media/media-entity.c | 16 ++++++++--------
>  include/media/media-device.h |  6 +-----
>  3 files changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 6e43c95629ea..c32fa15cc76e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -90,17 +90,17 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
>  
>  	id &= ~MEDIA_ENT_ID_FLAG_NEXT;
>  
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  
>  	media_device_for_each_entity(entity, mdev) {
>  		if (((media_entity_id(entity) == id) && !next) ||
>  		    ((media_entity_id(entity) > id) && next)) {
> -			spin_unlock(&mdev->lock);
> +			mutex_unlock(&mdev->graph_mutex);
>  			return entity;
>  		}
>  	}
>  
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  
>  	return NULL;
>  }
> @@ -590,12 +590,12 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  	if (!ida_pre_get(&mdev->entity_internal_idx, GFP_KERNEL))
>  		return -ENOMEM;
>  
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  
>  	ret = ida_get_new_above(&mdev->entity_internal_idx, 1,
>  				&entity->internal_idx);
>  	if (ret < 0) {
> -		spin_unlock(&mdev->lock);
> +		mutex_unlock(&mdev->graph_mutex);
>  		return ret;
>  	}
>  
> @@ -615,9 +615,6 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  		(notify)->notify(entity, notify->notify_data);
>  	}
>  
> -	spin_unlock(&mdev->lock);
> -
> -	mutex_lock(&mdev->graph_mutex);
>  	if (mdev->entity_internal_idx_max
>  	    >= mdev->pm_count_walk.ent_enum.idx_max) {
>  		struct media_entity_graph new = { .top = 0 };
> @@ -680,9 +677,9 @@ void media_device_unregister_entity(struct media_entity *entity)
>  	if (mdev == NULL)
>  		return;
>  
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_device_unregister_entity(entity);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity);
>  
> @@ -703,7 +700,6 @@ void media_device_init(struct media_device *mdev)
>  	INIT_LIST_HEAD(&mdev->pads);
>  	INIT_LIST_HEAD(&mdev->links);
>  	INIT_LIST_HEAD(&mdev->entity_notify);
> -	spin_lock_init(&mdev->lock);
>  	mutex_init(&mdev->graph_mutex);
>  	ida_init(&mdev->entity_internal_idx);
>  
> @@ -752,9 +748,9 @@ EXPORT_SYMBOL_GPL(__media_device_register);
>  int __must_check media_device_register_entity_notify(struct media_device *mdev,
>  					struct media_entity_notify *nptr)
>  {
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	list_add_tail(&nptr->list, &mdev->entity_notify);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
> @@ -771,9 +767,9 @@ static void __media_device_unregister_entity_notify(struct media_device *mdev,
>  void media_device_unregister_entity_notify(struct media_device *mdev,
>  					struct media_entity_notify *nptr)
>  {
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_device_unregister_entity_notify(mdev, nptr);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>  
> @@ -787,11 +783,11 @@ void media_device_unregister(struct media_device *mdev)
>  	if (mdev == NULL)
>  		return;
>  
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  
>  	/* Check if mdev was ever registered at all */
>  	if (!media_devnode_is_registered(&mdev->devnode)) {
> -		spin_unlock(&mdev->lock);
> +		mutex_unlock(&mdev->graph_mutex);
>  		return;
>  	}
>  
> @@ -811,7 +807,7 @@ void media_device_unregister(struct media_device *mdev)
>  		kfree(intf);
>  	}
>  
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  
>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>  	media_devnode_unregister(&mdev->devnode);
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e95070b3a3d4..c53c1d5589a0 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -219,7 +219,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
>  	entity->pads = pads;
>  
>  	if (mdev)
> -		spin_lock(&mdev->lock);
> +		mutex_lock(&mdev->graph_mutex);
>  
>  	for (i = 0; i < num_pads; i++) {
>  		pads[i].entity = entity;
> @@ -230,7 +230,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
>  	}
>  
>  	if (mdev)
> -		spin_unlock(&mdev->lock);
> +		mutex_unlock(&mdev->graph_mutex);
>  
>  	return 0;
>  }
> @@ -747,9 +747,9 @@ void media_entity_remove_links(struct media_entity *entity)
>  	if (mdev == NULL)
>  		return;
>  
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_entity_remove_links(entity);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_entity_remove_links);
>  
> @@ -951,9 +951,9 @@ void media_remove_intf_link(struct media_link *link)
>  	if (mdev == NULL)
>  		return;
>  
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_remove_intf_link(link);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_remove_intf_link);
>  
> @@ -975,8 +975,8 @@ void media_remove_intf_links(struct media_interface *intf)
>  	if (mdev == NULL)
>  		return;
>  
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_remove_intf_links(intf);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_remove_intf_links);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index df74cfa7da4a..0c2de97181f3 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -25,7 +25,6 @@
>  
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> -#include <linux/spinlock.h>
>  
>  #include <media/media-devnode.h>
>  #include <media/media-entity.h>
> @@ -304,8 +303,7 @@ struct media_entity_notify {
>   * @pads:	List of registered pads
>   * @links:	List of registered links
>   * @entity_notify: List of registered entity_notify callbacks
> - * @lock:	Entities list lock
> - * @graph_mutex: Entities graph operation lock
> + * @graph_mutex: Protects access to struct media_device data
>   * @pm_count_walk: Graph walk for power state walk. Access serialised using
>   *		   graph_mutex.
>   *
> @@ -371,8 +369,6 @@ struct media_device {
>  	/* notify callback list invoked when a new entity is registered */
>  	struct list_head entity_notify;
>  
> -	/* Protects the graph objects creation/removal */
> -	spinlock_t lock;
>  	/* Serializes graph operations. */
>  	struct mutex graph_mutex;
>  	struct media_entity_graph pm_count_walk;
> 


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH 1/5] [media] media-device: get rid of the spinlock

[Laurent Pinchart]

On Wednesday 16 Mar 2016 09:04:02 Mauro Carvalho Chehab wrote:
> Right now, the lock schema for media_device struct is messy,
> since sometimes, it is protected via a spin lock, while, for
> media graph traversal, it is protected by a mutex.
> 
> Solve this conflict by always using a mutex.
> 
> As a side effect, this prevents a bug where the media notifiers
> were called at atomic context.

And as a side effect the kernel now deadlocks in MEDIA_IOC_ENUM_LINKS. I'm all 
for fixing messy the lock schema, but maybe you should test patches before 
merging them ?

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/media-device.c | 32 ++++++++++++++------------------
>  drivers/media/media-entity.c | 16 ++++++++--------
>  include/media/media-device.h |  6 +-----
>  3 files changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 6e43c95629ea..c32fa15cc76e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -90,17 +90,17 @@ static struct media_entity *find_entity(struct
> media_device *mdev, u32 id)
> 
>  	id &= ~MEDIA_ENT_ID_FLAG_NEXT;
> 
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
> 
>  	media_device_for_each_entity(entity, mdev) {
>  		if (((media_entity_id(entity) == id) && !next) ||
>  		    ((media_entity_id(entity) > id) && next)) {
> -			spin_unlock(&mdev->lock);
> +			mutex_unlock(&mdev->graph_mutex);
>  			return entity;
>  		}
>  	}
> 
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
> 
>  	return NULL;
>  }
> @@ -590,12 +590,12 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, if (!ida_pre_get(&mdev->entity_internal_idx,
> GFP_KERNEL))
>  		return -ENOMEM;
> 
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
> 
>  	ret = ida_get_new_above(&mdev->entity_internal_idx, 1,
>  				&entity->internal_idx);
>  	if (ret < 0) {
> -		spin_unlock(&mdev->lock);
> +		mutex_unlock(&mdev->graph_mutex);
>  		return ret;
>  	}
> 
> @@ -615,9 +615,6 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, (notify)->notify(entity, notify->notify_data);
>  	}
> 
> -	spin_unlock(&mdev->lock);
> -
> -	mutex_lock(&mdev->graph_mutex);
>  	if (mdev->entity_internal_idx_max
> 
>  	    >= mdev->pm_count_walk.ent_enum.idx_max) {
> 
>  		struct media_entity_graph new = { .top = 0 };
> @@ -680,9 +677,9 @@ void media_device_unregister_entity(struct media_entity
> *entity) if (mdev == NULL)
>  		return;
> 
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_device_unregister_entity(entity);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity);
> 
> @@ -703,7 +700,6 @@ void media_device_init(struct media_device *mdev)
>  	INIT_LIST_HEAD(&mdev->pads);
>  	INIT_LIST_HEAD(&mdev->links);
>  	INIT_LIST_HEAD(&mdev->entity_notify);
> -	spin_lock_init(&mdev->lock);
>  	mutex_init(&mdev->graph_mutex);
>  	ida_init(&mdev->entity_internal_idx);
> 
> @@ -752,9 +748,9 @@ EXPORT_SYMBOL_GPL(__media_device_register);
>  int __must_check media_device_register_entity_notify(struct media_device
> *mdev, struct media_entity_notify *nptr)
>  {
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	list_add_tail(&nptr->list, &mdev->entity_notify);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
> @@ -771,9 +767,9 @@ static void
> __media_device_unregister_entity_notify(struct media_device *mdev, void
> media_device_unregister_entity_notify(struct media_device *mdev, struct
> media_entity_notify *nptr)
>  {
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_device_unregister_entity_notify(mdev, nptr);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
> 
> @@ -787,11 +783,11 @@ void media_device_unregister(struct media_device
> *mdev) if (mdev == NULL)
>  		return;
> 
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
> 
>  	/* Check if mdev was ever registered at all */
>  	if (!media_devnode_is_registered(&mdev->devnode)) {
> -		spin_unlock(&mdev->lock);
> +		mutex_unlock(&mdev->graph_mutex);
>  		return;
>  	}
> 
> @@ -811,7 +807,7 @@ void media_device_unregister(struct media_device *mdev)
>  		kfree(intf);
>  	}
> 
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
> 
>  	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>  	media_devnode_unregister(&mdev->devnode);
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e95070b3a3d4..c53c1d5589a0 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -219,7 +219,7 @@ int media_entity_pads_init(struct media_entity *entity,
> u16 num_pads, entity->pads = pads;
> 
>  	if (mdev)
> -		spin_lock(&mdev->lock);
> +		mutex_lock(&mdev->graph_mutex);
> 
>  	for (i = 0; i < num_pads; i++) {
>  		pads[i].entity = entity;
> @@ -230,7 +230,7 @@ int media_entity_pads_init(struct media_entity *entity,
> u16 num_pads, }
> 
>  	if (mdev)
> -		spin_unlock(&mdev->lock);
> +		mutex_unlock(&mdev->graph_mutex);
> 
>  	return 0;
>  }
> @@ -747,9 +747,9 @@ void media_entity_remove_links(struct media_entity
> *entity) if (mdev == NULL)
>  		return;
> 
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_entity_remove_links(entity);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_entity_remove_links);
> 
> @@ -951,9 +951,9 @@ void media_remove_intf_link(struct media_link *link)
>  	if (mdev == NULL)
>  		return;
> 
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_remove_intf_link(link);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_remove_intf_link);
> 
> @@ -975,8 +975,8 @@ void media_remove_intf_links(struct media_interface
> *intf) if (mdev == NULL)
>  		return;
> 
> -	spin_lock(&mdev->lock);
> +	mutex_lock(&mdev->graph_mutex);
>  	__media_remove_intf_links(intf);
> -	spin_unlock(&mdev->lock);
> +	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_remove_intf_links);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index df74cfa7da4a..0c2de97181f3 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -25,7 +25,6 @@
> 
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> -#include <linux/spinlock.h>
> 
>  #include <media/media-devnode.h>
>  #include <media/media-entity.h>
> @@ -304,8 +303,7 @@ struct media_entity_notify {
>   * @pads:	List of registered pads
>   * @links:	List of registered links
>   * @entity_notify: List of registered entity_notify callbacks
> - * @lock:	Entities list lock
> - * @graph_mutex: Entities graph operation lock
> + * @graph_mutex: Protects access to struct media_device data
>   * @pm_count_walk: Graph walk for power state walk. Access serialised using
> *		   graph_mutex.
>   *
> @@ -371,8 +369,6 @@ struct media_device {
>  	/* notify callback list invoked when a new entity is registered */
>  	struct list_head entity_notify;
> 
> -	/* Protects the graph objects creation/removal */
> -	spinlock_t lock;
>  	/* Serializes graph operations. */
>  	struct mutex graph_mutex;
>  	struct media_entity_graph pm_count_walk;

-- 
Regards,

Laurent Pinchart