[RFC PATCH v2 0/2] Media entity links handling

[RFC PATCH v2 0/2] Media entity links handling

[Sylwester Nawrocki]

Hi All,

This small patch set adds a function for removing all links at a media
entity. I found out such a function is needed when media entites that
belong to a single media device have drivers in different kernel modules.
This means virtually all camera drivers, since sensors are separate
modules from the host interface drivers.

More details can be found at each patch's description.

The links removal from a media entity is rather strightforward, but when
and where links should be created/removed is not immediately clear to me.

I assumed that links should normally be created/removed when an entity
is registered to its media device, with the graph mutex held.

I'm open to opinions whether it's good or not and possibly suggestions
on how those issues could be handled differently.

The changes since original version are listed in patch 1/2, in patch 2/2
only the commit description has changed slightly.

Thanks,
Sylwester

Sylwester Nawrocki (2):
  media: Add a function removing all links of a media entity
  V4L: Remove all links of a media entity when unregistering subdev

 drivers/media/media-entity.c          |   48 +++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-device.c |    4 ++-
 include/media/media-entity.h          |    3 ++
 3 files changed, 54 insertions(+), 1 deletions(-)

-- 
1.7.4.1

[RFC PATCH v2 1/2] media: Add a function removing all links of a media entity

[Sylwester Nawrocki]

This function allows to remove all media entity's links to other
entities, leaving no references to a media entity's links array
at its remote entities.

Currently, when a driver of some entity is removed it will free its
media entities links[] array, leaving dangling pointers at other
entities that are part of same media graph. This is troublesome when
drivers of a media device entities are in separate kernel modules,
removing only some modules will leave others in an incorrect state.

This function is intended to be used when an entity is being
unregistered from a media device.

With an assumption that normally the media links should be created
between media entities registered to a media device and with the
graph mutex held.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since v1:
 - couple code improvements like variable localization, type change, etc.
 - removed WARN_ON_ONCE() to avoid warnings when the media_entity_remove_links()
   function is called from within remove() callback of a driver of subdev
   which has not yet been registered to the media device, e.g. due to
   deferred probing, with a call stack like
      -> remove()
         -> v4l2_device_unregister_subdev()
            -> media_entity_remove_links()
---
 drivers/media/media-entity.c |   48 ++++++++++++++++++++++++++++++++++++++++++
 include/media/media-entity.h |    3 ++
 2 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e1cd132..f5ea82e 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -429,6 +429,54 @@ media_entity_create_link(struct media_entity *source, u16 source_pad,
 }
 EXPORT_SYMBOL_GPL(media_entity_create_link);
 
