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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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