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

Cédric Le Goater posted 8 patches 7 months ago
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Nicholas Piggin <npiggin@gmail.com>, "Frédéric Barrat" <fbarrat@linux.ibm.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>
[PATCH 3/8] spapr: Clean up local variable shadowing in spapr_dt_cpus()
Posted by Cédric Le Goater 7 months ago
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);
     }
 
     g_free(rev);
-- 
2.41.0


Re: [PATCH 3/8] spapr: Clean up local variable shadowing in spapr_dt_cpus()
Posted by Harsh Prateek Bora 7 months ago

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.

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?

Otherwise,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

>       }
>   
>       g_free(rev);

Re: [PATCH 3/8] spapr: Clean up local variable shadowing in spapr_dt_cpus()
Posted by Cédric Le Goater 7 months ago
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 :)

Thanks

C.

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


Re: [PATCH 3/8] spapr: Clean up local variable shadowing in spapr_dt_cpus()
Posted by Harsh Prateek Bora 7 months ago
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() ?


> Thanks
>
> C.
>
>
> >
> > Otherwise,
> > Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> >
> >>       }
> >>       g_free(rev);
>
>
>
Re: [PATCH 3/8] spapr: Clean up local variable shadowing in spapr_dt_cpus()
Posted by Harsh Prateek Bora 7 months ago
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);
>>
>>
>>
Re: [PATCH 3/8] spapr: Clean up local variable shadowing in spapr_dt_cpus()
Posted by Cédric Le Goater 7 months ago
On 9/19/23 16:59, Harsh Prateek Bora wrote:
> 
> 
> On Tue, 19 Sept, 2023, 6:34 pm Harsh Prateek Bora, <harsh.prateek.bora@gmail.com <mailto:harsh.prateek.bora@gmail.com>> wrote:
> 
> 
> 
>     On Tue, 19 Sept, 2023, 4:37 pm Cédric Le Goater, <clg@kaod.org <mailto: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 <mailto: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.

*_one_* is a common qualifier in QEMU and Linux routine names.

C.

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