Re: [PATCH] staging: media: atomisp: Use goto instead of return in ia_css_init()

[Martiros Shakhzadyan <vrzh@vrzh.net>]

On Thu, Apr 15, 2021 at 03:56:35PM +0200, Hans Verkuil wrote:
> Hi Martiros,
> 
> On 15/04/2021 02:51, Martiros Shakhzadyan wrote:
> > Replace multiple return statements with goto in ia_css_init(), matching
> > other functions.
> > 
> > Signed-off-by: Martiros Shakhzadyan <vrzh@vrzh.net>
> > ---
> >  drivers/staging/media/atomisp/pci/sh_css.c | 45 +++++++++-------------
> >  1 file changed, 19 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> > index bb752d47457c..4e3ef68014ec 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> > @@ -1695,10 +1695,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> >  	my_css.flush     = flush_func;
> >  
> >  	err = ia_css_rmgr_init();
> > -	if (err) {
> > -		IA_CSS_LEAVE_ERR(err);
> > -		return err;
> > -	}
> > +	if (err)
> > +		goto ERR;
> 
> Sorry, this doesn't work.
> 
> First a style issue: goto label are typically lowercase, not uppercase.
> I do see that elsewhere in this source they use ERR as well, but that should
> really all be changed to lowercase.

Will keep that in mind as I continue to work on this file.
> But more importantly, if you look up the definition of IA_CSS_LEAVE_ERR
> you see:
> 
> #define IA_CSS_LEAVE_ERR(__err) \
>         ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, \
>                 "%s() %d: leave: return_err=%d\n", __func__, __LINE__, __err)
> 
> I.e., it is used to debug the code and print where the error is returned
> (__func__ and __LINE__). By moving this to the end, the __LINE__ is always
> the same for all 'error' cases, thus defeating the purpose of this debug line.
Makes sense. I did consider this, however the rest of the error goto
instances have already been implemented with a similar macro
(IS_CSS_LEAVE_ERR_PRIVATE) at exit, so I thought perhaps it was
acceptable. Should I update these functions accordingly (e.g.
create_host_*pipeline() series of functions)?
> So I won't take this patch.
> 
> Regards,
> 
> 	Hans
> 
Thanks for the review! -M
> >  
> >  	IA_CSS_LOG("init: %d", my_css_save_initialized);
> >  
> > @@ -1730,27 +1728,23 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> >  	gpio_reg_store(GPIO0_ID, _gpio_block_reg_do_0, 0);
> >  
> >  	err = ia_css_refcount_init(REFCOUNT_SIZE);
> > -	if (err) {
> > -		IA_CSS_LEAVE_ERR(err);
> > -		return err;
> > -	}
> > +	if (err)
> > +		goto ERR;
> > +
> >  	err = sh_css_params_init();
> > -	if (err) {
> > -		IA_CSS_LEAVE_ERR(err);
> > -		return err;
> > -	}
> > +	if (err)
> > +		goto ERR;
> > +
> >  	if (fw) {
> >  		ia_css_unload_firmware(); /* in case we already had firmware loaded */
> >  		err = sh_css_load_firmware(dev, fw->data, fw->bytes);
> > -		if (err) {
> > -			IA_CSS_LEAVE_ERR(err);
> > -			return err;
> > -		}
> > +		if (err)
> > +			goto ERR;
> > +
> >  		err = ia_css_binary_init_infos();
> > -		if (err) {
> > -			IA_CSS_LEAVE_ERR(err);
> > -			return err;
> > -		}
> > +		if (err)
> > +			goto ERR;
> > +
> >  		fw_explicitly_loaded = false;
> >  #ifndef ISP2401
> >  		my_css_save.loaded_fw = (struct ia_css_fw *)fw;
> > @@ -1760,10 +1754,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> >  		return -EINVAL;
> >  
> >  	err = ia_css_spctrl_load_fw(SP0_ID, &spctrl_cfg);
> > -	if (err) {
> > -		IA_CSS_LEAVE_ERR(err);
> > -		return err;
> > -	}
> > +	if (err)
> > +		goto ERR;
> >  
> >  #if WITH_PC_MONITORING
> >  	if (!thread_alive) {
> > @@ -1774,8 +1766,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> >  	}
> >  #endif
> >  	if (!sh_css_hrt_system_is_idle()) {
> > -		IA_CSS_LEAVE_ERR(-EBUSY);
> > -		return -EBUSY;
> > +		err = -EBUSY;
> > +		goto ERR;
> >  	}
> >  	/* can be called here, queuing works, but:
> >  	   - when sp is started later, it will wipe queued items
> > @@ -1801,6 +1793,7 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
> >  
> >  	sh_css_params_map_and_store_default_gdc_lut();
> >  
> > +ERR:
> >  	IA_CSS_LEAVE_ERR(err);
> >  	return err;
> >  }
> > 
>