Re: [PATCH 3/8] spapr: Clean up local variable shadowing in spapr_dt_cpus()

[Harsh Prateek Bora <harsh.prateek.bora@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3873 bytes --]

On Tue, 19 Sept, 2023, 6:34 pm Harsh Prateek Bora, <
harsh.prateek.bora@gmail.com> wrote:

>
>
> On Tue, 19 Sept, 2023, 4:37 pm Cédric Le Goater, <clg@kaod.org> wrote:
>
>> On 9/19/23 09:28, Harsh Prateek Bora wrote:
>> >
>> >
>> > On 9/18/23 20:28, Cédric Le Goater wrote:
>> >> Introduce a helper routine defining one CPU device node to fix this
>> >> warning :
>> >>
>> >>    ../hw/ppc/spapr.c: In function ‘spapr_dt_cpus’:
>> >>    ../hw/ppc/spapr.c:812:19: warning: declaration of ‘cs’ shadows a
>> previous local [-Wshadow=compatible-local]
>> >>      812 |         CPUState *cs = rev[i];
>> >>          |                   ^~
>> >>    ../hw/ppc/spapr.c:786:15: note: shadowed declaration is here
>> >>      786 |     CPUState *cs;
>> >>          |               ^~
>> >>
>> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> >> ---
>> >>   hw/ppc/spapr.c | 36 +++++++++++++++++++++---------------
>> >>   1 file changed, 21 insertions(+), 15 deletions(-)
>> >>
>> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> >> index de3c616b4637..d89f0fd496b6 100644
>> >> --- a/hw/ppc/spapr.c
>> >> +++ b/hw/ppc/spapr.c
>> >> @@ -780,6 +780,26 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt,
>> int offset,
>> >>                                 pcc->lrg_decr_bits)));
>> >>   }
>> >> +static void spapr_dt_one_cpu(void *fdt, SpaprMachineState *spapr,
>> CPUState *cs,
>> >> +                             int cpus_offset)
>> >> +{
>> >> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> >> +    int index = spapr_get_vcpu_id(cpu);
>> >> +    DeviceClass *dc = DEVICE_GET_CLASS(cs);
>> >> +    g_autofree char *nodename = NULL;
>> >> +    int offset;
>> >> +
>> >> +    if (!spapr_is_thread0_in_vcore(spapr, cpu)) {
>> >> +        return;
>> >> +    }
>> >> +
>> >> +    nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
>> >> +    offset = fdt_add_subnode(fdt, cpus_offset, nodename);
>> >> +    _FDT(offset);
>> >> +    spapr_dt_cpu(cs, fdt, offset, spapr);
>> >> +}
>> >> +
>> >> +
>> >>   static void spapr_dt_cpus(void *fdt, SpaprMachineState *spapr)
>> >>   {
>> >>       CPUState **rev;
>> >> @@ -809,21 +829,7 @@ static void spapr_dt_cpus(void *fdt,
>> SpaprMachineState *spapr)
>> >>       }
>> >>       for (i = n_cpus - 1; i >= 0; i--) {
>> >> -        CPUState *cs = rev[i];
>> >> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
>> >> -        int index = spapr_get_vcpu_id(cpu);
>> >> -        DeviceClass *dc = DEVICE_GET_CLASS(cs);
>> >> -        g_autofree char *nodename = NULL;
>> >> -        int offset;
>> >> -
>> >> -        if (!spapr_is_thread0_in_vcore(spapr, cpu)) {
>> >> -            continue;
>> >> -        }
>> >> -
>> >> -        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
>> >> -        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
>> >> -        _FDT(offset);
>> >> -        spapr_dt_cpu(cs, fdt, offset, spapr);
>> >> +        spapr_dt_one_cpu(fdt, spapr, rev[i], cpus_offset);
>> >
>> > Do we want to replace the call to spapr_dt_cpu in
>> > spapr_core_dt_populate() with the _one_ as well?
>> > Not sure about the implication of additional instructions there.
>>
>> yeah may be we could rework spapr_dt_one_cpu() and
>> spapr_core_dt_populate()
>> in a single routine. They are similar. It can come later.
>>
>> > Also, could this code insider wrapper become part of spapr_dt_cpu()
>> itself?
>> > If can't, do we want a better renaming of wrapper here?
>>
>> I am open to proposal :)
>>
>
> How about spapr_dt_cpu_prepare() ?
>

I guess spapr_dt_cpu_process() may be more apt since it calls the
spapr_dt_cpu internally.


>
>> Thanks
>>
>> C.
>>
>>
>> >
>> > Otherwise,
>> > Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> >
>> >>       }
>> >>       g_free(rev);
>>
>>
>>

[-- Attachment #2: Type: text/html, Size: 5999 bytes --]