[PATCH v2 00/10] trivial patchs for static code analyzer fixes

[PATCH v2 00/10] trivial patchs for static code analyzer fixes

[Chen Qun]

Hi All,
   This series fix trivial warnings reported by the Clang static code analyzer.

Since v1:
- Patch1: Addressed Igor Mammedov review comment.
- Patch5：Addressed Raphael Norwitz review comment.
- Patch6: Addressed Philippe Mathieu-Daudé review comment.
- Patch7: Addressed Eric Auger review comment.
- Patch8: drop the patch "tcg/optimize:** " follow Richard Henderson comment.
- Patch9->Patch8: The patch code format is modified because spaces are missing.
- Patch11->Patch10: Addressed Gerd Hoffmann review comment.

Thanks.

Chen Qun (10):
  hw/arm/virt-acpi-build:Remove dead assignment in build_madt()
  hw/arm/omap1:Remove redundant statement in omap_clkdsp_read()
  target/arm/translate-a64:Remove dead assignment in
    handle_scalar_simd_shli()
  target/arm/translate-a64:Remove redundant statement in
    disas_simd_two_reg_misc_fp16()
  hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()
  hw/net/virtio-net:Remove redundant statement in
    virtio_net_rsc_tcp_ctrl_check()
  vfio/platform: Remove dead assignment in vfio_intp_interrupt()
  usb/bus: Remove dead assignment in usb_get_fw_dev_path()
  hw/intc: Remove redundant statement in exynos4210_combiner_read()
  hw/display/vga:Remove redundant statement in vga_draw_graphic()

 hw/arm/omap1.c                | 1 -
 hw/arm/virt-acpi-build.c      | 3 +--
 hw/display/vga.c              | 1 -
 hw/intc/exynos4210_combiner.c | 3 +--
 hw/net/virtio-net.c           | 1 -
 hw/usb/bus.c                  | 4 ++--
 hw/vfio/platform.c            | 2 +-
 hw/virtio/vhost-user.c        | 2 +-
 target/arm/translate-a64.c    | 7 ++-----
 9 files changed, 8 insertions(+), 16 deletions(-)

-- 
2.23.0

[PATCH v2 01/10] hw/arm/virt-acpi-build:Remove dead assignment in build_madt()

[Chen Qun]

Clang static code analyzer show warning:
hw/arm/virt-acpi-build.c:641:5: warning: Value stored to 'madt' is never read
    madt = acpi_data_push(table_data, sizeof *madt);
    ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: qemu-arm@nongnu.org
---
 hw/arm/virt-acpi-build.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 91f0df7b13..f830f9b779 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -633,12 +633,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     int madt_start = table_data->len;
     const MemMapEntry *memmap = vms->memmap;
     const int *irqmap = vms->irqmap;
-    AcpiMultipleApicTable *madt;
     AcpiMadtGenericDistributor *gicd;
     AcpiMadtGenericMsiFrame *gic_msi;
     int i;
 
-    madt = acpi_data_push(table_data, sizeof *madt);
+    acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
 
     gicd = acpi_data_push(table_data, sizeof *gicd);
     gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
-- 
2.23.0

[PATCH v2 02/10] hw/arm/omap1:Remove redundant statement in omap_clkdsp_read()

[Chen Qun]

Clang static code analyzer show warning:
hw/arm/omap1.c:1760:15: warning: Value stored to 'cpu' during its
initialization is never read
    CPUState *cpu = CPU(s->cpu);
              ^~~   ~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 hw/arm/omap1.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 6ba0df6b6d..02c0f66431 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -1774,7 +1774,6 @@ static uint64_t omap_clkdsp_read(void *opaque, hwaddr addr,
         return s->clkm.dsp_rstct2;
 
     case 0x18:	/* DSP_SYSST */
-        cpu = CPU(s->cpu);
         return (s->clkm.clocking_scheme << 11) | s->clkm.cold_start |
                 (cpu->halted << 6);      /* Quite useless... */
     }
