| Message ID | 20210415005106.530914-8-vrzh@vrzh.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: media: atomisp: Use goto instead of return in ia_css_init() | expand |
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; > } >
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 --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; }
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(-)