| Message ID | 20230918145850.241074-4-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: Clean up local variable shadowing | expand |
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);
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);
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); > > >
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); >> >> >>
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); > >
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);
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(-)