linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v3 0/2] Media entity links handling
@ 2013-06-10 14:54 Sylwester Nawrocki
  2013-06-10 14:54 ` Sylwester Nawrocki
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-10 14:54 UTC (permalink / raw)
  To: linux-media
  Cc: laurent.pinchart, sakari.ailus, kyungmin.park, a.hajda,
	Sylwester Nawrocki

Hi,

This is an updated version of the patch set
http://www.spinics.net/lists/linux-media/msg64536.html

Comparing to v2 it includes improvements of the __media_entity_remove_links()
function, thanks to Sakari. 

The cover letter of v2 is included below.

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          |   50 +++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-device.c |    4 ++-
 include/media/media-entity.h          |    3 ++
 3 files changed, 56 insertions(+), 1 deletion(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [RFC PATCH v3 0/2] Media entity links handling
  2013-06-10 14:54 [RFC PATCH v3 0/2] Media entity links handling Sylwester Nawrocki
@ 2013-06-10 14:54 ` Sylwester Nawrocki
  2013-06-11 10:50   ` Sakari Ailus
  2013-06-10 14:54 ` [RFC PATCH v3 1/2] media: Add a function removing all links of a media entity Sylwester Nawrocki
  2013-06-10 14:54 ` [RFC PATCH v3 2/2] V4L: Remove all links of a media entity when unregistering subdev Sylwester Nawrocki
  2 siblings, 1 reply; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-10 14:54 UTC (permalink / raw)
  To: linux-media
  Cc: laurent.pinchart, sakari.ailus, kyungmin.park, a.hajda,
	Sylwester Nawrocki

This is an updated version of the patch set
http://www.spinics.net/lists/linux-media/msg64536.html

Comparing to v2 it includes improvements of the __media_entity_remove_links()
function, thanks to Sakari. 

The cover letter of v2 is included below.

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          |   50 +++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-device.c |    4 ++-
 include/media/media-entity.h          |    3 ++
 3 files changed, 56 insertions(+), 1 deletion(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [RFC PATCH v3 1/2] media: Add a function removing all links of a media entity
  2013-06-10 14:54 [RFC PATCH v3 0/2] Media entity links handling Sylwester Nawrocki
  2013-06-10 14:54 ` Sylwester Nawrocki
@ 2013-06-10 14:54 ` Sylwester Nawrocki
  2013-06-11 11:44   ` Sylwester Nawrocki
  2013-06-10 14:54 ` [RFC PATCH v3 2/2] V4L: Remove all links of a media entity when unregistering subdev Sylwester Nawrocki
  2 siblings, 1 reply; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-10 14:54 UTC (permalink / raw)
  To: linux-media
  Cc: laurent.pinchart, sakari.ailus, kyungmin.park, a.hajda,
	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, 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 v2:
 - removed caching of remote->num_links, re-added rlink local variable,
 - refactored the loop iterating the remote entity links array.

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 |   50 ++++++++++++++++++++++++++++++++++++++++++
 include/media/media-entity.h |    3 +++
 2 files changed, 53 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 0438209..4f436f1 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -429,6 +429,56 @@ 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 = 0;
+
+		if (link->source->entity == entity)
+			remote = link->sink->entity;
+		else
+			remote = link->source->entity;
+
+		while (r < remote->num_links) {
+			struct media_link *rlink = &remote->links[r];
+
+			if (rlink != link->reverse) {
+				r++;
+				continue;
+			}
+
+			if (link->source->entity == entity)
+				remote->num_backlinks--;
+
+			if (--remote->num_links == 0)
+				break;
+
+			/* Insert last entry in place of the dropped link. */
+			*rlink = 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 4eefedc..06bacf9 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.9.5


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [RFC PATCH v3 2/2] V4L: Remove all links of a media entity when unregistering subdev
  2013-06-10 14:54 [RFC PATCH v3 0/2] Media entity links handling Sylwester Nawrocki
  2013-06-10 14:54 ` Sylwester Nawrocki
  2013-06-10 14:54 ` [RFC PATCH v3 1/2] media: Add a function removing all links of a media entity Sylwester Nawrocki
@ 2013-06-10 14:54 ` Sylwester Nawrocki
  2 siblings, 0 replies; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-10 14:54 UTC (permalink / raw)
  To: linux-media
  Cc: laurent.pinchart, sakari.ailus, kyungmin.park, a.hajda,
	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 file changed, 3 insertions(+), 1 deletion(-)

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.9.5


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH v3 0/2] Media entity links handling
  2013-06-10 14:54 ` Sylwester Nawrocki
@ 2013-06-11 10:50   ` Sakari Ailus
  2013-06-11 10:50     ` [RFC PATCH 1/2] smiapp: Clean up media entity after unregistering subdev Sakari Ailus
  2013-06-11 11:36     ` [RFC PATCH v3 0/2] Media entity links handling Sylwester Nawrocki
  0 siblings, 2 replies; 15+ messages in thread