-- 
2.23.0

[PATCH v2 03/10] target/arm/translate-a64:Remove dead assignment in handle_scalar_simd_shli()

[Chen Qun]

Clang static code analyzer show warning:
target/arm/translate-a64.c:8635:14: warning: Value stored to 'tcg_rn' during its
 initialization is never read
    TCGv_i64 tcg_rn = new_tmp_a64(s);
             ^~~~~~   ~~~~~~~~~~~~~~
target/arm/translate-a64.c:8636:14: warning: Value stored to 'tcg_rd' during its
 initialization is never read
    TCGv_i64 tcg_rd = new_tmp_a64(s);
             ^~~~~~   ~~~~~~~~~~~~~~

There is a memory leak for the variable new_tmp_a64 "s".
We should delete the assignment.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 target/arm/translate-a64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 534c3ff5f3..c83bb85e4e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -8632,8 +8632,8 @@ static void handle_scalar_simd_shli(DisasContext *s, bool insert,
     int size = 32 - clz32(immh) - 1;
     int immhb = immh << 3 | immb;
     int shift = immhb - (8 << size);
-    TCGv_i64 tcg_rn = new_tmp_a64(s);
-    TCGv_i64 tcg_rd = new_tmp_a64(s);
+    TCGv_i64 tcg_rn;
+    TCGv_i64 tcg_rd;
 
     if (!extract32(immh, 3, 1)) {
         unallocated_encoding(s);
-- 
2.23.0

[PATCH v2 04/10] target/arm/translate-a64:Remove redundant statement in disas_simd_two_reg_misc_fp16()

[Chen Qun]

Clang static code analyzer show warning:
target/arm/translate-a64.c:13007:5: warning: Value stored to 'rd' is never read
    rd = extract32(insn, 0, 5);
    ^    ~~~~~~~~~~~~~~~~~~~~~
target/arm/translate-a64.c:13008:5: warning: Value stored to 'rn' is never read
    rn = extract32(insn, 5, 5);
    ^    ~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 target/arm/translate-a64.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index c83bb85e4e..47cce160d8 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -13016,9 +13016,6 @@ static void disas_simd_two_reg_misc_fp16(DisasContext *s, uint32_t insn)
     fpop = deposit32(opcode, 5, 1, a);
     fpop = deposit32(fpop, 6, 1, u);
 
-    rd = extract32(insn, 0, 5);
-    rn = extract32(insn, 5, 5);
-
     switch (fpop) {
     case 0x1d: /* SCVTF */
     case 0x5d: /* UCVTF */
-- 
2.23.0

[PATCH v2 05/10] hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()

[Chen Qun]

Clang static code analyzer show warning:
hw/virtio/vhost-user.c:606:9: warning: Value stored to 'mr' is never read
        mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
        ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 hw/virtio/vhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d7e2423762..9c5b4f7fbc 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -603,7 +603,7 @@ static void scrub_shadow_regions(struct vhost_dev *dev,
      */
     for (i = 0; i < dev->mem->nregions; i++) {
         reg = &dev->mem->regions[i];
-        mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
+        vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
         if (fd > 0) {
             ++fd_num;
         }
-- 
2.23.0

[PATCH v2 06/10] hw/net/virtio-net:Remove redundant statement in virtio_net_rsc_tcp_ctrl_check()

[Chen Qun]

Clang static code analyzer show warning:
hw/net/virtio-net.c:2077:5: warning: Value stored to 'tcp_flag' is never read
    tcp_flag &= VIRTIO_NET_TCP_FLAG;
    ^           ~~~~~~~~~~~~~~~~~~~

The 'VIRTIO_NET_TCP_FLAG' is '0x3F'. The last ‘tcp_flag’ assignment statement is
 the same as that of the first two statements.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a1fe9e9285..cb0d27084c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2075,7 +2075,6 @@ static int virtio_net_rsc_tcp_ctrl_check(VirtioNetRscChain *chain,
     tcp_flag = htons(tcp->th_offset_flags);
     tcp_hdr = (tcp_flag & VIRTIO_NET_TCP_HDR_LENGTH) >> 10;
     tcp_flag &= VIRTIO_NET_TCP_FLAG;
-    tcp_flag = htons(tcp->th_offset_flags) & 0x3F;
     if (tcp_flag & TH_SYN) {
         chain->stat.tcp_syn++;
         return RSC_BYPASS;
-- 
2.23.0

[PATCH v2 07/10] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Chen Qun]

Clang static code analyzer show warning:
hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
        ret = event_notifier_test_and_clear(intp->interrupt);
        ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/vfio/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ac2cefc9b1..869ed2c39d 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
         trace_vfio_intp_interrupt_set_pending(intp->pin);
         QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
                              intp, pqnext);
-        ret = event_notifier_test_and_clear(intp->interrupt);
+        event_notifier_test_and_clear(intp->interrupt);
         return;
     }
 
-- 
2.23.0

[PATCH v2 08/10] usb/bus: Remove dead assignment in usb_get_fw_dev_path()

[Chen Qun]

Clang static code analyzer show warning:
qemu/hw/usb/bus.c:615:13: warning: Value stored to 'pos' is never read
            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index b17bda3b29..7bab0499ad 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState *qdev)
             in++;
         } else {
             /* the device itself */
-            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
-                            qdev_fw_name(qdev), nr);
+            snprintf(fw_path + pos, fw_len - pos, "%s@%lx", qdev_fw_name(qdev),
+                     nr);
             break;
         }
     }
