[2/2,media] avoid double locks with graph_mutex
diff mbox

Message ID 3cabc4b828abac3c6dea240ae22d4754a438ad1b.1459188623.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab March 28, 2016, 6:11 p.m. UTC
Add a note at the headers telling that the link setup
callbacks are called with the mutex hold. Also, removes a
double lock at the PM suspend callbacks.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/media-device.c      | 1 -
 drivers/media/v4l2-core/v4l2-mc.c | 4 ----
 include/media/media-device.h      | 3 ++-
 include/media/media-entity.h      | 3 +++
 4 files changed, 5 insertions(+), 6 deletions(-)

Comments

Javier Martinez Canillas March 28, 2016, 7:38 p.m. UTC | #1
Hello Mauro,

On Mon, Mar 28, 2016 at 2:11 PM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> Add a note at the headers telling that the link setup
> callbacks are called with the mutex hold. Also, removes a
> double lock at the PM suspend callbacks.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/media-device.c      | 1 -
>  drivers/media/v4l2-core/v4l2-mc.c | 4 ----
>  include/media/media-device.h      | 3 ++-
>  include/media/media-entity.h      | 3 +++
>  4 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 6cfa890af7b4..6af5e6932271 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -93,7 +93,6 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
>         media_device_for_each_entity(entity, mdev) {
>                 if (((media_entity_id(entity) == id) && !next) ||
>                     ((media_entity_id(entity) > id) && next)) {
> -                       mutex_unlock(&mdev->graph_mutex);

This change belongs to patch 1/2.

After this change, feel free to add to both patches:

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-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
Sakari Ailus March 28, 2016, 10:06 p.m. UTC | #2
Hi Mauro,

Please see my comments below.

On Mon, Mar 28, 2016 at 03:11:04PM -0300, Mauro Carvalho Chehab wrote:
> Add a note at the headers telling that the link setup
> callbacks are called with the mutex hold. Also, removes a
> double lock at the PM suspend callbacks.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/media-device.c      | 1 -
>  drivers/media/v4l2-core/v4l2-mc.c | 4 ----
>  include/media/media-device.h      | 3 ++-
>  include/media/media-entity.h      | 3 +++
>  4 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 6cfa890af7b4..6af5e6932271 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -93,7 +93,6 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
>  	media_device_for_each_entity(entity, mdev) {
>  		if (((media_entity_id(entity) == id) && !next) ||
>  		    ((media_entity_id(entity) > id) && next)) {
> -			mutex_unlock(&mdev->graph_mutex);

Unrelated to this patch.

Please do also consider compat IOCTL handling code.

>  			return entity;
>  		}
>  	}
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 2228cd3a846e..d44ff2ec314f 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -348,8 +348,6 @@ int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
>  	int change = use ? 1 : -1;
>  	int ret;
>  
> -	mutex_lock(&mdev->graph_mutex);

What's the motivation for this change and the one below?

As far as I can see, they remove access serialisation of graph state during
graph walk, which certainly is not the right thing to do.

> -
>  	/* Apply use count to node. */
>  	entity->use_count += change;
>  	WARN_ON(entity->use_count < 0);
> @@ -359,8 +357,6 @@ int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
>  	if (ret < 0)
>  		entity->use_count -= change;
>  
> -	mutex_unlock(&mdev->graph_mutex);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_use);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index b04cfa907350..e6ad30c323fc 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -312,7 +312,8 @@ struct media_entity_notify {
>   * @enable_source: Enable Source Handler function pointer
>   * @disable_source: Disable Source Handler function pointer
>   *
> - * @link_notify: Link state change notification callback
> + * @link_notify: Link state change notification callback. This callback is
> + * Called with the graph_mutex hold.

I don't mind adding documentation such as this, but I'd put it into a
separate patch.

>   *
>   * This structure represents an abstract high-level media device. It allows easy
>   * access to entities and provides basic media device-level support. The
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 6dc9e4e8cbd4..0b16ebe36db7 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -179,6 +179,9 @@ struct media_pad {
>   * @link_validate:	Return whether a link is valid from the entity point of
>   *			view. The media_entity_pipeline_start() function
>   *			validates all links by calling this operation. Optional.
> + *
> + * Note: Those ioctls should not touch the struct media_device.@graph_mutex
> + * field, as they're called with it already hold.
>   */
>  struct media_entity_operations {
>  	int (*link_setup)(struct media_entity *entity,
Mauro Carvalho Chehab March 29, 2016, 9:17 a.m. UTC | #3
Em Tue, 29 Mar 2016 01:06:42 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> Please see my comments below.
> 
> On Mon, Mar 28, 2016 at 03:11:04PM -0300, Mauro Carvalho Chehab wrote:
> > Add a note at the headers telling that the link setup
> > callbacks are called with the mutex hold. Also, removes a
> > double lock at the PM suspend callbacks.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > ---
> >  drivers/media/media-device.c      | 1 -
> >  drivers/media/v4l2-core/v4l2-mc.c | 4 ----
> >  include/media/media-device.h      | 3 ++-
> >  include/media/media-entity.h      | 3 +++
> >  4 files changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 6cfa890af7b4..6af5e6932271 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -93,7 +93,6 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
> >  	media_device_for_each_entity(entity, mdev) {
> >  		if (((media_entity_id(entity) == id) && !next) ||
> >  		    ((media_entity_id(entity) > id) && next)) {
> > -			mutex_unlock(&mdev->graph_mutex);  
> 
> Unrelated to this patch.

Yes. This belongs to patch 1.

> 
> Please do also consider compat IOCTL handling code.
> 
> >  			return entity;
> >  		}
> >  	}
> > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> > index 2228cd3a846e..d44ff2ec314f 100644
> > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > @@ -348,8 +348,6 @@ int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
> >  	int change = use ? 1 : -1;
> >  	int ret;
> >  
> > -	mutex_lock(&mdev->graph_mutex);  
> 
> What's the motivation for this change and the one below?
> 
> As far as I can see, they remove access serialisation of graph state during
> graph walk, which certainly is not the right thing to do.

You're right. This is not correct. I was thinking that v4l2_pipeline_pm_use()
was called by v4l2_pipeline_link_notify(), but this is not the case. The 
function called there is pipeline_pm_use_count(), with doesn't hold the mutex.

> > -
> >  	/* Apply use count to node. */
> >  	entity->use_count += change;
> >  	WARN_ON(entity->use_count < 0);
> > @@ -359,8 +357,6 @@ int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
> >  	if (ret < 0)
> >  		entity->use_count -= change;
> >  
> > -	mutex_unlock(&mdev->graph_mutex);
> > -
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_use);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index b04cfa907350..e6ad30c323fc 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -312,7 +312,8 @@ struct media_entity_notify {
> >   * @enable_source: Enable Source Handler function pointer
> >   * @disable_source: Disable Source Handler function pointer
> >   *
> > - * @link_notify: Link state change notification callback
> > + * @link_notify: Link state change notification callback. This callback is
> > + * Called with the graph_mutex hold.  
> 
> I don't mind adding documentation such as this, but I'd put it into a
> separate patch.

Ok. Patch 2 will be just a documentation patch. I'll resend the series in
a few.


> 
> >   *
> >   * This structure represents an abstract high-level media device. It allows easy
> >   * access to entities and provides basic media device-level support. The
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 6dc9e4e8cbd4..0b16ebe36db7 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -179,6 +179,9 @@ struct media_pad {
> >   * @link_validate:	Return whether a link is valid from the entity point of
> >   *			view. The media_entity_pipeline_start() function
> >   *			validates all links by calling this operation. Optional.
> > + *
> > + * Note: Those ioctls should not touch the struct media_device.@graph_mutex
> > + * field, as they're called with it already hold.
> >   */
> >  struct media_entity_operations {
> >  	int (*link_setup)(struct media_entity *entity,  
>
Mauro Carvalho Chehab March 29, 2016, 9:28 a.m. UTC | #4
Em Tue, 29 Mar 2016 06:17:34 -0300
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > > index 6cfa890af7b4..6af5e6932271 100644
> > > --- a/drivers/media/media-device.c
> > > +++ b/drivers/media/media-device.c
> > > @@ -93,7 +93,6 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
> > >  	media_device_for_each_entity(entity, mdev) {
> > >  		if (((media_entity_id(entity) == id) && !next) ||
> > >  		    ((media_entity_id(entity) > id) && next)) {
> > > -			mutex_unlock(&mdev->graph_mutex);    
> > 
> > Unrelated to this patch.  
> 
> Yes. This belongs to patch 1.
> 
> > 
> > Please do also consider compat IOCTL handling code.
> >   

Sorry, I forgot to mention this one on my previous email.

Compat32 handling is just:

        switch (cmd) {
       	case MEDIA_IOC_ENUM_LINKS32:
               	mutex_lock(&dev->graph_mutex);
                ret = media_device_enum_links32(dev,
                                (struct media_links_enum32 __user *)arg);
               	mutex_unlock(&dev->graph_mutex);
                break;

        default:
                return media_device_ioctl(filp, cmd, arg);
        }

and media_device_enum_links32() doesn't call an function with a
mutex. So, it is safe.

Thanks,
Mauro
--
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
Sakari Ailus March 29, 2016, 9:38 a.m. UTC | #5
Hi Mauro,

On Tue, Mar 29, 2016 at 06:28:27AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 29 Mar 2016 06:17:34 -0300
> Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:
> 
> > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > > > index 6cfa890af7b4..6af5e6932271 100644
> > > > --- a/drivers/media/media-device.c
> > > > +++ b/drivers/media/media-device.c
> > > > @@ -93,7 +93,6 @@ static struct media_entity *find_entity(struct media_device *mdev, u32 id)
> > > >  	media_device_for_each_entity(entity, mdev) {
> > > >  		if (((media_entity_id(entity) == id) && !next) ||
> > > >  		    ((media_entity_id(entity) > id) && next)) {
> > > > -			mutex_unlock(&mdev->graph_mutex);    
> > > 
> > > Unrelated to this patch.  
> > 
> > Yes. This belongs to patch 1.
> > 
> > > 
> > > Please do also consider compat IOCTL handling code.
> > >   
> 
> Sorry, I forgot to mention this one on my previous email.
> 
> Compat32 handling is just:
> 
>         switch (cmd) {
>        	case MEDIA_IOC_ENUM_LINKS32:
>                	mutex_lock(&dev->graph_mutex);
>                 ret = media_device_enum_links32(dev,
>                                 (struct media_links_enum32 __user *)arg);
>                	mutex_unlock(&dev->graph_mutex);
>                 break;
> 
>         default:
>                 return media_device_ioctl(filp, cmd, arg);
>         }
> 
> and media_device_enum_links32() doesn't call an function with a
> mutex. So, it is safe.

I agree. I noticed I had some leftover cruft in my working tree...

Patch
diff mbox

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 6cfa890af7b4..6af5e6932271 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -93,7 +93,6 @@  static struct media_entity *find_entity(struct media_device *mdev, u32 id)
 	media_device_for_each_entity(entity, mdev) {
 		if (((media_entity_id(entity) == id) && !next) ||
 		    ((media_entity_id(entity) > id) && next)) {
-			mutex_unlock(&mdev->graph_mutex);
 			return entity;
 		}
 	}
diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
index 2228cd3a846e..d44ff2ec314f 100644
--- a/drivers/media/v4l2-core/v4l2-mc.c
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -348,8 +348,6 @@  int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
 	int change = use ? 1 : -1;
 	int ret;
 
-	mutex_lock(&mdev->graph_mutex);
-
 	/* Apply use count to node. */
 	entity->use_count += change;
 	WARN_ON(entity->use_count < 0);
@@ -359,8 +357,6 @@  int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
 	if (ret < 0)
 		entity->use_count -= change;
 
-	mutex_unlock(&mdev->graph_mutex);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_use);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index b04cfa907350..e6ad30c323fc 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -312,7 +312,8 @@  struct media_entity_notify {
  * @enable_source: Enable Source Handler function pointer
  * @disable_source: Disable Source Handler function pointer
  *
- * @link_notify: Link state change notification callback
+ * @link_notify: Link state change notification callback. This callback is
+ * Called with the graph_mutex hold.
  *
  * This structure represents an abstract high-level media device. It allows easy
  * access to entities and provides basic media device-level support. The
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 6dc9e4e8cbd4..0b16ebe36db7 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -179,6 +179,9 @@  struct media_pad {
  * @link_validate:	Return whether a link is valid from the entity point of
  *			view. The media_entity_pipeline_start() function
  *			validates all links by calling this operation. Optional.
+ *
+ * Note: Those ioctls should not touch the struct media_device.@graph_mutex
+ * field, as they're called with it already hold.
  */
 struct media_entity_operations {
 	int (*link_setup)(struct media_entity *entity,