[PATCH 1/2] [media] media-device: move media entity register/unregister functions

[PATCH 1/2] [media] media-device: move media entity register/unregister functions

[Mauro Carvalho Chehab]

media entity register and unregister functions are called by media
device register/unregister. Move them to occur earlier, as we'll need
an unlocked version of media_device_entity_unregister() and we don't
want to add a function prototype without needing it.

No functional changes.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/media-device.c | 160 +++++++++++++++++++++----------------------
 1 file changed, 80 insertions(+), 80 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index da4126863ecc..1222fa642ad8 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -529,6 +529,86 @@ static void media_device_release(struct media_devnode *mdev)
 }
 
 /**
+ * media_device_register_entity - Register an entity with a media device
+ * @mdev:	The media device
+ * @entity:	The entity
+ */
+int __must_check media_device_register_entity(struct media_device *mdev,
+					      struct media_entity *entity)
+{
+	unsigned int i;
+
+	if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
+	    entity->function == MEDIA_ENT_F_UNKNOWN)
+		dev_warn(mdev->dev,
+			 "Entity type for entity %s was not initialized!\n",
+			 entity->name);
+
+	/* Warn if we apparently re-register an entity */
+	WARN_ON(entity->graph_obj.mdev != NULL);
+	entity->graph_obj.mdev = mdev;
+	INIT_LIST_HEAD(&entity->links);
+	entity->num_links = 0;
+	entity->num_backlinks = 0;
+
+	spin_lock(&mdev->lock);
+	/* Initialize media_gobj embedded at the entity */
+	media_gobj_create(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
+
+	/* Initialize objects at the pads */
+	for (i = 0; i < entity->num_pads; i++)
+		media_gobj_create(mdev, MEDIA_GRAPH_PAD,
+			       &entity->pads[i].graph_obj);
+
+	spin_unlock(&mdev->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity);
+
+/**
+ * media_device_unregister_entity - Unregister an entity
+ * @entity:	The entity
+ *
+ * If the entity has never been registered this function will return
+ * immediately.
+ */
+void media_device_unregister_entity(struct media_entity *entity)
+{
+	struct media_device *mdev = entity->graph_obj.mdev;
+	struct media_link *link, *tmp;
+	struct media_interface *intf;
+	unsigned int i;
+
+	if (mdev == NULL)
+		return;
+
+	spin_lock(&mdev->lock);
+
+	/* Remove all interface links pointing to this entity */
+	list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
+		list_for_each_entry_safe(link, tmp, &intf->links, list) {
+			if (link->entity == entity)
+				__media_remove_intf_link(link);
+		}
+	}
+
+	/* Remove all data links that belong to this entity */
+	__media_entity_remove_links(entity);
+
+	/* Remove all pads that belong to this entity */
+	for (i = 0; i < entity->num_pads; i++)
+		media_gobj_destroy(&entity->pads[i].graph_obj);
+
+	/* Remove the entity */
+	media_gobj_destroy(&entity->graph_obj);
+
+	spin_unlock(&mdev->lock);
+	entity->graph_obj.mdev = NULL;
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity);
+
+/**
  * media_device_register - register a media device
  * @mdev:	The media device
  *
@@ -611,86 +691,6 @@ void media_device_unregister(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
-/**
- * media_device_register_entity - Register an entity with a media device
- * @mdev:	The media device
- * @entity:	The entity
- */
-int __must_check media_device_register_entity(struct media_device *mdev,
-					      struct media_entity *entity)
-{
-	unsigned int i;
-
-	if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
-	    entity->function == MEDIA_ENT_F_UNKNOWN)
-		dev_warn(mdev->dev,
-			 "Entity type for entity %s was not initialized!\n",
-			 entity->name);
-
-	/* Warn if we apparently re-register an entity */
-	WARN_ON(entity->graph_obj.mdev != NULL);
-	entity->graph_obj.mdev = mdev;
-	INIT_LIST_HEAD(&entity->links);
-	entity->num_links = 0;
-	entity->num_backlinks = 0;
-
-	spin_lock(&mdev->lock);
-	/* Initialize media_gobj embedded at the entity */
-	media_gobj_create(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
-
-	/* Initialize objects at the pads */
-	for (i = 0; i < entity->num_pads; i++)
-		media_gobj_create(mdev, MEDIA_GRAPH_PAD,
-			       &entity->pads[i].graph_obj);
-
-	spin_unlock(&mdev->lock);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(media_device_register_entity);
-
-/**
- * media_device_unregister_entity - Unregister an entity
- * @entity:	The entity
- *
- * If the entity has never been registered this function will return
- * immediately.
- */
-void media_device_unregister_entity(struct media_entity *entity)
-{
-	struct media_device *mdev = entity->graph_obj.mdev;
-	struct media_link *link, *tmp;
-	struct media_interface *intf;
-	unsigned int i;
-
-	if (mdev == NULL)
-		return;
-
-	spin_lock(&mdev->lock);
-
-	/* Remove all interface links pointing to this entity */
-	list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
-		list_for_each_entry_safe(link, tmp, &intf->links, list) {
-			if (link->entity == entity)
-				__media_remove_intf_link(link);
-		}
-	}
-
-	/* Remove all data links that belong to this entity */
-	__media_entity_remove_links(entity);
-
-	/* Remove all pads that belong to this entity */
-	for (i = 0; i < entity->num_pads; i++)
-		media_gobj_destroy(&entity->pads[i].graph_obj);
-
-	/* Remove the entity */
-	media_gobj_destroy(&entity->graph_obj);
-
-	spin_unlock(&mdev->lock);
-	entity->graph_obj.mdev = NULL;
-}
-EXPORT_SYMBOL_GPL(media_device_unregister_entity);
-
 static void media_device_release_devres(struct device *dev, void *res)
 {
 }