-- 
2.23.0

[PATCH v2 09/10] hw/intc: Remove redundant statement in exynos4210_combiner_read()

[Chen Qun]

Clang static code analyzer show warning:
hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never read
        val = s->reg_set[offset >> 2];

The default value of 'val' is '0', so we can break the 'default' branch and return 'val'.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Igor Mitsyanko <i.mitsyanko@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/exynos4210_combiner.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
index b8561e4180..e2e745bbaa 100644
--- a/hw/intc/exynos4210_combiner.c
+++ b/hw/intc/exynos4210_combiner.c
@@ -228,8 +228,7 @@ exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
             hw_error("exynos4210.combiner: overflow of reg_set by 0x"
                     TARGET_FMT_plx "offset\n", offset);
         }
-        val = s->reg_set[offset >> 2];
-        return 0;
+        break;
     }
     return val;
 }
-- 
2.23.0

[PATCH v2 10/10] hw/display/vga:Remove redundant statement in vga_draw_graphic()

[Chen Qun]

Clang static code analyzer show warning:
hw/display/vga.c:1677:9: warning: Value stored to 'update' is never read
        update = full_update;
        ^        ~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 061fd9ab8f..836ad50c7b 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1674,7 +1674,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         if (!(s->cr[VGA_CRTC_MODE] & 2)) {
             addr = (addr & ~0x8000) | ((y1 & 2) << 14);
         }
-        update = full_update;
         page0 = addr & s->vbe_size_mask;
         page1 = (addr + bwidth - 1) & s->vbe_size_mask;
         if (full_update) {
-- 
2.23.0

Re: [PATCH v2 02/10] hw/arm/omap1:Remove redundant statement in omap_clkdsp_read()

[Peter Maydell]

On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> Clang static code analyzer show warning:
> hw/arm/omap1.c:1760:15: warning: Value stored to 'cpu' during its
> initialization is never read
>     CPUState *cpu = CPU(s->cpu);
>               ^~~   ~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 03/10] target/arm/translate-a64:Remove dead assignment in handle_scalar_simd_shli()

[Peter Maydell]