From: Sakari Ailus @ 2013-06-11 10:50 UTC (permalink / raw)
  To: Sylwester Nawrocki; +Cc: linux-media, laurent.pinchart, kyungmin.park, a.hajda

Hi Sylwester,

On Mon, Jun 10, 2013 at 04:54:28PM +0200, Sylwester Nawrocki wrote:
> This is an updated version of the patch set
> http://www.spinics.net/lists/linux-media/msg64536.html
> 
> Comparing to v2 it includes improvements of the __media_entity_remove_links()
> function, thanks to Sakari. 
> 
> The cover letter of v2 is included below.
> 
> 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.

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

That said, I'd wish they won't be merged before the two patches I'm sending
shortly. The thing is that the media entity links array is freed by
media_entity_cleanup(), and there are two drivers that call
media_entity_cleanup() first. The patches fix the issue, so could you
prepend them to your set (after review, naturally)?

-- 
Kind regards,

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [RFC PATCH 1/2] smiapp: Clean up media entity after unregistering subdev
  2013-06-11 10:50   ` Sakari Ailus
@ 2013-06-11 10:50     ` Sakari Ailus
  2013-06-11 10:50       ` [RFC PATCH 2/2] davinci_vpfe: " Sakari Ailus
  2013-06-11 11:38       ` [RFC PATCH 1/2] smiapp: " Sylwester Nawrocki
  2013-06-11 11:36     ` [RFC PATCH v3 0/2] Media entity links handling Sylwester Nawrocki
  1 sibling, 2 replies; 15+ messages in thread
From: Sakari Ailus @ 2013-06-11 10:50 UTC (permalink / raw)
  To: s.nawrocki
  Cc: prabhakar.csengg, linux-media, laurent.pinchart, kyungmin.park, a.hajda

media_entity_cleanup() frees the links array which will be accessed by
media_entity_remove_links() called by v4l2_device_unregister_subdev().

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/i2c/smiapp/smiapp-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index c385454..7ac7580 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2848,8 +2848,8 @@ static int smiapp_remove(struct i2c_client *client)
 		device_remove_file(&client->dev, &dev_attr_nvm);
 
 	for (i = 0; i < sensor->ssds_used; i++) {
-		media_entity_cleanup(&sensor->ssds[i].sd.entity);
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
+		media_entity_cleanup(&sensor->ssds[i].sd.entity);
 	}
 	smiapp_free_controls(sensor);
 
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [RFC PATCH 2/2] davinci_vpfe: Clean up media entity after unregistering subdev
  2013-06-11 10:50     ` [RFC PATCH 1/2] smiapp: Clean up media entity after unregistering subdev Sakari Ailus
@ 2013-06-11 10:50       ` Sakari Ailus
  2013-06-11 11:38         ` Sylwester Nawrocki
  2013-06-12  4:44         ` Prabhakar Lad
  2013-06-11 11:38       ` [RFC PATCH 1/2] smiapp: " Sylwester Nawrocki
  1 sibling, 2 replies; 15+ messages in thread
