[PATCH 0/8] ppc: Clean up local variable shadowing

[PATCH 0/8] ppc: Clean up local variable shadowing

[Cédric Le Goater]

Hello,

Here is a first round of cleanups for local variable shadowing
warrnings.

Thanks,

C. 

Cédric Le Goater (8):
  hw/ppc: Clean up local variable shadowing in _FDT helper routine
  pnv/psi: Clean up local variable shadowing
  spapr: Clean up local variable shadowing in spapr_dt_cpus()
  spapr: Clean up local variable shadowing in spapr_init_cpus()
  spapr: Clean up local variable shadowing in spapr_get_fw_dev_path()
  spapr/drc: Clean up local variable shadowing in
    rtas_ibm_configure_connector()
  spapr/pci: Clean up local variable shadowing in spapr_phb_realize()
  spapr/drc: Clean up local variable shadowing in prop_get_fdt()

 include/hw/ppc/fdt.h |  8 ++++----
 hw/ppc/pnv_psi.c     |  5 +++--
 hw/ppc/spapr.c       | 42 +++++++++++++++++++++++-------------------
 hw/ppc/spapr_drc.c   | 12 +++++-------
 hw/ppc/spapr_pci.c   |  6 +++---
 5 files changed, 38 insertions(+), 35 deletions(-)

-- 
2.41.0

[PATCH 1/8] hw/ppc: Clean up local variable shadowing in _FDT helper routine

[Cédric Le Goater]

this fixes numerous warnings of this type :

  In file included from ../hw/ppc/spapr_pci.c:43:
  ../hw/ppc/spapr_pci.c: In function ‘spapr_dt_phb’:
  ../include/hw/ppc/fdt.h:18:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=compatible-local]
     18 |         int ret = (exp);                                           \
        |             ^~~
  ../hw/ppc/spapr_pci.c:2355:5: note: in expansion of macro ‘_FDT’
   2355 |     _FDT(bus_off = fdt_add_subnode(fdt, 0, phb->dtbusname));
        |     ^~~~
  ../hw/ppc/spapr_pci.c:2311:24: note: shadowed declaration is here
   2311 |     int bus_off, i, j, ret;
        |                        ^~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/fdt.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/ppc/fdt.h b/include/hw/ppc/fdt.h