-- 
2.5.0

[PATCH 2/2] [media] media-device: better lock media_device_unregister()

[Mauro Carvalho Chehab]

If media_device_unregister() is called by two different
drivers, a race condition may happen, as the check if the
device is not registered is not protected.

Move the spin_lock() to happen earlier in the function, in order
to prevent such race condition.

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 1222fa642ad8..189c2ba8c3d3 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -573,18 +573,13 @@ EXPORT_SYMBOL_GPL(media_device_register_entity);
  * If the entity has never been registered this function will return
  * immediately.
  */
-void media_device_unregister_entity(struct media_entity *entity)
+static void __media_device_unregister_entity(struct media_entity *entity)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_link *link, *tmp;
 	struct media_interface *intf;
 	unsigned int i;
 
-	if (mdev == NULL)
-		return;
-
-	spin_lock(&mdev->lock);
-
 	/* Remove all interface links pointing to this entity */
 	list_for_each_entry(intf, &mdev->interfaces, graph_obj.list) {
 		list_for_each_entry_safe(link, tmp, &intf->links, list) {
@@ -603,11 +598,23 @@ void media_device_unregister_entity(struct media_entity *entity)
 	/* Remove the entity */
 	media_gobj_destroy(&entity->graph_obj);
 
-	spin_unlock(&mdev->lock);
 	entity->graph_obj.mdev = NULL;
 }
+
+void media_device_unregister_entity(struct media_entity *entity)
+{
+	struct media_device *mdev = entity->graph_obj.mdev;
+
+	if (mdev == NULL)
+		return;
+
+	spin_lock(&mdev->lock);
+	__media_device_unregister_entity(entity);
+	spin_unlock(&mdev->lock);
+}
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
+
 /**
  * media_device_register - register a media device
  * @mdev:	The media device
@@ -666,22 +673,29 @@ void media_device_unregister(struct media_device *mdev)
 	struct media_entity *next;
 	struct media_interface *intf, *tmp_intf;
 
+	if (mdev == NULL)
+		return;
+
+	spin_lock(&mdev->lock);
+
 	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(&mdev->devnode))
+	if (!media_devnode_is_registered(&mdev->devnode)) {
+		spin_unlock(&mdev->lock);
 		return;
+	}
 
 	/* Remove all entities from the media device */
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
-		media_device_unregister_entity(entity);
+		__media_device_unregister_entity(entity);
 
 	/* Remove all interfaces from the media device */
-	spin_lock(&mdev->lock);
 	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
 				 graph_obj.list) {
 		__media_remove_intf_links(intf);
 		media_gobj_destroy(&intf->graph_obj);
 		kfree(intf);
 	}
+
 	spin_unlock(&mdev->lock);
 
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
-- 
2.5.0

Re: [PATCH 1/2] [media] media-device: move media entity register/unregister functions

[Javier Martinez Canillas]

Hello Mauro,

On 12/15/2015 07:43 AM, Mauro Carvalho Chehab wrote:
> media entity register and unregister functions are called by media
> device register/unregister. Move them to occur earlier, as we'll need
> an unlocked version of media_device_entity_unregister() and we don't
> want to add a function prototype without needing it.
> 
> No functional changes.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Patch looks good to me.

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

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] [media] media-device: better lock media_device_unregister()

[Javier Martinez Canillas]

Hello Mauro,

On 12/15/2015 07:43 AM, Mauro Carvalho Chehab wrote:
> If media_device_unregister() is called by two different
> drivers, a race condition may happen, as the check if the
> device is not registered is not protected.
> 
> Move the spin_lock() to happen earlier in the function, in order
> to prevent such race condition.
> 
> Reported-by: Shuah Khan <shuahkh@osg.samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---

The patch looks good and I also tested it on an OMAP3 IGEPv2 board:

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

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America