From: Sakari Ailus @ 2013-06-11 10:50 UTC (permalink / raw)
  To: s.nawrocki
  Cc: prabhakar.csengg, linux-media, laurent.pinchart, kyungmin.park, a.hajda

media_entity_cleanup() frees the links array which will be accessed by
media_entity_remove_links() called by v4l2_device_unregister_subdev().

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/staging/media/davinci_vpfe/dm365_ipipe.c   |    4 ++--
 drivers/staging/media/davinci_vpfe/dm365_ipipeif.c |    4 ++--
 drivers/staging/media/davinci_vpfe/dm365_isif.c    |    4 ++--
 drivers/staging/media/davinci_vpfe/dm365_resizer.c |   14 +++++++-------
 drivers/staging/media/davinci_vpfe/vpfe_video.c    |    2 +-
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 05673ed..766a071 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1751,10 +1751,10 @@ static const struct media_entity_operations ipipe_media_ops = {
  */
 void vpfe_ipipe_unregister_entities(struct vpfe_ipipe_device *vpfe_ipipe)
 {
-	/* cleanup entity */
-	media_entity_cleanup(&vpfe_ipipe->subdev.entity);
 	/* unregister subdev */
 	v4l2_device_unregister_subdev(&vpfe_ipipe->subdev);
+	/* cleanup entity */
+	media_entity_cleanup(&vpfe_ipipe->subdev.entity);
 }
 
 /*
diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c b/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c
index b2f4ef8..59540cd 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipeif.c
@@ -947,10 +947,10 @@ void vpfe_ipipeif_unregister_entities(struct vpfe_ipipeif_device *ipipeif)
 	/* unregister video device */
 	vpfe_video_unregister(&ipipeif->video_in);
 
-	/* cleanup entity */
-	media_entity_cleanup(&ipipeif->subdev.entity);
 	/* unregister subdev */
 	v4l2_device_unregister_subdev(&ipipeif->subdev);
+	/* cleanup entity */
+	media_entity_cleanup(&ipipeif->subdev.entity);
 }
 
 int