On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> Clang static code analyzer show warning:
> target/arm/translate-a64.c:8635:14: warning: Value stored to 'tcg_rn' during its
>  initialization is never read
>     TCGv_i64 tcg_rn = new_tmp_a64(s);
>              ^~~~~~   ~~~~~~~~~~~~~~
> target/arm/translate-a64.c:8636:14: warning: Value stored to 'tcg_rd' during its
>  initialization is never read
>     TCGv_i64 tcg_rd = new_tmp_a64(s);
>              ^~~~~~   ~~~~~~~~~~~~~~
>
> There is a memory leak for the variable new_tmp_a64 "s".

There is not, because TCG temps allocated via new_tmp_a64()
are all freed via free_tmp_a64() at the end of disas_a64_insn().

> We should delete the assignment.

But yes, the assignments are unused and should be deleted.

If you fix the commit message, then
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 04/10] target/arm/translate-a64:Remove redundant statement in disas_simd_two_reg_misc_fp16()

[Peter Maydell]

On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> Clang static code analyzer show warning:
> target/arm/translate-a64.c:13007:5: warning: Value stored to 'rd' is never read
>     rd = extract32(insn, 0, 5);
>     ^    ~~~~~~~~~~~~~~~~~~~~~
> target/arm/translate-a64.c:13008:5: warning: Value stored to 'rn' is never read
>     rn = extract32(insn, 5, 5);
>     ^    ~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> ---
>  target/arm/translate-a64.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index c83bb85e4e..47cce160d8 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -13016,9 +13016,6 @@ static void disas_simd_two_reg_misc_fp16(DisasContext *s, uint32_t insn)
>      fpop = deposit32(opcode, 5, 1, a);
>      fpop = deposit32(fpop, 6, 1, u);
>
> -    rd = extract32(insn, 0, 5);
> -    rn = extract32(insn, 5, 5);
> -

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in exynos4210_combiner_read()

[Peter Maydell]

On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> Clang static code analyzer show warning:
> hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never read
>         val = s->reg_set[offset >> 2];
>
> The default value of 'val' is '0', so we can break the 'default' branch and return 'val'.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Igor Mitsyanko <i.mitsyanko@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/exynos4210_combiner.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
> index b8561e4180..e2e745bbaa 100644
> --- a/hw/intc/exynos4210_combiner.c
> +++ b/hw/intc/exynos4210_combiner.c
> @@ -228,8 +228,7 @@ exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
>              hw_error("exynos4210.combiner: overflow of reg_set by 0x"
>                      TARGET_FMT_plx "offset\n", offset);
>          }
> -        val = s->reg_set[offset >> 2];
> -        return 0;
> +        break;
>      }
>      return val;
>  }

The code as it stands is definitely wrong, but I'm not sure
this is the correct fix. Surely the intention must have been
to return the actual register value from the reg_set[] array,
not to return 0 ?

I suspect the correct fix here is simply to delete the
"return 0" line and leave the assignment to val as it is.
Ideally you should check the h/w datasheet to confirm.

thanks
-- PMM

Re: [PATCH v2 08/10] usb/bus: Remove dead assignment in usb_get_fw_dev_path()

[Markus Armbruster]

Chen Qun <kuhn.chenqun@huawei.com> writes:

> Clang static code analyzer show warning:
> qemu/hw/usb/bus.c:615:13: warning: Value stored to 'pos' is never read
>             pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/usb/bus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index b17bda3b29..7bab0499ad 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState *qdev)
           if (in[0] == '.') {
               /* some hub between root port and device */
               pos += snprintf(fw_path + pos, fw_len - pos, "hub@%lx/", nr);
>              in++;
>          } else {
>              /* the device itself */
> -            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> -                            qdev_fw_name(qdev), nr);
> +            snprintf(fw_path + pos, fw_len - pos, "%s@%lx", qdev_fw_name(qdev),
> +                     nr);
>              break;
>          }
>      }

I'd prefer to keep the line break where it is:

            snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
                     qdev_fw_name(qdev), nr);

The patch is safe, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

The loss of symmetry betwen the two arms of the if is a bit sad.  Up to
Gerd.

Re: [PATCH v2 07/10] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Stefan Hajnoczi]

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