index a8cd85069fe0..b56ac2a8cbb5 100644
--- a/include/hw/ppc/fdt.h
+++ b/include/hw/ppc/fdt.h
@@ -15,10 +15,10 @@
 
 #define _FDT(exp)                                                  \
     do {                                                           \
-        int ret = (exp);                                           \
-        if (ret < 0) {                                             \
-            error_report("error creating device tree: %s: %s",   \
-                    #exp, fdt_strerror(ret));                      \
+        int _ret = (exp);                                          \
+        if (_ret < 0) {                                            \
+            error_report("error creating device tree: %s: %s",     \
+                    #exp, fdt_strerror(_ret));                     \
             exit(1);                                               \
         }                                                          \
     } while (0)
-- 
2.41.0

[PATCH 2/8] pnv/psi: Clean up local variable shadowing

[Cédric Le Goater]

to fix :

  ../hw/ppc/pnv_psi.c: In function ‘pnv_psi_p9_mmio_write’:
  ../hw/ppc/pnv_psi.c:741:24: warning: declaration of ‘addr’ shadows a parameter [-Wshadow=compatible-local]
    741 |                 hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
        |                        ^~~~
  ../hw/ppc/pnv_psi.c:702:56: note: shadowed declaration is here
    702 | static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
        |                                                 ~~~~~~~^~~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_psi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index daaa2f0575fd..26460d210deb 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -738,8 +738,9 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
             }
         } else {
             if (!(psi->regs[reg] & PSIHB9_ESB_CI_VALID)) {
-                hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
-                memory_region_add_subregion(sysmem, addr,
+                hwaddr esb_addr =
+                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
+                memory_region_add_subregion(sysmem, esb_addr,
                                             &psi9->source.esb_mmio);
             }
         }
-- 
2.41.0

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

[Cédric Le Goater]

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

[PATCH 4/8] spapr: Clean up local variable shadowing in spapr_init_cpus()

[Cédric Le Goater]

Remove extra 'i' variable to fix this warning :

  ../hw/ppc/spapr.c: In function ‘spapr_init_cpus’:
  ../hw/ppc/spapr.c:2668:13: warning: declaration of ‘i’ shadows a previous local [-Wshadow=compatible-local]
   2668 |         int i;
        |             ^
  ../hw/ppc/spapr.c:2645:9: note: shadowed declaration is here
   2645 |     int i;
        |         ^

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d89f0fd496b6..41ce7de77c14 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2665,8 +2665,6 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
     }
 
     if (smc->pre_2_10_has_unused_icps) {
-        int i;
-
         for (i = 0; i < spapr_max_server_number(spapr); i++) {
             /* Dummy entries get deregistered when real ICPState objects
              * are registered during CPU core hotplug.
-- 
2.41.0

[PATCH 5/8] spapr: Clean up local variable shadowing in spapr_get_fw_dev_path()

[Cédric Le Goater]

Rename PCIDevice variable to avoid this warning :

  ../hw/ppc/spapr.c: In function ‘spapr_get_fw_dev_path’:
  ../hw/ppc/spapr.c:3217:20: warning: declaration of ‘pcidev’ shadows a previous local [-Wshadow=compatible-local]
   3217 |         PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
        |                    ^~~~~~
  ../hw/ppc/spapr.c:3147:16: note: shadowed declaration is here
   3147 |     PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
        |                ^~~~~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 41ce7de77c14..8090fb0302df 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3214,8 +3214,8 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
 
     if (g_str_equal("pci-bridge", qdev_fw_name(dev))) {
         /* SLOF uses "pci" instead of "pci-bridge" for PCI bridges */
-        PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
-        return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn));
+        PCIDevice *pdev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
+        return g_strdup_printf("pci@%x", PCI_SLOT(pdev->devfn));
     }
 
     if (pcidev) {
-- 
2.41.0

[PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()

[Cédric Le Goater]

Remove extra 'drc_index' variable to avoid this warning :

  ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
  ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’ shadows a previous local [-Wshadow=compatible-local]
   1240 |                 uint32_t drc_index = spapr_drc_index(drc);
        |                          ^~~~~~~~~
  ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
   1155 |     uint32_t drc_index;
        |              ^~~~~~~~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_drc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index b5c400a94d1c..843e318312d3 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -1237,8 +1237,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
         case FDT_END_NODE:
             drc->ccs_depth--;
             if (drc->ccs_depth == 0) {
-                uint32_t drc_index = spapr_drc_index(drc);
-
                 /* done sending the device tree, move to configured state */
                 trace_spapr_drc_set_configured(drc_index);
                 drc->state = drck->ready_state;
-- 
2.41.0

[PATCH 7/8] spapr/pci: Clean up local variable shadowing in spapr_phb_realize()

[Cédric Le Goater]

Rename SysBusDevice variable to avoid this warning :

  ../hw/ppc/spapr_pci.c: In function ‘spapr_phb_realize’:
  ../hw/ppc/spapr_pci.c:1872:24: warning: declaration of ‘s’ shadows a previous local [-Wshadow=local]
   1872 |         SpaprPhbState *s;
        |                        ^
  ../hw/ppc/spapr_pci.c:1829:19: note: shadowed declaration is here
   1829 |     SysBusDevice *s = SYS_BUS_DEVICE(dev);
        |                   ^

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_pci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ce1495931744..370c5a90f218 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1826,9 +1826,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
                                                   TYPE_SPAPR_MACHINE);
     SpaprMachineClass *smc = spapr ? SPAPR_MACHINE_GET_CLASS(spapr) : NULL;
-    SysBusDevice *s = SYS_BUS_DEVICE(dev);
-    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
-    PCIHostState *phb = PCI_HOST_BRIDGE(s);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(sbd);
+    PCIHostState *phb = PCI_HOST_BRIDGE(sbd);
     MachineState *ms = MACHINE(spapr);
     char *namebuf;
     int i;
-- 
2.41.0

[PATCH 8/8] spapr/drc: Clean up local variable shadowing in prop_get_fdt()

[Cédric Le Goater]

Rename 'name' variable to avoid this warning :

  ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
  ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ shadows a parameter [-Wshadow=compatible-local]
    344 |         const char *name = NULL;
        |                     ^~~~
  ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
    325 | static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
        |                                                   ~~~~~~~~~~~~^~~~

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_drc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 843e318312d3..2b99d3b4b1a6 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -341,7 +341,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
     fdt_depth = 0;
 
     do {
-        const char *name = NULL;
+        const char *dt_name = NULL;
         const struct fdt_property *prop = NULL;
         int prop_len = 0, name_len = 0;
         uint32_t tag;
@@ -351,8 +351,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
         switch (tag) {
         case FDT_BEGIN_NODE:
             fdt_depth++;
-            name = fdt_get_name(fdt, fdt_offset, &name_len);
-            if (!visit_start_struct(v, name, NULL, 0, errp)) {
+            dt_name = fdt_get_name(fdt, fdt_offset, &name_len);
+            if (!visit_start_struct(v, dt_name, NULL, 0, errp)) {
                 return;
             }
             break;
@@ -369,8 +369,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
         case FDT_PROP: {
             int i;
             prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
-            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
-            if (!visit_start_list(v, name, NULL, 0, errp)) {
+            dt_name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
+            if (!visit_start_list(v, dt_name, NULL, 0, errp)) {
                 return;
             }
             for (i = 0; i < prop_len; i++) {
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

On 18/9/23 16:58, Cédric Le Goater wrote:
> Rename PCIDevice variable to avoid this warning :
> 
>    ../hw/ppc/spapr.c: In function ‘spapr_get_fw_dev_path’:
>    ../hw/ppc/spapr.c:3217:20: warning: declaration of ‘pcidev’ shadows a previous local [-Wshadow=compatible-local]
>     3217 |         PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>          |                    ^~~~~~
>    ../hw/ppc/spapr.c:3147:16: note: shadowed declaration is here
>     3147 |     PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>          |                ^~~~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/spapr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

On 18/9/23 16:58, Cédric Le Goater wrote:
> Rename 'name' variable to avoid this warning :
> 
>    ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
>    ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ shadows a parameter [-Wshadow=compatible-local]
>      344 |         const char *name = NULL;
>          |                     ^~~~
>    ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
>      325 | static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>          |                                                   ~~~~~~~~~~~~^~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/spapr_drc.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/8] hw/ppc: Clean up local variable shadowing in _FDT helper routine

[Harsh Prateek Bora]

On 9/18/23 20:28, Cédric Le Goater wrote:
> this fixes numerous warnings of this type :
> 
>    In file included from ../hw/ppc/spapr_pci.c:43:
>    ../hw/ppc/spapr_pci.c: In function ‘spapr_dt_phb’:
>    ../include/hw/ppc/fdt.h:18:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=compatible-local]
>       18 |         int ret = (exp);                                           \
>          |             ^~~
>    ../hw/ppc/spapr_pci.c:2355:5: note: in expansion of macro ‘_FDT’
>     2355 |     _FDT(bus_off = fdt_add_subnode(fdt, 0, phb->dtbusname));
>          |     ^~~~
>    ../hw/ppc/spapr_pci.c:2311:24: note: shadowed declaration is here
>     2311 |     int bus_off, i, j, ret;
>          |                        ^~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

> ---
>   include/hw/ppc/fdt.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/ppc/fdt.h b/include/hw/ppc/fdt.h
> index a8cd85069fe0..b56ac2a8cbb5 100644
> --- a/include/hw/ppc/fdt.h
> +++ b/include/hw/ppc/fdt.h
> @@ -15,10 +15,10 @@
>   
>   #define _FDT(exp)                                                  \
>       do {                                                           \
> -        int ret = (exp);                                           \
> -        if (ret < 0) {                                             \
> -            error_report("error creating device tree: %s: %s",   \
> -                    #exp, fdt_strerror(ret));                      \
> +        int _ret = (exp);                                          \
> +        if (_ret < 0) {                                            \
> +            error_report("error creating device tree: %s: %s",     \
> +                    #exp, fdt_strerror(_ret));                     \
>               exit(1);                                               \
>           }                                                          \
>       } while (0)

Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing

[Harsh Prateek Bora]

On 9/18/23 20:28, Cédric Le Goater wrote:
> to fix :
> 
>    ../hw/ppc/pnv_psi.c: In function ‘pnv_psi_p9_mmio_write’:
>    ../hw/ppc/pnv_psi.c:741:24: warning: declaration of ‘addr’ shadows a parameter [-Wshadow=compatible-local]
>      741 |                 hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>          |                        ^~~~
>    ../hw/ppc/pnv_psi.c:702:56: note: shadowed declaration is here
>      702 | static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>          |                                                 ~~~~~~~^~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/pnv_psi.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index daaa2f0575fd..26460d210deb 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -738,8 +738,9 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>               }
>           } else {
>               if (!(psi->regs[reg] & PSIHB9_ESB_CI_VALID)) {
> -                hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
> -                memory_region_add_subregion(sysmem, addr,
> +                hwaddr esb_addr =

While at it, we may want to move the declaration to the beginning of the 
function. Anyways,

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

> +                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
> +                memory_region_add_subregion(sysmem, esb_addr,
>                                               &psi9->source.esb_mmio);
>               }
>           }

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

[Harsh Prateek Bora]

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 4/8] spapr: Clean up local variable shadowing in spapr_init_cpus()

[Harsh Prateek Bora]

On 9/18/23 20:28, Cédric Le Goater wrote:
> Remove extra 'i' variable to fix this warning :
> 
>    ../hw/ppc/spapr.c: In function ‘spapr_init_cpus’:
>    ../hw/ppc/spapr.c:2668:13: warning: declaration of ‘i’ shadows a previous local [-Wshadow=compatible-local]
>     2668 |         int i;
>          |             ^
>    ../hw/ppc/spapr.c:2645:9: note: shadowed declaration is here
>     2645 |     int i;
>          |         ^
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

> ---
>   hw/ppc/spapr.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d89f0fd496b6..41ce7de77c14 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2665,8 +2665,6 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
>       }
>   
>       if (smc->pre_2_10_has_unused_icps) {
> -        int i;
> -
>           for (i = 0; i < spapr_max_server_number(spapr); i++) {
>               /* Dummy entries get deregistered when real ICPState objects
>                * are registered during CPU core hotplug.

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

[Harsh Prateek Bora]

On 9/18/23 20:28, Cédric Le Goater wrote:
> Rename PCIDevice variable to avoid this warning :
> 
>    ../hw/ppc/spapr.c: In function ‘spapr_get_fw_dev_path’:
>    ../hw/ppc/spapr.c:3217:20: warning: declaration of ‘pcidev’ shadows a previous local [-Wshadow=compatible-local]
>     3217 |         PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>          |                    ^~~~~~
>    ../hw/ppc/spapr.c:3147:16: note: shadowed declaration is here
>     3147 |     PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>          |                ^~~~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/spapr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 41ce7de77c14..8090fb0302df 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3214,8 +3214,8 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
>   
>       if (g_str_equal("pci-bridge", qdev_fw_name(dev))) {
>           /* SLOF uses "pci" instead of "pci-bridge" for PCI bridges */
> -        PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
> -        return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn));
> +        PCIDevice *pdev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
> +        return g_strdup_printf("pci@%x", PCI_SLOT(pdev->devfn));

Instead of renaming, can't we use pcidev itself without re-declaring ?

>       }
>   
>       if (pcidev) {

Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()

[Harsh Prateek Bora]

On 9/18/23 20:28, Cédric Le Goater wrote:
> Remove extra 'drc_index' variable to avoid this warning :
> 
>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’ shadows a previous local [-Wshadow=compatible-local]
>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>          |                          ^~~~~~~~~
>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>     1155 |     uint32_t drc_index;
>          |              ^~~~~~~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/spapr_drc.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index b5c400a94d1c..843e318312d3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -1237,8 +1237,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>           case FDT_END_NODE:
>               drc->ccs_depth--;
>               if (drc->ccs_depth == 0) {
> -                uint32_t drc_index = spapr_drc_index(drc);
> -
I guess you only wanted to remove re-declaration part. Assigning the 
value returned by this function doesnt seem to happen before.

>                   /* done sending the device tree, move to configured state */
>                   trace_spapr_drc_set_configured(drc_index);
>                   drc->state = drck->ready_state;

Re: [PATCH 7/8] spapr/pci: Clean up local variable shadowing in spapr_phb_realize()

[Harsh Prateek Bora]

On 9/18/23 20:28, Cédric Le Goater wrote:
> Rename SysBusDevice variable to avoid this warning :
> 
>    ../hw/ppc/spapr_pci.c: In function ‘spapr_phb_realize’:
>    ../hw/ppc/spapr_pci.c:1872:24: warning: declaration of ‘s’ shadows a previous local [-Wshadow=local]
>     1872 |         SpaprPhbState *s;
>          |                        ^
>    ../hw/ppc/spapr_pci.c:1829:19: note: shadowed declaration is here
>     1829 |     SysBusDevice *s = SYS_BUS_DEVICE(dev);
>          |                   ^
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/spapr_pci.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index ce1495931744..370c5a90f218 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1826,9 +1826,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>           (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
>                                                     TYPE_SPAPR_MACHINE);
>       SpaprMachineClass *smc = spapr ? SPAPR_MACHINE_GET_CLASS(spapr) : NULL;
> -    SysBusDevice *s = SYS_BUS_DEVICE(dev);
> -    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(sbd);

Declaration of SpaprPhbState *s later in the code could be brought here?

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

> +    PCIHostState *phb = PCI_HOST_BRIDGE(sbd);
>       MachineState *ms = MACHINE(spapr);
>       char *namebuf;
>       int i;

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

[Harsh Prateek Bora]

On 9/18/23 20:28, Cédric Le Goater wrote:
> Rename 'name' variable to avoid this warning :
> 
>    ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
>    ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ shadows a parameter [-Wshadow=compatible-local]
>      344 |         const char *name = NULL;
>          |                     ^~~~
>    ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
>      325 | static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>          |                                                   ~~~~~~~~~~~~^~~~
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/spapr_drc.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 843e318312d3..2b99d3b4b1a6 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -341,7 +341,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>       fdt_depth = 0;
>   
>       do {
> -        const char *name = NULL;
> +        const char *dt_name = NULL;

I guess you wanted to use the input arg "name" here without 
re-declaration. I do not see "name" being used elsewhere in this routine.

regards,
Harsh
>           const struct fdt_property *prop = NULL;
>           int prop_len = 0, name_len = 0;
>           uint32_t tag;
> @@ -351,8 +351,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>           switch (tag) {
>           case FDT_BEGIN_NODE:
>               fdt_depth++;
> -            name = fdt_get_name(fdt, fdt_offset, &name_len);
> -            if (!visit_start_struct(v, name, NULL, 0, errp)) {
> +            dt_name = fdt_get_name(fdt, fdt_offset, &name_len);
> +            if (!visit_start_struct(v, dt_name, NULL, 0, errp)) {
>                   return;
>               }
>               break;
> @@ -369,8 +369,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>           case FDT_PROP: {
>               int i;
>               prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> -            if (!visit_start_list(v, name, NULL, 0, errp)) {
> +            dt_name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> +            if (!visit_start_list(v, dt_name, NULL, 0, errp)) {
>                   return;
>               }
>               for (i = 0; i < prop_len; i++) {

Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing

[Cédric Le Goater]

On 9/19/23 08:57, Harsh Prateek Bora wrote:
> 
> 
> On 9/18/23 20:28, Cédric Le Goater wrote:
>> to fix :
>>
>>    ../hw/ppc/pnv_psi.c: In function ‘pnv_psi_p9_mmio_write’:
>>    ../hw/ppc/pnv_psi.c:741:24: warning: declaration of ‘addr’ shadows a parameter [-Wshadow=compatible-local]
>>      741 |                 hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>>          |                        ^~~~
>>    ../hw/ppc/pnv_psi.c:702:56: note: shadowed declaration is here
>>      702 | static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>>          |                                                 ~~~~~~~^~~~
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/ppc/pnv_psi.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
>> index daaa2f0575fd..26460d210deb 100644
>> --- a/hw/ppc/pnv_psi.c
>> +++ b/hw/ppc/pnv_psi.c
>> @@ -738,8 +738,9 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>>               }
>>           } else {
>>               if (!(psi->regs[reg] & PSIHB9_ESB_CI_VALID)) {
>> -                hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>> -                memory_region_add_subregion(sysmem, addr,
>> +                hwaddr esb_addr =
> 
> While at it, we may want to move the declaration to the beginning of the function. 

I am more in favor of declaring the variables where they are needed.
I think it is better pratice since it identifies a functional block
which could be move in a external routine at some point if it becomes
too complex.

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


Thanks,

C.


> 
>> +                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>> +                memory_region_add_subregion(sysmem, esb_addr,
>>                                               &psi9->source.esb_mmio);
>>               }
>>           }

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

[Cédric Le Goater]

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 5/8] spapr: Clean up local variable shadowing in spapr_get_fw_dev_path()

[Cédric Le Goater]

On 9/19/23 10:23, Harsh Prateek Bora wrote:
> 
> 
> On 9/18/23 20:28, Cédric Le Goater wrote:
>> Rename PCIDevice variable to avoid this warning :
>>
>>    ../hw/ppc/spapr.c: In function ‘spapr_get_fw_dev_path’:
>>    ../hw/ppc/spapr.c:3217:20: warning: declaration of ‘pcidev’ shadows a previous local [-Wshadow=compatible-local]
>>     3217 |         PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>>          |                    ^~~~~~
>>    ../hw/ppc/spapr.c:3147:16: note: shadowed declaration is here
>>     3147 |     PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>>          |                ^~~~~~
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/ppc/spapr.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 41ce7de77c14..8090fb0302df 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3214,8 +3214,8 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus,
>>       if (g_str_equal("pci-bridge", qdev_fw_name(dev))) {
>>           /* SLOF uses "pci" instead of "pci-bridge" for PCI bridges */
>> -        PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>> -        return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn));
>> +        PCIDevice *pdev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
>> +        return g_strdup_printf("pci@%x", PCI_SLOT(pdev->devfn));
> 
> Instead of renaming, can't we use pcidev itself without re-declaring ?

I agree this could be done. But there is this test below :
  
> 
>>       }
>>       if (pcidev) {

which makes use of the same variable. I would rather keep the same
logic in the code or rewrite the routine completely with something
like:

     if (object_dynamic_cast(OBJECT(dev), TYPE_1 {
         /* return this */
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_2 {
         /* or that */
     } else if (object_dynamic_cast(OBJECT(dev), "some string"))
     ....
     } else {
         error_report("...");
     }

     return NULL;


This is beyond the issue that this patch is trying to fix.

Thanks,

C.

Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()

[Cédric Le Goater]

On 9/19/23 10:29, Harsh Prateek Bora wrote:
> 
> 
> On 9/18/23 20:28, Cédric Le Goater wrote:
>> Remove extra 'drc_index' variable to avoid this warning :
>>
>>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’ shadows a previous local [-Wshadow=compatible-local]
>>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>>          |                          ^~~~~~~~~
>>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>>     1155 |     uint32_t drc_index;
>>          |              ^~~~~~~~~
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/ppc/spapr_drc.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index b5c400a94d1c..843e318312d3 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -1237,8 +1237,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
>>           case FDT_END_NODE:
>>               drc->ccs_depth--;
>>               if (drc->ccs_depth == 0) {
>> -                uint32_t drc_index = spapr_drc_index(drc);
>> -
> I guess you only wanted to remove re-declaration part. Assigning the value returned by this function doesnt seem to happen before.

drc_index is assigned at the top of this large routine with :

     drc_index = rtas_ld(wa_addr, 0);
     drc = spapr_drc_by_index(drc_index);


So, the extra local variable 'drc_index' is simply redundant because
there are no reason for it to change. The drc object is the same AFAICT.
Correct ? I should have explained that better in the commit log.

Thanks,

C.


> 
>>                   /* done sending the device tree, move to configured state */
>>                   trace_spapr_drc_set_configured(drc_index);
>>                   drc->state = drck->ready_state;

Re: [PATCH 7/8] spapr/pci: Clean up local variable shadowing in spapr_phb_realize()

[Cédric Le Goater]

On 9/19/23 10:38, Harsh Prateek Bora wrote:
> 
> 
> On 9/18/23 20:28, Cédric Le Goater wrote:
>> Rename SysBusDevice variable to avoid this warning :
>>
>>    ../hw/ppc/spapr_pci.c: In function ‘spapr_phb_realize’:
>>    ../hw/ppc/spapr_pci.c:1872:24: warning: declaration of ‘s’ shadows a previous local [-Wshadow=local]
>>     1872 |         SpaprPhbState *s;
>>          |                        ^
>>    ../hw/ppc/spapr_pci.c:1829:19: note: shadowed declaration is here
>>     1829 |     SysBusDevice *s = SYS_BUS_DEVICE(dev);
>>          |                   ^
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/ppc/spapr_pci.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index ce1495931744..370c5a90f218 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1826,9 +1826,9 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>>           (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
>>                                                     TYPE_SPAPR_MACHINE);
>>       SpaprMachineClass *smc = spapr ? SPAPR_MACHINE_GET_CLASS(spapr) : NULL;
>> -    SysBusDevice *s = SYS_BUS_DEVICE(dev);
>> -    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(sbd);
> 
> Declaration of SpaprPhbState *s later in the code could be brought here?

nah. 's' is really local. It could be even called 'tmp' IMO.

Thanks,

C.


> 
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> 
>> +    PCIHostState *phb = PCI_HOST_BRIDGE(sbd);
>>       MachineState *ms = MACHINE(spapr);
>>       char *namebuf;
>>       int i;

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

[Cédric Le Goater]

On 9/19/23 10:48, Harsh Prateek Bora wrote:
> 
> 
> On 9/18/23 20:28, Cédric Le Goater wrote:
>> Rename 'name' variable to avoid this warning :
>>
>>    ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
>>    ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ shadows a parameter [-Wshadow=compatible-local]
>>      344 |         const char *name = NULL;
>>          |                     ^~~~
>>    ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
>>      325 | static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>>          |                                                   ~~~~~~~~~~~~^~~~
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/ppc/spapr_drc.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 843e318312d3..2b99d3b4b1a6 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -341,7 +341,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>>       fdt_depth = 0;
>>       do {
>> -        const char *name = NULL;
>> +        const char *dt_name = NULL;
> 
> I guess you wanted to use the input arg "name" here without re-declaration. 

I don't understand. I don't want to use the input arg "name" here.
It seems useless in this case.

C.

> I do not see "name" being used elsewhere in this routine.
> 
> regards,
> Harsh
>>           const struct fdt_property *prop = NULL;
>>           int prop_len = 0, name_len = 0;
>>           uint32_t tag;
>> @@ -351,8 +351,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>>           switch (tag) {
>>           case FDT_BEGIN_NODE:
>>               fdt_depth++;
>> -            name = fdt_get_name(fdt, fdt_offset, &name_len);
>> -            if (!visit_start_struct(v, name, NULL, 0, errp)) {
>> +            dt_name = fdt_get_name(fdt, fdt_offset, &name_len);
>> +            if (!visit_start_struct(v, dt_name, NULL, 0, errp)) {
>>                   return;
>>               }
>>               break;
>> @@ -369,8 +369,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>>           case FDT_PROP: {
>>               int i;
>>               prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
>> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>> -            if (!visit_start_list(v, name, NULL, 0, errp)) {
>> +            dt_name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>> +            if (!visit_start_list(v, dt_name, NULL, 0, errp)) {
>>                   return;
>>               }
>>               for (i = 0; i < prop_len; i++) {

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

[Harsh Prateek Bora]

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

On Tue, 19 Sept, 2023, 5:39 pm Cédric Le Goater, <clg@kaod.org> wrote:

> On 9/19/23 10:48, Harsh Prateek Bora wrote:
> >
> >
> > On 9/18/23 20:28, Cédric Le Goater wrote:
> >> Rename 'name' variable to avoid this warning :
> >>
> >>    ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
> >>    ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ shadows
> a parameter [-Wshadow=compatible-local]
> >>      344 |         const char *name = NULL;
> >>          |                     ^~~~
> >>    ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
> >>      325 | static void prop_get_fdt(Object *obj, Visitor *v, const char
> *name,
> >>          |
> ~~~~~~~~~~~~^~~~
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>   hw/ppc/spapr_drc.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >> index 843e318312d3..2b99d3b4b1a6 100644
> >> --- a/hw/ppc/spapr_drc.c
> >> +++ b/hw/ppc/spapr_drc.c
> >> @@ -341,7 +341,7 @@ static void prop_get_fdt(Object *obj, Visitor *v,
> const char *name,
> >>       fdt_depth = 0;
> >>       do {
> >> -        const char *name = NULL;
> >> +        const char *dt_name = NULL;
> >
> > I guess you wanted to use the input arg "name" here without
> re-declaration.
>
> I don't understand. I don't want to use the input arg "name" here.
> It seems useless in this case.
>

Yeh, I realize now. This patch can actually remove the unused arg "name" as
well?

C.
>
> > I do not see "name" being used elsewhere in this routine.
> >
> > regards,
> > Harsh
> >>           const struct fdt_property *prop = NULL;
> >>           int prop_len = 0, name_len = 0;
> >>           uint32_t tag;
> >> @@ -351,8 +351,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
> const char *name,
> >>           switch (tag) {
> >>           case FDT_BEGIN_NODE:
> >>               fdt_depth++;
> >> -            name = fdt_get_name(fdt, fdt_offset, &name_len);
> >> -            if (!visit_start_struct(v, name, NULL, 0, errp)) {
> >> +            dt_name = fdt_get_name(fdt, fdt_offset, &name_len);
> >> +            if (!visit_start_struct(v, dt_name, NULL, 0, errp)) {
> >>                   return;
> >>               }
> >>               break;
> >> @@ -369,8 +369,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
> const char *name,
> >>           case FDT_PROP: {
> >>               int i;
> >>               prop = fdt_get_property_by_offset(fdt, fdt_offset,
> &prop_len);
> >> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> >> -            if (!visit_start_list(v, name, NULL, 0, errp)) {
> >> +            dt_name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> >> +            if (!visit_start_list(v, dt_name, NULL, 0, errp)) {
> >>                   return;
> >>               }
> >>               for (i = 0; i < prop_len; i++) {
>
>
>

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

Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()

[Harsh Prateek Bora]

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

On Tue, 19 Sept, 2023, 5:33 pm Cédric Le Goater, <clg@kaod.org> wrote:

> On 9/19/23 10:29, Harsh Prateek Bora wrote:
> >
> >
> > On 9/18/23 20:28, Cédric Le Goater wrote:
> >> Remove extra 'drc_index' variable to avoid this warning :
> >>
> >>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
> >>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’
> shadows a previous local [-Wshadow=compatible-local]
> >>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
> >>          |                          ^~~~~~~~~
> >>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
> >>     1155 |     uint32_t drc_index;
> >>          |              ^~~~~~~~~
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>   hw/ppc/spapr_drc.c | 2 --
> >>   1 file changed, 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >> index b5c400a94d1c..843e318312d3 100644
> >> --- a/hw/ppc/spapr_drc.c
> >> +++ b/hw/ppc/spapr_drc.c
> >> @@ -1237,8 +1237,6 @@ static void
> rtas_ibm_configure_connector(PowerPCCPU *cpu,
> >>           case FDT_END_NODE:
> >>               drc->ccs_depth--;
> >>               if (drc->ccs_depth == 0) {
> >> -                uint32_t drc_index = spapr_drc_index(drc);
> >> -
> > I guess you only wanted to remove re-declaration part. Assigning the
> value returned by this function doesnt seem to happen before.
>
> drc_index is assigned at the top of this large routine with :
>
>      drc_index = rtas_ld(wa_addr, 0);
>      drc = spapr_drc_by_index(drc_index);
>
>
> So, the extra local variable 'drc_index' is simply redundant because
> there are no reason for it to change. The drc object is the same AFAICT.
> Correct ? I should have explained that better in the commit log.
>

Okay, since both routines were implemented differently, I wasn't sure about
the impact of reassignment. Better commit log is always welcome.

Regards
Harsh

Thanks,
>
> C.
>
>
> >
> >>                   /* done sending the device tree, move to configured
> state */
> >>                   trace_spapr_drc_set_configured(drc_index);
> >>                   drc->state = drck->ready_state;
>
>
>

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

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

[Harsh Prateek Bora]

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

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);
>
>
>

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

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

[Harsh Prateek Bora]

[-- 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 --]

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

[Cédric Le Goater]

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);
> 
> 

Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 9/19/23 08:57, Harsh Prateek Bora wrote:
>> On 9/18/23 20:28, Cédric Le Goater wrote:
>>> to fix :
>>>
>>>    ../hw/ppc/pnv_psi.c: In function ‘pnv_psi_p9_mmio_write’:
>>>    ../hw/ppc/pnv_psi.c:741:24: warning: declaration of ‘addr’ shadows a parameter [-Wshadow=compatible-local]
>>>      741 |                 hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>>>          |                        ^~~~
>>>    ../hw/ppc/pnv_psi.c:702:56: note: shadowed declaration is here
>>>      702 | static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>>>          |                                                 ~~~~~~~^~~~
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   hw/ppc/pnv_psi.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
>>> index daaa2f0575fd..26460d210deb 100644
>>> --- a/hw/ppc/pnv_psi.c
>>> +++ b/hw/ppc/pnv_psi.c
>>> @@ -738,8 +738,9 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>>>               }
>>>           } else {
>>>               if (!(psi->regs[reg] & PSIHB9_ESB_CI_VALID)) {
>>> -                hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>>> -                memory_region_add_subregion(sysmem, addr,
>>> +                hwaddr esb_addr =
>>
>> While at it, we may want to move the declaration to the beginning of the function. 
>
> I am more in favor of declaring the variables where they are needed.
> I think it is better pratice since it identifies a functional block
> which could be move in a external routine at some point if it becomes
> too complex.

I'm old-fashioned and prefer my declarations in one place, where I can
find them easily, except perhaps for for-loop counters.  I suspect when
a function is big enough so that moving declarations to inner blocks
improves it, factoring out these inner blocks would improve it more.

My other reason is the risk for accidental shadowing, but us enabling
-Wshadow=local will render that point moot.

Maintainers get to decide such matters of taste, and I'm not the
maintainer here :)

>> Anyways,
>> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
>
> Thanks,
>
> C.
>
>
>> 
>>> +                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>>> +                memory_region_add_subregion(sysmem, esb_addr,
>>>                                               &psi9->source.esb_mmio);
>>>               }
>>>           }

Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()

[Markus Armbruster]

Harsh Prateek Bora <harsh.prateek.bora@gmail.com> writes:

> On Tue, 19 Sept, 2023, 5:33 pm Cédric Le Goater, <clg@kaod.org> wrote:
>
>> On 9/19/23 10:29, Harsh Prateek Bora wrote:
>> >
>> >
>> > On 9/18/23 20:28, Cédric Le Goater wrote:
>> >> Remove extra 'drc_index' variable to avoid this warning :
>> >>
>> >>    ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>> >>    ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’
>> shadows a previous local [-Wshadow=compatible-local]
>> >>     1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>> >>          |                          ^~~~~~~~~
>> >>    ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>> >>     1155 |     uint32_t drc_index;
>> >>          |              ^~~~~~~~~
>> >>
>> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> >> ---
>> >>   hw/ppc/spapr_drc.c | 2 --
>> >>   1 file changed, 2 deletions(-)
>> >>
>> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> >> index b5c400a94d1c..843e318312d3 100644
>> >> --- a/hw/ppc/spapr_drc.c
>> >> +++ b/hw/ppc/spapr_drc.c
>> >> @@ -1237,8 +1237,6 @@ static void
>> rtas_ibm_configure_connector(PowerPCCPU *cpu,
>> >>           case FDT_END_NODE:
>> >>               drc->ccs_depth--;
>> >>               if (drc->ccs_depth == 0) {
>> >> -                uint32_t drc_index = spapr_drc_index(drc);
>> >> -
>> > I guess you only wanted to remove re-declaration part. Assigning the
>>
>> value returned by this function doesnt seem to happen before.
>>
>> drc_index is assigned at the top of this large routine with :
>>
>>      drc_index = rtas_ld(wa_addr, 0);
>>      drc = spapr_drc_by_index(drc_index);
>>
>>
>> So, the extra local variable 'drc_index' is simply redundant because
>> there are no reason for it to change. The drc object is the same AFAICT.
>> Correct ? I should have explained that better in the commit log.
>>
>
> Okay, since both routines were implemented differently, I wasn't sure about
> the impact of reassignment. Better commit log is always welcome.

Do you expect a respin?  If not, would you like to give your R-by?

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

[Markus Armbruster]

Harsh Prateek Bora <harsh.prateek.bora@gmail.com> writes:

> On Tue, 19 Sept, 2023, 5:39 pm Cédric Le Goater, <clg@kaod.org> wrote:
>
>> On 9/19/23 10:48, Harsh Prateek Bora wrote:
>> >
>> >
>> > On 9/18/23 20:28, Cédric Le Goater wrote:
>> >> Rename 'name' variable to avoid this warning :
>> >>
>> >>    ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
>> >>    ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ shadows
>> a parameter [-Wshadow=compatible-local]
>> >>      344 |         const char *name = NULL;
>> >>          |                     ^~~~
>> >>    ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
>> >>      325 | static void prop_get_fdt(Object *obj, Visitor *v, const char
>> *name,
>> >>          |
>> ~~~~~~~~~~~~^~~~
>> >>
>> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> >> ---
>> >>   hw/ppc/spapr_drc.c | 10 +++++-----
>> >>   1 file changed, 5 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> >> index 843e318312d3..2b99d3b4b1a6 100644
>> >> --- a/hw/ppc/spapr_drc.c
>> >> +++ b/hw/ppc/spapr_drc.c
>> >> @@ -341,7 +341,7 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>> const char *name,
>> >>       fdt_depth = 0;
>> >>       do {
>> >> -        const char *name = NULL;
>> >> +        const char *dt_name = NULL;
>> >
>> > I guess you wanted to use the input arg "name" here without
>> re-declaration.
>>
>> I don't understand. I don't want to use the input arg "name" here.
>> It seems useless in this case.
>>
>
> Yeh, I realize now. This patch can actually remove the unused arg "name" as
> well?

Cédric?

Lose ends like this one make me reluctant to queue a series, even when
they look minor to me.

>> C.
>>
>> > I do not see "name" being used elsewhere in this routine.
>> >
>> > regards,
>> > Harsh
>> >>           const struct fdt_property *prop = NULL;
>> >>           int prop_len = 0, name_len = 0;
>> >>           uint32_t tag;
>> >> @@ -351,8 +351,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>> const char *name,
>> >>           switch (tag) {
>> >>           case FDT_BEGIN_NODE:
>> >>               fdt_depth++;
>> >> -            name = fdt_get_name(fdt, fdt_offset, &name_len);
>> >> -            if (!visit_start_struct(v, name, NULL, 0, errp)) {
>> >> +            dt_name = fdt_get_name(fdt, fdt_offset, &name_len);
>> >> +            if (!visit_start_struct(v, dt_name, NULL, 0, errp)) {
>> >>                   return;
>> >>               }
>> >>               break;
>> >> @@ -369,8 +369,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>> const char *name,
>> >>           case FDT_PROP: {
>> >>               int i;
>> >>               prop = fdt_get_property_by_offset(fdt, fdt_offset,
>> &prop_len);
>> >> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>> >> -            if (!visit_start_list(v, name, NULL, 0, errp)) {
>> >> +            dt_name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>> >> +            if (!visit_start_list(v, dt_name, NULL, 0, errp)) {
>> >>                   return;
>> >>               }
>> >>               for (i = 0; i < prop_len; i++) {

Re: [PATCH 6/8] spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector()

[Harsh Prateek Bora]

On 9/29/23 11:04, Markus Armbruster wrote:
> Harsh Prateek Bora <harsh.prateek.bora@gmail.com> writes:
> 
>> On Tue, 19 Sept, 2023, 5:33 pm Cédric Le Goater, <clg@kaod.org> wrote:
>>
>>> On 9/19/23 10:29, Harsh Prateek Bora wrote:
>>>>
>>>>
>>>> On 9/18/23 20:28, Cédric Le Goater wrote:
>>>>> Remove extra 'drc_index' variable to avoid this warning :
>>>>>
>>>>>     ../hw/ppc/spapr_drc.c: In function ‘rtas_ibm_configure_connector’:
>>>>>     ../hw/ppc/spapr_drc.c:1240:26: warning: declaration of ‘drc_index’
>>> shadows a previous local [-Wshadow=compatible-local]
>>>>>      1240 |                 uint32_t drc_index = spapr_drc_index(drc);
>>>>>           |                          ^~~~~~~~~
>>>>>     ../hw/ppc/spapr_drc.c:1155:14: note: shadowed declaration is here
>>>>>      1155 |     uint32_t drc_index;
>>>>>           |              ^~~~~~~~~
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>    hw/ppc/spapr_drc.c | 2 --
>>>>>    1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>>>>> index b5c400a94d1c..843e318312d3 100644
>>>>> --- a/hw/ppc/spapr_drc.c
>>>>> +++ b/hw/ppc/spapr_drc.c
>>>>> @@ -1237,8 +1237,6 @@ static void
>>> rtas_ibm_configure_connector(PowerPCCPU *cpu,
>>>>>            case FDT_END_NODE:
>>>>>                drc->ccs_depth--;
>>>>>                if (drc->ccs_depth == 0) {
>>>>> -                uint32_t drc_index = spapr_drc_index(drc);
>>>>> -
>>>> I guess you only wanted to remove re-declaration part. Assigning the
>>>
>>> value returned by this function doesnt seem to happen before.
>>>
>>> drc_index is assigned at the top of this large routine with :
>>>
>>>       drc_index = rtas_ld(wa_addr, 0);
>>>       drc = spapr_drc_by_index(drc_index);
>>>
>>>
>>> So, the extra local variable 'drc_index' is simply redundant because
>>> there are no reason for it to change. The drc object is the same AFAICT.
>>> Correct ? I should have explained that better in the commit log.
>>>
>>
>> Okay, since both routines were implemented differently, I wasn't sure about
>> the impact of reassignment. Better commit log is always welcome.
> 
> Do you expect a respin?  If not, would you like to give your R-by?
> 
Oh sure,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

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

[Cédric Le Goater]

On 9/29/23 07:39, Markus Armbruster wrote:
> Harsh Prateek Bora <harsh.prateek.bora@gmail.com> writes:
> 
>> On Tue, 19 Sept, 2023, 5:39 pm Cédric Le Goater, <clg@kaod.org> wrote:
>>
>>> On 9/19/23 10:48, Harsh Prateek Bora wrote:
>>>>
>>>>
>>>> On 9/18/23 20:28, Cédric Le Goater wrote:
>>>>> Rename 'name' variable to avoid this warning :
>>>>>
>>>>>     ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
>>>>>     ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ shadows
>>> a parameter [-Wshadow=compatible-local]
>>>>>       344 |         const char *name = NULL;
>>>>>           |                     ^~~~
>>>>>     ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
>>>>>       325 | static void prop_get_fdt(Object *obj, Visitor *v, const char
>>> *name,
>>>>>           |
>>> ~~~~~~~~~~~~^~~~
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>    hw/ppc/spapr_drc.c | 10 +++++-----
>>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>>>>> index 843e318312d3..2b99d3b4b1a6 100644
>>>>> --- a/hw/ppc/spapr_drc.c
>>>>> +++ b/hw/ppc/spapr_drc.c
>>>>> @@ -341,7 +341,7 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>>> const char *name,
>>>>>        fdt_depth = 0;
>>>>>        do {
>>>>> -        const char *name = NULL;
>>>>> +        const char *dt_name = NULL;
>>>>
>>>> I guess you wanted to use the input arg "name" here without
>>> re-declaration.
>>>
>>> I don't understand. I don't want to use the input arg "name" here.
>>> It seems useless in this case.
>>>
>>
>> Yeh, I realize now. This patch can actually remove the unused arg "name" as
>> well?
> 
> Cédric?
> 
> Lose ends like this one make me reluctant to queue a series, even when
> they look minor to me.

Unfortunately, we can not remove the unused arg "name" from the prototype.
The routine is a ObjectPropertyAccessor argument of object_property_add().

Thanks,

C.


> 
>>> C.
>>>
>>>> I do not see "name" being used elsewhere in this routine.
>>>>
>>>> regards,
>>>> Harsh
>>>>>            const struct fdt_property *prop = NULL;
>>>>>            int prop_len = 0, name_len = 0;
>>>>>            uint32_t tag;
>>>>> @@ -351,8 +351,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>>> const char *name,
>>>>>            switch (tag) {
>>>>>            case FDT_BEGIN_NODE:
>>>>>                fdt_depth++;
>>>>> -            name = fdt_get_name(fdt, fdt_offset, &name_len);
>>>>> -            if (!visit_start_struct(v, name, NULL, 0, errp)) {
>>>>> +            dt_name = fdt_get_name(fdt, fdt_offset, &name_len);
>>>>> +            if (!visit_start_struct(v, dt_name, NULL, 0, errp)) {
>>>>>                    return;
>>>>>                }
>>>>>                break;
>>>>> @@ -369,8 +369,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>>> const char *name,
>>>>>            case FDT_PROP: {
>>>>>                int i;
>>>>>                prop = fdt_get_property_by_offset(fdt, fdt_offset,
>>> &prop_len);
>>>>> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>>>>> -            if (!visit_start_list(v, name, NULL, 0, errp)) {
>>>>> +            dt_name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>>>>> +            if (!visit_start_list(v, dt_name, NULL, 0, errp)) {
>>>>>                    return;
>>>>>                }
>>>>>                for (i = 0; i < prop_len; i++) {
> 

Re: [PATCH 2/8] pnv/psi: Clean up local variable shadowing

[Cédric Le Goater]

On 9/29/23 07:30, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> On 9/19/23 08:57, Harsh Prateek Bora wrote:
>>> On 9/18/23 20:28, Cédric Le Goater wrote:
>>>> to fix :
>>>>
>>>>     ../hw/ppc/pnv_psi.c: In function ‘pnv_psi_p9_mmio_write’:
>>>>     ../hw/ppc/pnv_psi.c:741:24: warning: declaration of ‘addr’ shadows a parameter [-Wshadow=compatible-local]
>>>>       741 |                 hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>>>>           |                        ^~~~
>>>>     ../hw/ppc/pnv_psi.c:702:56: note: shadowed declaration is here
>>>>       702 | static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>>>>           |                                                 ~~~~~~~^~~~
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>    hw/ppc/pnv_psi.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
>>>> index daaa2f0575fd..26460d210deb 100644
>>>> --- a/hw/ppc/pnv_psi.c
>>>> +++ b/hw/ppc/pnv_psi.c
>>>> @@ -738,8 +738,9 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
>>>>                }
>>>>            } else {
>>>>                if (!(psi->regs[reg] & PSIHB9_ESB_CI_VALID)) {
>>>> -                hwaddr addr = val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>>>> -                memory_region_add_subregion(sysmem, addr,
>>>> +                hwaddr esb_addr =
>>>
>>> While at it, we may want to move the declaration to the beginning of the function.
>>
>> I am more in favor of declaring the variables where they are needed.
>> I think it is better pratice since it identifies a functional block
>> which could be move in a external routine at some point if it becomes
>> too complex.
> 
> I'm old-fashioned and prefer my declarations in one place, where I can
> find them easily, except perhaps for for-loop counters.  

This is also a good practice since too much local variables in a
routine is a good sign of further splitting need. Anyway as you
said, we have -Wshadow=local, we should be safe.

Thanks,

C.



> I suspect when
> a function is big enough so that moving declarations to inner blocks
> improves it, factoring out these inner blocks would improve it more.
> 
> My other reason is the risk for accidental shadowing, but us enabling
> -Wshadow=local will render that point moot.
> 
> Maintainers get to decide such matters of taste, and I'm not the
> maintainer here :)
> 
>>> Anyways,
>>> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>> +                    val & ~(PSIHB9_ESB_CI_VALID | PSIHB10_ESB_CI_64K);
>>>> +                memory_region_add_subregion(sysmem, esb_addr,
>>>>                                                &psi9->source.esb_mmio);
>>>>                }
>>>>            }

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

[Harsh Prateek Bora]

On 9/29/23 11:37, Cédric Le Goater wrote:
> On 9/29/23 07:39, Markus Armbruster wrote:
>> Harsh Prateek Bora <harsh.prateek.bora@gmail.com> writes:
>>
>>> On Tue, 19 Sept, 2023, 5:39 pm Cédric Le Goater, <clg@kaod.org> wrote:
>>>
>>>> On 9/19/23 10:48, Harsh Prateek Bora wrote:
>>>>>
>>>>>
>>>>> On 9/18/23 20:28, Cédric Le Goater wrote:
>>>>>> Rename 'name' variable to avoid this warning :
>>>>>>
>>>>>>     ../hw/ppc/spapr_drc.c: In function ‘prop_get_fdt’:
>>>>>>     ../hw/ppc/spapr_drc.c:344:21: warning: declaration of ‘name’ 
>>>>>> shadows
>>>> a parameter [-Wshadow=compatible-local]
>>>>>>       344 |         const char *name = NULL;
>>>>>>           |                     ^~~~
>>>>>>     ../hw/ppc/spapr_drc.c:325:63: note: shadowed declaration is here
>>>>>>       325 | static void prop_get_fdt(Object *obj, Visitor *v, 
>>>>>> const char
>>>> *name,
>>>>>>           |
>>>> ~~~~~~~~~~~~^~~~
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>> ---
>>>>>>    hw/ppc/spapr_drc.c | 10 +++++-----
>>>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>>>>>> index 843e318312d3..2b99d3b4b1a6 100644
>>>>>> --- a/hw/ppc/spapr_drc.c
>>>>>> +++ b/hw/ppc/spapr_drc.c
>>>>>> @@ -341,7 +341,7 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>>>> const char *name,
>>>>>>        fdt_depth = 0;
>>>>>>        do {
>>>>>> -        const char *name = NULL;
>>>>>> +        const char *dt_name = NULL;
>>>>>
>>>>> I guess you wanted to use the input arg "name" here without
>>>> re-declaration.
>>>>
>>>> I don't understand. I don't want to use the input arg "name" here.
>>>> It seems useless in this case.
>>>>
>>>
>>> Yeh, I realize now. This patch can actually remove the unused arg 
>>> "name" as
>>> well?
>>
>> Cédric?
>>
>> Lose ends like this one make me reluctant to queue a series, even when
>> they look minor to me.
> 
> Unfortunately, we can not remove the unused arg "name" from the prototype.
> The routine is a ObjectPropertyAccessor argument of object_property_add().
> 

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

> Thanks,
> 
> C.
> 
> 
>>
>>>> C.
>>>>
>>>>> I do not see "name" being used elsewhere in this routine.
>>>>>
>>>>> regards,
>>>>> Harsh
>>>>>>            const struct fdt_property *prop = NULL;
>>>>>>            int prop_len = 0, name_len = 0;
>>>>>>            uint32_t tag;
>>>>>> @@ -351,8 +351,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>>>> const char *name,
>>>>>>            switch (tag) {
>>>>>>            case FDT_BEGIN_NODE:
>>>>>>                fdt_depth++;
>>>>>> -            name = fdt_get_name(fdt, fdt_offset, &name_len);
>>>>>> -            if (!visit_start_struct(v, name, NULL, 0, errp)) {
>>>>>> +            dt_name = fdt_get_name(fdt, fdt_offset, &name_len);
>>>>>> +            if (!visit_start_struct(v, dt_name, NULL, 0, errp)) {
>>>>>>                    return;
>>>>>>                }
>>>>>>                break;
>>>>>> @@ -369,8 +369,8 @@ static void prop_get_fdt(Object *obj, Visitor *v,
>>>> const char *name,
>>>>>>            case FDT_PROP: {
>>>>>>                int i;
>>>>>>                prop = fdt_get_property_by_offset(fdt, fdt_offset,
>>>> &prop_len);
>>>>>> -            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>>>>>> -            if (!visit_start_list(v, name, NULL, 0, errp)) {
>>>>>> +            dt_name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>>>>>> +            if (!visit_start_list(v, dt_name, NULL, 0, errp)) {
>>>>>>                    return;
>>>>>>                }
>>>>>>                for (i = 0; i < prop_len; i++) {
>>
> 

Re: [PATCH 0/8] ppc: Clean up local variable shadowing

[Markus Armbruster]

Looks like I forgot to notify you when I queued, my apologies.  Series
has since been merged.

Re: [PATCH 0/8] ppc: Clean up local variable shadowing

[Cédric Le Goater]

On 10/4/23 12:26, Markus Armbruster wrote:
> Looks like I forgot to notify you when I queued, my apologies.  Series
> has since been merged.

Perfect !

C.