diff mbox series

[v3,03/17] media: rkisp1: isp: Fix and simplify (un)registration

Message ID 20220319163100.3083-4-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: rkisp1: Misc bug fixes and cleanups | expand

Commit Message

Laurent Pinchart March 19, 2022, 4:30 p.m. UTC
The rkisp1_isp_register() and rkisp1_isp_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_isp_unregister() and
safe to be called on an unregistered isp subdev to prepare for
simplification of error handling at probe time.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 20 ++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Dafna Hirschfeld March 22, 2022, 4:23 a.m. UTC | #1
On 19.03.2022 18:30, Laurent Pinchart wrote:
> The rkisp1_isp_register() and rkisp1_isp_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_isp_unregister() and
> safe to be called on an unregistered isp subdev to prepare for
> simplification of error handling at probe time.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-isp.c     | 20 ++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index 2a35bf24e54e..f84e53b60ee1 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -1090,29 +1090,35 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1)
>  	mutex_init(&isp->ops_lock);
>  	ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads);
>  	if (ret)
> -		return ret;
> +		goto error;
>  
>  	ret = v4l2_device_register_subdev(&rkisp1->v4l2_dev, sd);
>  	if (ret) {
>  		dev_err(rkisp1->dev, "Failed to register isp subdev\n");
> -		goto err_cleanup_media_entity;
> +		goto error;
>  	}
>  
>  	rkisp1_isp_init_config(sd, &state);
> +
>  	return 0;
>  
> -err_cleanup_media_entity:
> +error:
>  	media_entity_cleanup(&sd->entity);

I remember long ago that Ezequiel suggested removing that
'media_entity_cleanup' since it was never implemented which
indicates there is probably no need for it.

> -
> +	mutex_destroy(&isp->ops_lock);
> +	isp->sd.flags = 0;
>  	return ret;
>  }
>  
>  void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
>  {
> -	struct v4l2_subdev *sd = &rkisp1->isp.sd;
> +	struct rkisp1_isp *isp = &rkisp1->isp;
>  
> -	v4l2_device_unregister_subdev(sd);
> -	media_entity_cleanup(&sd->entity);
> +	if (!isp->sd.flags)

We assume no flags means that the device is not registered. This might
stop working if we ever decide to remove the existing flags.
So better "if (!isp->sd.v4l2_dev)" ?

Thanks,
Dafna

> +		return;
> +
> +	v4l2_device_unregister_subdev(&isp->sd);
> +	media_entity_cleanup(&isp->sd.entity);
> +	mutex_destroy(&isp->ops_lock);
>  }
>  
>  /* ----------------------------------------------------------------------------
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart March 22, 2022, 9:58 a.m. UTC | #2
Hi Dafna,

On Tue, Mar 22, 2022 at 06:23:05AM +0200, Dafna Hirschfeld wrote:
> On 19.03.2022 18:30, Laurent Pinchart wrote:
> > The rkisp1_isp_register() and rkisp1_isp_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_isp_unregister() and
> > safe to be called on an unregistered isp subdev to prepare for
> > simplification of error handling at probe time.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-isp.c     | 20 ++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index 2a35bf24e54e..f84e53b60ee1 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -1090,29 +1090,35 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1)
> >  	mutex_init(&isp->ops_lock);
> >  	ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads);
> >  	if (ret)
> > -		return ret;
> > +		goto error;
> >  
> >  	ret = v4l2_device_register_subdev(&rkisp1->v4l2_dev, sd);
> >  	if (ret) {
> >  		dev_err(rkisp1->dev, "Failed to register isp subdev\n");
> > -		goto err_cleanup_media_entity;
> > +		goto error;
> >  	}
> >  
> >  	rkisp1_isp_init_config(sd, &state);
> > +
> >  	return 0;
> >  
> > -err_cleanup_media_entity:
> > +error:
> >  	media_entity_cleanup(&sd->entity);
> 
> I remember long ago that Ezequiel suggested removing that
> 'media_entity_cleanup' since it was never implemented which
> indicates there is probably no need for it.

The function is empty indeed, but I'd rather keep it. If we happen to
need to cleanup anything in the future, having to patch all drivers to
add media_entity_cleanup() calls would be very painful.

> > -
> > +	mutex_destroy(&isp->ops_lock);
> > +	isp->sd.flags = 0;
> >  	return ret;
> >  }
> >  
> >  void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
> >  {
> > -	struct v4l2_subdev *sd = &rkisp1->isp.sd;
> > +	struct rkisp1_isp *isp = &rkisp1->isp;
> >  
> > -	v4l2_device_unregister_subdev(sd);
> > -	media_entity_cleanup(&sd->entity);
> > +	if (!isp->sd.flags)
> 
> We assume no flags means that the device is not registered. This might
> stop working if we ever decide to remove the existing flags.
> So better "if (!isp->sd.v4l2_dev)" ?

Good point. I'll change that.

> > +		return;
> > +
> > +	v4l2_device_unregister_subdev(&isp->sd);
> > +	media_entity_cleanup(&isp->sd.entity);
> > +	mutex_destroy(&isp->ops_lock);
> >  }
> >  
> >  /* ----------------------------------------------------------------------------
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 2a35bf24e54e..f84e53b60ee1 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -1090,29 +1090,35 @@  int rkisp1_isp_register(struct rkisp1_device *rkisp1)
 	mutex_init(&isp->ops_lock);
 	ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads);
 	if (ret)
-		return ret;
+		goto error;
 
 	ret = v4l2_device_register_subdev(&rkisp1->v4l2_dev, sd);
 	if (ret) {
 		dev_err(rkisp1->dev, "Failed to register isp subdev\n");
-		goto err_cleanup_media_entity;
+		goto error;
 	}
 
 	rkisp1_isp_init_config(sd, &state);
+
 	return 0;
 
-err_cleanup_media_entity:
+error:
 	media_entity_cleanup(&sd->entity);
-
+	mutex_destroy(&isp->ops_lock);
+	isp->sd.flags = 0;
 	return ret;
 }
 
 void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
 {
-	struct v4l2_subdev *sd = &rkisp1->isp.sd;
+	struct rkisp1_isp *isp = &rkisp1->isp;
 
-	v4l2_device_unregister_subdev(sd);
-	media_entity_cleanup(&sd->entity);
+	if (!isp->sd.flags)
+		return;
+
+	v4l2_device_unregister_subdev(&isp->sd);
+	media_entity_cleanup(&isp->sd.entity);
+	mutex_destroy(&isp->ops_lock);
 }
 
 /* ----------------------------------------------------------------------------