On Tue, Aug 25, 2020 at 07:24:44PM +0800, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>         ret = event_notifier_test_and_clear(intp->interrupt);
>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/vfio/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 01/10] hw/arm/virt-acpi-build:Remove dead assignment in build_madt()

[Li Qiang]

Chen Qun <kuhn.chenqun@huawei.com> 于2020年8月25日周二 下午7:26写道：
>
> Clang static code analyzer show warning:
> hw/arm/virt-acpi-build.c:641:5: warning: Value stored to 'madt' is never read
>     madt = acpi_data_push(table_data, sizeof *madt);
>     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/arm/virt-acpi-build.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 91f0df7b13..f830f9b779 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -633,12 +633,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      int madt_start = table_data->len;
>      const MemMapEntry *memmap = vms->memmap;
>      const int *irqmap = vms->irqmap;
> -    AcpiMultipleApicTable *madt;
>      AcpiMadtGenericDistributor *gicd;
>      AcpiMadtGenericMsiFrame *gic_msi;
>      int i;
>
> -    madt = acpi_data_push(table_data, sizeof *madt);
> +    acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>
>      gicd = acpi_data_push(table_data, sizeof *gicd);
>      gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
> --
> 2.23.0
>
>

Re: [PATCH v2 02/10] hw/arm/omap1:Remove redundant statement in omap_clkdsp_read()

[Li Qiang]

Chen Qun <kuhn.chenqun@huawei.com> 于2020年8月25日周二 下午7:30写道：
>
> Clang static code analyzer show warning:
> hw/arm/omap1.c:1760:15: warning: Value stored to 'cpu' during its
> initialization is never read
>     CPUState *cpu = CPU(s->cpu);
>               ^~~   ~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/arm/omap1.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
> index 6ba0df6b6d..02c0f66431 100644
> --- a/hw/arm/omap1.c
> +++ b/hw/arm/omap1.c
> @@ -1774,7 +1774,6 @@ static uint64_t omap_clkdsp_read(void *opaque, hwaddr addr,
>          return s->clkm.dsp_rstct2;
>
>      case 0x18: /* DSP_SYSST */
> -        cpu = CPU(s->cpu);
>          return (s->clkm.clocking_scheme << 11) | s->clkm.cold_start |
>                  (cpu->halted << 6);      /* Quite useless... */
>      }
> --
> 2.23.0
>
>

Re: [PATCH v2 06/10] hw/net/virtio-net:Remove redundant statement in virtio_net_rsc_tcp_ctrl_check()

[Li Qiang]

Chen Qun <kuhn.chenqun@huawei.com> 于2020年8月25日周二 下午7:32写道：
>
> Clang static code analyzer show warning:
> hw/net/virtio-net.c:2077:5: warning: Value stored to 'tcp_flag' is never read
>     tcp_flag &= VIRTIO_NET_TCP_FLAG;
>     ^           ~~~~~~~~~~~~~~~~~~~
>
> The 'VIRTIO_NET_TCP_FLAG' is '0x3F'. The last ‘tcp_flag’ assignment statement is
>  the same as that of the first two statements.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a1fe9e9285..cb0d27084c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2075,7 +2075,6 @@ static int virtio_net_rsc_tcp_ctrl_check(VirtioNetRscChain *chain,
>      tcp_flag = htons(tcp->th_offset_flags);
>      tcp_hdr = (tcp_flag & VIRTIO_NET_TCP_HDR_LENGTH) >> 10;
>      tcp_flag &= VIRTIO_NET_TCP_FLAG;
> -    tcp_flag = htons(tcp->th_offset_flags) & 0x3F;
>      if (tcp_flag & TH_SYN) {
>          chain->stat.tcp_syn++;
>          return RSC_BYPASS;
> --
> 2.23.0
>
>

Re: [PATCH v2 07/10] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Li Qiang]

Chen Qun <kuhn.chenqun@huawei.com> 于2020年8月25日周二 下午7:31写道：
>
> Clang static code analyzer show warning:
> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>         ret = event_notifier_test_and_clear(intp->interrupt);
>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/vfio/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ac2cefc9b1..869ed2c39d 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>                               intp, pqnext);
> -        ret = event_notifier_test_and_clear(intp->interrupt);

