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

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

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);
> >  }
> >  
> >  /* ----------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip