diff mbox series

staging: media: atomisp: Use goto instead of return in ia_css_init()

Message ID 20210415005106.530914-8-vrzh@vrzh.net (mailing list archive)
State New
Headers show
Series staging: media: atomisp: Use goto instead of return in ia_css_init() | expand

Commit Message

Martiros Shakhzadyan April 15, 2021, 12:51 a.m. UTC
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(-)

Comments

Hans Verkuil April 15, 2021, 1:56 p.m. UTC | #1
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.

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.

So I won't take this patch.

Regards,

	Hans

>  
>  	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;
>  }
>
Martiros Shakhzadyan April 15, 2021, 4:39 p.m. UTC | #2
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;
> >  }
> > 
>
diff mbox series

Patch

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;
 
 	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;
 }