Shouldn't we check the 'ret' like the other place in this function?

Thanks,
Li Qiang

> +        event_notifier_test_and_clear(intp->interrupt);
>          return;
>      }
>
> --
> 2.23.0
>
>

RE: [PATCH v2 07/10] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Chenqun (kuhn)]

> > Clang static code analyzer show warning:
> > hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
> >         ret = event_notifier_test_and_clear(intp->interrupt);
> >         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > ---
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Eric Auger <eric.auger@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/vfio/platform.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index
> > ac2cefc9b1..869ed2c39d 100644
> > --- a/hw/vfio/platform.c
> > +++ b/hw/vfio/platform.c
> > @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
> >          trace_vfio_intp_interrupt_set_pending(intp->pin);
> >          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
> >                               intp, pqnext);
> > -        ret = event_notifier_test_and_clear(intp->interrupt);
> 
> Shouldn't we check the 'ret' like the other place in this function?

Hi, Li Qiang,

Eric、Alex、Stefan has already discussed this point in the V1 version.
https://patchwork.kernel.org/patch/11711897/

Thanks.

Re: [PATCH v2 10/10] hw/display/vga:Remove redundant statement in vga_draw_graphic()

[Li Qiang]

Chen Qun <kuhn.chenqun@huawei.com> 于2020年8月25日周二 下午7:33写道：
>
> Clang static code analyzer show warning:
> hw/display/vga.c:1677:9: warning: Value stored to 'update' is never read
>         update = full_update;
>         ^        ~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/vga.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 061fd9ab8f..836ad50c7b 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -1674,7 +1674,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
>          if (!(s->cr[VGA_CRTC_MODE] & 2)) {
>              addr = (addr & ~0x8000) | ((y1 & 2) << 14);
>          }
> -        update = full_update;
>          page0 = addr & s->vbe_size_mask;
>          page1 = (addr + bwidth - 1) & s->vbe_size_mask;
>          if (full_update) {
> --
> 2.23.0
>
>

RE: [PATCH v2 08/10] usb/bus: Remove dead assignment in usb_get_fw_dev_path()

[Chenqun (kuhn)]

> >  hw/usb/bus.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/usb/bus.c b/hw/usb/bus.c index b17bda3b29..7bab0499ad
> > 100644
> > --- a/hw/usb/bus.c
> > +++ b/hw/usb/bus.c
> > @@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState
> > *qdev)
>            if (in[0] == '.') {
>                /* some hub between root port and device */
>                pos += snprintf(fw_path + pos, fw_len - pos, "hub@%lx/",
> nr);
> >              in++;
> >          } else {
> >              /* the device itself */
> > -            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> > -                            qdev_fw_name(qdev), nr);
> > +            snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> qdev_fw_name(qdev),
> > +                     nr);
> >              break;
> >          }
> >      }
> 
> I'd prefer to keep the line break where it is:
> 
>             snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
>                      qdev_fw_name(qdev), nr);
> 
> The patch is safe, so
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> The loss of symmetry betwen the two arms of the if is a bit sad.  Up to Gerd.

If symmetry looks better. I should change it later.

Thanks,
Chen Qun

RE: [PATCH v2 08/10] usb/bus: Remove dead assignment in usb_get_fw_dev_path()

[Chenqun (kuhn)]

