Re: [PATCH v18 6/7] firmware: arm_rmm: Ensure the RMM has GPT entries for memory

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

> >>>   static int __init arm64_init_rmi(void)
> >>>   {
> >>>       int ret;
> >>> @@ -786,8 +970,24 @@ static int __init arm64_init_rmi(void)
> >>>       if (ret) {
> >>>           pr_err("RMM activate failed\n");
> >>>           ret = ret < 0 ? ret : -ENXIO;
> >>> +        return ret;  
> >>
> >> Why did this change?  
> > 
> > Rebase messed up. I will restore it.  
> 
> Actually this is not. We dont have to check the metadata if
> we couldn't activate the RMM. Also, the failure path at the
> bottom has "deactivate", which again is not needed. So
> it is the right thing to do.

Only after this patch? Not from the previous patch?

> 
> >   
> >>  
> >>>       }
> >>> +    ret = rmi_init_metadata();
> >>> +    if (ret)  
> >>
> >> And this is hitting another bit of guidance in cleanup.h.
> >> Functions shouldn't be mixing __free and friends with
> >> gotos.  Again, not a bug here but there are large ugly
> >> monsters around this stuff, hence the blanket guidance.
> >> I haven't thought that hard on how you avoid it here, but
> >> usually it's a combination of suitable helpers and wrappers
> >> and resulting code is often more readable as a result.  
> 
> I could change the hunk to something like, but that looks ugly.
> 
> @@ -1010,20 +1010,12 @@ static int __init arm64_init_rmi(void)
>                  return ret;
>          }
> 
> -       ret = rmi_init_metadata();
> -       if (ret)
> -               goto out_deactivate;
> +       if (!rmi_init_metadata() && 
> !register_memory_notifier(&rmi_memory_nb)) {
> +               arm64_rmi_is_available = true;
> +               pr_info("RMI configured\n");
> +               return 0;
> +       }
> 
> -       ret = register_memory_notifier(&rmi_memory_nb);
> -       if (ret)
> -               goto out_deactivate;
> -
> -       arm64_rmi_is_available = true;
> -       pr_info("RMI configured\n");
> -
> -       return 0;
> -
> -out_deactivate:
>          WARN_ON(rmi_sro_memxfer_cmd(sro, GFP_KERNEL, 
> SMC_RMI_RMM_DEACTIVATE));
>          return ret;
>   }
> 
> 
> Either ways, we have to cleanup the object on return, no matter
> the route we take. So the original form is much more readable
> for me.

Agree to more readable, but that fragility of mixing __free() and
goto is a real problem that has tripped many folk up - hence
the perhaps overly strict guidance.  Rather than avoiding the goto, I'd just
not use __free()  - go old school and have two labels for errors
and an extra manual free in the good path.

	pr_info("RMI configured\n:);
	kfree(sro);

	return 0;

out_deactivate:
	WARN_ON(rmi_sro_memxfer_cmd(sro, GFP_KERNEL, SMC_RMI_RMM_DEACTIVATE));
out_free_sro:
	kfree(sro);
	return ret;
}

Sometime the new toys aren't the right answer.

Jonathan




> 
> Cheers
> Suzuki
> 
> >>  
> > 
> > I will see if I can improve it.
> > 
> > Cheers
> > Suzuki
> >   
>