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