> > >  hw/usb/bus.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/usb/bus.c b/hw/usb/bus.c index
> > > b17bda3b29..7bab0499ad
> > > 100644
> > > --- a/hw/usb/bus.c
> > > +++ b/hw/usb/bus.c
> > > @@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState
> > > *qdev)
> >            if (in[0] == '.') {
> >                /* some hub between root port and device */
> >                pos += snprintf(fw_path + pos, fw_len - pos,
> > "hub@%lx/", nr);
> > >              in++;
> > >          } else {
> > >              /* the device itself */
> > > -            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> > > -                            qdev_fw_name(qdev), nr);
> > > +            snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> > qdev_fw_name(qdev),
> > > +                     nr);
> > >              break;
> > >          }
> > >      }
> >
> > I'd prefer to keep the line break where it is:
> >
> >             snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> >                      qdev_fw_name(qdev), nr);
> >
> > The patch is safe, so
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >
> > The loss of symmetry betwen the two arms of the if is a bit sad.  Up to Gerd.
> 
> If symmetry looks better. I should change it later.
Oops, I think I just misunderstood you. I agree with your suggestion to the formatting of the "snprintf(***)" statement.

If the 'pos' assignment is useless, we delete it avoid warning that are always detected by some tools.

Thanks,
Chen Qun

RE: [PATCH v2 03/10] target/arm/translate-a64:Remove dead assignment in handle_scalar_simd_shli()

[Chenqun (kuhn)]

> On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >
> > Clang static code analyzer show warning:
> > target/arm/translate-a64.c:8635:14: warning: Value stored to 'tcg_rn'
> > during its  initialization is never read
> >     TCGv_i64 tcg_rn = new_tmp_a64(s);
> >              ^~~~~~   ~~~~~~~~~~~~~~
> > target/arm/translate-a64.c:8636:14: warning: Value stored to 'tcg_rd'
> > during its  initialization is never read
> >     TCGv_i64 tcg_rd = new_tmp_a64(s);
> >              ^~~~~~   ~~~~~~~~~~~~~~
> >
> > There is a memory leak for the variable new_tmp_a64 "s".
> 
> There is not, because TCG temps allocated via new_tmp_a64() are all freed via
> free_tmp_a64() at the end of disas_a64_insn().
> 
OK, I'll delete that description later.

Thanks,
ChenQun

Re: [PATCH v2 07/10] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Li Qiang]

Chenqun (kuhn) <kuhn.chenqun@huawei.com> 于2020年8月26日周三 上午9:47写道：
>
>
> > > Clang static code analyzer show warning:
> > > hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
> > >         ret = event_notifier_test_and_clear(intp->interrupt);
> > >         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Reported-by: Euler Robot <euler.robot@huawei.com>
> > > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > > ---
> > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > Cc: Eric Auger <eric.auger@redhat.com>
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  hw/vfio/platform.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index
> > > ac2cefc9b1..869ed2c39d 100644
> > > --- a/hw/vfio/platform.c
> > > +++ b/hw/vfio/platform.c
> > > @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
> > >          trace_vfio_intp_interrupt_set_pending(intp->pin);
> > >          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
> > >                               intp, pqnext);
> > > -        ret = event_notifier_test_and_clear(intp->interrupt);
> >
> > Shouldn't we check the 'ret' like the other place in this function?
>
> Hi, Li Qiang,
>
> Eric、Alex、Stefan has already discussed this point in the V1 version.
> https://patchwork.kernel.org/patch/11711897/

Ok I see, then

Reviewed-by: Li Qiang <liq3ea@gmail.com>

Thanks,
Li Qiang

>
> Thanks.

Re: [PATCH v2 05/10] hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()

[Li Qiang]