diff --git a/drivers/staging/media/davinci_vpfe/dm365_isif.c b/drivers/staging/media/davinci_vpfe/dm365_isif.c
index 5829360..ff48fce 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_isif.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_isif.c
@@ -1750,10 +1750,10 @@ static const struct media_entity_operations isif_media_ops = {
 void vpfe_isif_unregister_entities(struct vpfe_isif_device *isif)
 {
 	vpfe_video_unregister(&isif->video_out);
-	/* cleanup entity */
-	media_entity_cleanup(&isif->subdev.entity);
 	/* unregister subdev */
 	v4l2_device_unregister_subdev(&isif->subdev);
+	/* cleanup entity */
+	media_entity_cleanup(&isif->subdev.entity);
 }
 
 static void isif_restore_defaults(struct vpfe_isif_device *isif)
diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
index 126f84c..8e13bd4 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
@@ -1777,14 +1777,14 @@ void vpfe_resizer_unregister_entities(struct vpfe_resizer_device *vpfe_rsz)
 	vpfe_video_unregister(&vpfe_rsz->resizer_a.video_out);
 	vpfe_video_unregister(&vpfe_rsz->resizer_b.video_out);
 
-	/* cleanup entity */
-	media_entity_cleanup(&vpfe_rsz->crop_resizer.subdev.entity);
-	media_entity_cleanup(&vpfe_rsz->resizer_a.subdev.entity);
-	media_entity_cleanup(&vpfe_rsz->resizer_b.subdev.entity);
 	/* unregister subdev */
 	v4l2_device_unregister_subdev(&vpfe_rsz->crop_resizer.subdev);
 	v4l2_device_unregister_subdev(&vpfe_rsz->resizer_a.subdev);
 	v4l2_device_unregister_subdev(&vpfe_rsz->resizer_b.subdev);
+	/* cleanup entity */
+	media_entity_cleanup(&vpfe_rsz->crop_resizer.subdev.entity);
+	media_entity_cleanup(&vpfe_rsz->resizer_a.subdev.entity);
+	media_entity_cleanup(&vpfe_rsz->resizer_b.subdev.entity);
 }
 
 /*
@@ -1865,12 +1865,12 @@ out_create_link:
 	vpfe_video_unregister(&resizer->resizer_b.video_out);
 out_video_out2_register:
 	vpfe_video_unregister(&resizer->resizer_a.video_out);
-	media_entity_cleanup(&resizer->crop_resizer.subdev.entity);
-	media_entity_cleanup(&resizer->resizer_a.subdev.entity);
-	media_entity_cleanup(&resizer->resizer_b.subdev.entity);
 	v4l2_device_unregister_subdev(&resizer->crop_resizer.subdev);
 	v4l2_device_unregister_subdev(&resizer->resizer_a.subdev);
 	v4l2_device_unregister_subdev(&resizer->resizer_b.subdev);
+	media_entity_cleanup(&resizer->crop_resizer.subdev.entity);
+	media_entity_cleanup(&resizer->resizer_a.subdev.entity);
+	media_entity_cleanup(&resizer->resizer_b.subdev.entity);
 	return ret;
 }
 
diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c b/drivers/staging/media/davinci_vpfe/vpfe_video.c
index cb5410b..24d98a6 100644
--- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
+++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
@@ -1614,7 +1614,7 @@ int vpfe_video_register(struct vpfe_video_device *video,
 void vpfe_video_unregister(struct vpfe_video_device *video)
 {
 	if (video_is_registered(&video->video_dev)) {
-		media_entity_cleanup(&video->video_dev.entity);
 		video_unregister_device(&video->video_dev);
+		media_entity_cleanup(&video->video_dev.entity);
 	}
 }
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH v3 0/2] Media entity links handling
  2013-06-11 10:50   ` Sakari Ailus
  2013-06-11 10:50     ` [RFC PATCH 1/2] smiapp: Clean up media entity after unregistering subdev Sakari Ailus
@ 2013-06-11 11:36     ` Sylwester Nawrocki
  1 sibling, 0 replies; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-11 11:36 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, kyungmin.park, a.hajda

Hi Sakari,

On 06/11/2013 12:50 PM, Sakari Ailus wrote:
> On Mon, Jun 10, 2013 at 04:54:28PM +0200, Sylwester Nawrocki wrote:
>> This is an updated version of the patch set
>> http://www.spinics.net/lists/linux-media/msg64536.html
>>
>> Comparing to v2 it includes improvements of the __media_entity_remove_links()
>> function, thanks to Sakari. 
>>
>> The cover letter of v2 is included below.
>>
>> 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.
> 
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> That said, I'd wish they won't be merged before the two patches I'm sending
> shortly. The thing is that the media entity links array is freed by
> media_entity_cleanup(), and there are two drivers that call
> media_entity_cleanup() first. The patches fix the issue, so could you
> prepend them to your set (after review, naturally)?

Your patches look good, thanks a lot for inspecting those drivers! I'm going
to put all those patches on a separate branch and will hold on with sending
a pull request for a couple days.

Regards,
Sylwester

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH 2/2] davinci_vpfe: Clean up media entity after unregistering subdev
  2013-06-11 10:50       ` [RFC PATCH 2/2] davinci_vpfe: " Sakari Ailus
@ 2013-06-11 11:38         ` Sylwester Nawrocki
  2013-06-12  4:44         ` Prabhakar Lad
  1 sibling, 0 replies; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-11 11:38 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: prabhakar.csengg, linux-media, laurent.pinchart, kyungmin.park, a.hajda

On 06/11/2013 12:50 PM, Sakari Ailus wrote:
> media_entity_cleanup() frees the links array which will be accessed by
> media_entity_remove_links() called by v4l2_device_unregister_subdev().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH 1/2] smiapp: Clean up media entity after unregistering subdev
  2013-06-11 10:50     ` [RFC PATCH 1/2] smiapp: Clean up media entity after unregistering subdev Sakari Ailus
  2013-06-11 10:50       ` [RFC PATCH 2/2] davinci_vpfe: " Sakari Ailus
@ 2013-06-11 11:38       ` Sylwester Nawrocki
  1 sibling, 0 replies; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-11 11:38 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: prabhakar.csengg, linux-media, laurent.pinchart, kyungmin.park, a.hajda

On 06/11/2013 12:50 PM, Sakari Ailus wrote:
> media_entity_cleanup() frees the links array which will be accessed by
> media_entity_remove_links() called by v4l2_device_unregister_subdev().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH v3 1/2] media: Add a function removing all links of a media entity
  2013-06-10 14:54 ` [RFC PATCH v3 1/2] media: Add a function removing all links of a media entity Sylwester Nawrocki
@ 2013-06-11 11:44   ` Sylwester Nawrocki
  0 siblings, 0 replies; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-11 11:44 UTC (permalink / raw)
  To: linux-media; +Cc: laurent.pinchart, sakari.ailus, kyungmin.park, a.hajda

On 06/10/2013 04:54 PM, Sylwester Nawrocki wrote:
> +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);

And this bug snicked in again during rebase, I've noticed this yesterday,
after posting and more testing. I'll keep it corrected locally, unless
there is v4 needed.

Regards,
Sylwester

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH 2/2] davinci_vpfe: Clean up media entity after unregistering subdev
  2013-06-11 10:50       ` [RFC PATCH 2/2] davinci_vpfe: " Sakari Ailus
  2013-06-11 11:38         ` Sylwester Nawrocki
@ 2013-06-12  4:44         ` Prabhakar Lad
  2013-06-16 21:16           ` Sylwester Nawrocki
  1 sibling, 1 reply; 15+ messages in thread
From: Prabhakar Lad @ 2013-06-12  4:44 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: s.nawrocki, linux-media, laurent.pinchart, kyungmin.park, a.hajda

Hi Sakari,

Thanks for the patch.

On Tue, Jun 11, 2013 at 4:20 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> media_entity_cleanup() frees the links array which will be accessed by
> media_entity_remove_links() called by v4l2_device_unregister_subdev().
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Regards,
--Prabhakar Lad

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH 2/2] davinci_vpfe: Clean up media entity after unregistering subdev
  2013-06-12  4:44         ` Prabhakar Lad
@ 2013-06-16 21:16           ` Sylwester Nawrocki
  2013-06-17  8:07             ` Prabhakar Lad
  2013-06-17 20:43             ` Sakari Ailus
  0 siblings, 2 replies; 15+ messages in thread
From: Sylwester Nawrocki @ 2013-06-16 21:16 UTC (permalink / raw)
  To: Prabhakar Lad, Sakari Ailus, laurent.pinchart
  Cc: s.nawrocki, linux-media, kyungmin.park, a.hajda

Hi,

On 06/12/2013 06:44 AM, Prabhakar Lad wrote:
> On Tue, Jun 11, 2013 at 4:20 PM, Sakari Ailus<sakari.ailus@iki.fi>  wrote:
>> media_entity_cleanup() frees the links array which will be accessed by
>> media_entity_remove_links() called by v4l2_device_unregister_subdev().
>>
>> Signed-off-by: Sakari Ailus<sakari.ailus@iki.fi>
>
> Acked-by: Lad, Prabhakar<prabhakar.csengg@gmail.com>

I have added these two patches to my tree for 3.11 (in branch for-v3.11-2).
Please let me know if you would like it to be handled differently.

Regards,
Sylwester

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH 2/2] davinci_vpfe: Clean up media entity after unregistering subdev
  2013-06-16 21:16           ` Sylwester Nawrocki
@ 2013-06-17  8:07             ` Prabhakar Lad
  2013-06-17 20:43             ` Sakari Ailus
  1 sibling, 0 replies; 15+ messages in thread
From: Prabhakar Lad @ 2013-06-17  8:07 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Sakari Ailus, laurent.pinchart, s.nawrocki, linux-media,
	kyungmin.park, a.hajda

Hi Sylwester,

On Mon, Jun 17, 2013 at 2:46 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi,
>
>
> On 06/12/2013 06:44 AM, Prabhakar Lad wrote:
>>
>> On Tue, Jun 11, 2013 at 4:20 PM, Sakari Ailus<sakari.ailus@iki.fi>  wrote:
>>>
>>> media_entity_cleanup() frees the links array which will be accessed by
>>> media_entity_remove_links() called by v4l2_device_unregister_subdev().
>>>
>>> Signed-off-by: Sakari Ailus<sakari.ailus@iki.fi>
>>
>>
>> Acked-by: Lad, Prabhakar<prabhakar.csengg@gmail.com>
>
>
> I have added these two patches to my tree for 3.11 (in branch for-v3.11-2).
> Please let me know if you would like it to be handled differently.
>
I am fine with it.

Regards,
--Prabhakar Lad

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [RFC PATCH 2/2] davinci_vpfe: Clean up media entity after unregistering subdev
  2013-06-16 21:16           ` Sylwester Nawrocki
  2013-06-17  8:07             ` Prabhakar Lad
@ 2013-06-17 20:43             ` Sakari Ailus
  1 sibling, 0 replies; 15+ messages in thread
From: Sakari Ailus @ 2013-06-17 20:43 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Prabhakar Lad, laurent.pinchart, s.nawrocki, linux-media,
	kyungmin.park, a.hajda

Hi Sylwester,

On Sun, Jun 16, 2013 at 11:16:44PM +0200, Sylwester Nawrocki wrote:
> Hi,
> 
> On 06/12/2013 06:44 AM, Prabhakar Lad wrote:
> >On Tue, Jun 11, 2013 at 4:20 PM, Sakari Ailus<sakari.ailus@iki.fi>  wrote:
> >>media_entity_cleanup() frees the links array which will be accessed by
> >>media_entity_remove_links() called by v4l2_device_unregister_subdev().
> >>
> >>Signed-off-by: Sakari Ailus<sakari.ailus@iki.fi>
> >
> >Acked-by: Lad, Prabhakar<prabhakar.csengg@gmail.com>
> 
> I have added these two patches to my tree for 3.11 (in branch for-v3.11-2).
> Please let me know if you would like it to be handled differently.

This was what I was looking forward to. Thanks!

-- 
Kind regards,

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-06-17 20:43 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-10 14:54 [RFC PATCH v3 0/2] Media entity links handling Sylwester Nawrocki
2013-06-10 14:54 ` Sylwester Nawrocki
2013-06-11 10:50   ` Sakari Ailus
2013-06-11 10:50     ` [RFC PATCH 1/2] smiapp: Clean up media entity after unregistering subdev Sakari Ailus
2013-06-11 10:50       ` [RFC PATCH 2/2] davinci_vpfe: " Sakari Ailus
2013-06-11 11:38         ` Sylwester Nawrocki
2013-06-12  4:44         ` Prabhakar Lad
2013-06-16 21:16           ` Sylwester Nawrocki
2013-06-17  8:07             ` Prabhakar Lad
2013-06-17 20:43             ` Sakari Ailus
2013-06-11 11:38       ` [RFC PATCH 1/2] smiapp: " Sylwester Nawrocki
2013-06-11 11:36     ` [RFC PATCH v3 0/2] Media entity links handling Sylwester Nawrocki
2013-06-10 14:54 ` [RFC PATCH v3 1/2] media: Add a function removing all links of a media entity Sylwester Nawrocki
2013-06-11 11:44   ` Sylwester Nawrocki
2013-06-10 14:54 ` [RFC PATCH v3 2/2] V4L: Remove all links of a media entity when unregistering subdev Sylwester Nawrocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).