Re: [PATCH 3/7] drivers: firmware: psci: Split psci_dt_cpu_init_idle()

[Ulf Hansson]

On Thu, 28 Feb 2019 at 15:42, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 28/02/2019 14:59, Ulf Hansson wrote:
> > Let's split the psci_dt_cpu_init_idle() function into two functions. This
> > makes the code clearer and provides better re-usability.
> >
> > Cc: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> > Co-developed-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
> > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>
> Acked-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>
> but one question below.
>
>
> > ---
> >  drivers/firmware/psci/psci.c | 42 ++++++++++++++++++++----------------
> >  1 file changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index c80ec1d03274..9788bfc1cf8b 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -270,9 +270,26 @@ static int __init psci_features(u32 psci_func_id)
> >  #ifdef CONFIG_CPU_IDLE
> >  static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> >
> > +static int psci_dt_parse_state_node(struct device_node *np, u32 *state)
> > +{
> > +     int err = of_property_read_u32(np, "arm,psci-suspend-param", state);
> > +
> > +     if (err) {
> > +             pr_warn("%pOF missing arm,psci-suspend-param property\n", np);
> > +             return err;
> > +     }
> > +
> > +     if (!psci_power_state_is_valid(*state)) {
> > +             pr_warn("Invalid PSCI power state %#x\n", *state);
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
> >  {
> > -     int i, ret, count = 0;
> > +     int i, ret = 0, count = 0;
>
> Why do you need to intialize the ret variable? If the count is zero we
> go directly to return 0, otherwise we enter in the loop and ret is
> affected with the new function call.

Depending on the compiler and the compiler flags, one could otherwise
potentially get a warning about using an uninitialized variable at the
free_mem label (not shown in the patch).

So, I just wanted to play safe.

Thanks a lot for reviewing!

Kind regards
Uffe