Chen Qun <kuhn.chenqun@huawei.com> 于2020年8月25日周二 下午7:28写道：
>
> Clang static code analyzer show warning:
> hw/virtio/vhost-user.c:606:9: warning: Value stored to 'mr' is never read
>         mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
>         ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index d7e2423762..9c5b4f7fbc 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -603,7 +603,7 @@ static void scrub_shadow_regions(struct vhost_dev *dev,
>       */
>      for (i = 0; i < dev->mem->nregions; i++) {
>          reg = &dev->mem->regions[i];
> -        mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
> +        vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
>          if (fd > 0) {
>              ++fd_num;
>          }
> --
> 2.23.0
>
>

RE: [PATCH v2 09/10] hw/intc: Remove redundant statement in exynos4210_combiner_read()

[Chenqun (kuhn)]

> Subject: Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in
> exynos4210_combiner_read()
> 
> On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >
> > Clang static code analyzer show warning:
> > hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never
> read
> >         val = s->reg_set[offset >> 2];
> >
> > The default value of 'val' is '0', so we can break the 'default' branch and return
> 'val'.
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > ---
> > Cc: Igor Mitsyanko <i.mitsyanko@gmail.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/intc/exynos4210_combiner.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/exynos4210_combiner.c
> > b/hw/intc/exynos4210_combiner.c index b8561e4180..e2e745bbaa 100644
> > --- a/hw/intc/exynos4210_combiner.c
> > +++ b/hw/intc/exynos4210_combiner.c
> > @@ -228,8 +228,7 @@ exynos4210_combiner_read(void *opaque, hwaddr
> offset, unsigned size)
> >              hw_error("exynos4210.combiner: overflow of reg_set by 0x"
> >                      TARGET_FMT_plx "offset\n", offset);
> >          }
> > -        val = s->reg_set[offset >> 2];
> > -        return 0;
> > +        break;
> >      }
> >      return val;
> >  }
> 
> The code as it stands is definitely wrong, but I'm not sure this is the correct fix.
> Surely the intention must have been to return the actual register value from
> the reg_set[] array, not to return 0 ?
> 
> I suspect the correct fix here is simply to delete the "return 0" line and leave
> the assignment to val as it is.

Hi Peter,
  I think you're right.

> Ideally you should check the h/w datasheet to confirm.
 I checked the Exynos 4210 datasheet and found that 'Interrupt Combiner Operation' had only five types of registers.

Register             Description                                              Type
----------------------------------------------------------------------------------------------------------------------------------------
IESR(0/1/2/3)   Interrupt Enable Set Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15)         RW
IECR(0/1/2/3)   Interrupt Enable Clear Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15)       RW
ISTR(0/1/2/3)   Interrupt Status Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15)             R
IMSR(0/1/2/3)  Interrupt Masked Status Register for Group(0~3/ 4 ~7/ 8~ 11/ 12~ 15)       R
CIPSR0        Combined Interrupt Pending Status0                                  R

The exynos4210_combiner_write() function has write operations for IESR and IECR.
The CIPSR, ISTR, and IMSR read operations are specified in the exynos4210_combiner_read() function.

So, This 'default' branch in exynos4210_combiner_read() function read operation is only for IESR and IECR.

And, in the exynos4210_combiner_write function, the default assignment statement for IESR and IECR is " s->reg_set[offset >> 2] = val;".
Therefore, if we read the two register(IESR and IECR) values, we can directly use this assignment statement "val = s->reg_set[offset >> 2];".

Please confirm that if there is no problem, I will modify it in later version.

Thanks,
Chen Qun

Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in exynos4210_combiner_read()

[Peter Maydell]

On Thu, 27 Aug 2020 at 08:01, Chenqun (kuhn) <kuhn.chenqun@huawei.com> wrote:
>
> > Subject: Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in
> > exynos4210_combiner_read()

> > The code as it stands is definitely wrong, but I'm not sure this is the correct fix.
> > Surely the intention must have been to return the actual register value from
> > the reg_set[] array, not to return 0 ?
> >
> > I suspect the correct fix here is simply to delete the "return 0" line and leave
> > the assignment to val as it is.
>
> Hi Peter,
>   I think you're right.
>
> > Ideally you should check the h/w datasheet to confirm.
>  I checked the Exynos 4210 datasheet and found that 'Interrupt Combiner Operation' had only five types of registers.

> Please confirm that if there is no problem, I will modify it in later version.

Thanks for checking the datasheet -- I agree and we should keep
the "val = s->reg_set[offset >> 2];" and only delete the "return 0".

-- PMM