+void __media_entity_remove_links(struct media_entity *entity)
+{
+	unsigned int i;
+
+	for (i = 0; i < entity->num_links; i++) {
+		struct media_link *link = &entity->links[i];
+		struct media_entity *remote;
+		unsigned int r, num_links;
+
+		if (link->source->entity == entity)
+			remote = link->sink->entity;
+		else
+			remote = link->source->entity;
+
+		num_links = remote->num_links;
+
+		for (r = 0; r < num_links; r++) {
+			if (remote->links[r] != link->reverse)
+				continue;
+
+			if (link->source->entity == entity)
+				remote->num_backlinks--;
+
+			if (--remote->num_links == 0)
+				break;
+
+			/* Insert last entry in place of the dropped link. */
+			remote->links[r--] = remote->links[remote->num_links];
+		}
+	}
+
+	entity->num_links = 0;
+	entity->num_backlinks = 0;
+}
+EXPORT_SYMBOL_GPL(__media_entity_remove_links);
+
+void media_entity_remove_links(struct media_entity *entity)
+{
+	/* Do nothing if the entity is not registered. */
+	if (entity->parent == NULL)
+		return;
+
+	mutex_lock(&entity->parent->graph_mutex);
+	__media_entity_remove_links(entity);
+	mutex_lock(&entity->parent->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_entity_remove_links);
+
 static int __media_entity_setup_link_notify(struct media_link *link, u32 flags)
 {
 	int ret;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 0c16f51..0d941d2 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -128,6 +128,9 @@ void media_entity_cleanup(struct media_entity *entity);
 
 int media_entity_create_link(struct media_entity *source, u16 source_pad,
 		struct media_entity *sink, u16 sink_pad, u32 flags);
+void __media_entity_remove_links(struct media_entity *entity);
+void media_entity_remove_links(struct media_entity *entity);
+
 int __media_entity_setup_link(struct media_link *link, u32 flags);
 int media_entity_setup_link(struct media_link *link, u32 flags);
 struct media_link *media_entity_find_link(struct media_pad *source,
-- 
1.7.4.1

[RFC PATCH v2 2/2] V4L: Remove all links of a media entity when unregistering subdev

[Sylwester Nawrocki]

Remove all links of the subdev's media entity after internal_ops
'unregistered' call and right before unregistering the entity from
a media device.

It is assumed here that an unregistered (orphan) media entity cannot
have links to other entities registered to a media device.

It is also assumed the media links should be created/removed with
the media graph's mutex held.

The above implies that the caller of v4l2_device_unregister_subdev()
must not hold the graph's mutex.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/v4l2-core/v4l2-device.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 8ed5da2..2dbfebc 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -269,8 +269,10 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	sd->v4l2_dev = NULL;
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev)
+	if (v4l2_dev->mdev) {
+		media_entity_remove_links(&sd->entity);
 		media_device_unregister_entity(&sd->entity);
+	}
 #endif
 	video_unregister_device(sd->devnode);
 	module_put(sd->owner);
-- 
1.7.4.1

Re: [RFC PATCH v2 0/2] Media entity links handling

[Laurent Pinchart]

Hi Sylwester,

On Sunday 09 June 2013 21:41:37 Sylwester Nawrocki wrote:
> Hi All,
> 
> This small patch set adds a function for removing all links at a media
> entity. I found out such a function is needed when media entites that
> belong to a single media device have drivers in different kernel modules.
> This means virtually all camera drivers, since sensors are separate
> modules from the host interface drivers.
> 
> More details can be found at each patch's description.

The patches look good to me.

> The links removal from a media entity is rather strightforward, but when
> and where links should be created/removed is not immediately clear to me.
> 
> I assumed that links should normally be created/removed when an entity
> is registered to its media device, with the graph mutex held.
> 
> I'm open to opinions whether it's good or not and possibly suggestions
> on how those issues could be handled differently.

It's a very good question. So far links were created at initialization time 
and assumed to stay until the device gets torn down. Existing drivers thus 
access the links without holding the graph mutex.

An easy solution to fix race conditions at link creation time would be to take 
the graph mutex in media_entity_create_link(). We will still have to fix 
drivers to take the mutex when accessing links, I don't think there's a way 
around that. We also need to precisely define what the graph mutex protects 
and how drivers can access links and entities. This will become especially 
important when the media controller framework will be used in DRM/KMS.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 0/2] Media entity links handling

[Sylwester Nawrocki]

Hi Laurent,

On 06/09/2013 10:23 PM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Sunday 09 June 2013 21:41:37 Sylwester Nawrocki wrote:
>> Hi All,
>>
>> This small patch set adds a function for removing all links at a media
>> entity. I found out such a function is needed when media entites that
>> belong to a single media device have drivers in different kernel modules.
>> This means virtually all camera drivers, since sensors are separate
>> modules from the host interface drivers.
>>
>> More details can be found at each patch's description.
> 
> The patches look good to me.

Thanks, I would push v3 upstream if everyone agrees. I think there is further
work to be done, to allow the media entities race-free access to their parent
media device and the graph mutex. For example, entity->parent->graph_mutex is
needed to access entity->stream_count in a subdev driver, but for that it needs
to be ensured that entity->parent doesn't change unexpectedly.

>> The links removal from a media entity is rather strightforward, but when
>> and where links should be created/removed is not immediately clear to me.
>>
>> I assumed that links should normally be created/removed when an entity
>> is registered to its media device, with the graph mutex held.
>>
>> I'm open to opinions whether it's good or not and possibly suggestions
>> on how those issues could be handled differently.
> 
> It's a very good question. So far links were created at initialization time 
> and assumed to stay until the device gets torn down. Existing drivers thus 
> access the links without holding the graph mutex.
> 
> An easy solution to fix race conditions at link creation time would be to take 
> the graph mutex in media_entity_create_link(). We will still have to fix 
> drivers to take the mutex when accessing links, I don't think there's a way 
> around that. We also need to precisely define what the graph mutex protects 
> and how drivers can access links and entities. This will become especially 
> important when the media controller framework will be used in DRM/KMS.

I agree with that. Clear and documented semantics of the graph mutex is
essential. And it all really seems required for proper subdevs hotplugging,
deferred probing support etc.

Regards,
Sylwester