| Message ID | 20220319163100.3083-6-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rkisp1: Misc bug fixes and cleanups | expand |
On 19.03.2022 18:30, Laurent Pinchart wrote: > The rkisp1_params_register() and rkisp1_params_unregister() functions > don't destroy the mutex (in the error path for the former). Fix this, > simplify error handling at registration time as media_entity_cleanup() > can be called on an uninitialized entity, and make > rkisp1_params_unregister() safe to be called on an unregistered params > node to prepare for simplification of error handling at probe time. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../platform/rockchip/rkisp1/rkisp1-params.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > index 8f62f09e635f..d41823c861ca 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > @@ -1844,16 +1844,21 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1) > node->pad.flags = MEDIA_PAD_FL_SOURCE; > ret = media_entity_pads_init(&vdev->entity, 1, &node->pad); > if (ret) > - return ret; > + goto error; > + > ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > if (ret) { > dev_err(rkisp1->dev, > "failed to register %s, ret=%d\n", vdev->name, ret); > - goto err_cleanup_media_entity; > + goto error; > } > + > return 0; > -err_cleanup_media_entity: > + > +error: > media_entity_cleanup(&vdev->entity); > + mutex_destroy(&node->vlock); > + params->rkisp1 = NULL; > return ret; > } > > @@ -1863,6 +1868,10 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1) > struct rkisp1_vdev_node *node = ¶ms->vnode; > struct video_device *vdev = &node->vdev; > > + if (!params->rkisp1) > + return; > + For the capture devices, you did: if (!video_is_registered(&cap->vnode.vdev)) return; Better be consistend and do the same here and for stats? Thanks, Dafna > vb2_video_unregister_device(vdev); > media_entity_cleanup(&vdev->entity); > + mutex_destroy(&node->vlock); > } > -- > Regards, > > Laurent Pinchart >
Hi Dafna, On Tue, Mar 22, 2022 at 06:27:26AM +0200, Dafna Hirschfeld wrote: > On 19.03.2022 18:30, Laurent Pinchart wrote: > > The rkisp1_params_register() and rkisp1_params_unregister() functions > > don't destroy the mutex (in the error path for the former). Fix this, > > simplify error handling at registration time as media_entity_cleanup() > > can be called on an uninitialized entity, and make > > rkisp1_params_unregister() safe to be called on an unregistered params > > node to prepare for simplification of error handling at probe time. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../platform/rockchip/rkisp1/rkisp1-params.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > index 8f62f09e635f..d41823c861ca 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > @@ -1844,16 +1844,21 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1) > > node->pad.flags = MEDIA_PAD_FL_SOURCE; > > ret = media_entity_pads_init(&vdev->entity, 1, &node->pad); > > if (ret) > > - return ret; > > + goto error; > > + > > ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > > if (ret) { > > dev_err(rkisp1->dev, > > "failed to register %s, ret=%d\n", vdev->name, ret); > > - goto err_cleanup_media_entity; > > + goto error; > > } > > + > > return 0; > > -err_cleanup_media_entity: > > + > > +error: > > media_entity_cleanup(&vdev->entity); > > + mutex_destroy(&node->vlock); > > + params->rkisp1 = NULL; > > return ret; > > } > > > > @@ -1863,6 +1868,10 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1) > > struct rkisp1_vdev_node *node = ¶ms->vnode; > > struct video_device *vdev = &node->vdev; > > > > + if (!params->rkisp1) > > + return; > > + > > For the capture devices, you did: > if (!video_is_registered(&cap->vnode.vdev)) > return; > Better be consistend and do the same here and for stats? I'll do that. > > vb2_video_unregister_device(vdev); > > media_entity_cleanup(&vdev->entity); > > + mutex_destroy(&node->vlock); > > }
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c index 8f62f09e635f..d41823c861ca 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c @@ -1844,16 +1844,21 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1) node->pad.flags = MEDIA_PAD_FL_SOURCE; ret = media_entity_pads_init(&vdev->entity, 1, &node->pad); if (ret) - return ret; + goto error; + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); if (ret) { dev_err(rkisp1->dev, "failed to register %s, ret=%d\n", vdev->name, ret); - goto err_cleanup_media_entity; + goto error; } + return 0; -err_cleanup_media_entity: + +error: media_entity_cleanup(&vdev->entity); + mutex_destroy(&node->vlock); + params->rkisp1 = NULL; return ret; } @@ -1863,6 +1868,10 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1) struct rkisp1_vdev_node *node = ¶ms->vnode; struct video_device *vdev = &node->vdev; + if (!params->rkisp1) + return; + vb2_video_unregister_device(vdev); media_entity_cleanup(&vdev->entity); + mutex_destroy(&node->vlock); }
The rkisp1_params_register() and rkisp1_params_unregister() functions don't destroy the mutex (in the error path for the former). Fix this, simplify error handling at registration time as media_entity_cleanup() can be called on an uninitialized entity, and make rkisp1_params_unregister() safe to be called on an unregistered params node to prepare for simplification of error handling at probe time. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../platform/rockchip/rkisp1/rkisp1-params.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)