| Message ID | 20220304171925.1592-3-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rkisp1: Misc bug fixes and cleanups | expand |
Hi Laurent On Fri, Mar 04, 2022 at 07:19:10PM +0200, Laurent Pinchart wrote: > The rkisp1_register_capture() and rkisp1_unregister_capture() functions > don't destroy the mutex (in the error path for the former). Fix this and > make rkisp1_unregister_capture() and rkisp1_capture_devs_unregister() > safe to be called on an unregistered capture node to prepare for > simplification of error handling at probe time. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Reset cap->rkisp1 in rkisp1_capture_devs_register() > --- > .../platform/rockchip/rkisp1/rkisp1-capture.c | 26 ++++++++++--------- > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > index 9c11f2b8e5f5..96a7e6c0a622 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > @@ -1312,8 +1312,12 @@ static const struct v4l2_file_operations rkisp1_fops = { > > static void rkisp1_unregister_capture(struct rkisp1_capture *cap) > { > + if (!cap->rkisp1) > + return; > + > media_entity_cleanup(&cap->vnode.vdev.entity); > vb2_video_unregister_device(&cap->vnode.vdev); > + mutex_destroy(&cap->vnode.vlock); > } > > void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1) > @@ -1390,6 +1394,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap) > > error: > media_entity_cleanup(&vdev->entity); > + mutex_destroy(&node->vlock); > return ret; > } > > @@ -1425,26 +1430,23 @@ rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id) > > int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1) > { > - struct rkisp1_capture *cap; > - unsigned int i, j; > + unsigned int i; > int ret; > > for (i = 0; i < ARRAY_SIZE(rkisp1->capture_devs); i++) { > + struct rkisp1_capture *cap = &rkisp1->capture_devs[i]; > + > rkisp1_capture_init(rkisp1, i); > - cap = &rkisp1->capture_devs[i]; > cap->rkisp1 = rkisp1; This is set by rkisp1_capture_init() already > + > ret = rkisp1_register_capture(cap); > - if (ret) > - goto err_unreg_capture_devs; > + if (ret) { > + cap->rkisp1 = NULL; Is this correct ? As: > + rkisp1_capture_devs_unregister(rkisp1); Calls rkisp1_capture_devs_unregister(): rkisp1_unregister_capture(): if (!cap->rkisp1) return 0; Effectively making rkisp1_capture_devs_unregister() a nop. Thanks j > + return ret; > + } > } > > return 0; > > -err_unreg_capture_devs: > - for (j = 0; j < i; j++) { > - cap = &rkisp1->capture_devs[j]; > - rkisp1_unregister_capture(cap); > - } > - > - return ret; > } > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Mar 07, 2022 at 12:45:30PM +0100, Jacopo Mondi wrote: > On Fri, Mar 04, 2022 at 07:19:10PM +0200, Laurent Pinchart wrote: > > The rkisp1_register_capture() and rkisp1_unregister_capture() functions > > don't destroy the mutex (in the error path for the former). Fix this and > > make rkisp1_unregister_capture() and rkisp1_capture_devs_unregister() > > safe to be called on an unregistered capture node to prepare for > > simplification of error handling at probe time. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Reset cap->rkisp1 in rkisp1_capture_devs_register() > > --- > > .../platform/rockchip/rkisp1/rkisp1-capture.c | 26 ++++++++++--------- > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > index 9c11f2b8e5f5..96a7e6c0a622 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c > > @@ -1312,8 +1312,12 @@ static const struct v4l2_file_operations rkisp1_fops = { > > > > static void rkisp1_unregister_capture(struct rkisp1_capture *cap) > > { > > + if (!cap->rkisp1) > > + return; > > + > > media_entity_cleanup(&cap->vnode.vdev.entity); > > vb2_video_unregister_device(&cap->vnode.vdev); > > + mutex_destroy(&cap->vnode.vlock); > > } > > > > void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1) > > @@ -1390,6 +1394,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap) > > > > error: > > media_entity_cleanup(&vdev->entity); > > + mutex_destroy(&node->vlock); > > return ret; > > } > > > > @@ -1425,26 +1430,23 @@ rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id) > > > > int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1) > > { > > - struct rkisp1_capture *cap; > > - unsigned int i, j; > > + unsigned int i; > > int ret; > > > > for (i = 0; i < ARRAY_SIZE(rkisp1->capture_devs); i++) { > > + struct rkisp1_capture *cap = &rkisp1->capture_devs[i]; > > + > > rkisp1_capture_init(rkisp1, i); > > - cap = &rkisp1->capture_devs[i]; > > cap->rkisp1 = rkisp1; > > This is set by rkisp1_capture_init() already This isn't introduced by this patch, but indeed, I can remove it here. > > + > > ret = rkisp1_register_capture(cap); > > - if (ret) > > - goto err_unreg_capture_devs; > > + if (ret) { > > + cap->rkisp1 = NULL; > > Is this correct ? > > As: > > + rkisp1_capture_devs_unregister(rkisp1); > > Calls > rkisp1_capture_devs_unregister(): > rkisp1_unregister_capture(): > if (!cap->rkisp1) > return 0; > > Effectively making rkisp1_capture_devs_unregister() a nop. It will be a nop for the rkisp1_capture that fails to register here (which shouldn't be an issue as rkisp1_register_capture() cleans up internally when failing), but will still call rkisp1_unregister_capture() on any rkisp1_capture previously registered successfully (in practice that would be the MP only). > > + return ret; > > + } > > } > > > > return 0; > > > > -err_unreg_capture_devs: > > - for (j = 0; j < i; j++) { > > - cap = &rkisp1->capture_devs[j]; > > - rkisp1_unregister_capture(cap); > > - } > > - > > - return ret; > > }
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c index 9c11f2b8e5f5..96a7e6c0a622 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c @@ -1312,8 +1312,12 @@ static const struct v4l2_file_operations rkisp1_fops = { static void rkisp1_unregister_capture(struct rkisp1_capture *cap) { + if (!cap->rkisp1) + return; + media_entity_cleanup(&cap->vnode.vdev.entity); vb2_video_unregister_device(&cap->vnode.vdev); + mutex_destroy(&cap->vnode.vlock); } void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1) @@ -1390,6 +1394,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap) error: media_entity_cleanup(&vdev->entity); + mutex_destroy(&node->vlock); return ret; } @@ -1425,26 +1430,23 @@ rkisp1_capture_init(struct rkisp1_device *rkisp1, enum rkisp1_stream_id id) int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1) { - struct rkisp1_capture *cap; - unsigned int i, j; + unsigned int i; int ret; for (i = 0; i < ARRAY_SIZE(rkisp1->capture_devs); i++) { + struct rkisp1_capture *cap = &rkisp1->capture_devs[i]; + rkisp1_capture_init(rkisp1, i); - cap = &rkisp1->capture_devs[i]; cap->rkisp1 = rkisp1; + ret = rkisp1_register_capture(cap); - if (ret) - goto err_unreg_capture_devs; + if (ret) { + cap->rkisp1 = NULL; + rkisp1_capture_devs_unregister(rkisp1); + return ret; + } } return 0; -err_unreg_capture_devs: - for (j = 0; j < i; j++) { - cap = &rkisp1->capture_devs[j]; - rkisp1_unregister_capture(cap); - } - - return ret; }
The rkisp1_register_capture() and rkisp1_unregister_capture() functions don't destroy the mutex (in the error path for the former). Fix this and make rkisp1_unregister_capture() and rkisp1_capture_devs_unregister() safe to be called on an unregistered capture node to prepare for simplification of error handling at probe time. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Reset cap->rkisp1 in rkisp1_capture_devs_register() --- .../platform/rockchip/rkisp1/rkisp1-capture.c | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-)