| Message ID | 20240909154828.1661627-1-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: platform: video-mux: Fix mutex locking | expand |
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; > } >
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.
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.
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; }
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(-)