media: platform: video-mux: Fix mutex locking

[PATCH] media: platform: video-mux: Fix mutex locking

Posted by Paul Elder 1 year, 5 months ago

The current order of locking between the driver mutex and the v4l2
subdev state lock causes a circuluar locking dependency when trying to
set up a link. Fix this.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 drivers/media/platform/video-mux.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
index 31e9e92e723eb..f43849db56800 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -52,6 +52,7 @@ static int video_mux_link_setup(struct media_entity *entity,
 				const struct media_pad *remote, u32 flags)
 {
 	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct v4l2_subdev_state *sd_state;
 	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
 	u16 source_pad = entity->num_pads - 1;
 	int ret = 0;
@@ -67,10 +68,10 @@ static int video_mux_link_setup(struct media_entity *entity,
 		remote->entity->name, remote->index, local->entity->name,
 		local->index, flags & MEDIA_LNK_FL_ENABLED);
 
+	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
 	mutex_lock(&vmux->lock);
 
 	if (flags & MEDIA_LNK_FL_ENABLED) {
-		struct v4l2_subdev_state *sd_state;
 		struct v4l2_mbus_framefmt *source_mbusformat;
 
 		if (vmux->active == local->index)
@@ -88,12 +89,10 @@ static int video_mux_link_setup(struct media_entity *entity,
 		vmux->active = local->index;
 
 		/* Propagate the active format to the source */
-		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
 		source_mbusformat = v4l2_subdev_state_get_format(sd_state,
 								 source_pad);
 		*source_mbusformat = *v4l2_subdev_state_get_format(sd_state,
 								   vmux->active);
-		v4l2_subdev_unlock_state(sd_state);
 	} else {
 		if (vmux->active != local->index)
 			goto out;
@@ -105,6 +104,7 @@ static int video_mux_link_setup(struct media_entity *entity,
 
 out:
 	mutex_unlock(&vmux->lock);
+	v4l2_subdev_unlock_state(sd_state);
 	return ret;
 }
 
-- 
2.39.2

Re: [PATCH] media: platform: video-mux: Fix mutex locking

Posted by Laurent Pinchart 1 year, 5 months ago

Hi Paul,

Thank you for the patch.

On Mon, Sep 09, 2024 at 05:48:28PM +0200, Paul Elder wrote:
> The current order of locking between the driver mutex and the v4l2
> subdev state lock causes a circuluar locking dependency when trying to
> set up a link. Fix this.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This being said, I think we should deprecate the video-mux driver and
implement a driver that uses the V4L2 subdev internal routing API
instead of basing the routing configuration on link setup. Any opinion ?

> ---
>  drivers/media/platform/video-mux.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> index 31e9e92e723eb..f43849db56800 100644
> --- a/drivers/media/platform/video-mux.c
> +++ b/drivers/media/platform/video-mux.c
> @@ -52,6 +52,7 @@ static int video_mux_link_setup(struct media_entity *entity,
>  				const struct media_pad *remote, u32 flags)
>  {
>  	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct v4l2_subdev_state *sd_state;
>  	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>  	u16 source_pad = entity->num_pads - 1;
>  	int ret = 0;
> @@ -67,10 +68,10 @@ static int video_mux_link_setup(struct media_entity *entity,
>  		remote->entity->name, remote->index, local->entity->name,
>  		local->index, flags & MEDIA_LNK_FL_ENABLED);
>  
> +	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
>  	mutex_lock(&vmux->lock);
>  
>  	if (flags & MEDIA_LNK_FL_ENABLED) {
> -		struct v4l2_subdev_state *sd_state;
>  		struct v4l2_mbus_framefmt *source_mbusformat;
>  
>  		if (vmux->active == local->index)
> @@ -88,12 +89,10 @@ static int video_mux_link_setup(struct media_entity *entity,
>  		vmux->active = local->index;
>  
>  		/* Propagate the active format to the source */
> -		sd_state = v4l2_subdev_lock_and_get_active_state(sd);
>  		source_mbusformat = v4l2_subdev_state_get_format(sd_state,
>  								 source_pad);
>  		*source_mbusformat = *v4l2_subdev_state_get_format(sd_state,
>  								   vmux->active);
> -		v4l2_subdev_unlock_state(sd_state);
>  	} else {
>  		if (vmux->active != local->index)
>  			goto out;
> @@ -105,6 +104,7 @@ static int video_mux_link_setup(struct media_entity *entity,
>  
>  out:
>  	mutex_unlock(&vmux->lock);
> +	v4l2_subdev_unlock_state(sd_state);
>  	return ret;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: platform: video-mux: Fix mutex locking

Posted by Sakari Ailus 1 year, 4 months ago

Hi Laurent,

On Mon, Sep 09, 2024 at 06:56:03PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Sep 09, 2024 at 05:48:28PM +0200, Paul Elder wrote:
> > The current order of locking between the driver mutex and the v4l2
> > subdev state lock causes a circuluar locking dependency when trying to
> > set up a link. Fix this.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> This being said, I think we should deprecate the video-mux driver and
> implement a driver that uses the V4L2 subdev internal routing API
> instead of basing the routing configuration on link setup. Any opinion ?

Sounds good to me. But do we need a new driver? The subdev client streams
capability flag should help here, just as it does on other drivers.

I applied this one, with spelling fixed in the commit message.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: platform: video-mux: Fix mutex locking

Posted by Laurent Pinchart 1 year, 4 months ago

On Thu, Oct 10, 2024 at 08:45:04AM +0000, Sakari Ailus wrote:
> On Mon, Sep 09, 2024 at 06:56:03PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 09, 2024 at 05:48:28PM +0200, Paul Elder wrote:
> > > The current order of locking between the driver mutex and the v4l2
> > > subdev state lock causes a circuluar locking dependency when trying to
> > > set up a link. Fix this.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > This being said, I think we should deprecate the video-mux driver and
> > implement a driver that uses the V4L2 subdev internal routing API
> > instead of basing the routing configuration on link setup. Any opinion ?
> 
> Sounds good to me. But do we need a new driver? The subdev client streams
> capability flag should help here, just as it does on other drivers.

Good point, we could switch the driver behaviour depending on client
capabilities. I like this !

> I applied this one, with spelling fixed in the commit message.

-- 
Regards,

Laurent Pinchart