hw/arm/exynos: Convert fprintf to error_report()

In few places the function arguments and local variables are not modifying data passed through pointers so this can be made const for code safeness. Also the static array exynos4210_uart_regs is not being modified.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 hw/char/exynos4210_uart.c     | 10 +++++-----
 hw/intc/exynos4210_combiner.c |  2 +-
 hw/intc/exynos4210_gic.c      |  8 ++++----
 hw/misc/exynos4210_pmu.c      |  2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

hw/char/exynos4210_uart.c +5 -5

index b75f28d473bf..83e1be253255 100644
@@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
     uint32_t            reset_value;
 } Exynos4210UartReg;
 
-static Exynos4210UartReg exynos4210_uart_regs[] = {
+static const Exynos4210UartReg exynos4210_uart_regs[] = {
     {"ULCON",    ULCON,    0x00000000},
     {"UCON",     UCON,     0x00003000},
     {"UFCON",    UFCON,    0x00000000},

@@ -220,7 +220,7 @@ static uint8_t fifo_retrieve(Exynos4210UartFIFO *q)
     return  ret;
 }
 
-static int fifo_elements_number(Exynos4210UartFIFO *q)
+static int fifo_elements_number(const Exynos4210UartFIFO *q)
 {
     if (q->sp < q->rp) {
         return q->size - q->rp + q->sp;

@@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
     return q->sp - q->rp;
 }
 
-static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
+static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
 {
     return q->size - fifo_elements_number(q);
 }

@@ -245,7 +245,7 @@ static void fifo_reset(Exynos4210UartFIFO *q)
     q->rp = 0;
 }
 
-static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
+static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
 {
     uint32_t level = 0;
     uint32_t reg;

@@ -488,7 +488,7 @@ static const MemoryRegionOps exynos4210_uart_ops = {
 
 static int exynos4210_uart_can_receive(void *opaque)
 {
-    Exynos4210UartState *s = (Exynos4210UartState *)opaque;
+    const Exynos4210UartState *s = (Exynos4210UartState *)opaque;
 
     return fifo_empty_elements_number(&s->rx);
 }

hw/intc/exynos4210_combiner.c +1 -1

index f19a7062be3a..b057921e0504 100644
@@ -180,7 +180,7 @@ void exynos4210_combiner_get_gpioin(Exynos4210Irq *irqs, DeviceState *dev,
 static uint64_t
 exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
 {
-    struct Exynos4210CombinerState *s =
+    const struct Exynos4210CombinerState *s =
             (struct Exynos4210CombinerState *)opaque;
     uint32_t req_quad_base_n;    /* Base of registers quad. Multiply it by 4 and
                                    get a start of corresponding group quad */

hw/intc/exynos4210_gic.c +4 -4

index 2a55817b7660..432b8425d09d 100644
@@ -190,7 +190,7 @@ combiner_grp_to_gic_id[64-EXYNOS4210_MAX_EXT_COMBINER_OUT_IRQ][8] = {
 
 static void exynos4210_irq_handler(void *opaque, int irq, int level)
 {
-    Exynos4210Irq *s = (Exynos4210Irq *)opaque;
+    const Exynos4210Irq *s = (Exynos4210Irq *)opaque;
 
     /* Bypass */
     qemu_set_irq(s->board_irqs[irq], level);

@@ -277,7 +277,7 @@ typedef struct {
 
 static void exynos4210_gic_set_irq(void *opaque, int irq, int level)
 {
-    Exynos4210GicState *s = (Exynos4210GicState *)opaque;
+    const Exynos4210GicState *s = (Exynos4210GicState *)opaque;
     qemu_set_irq(qdev_get_gpio_in(s->gic, irq), level);
 }
 

@@ -401,7 +401,7 @@ static const VMStateDescription vmstate_exynos4210_irq_gate = {
 /* Process a change in IRQ input. */
 static void exynos4210_irq_gate_handler(void *opaque, int irq, int level)
 {
-    Exynos4210IRQGateState *s = (Exynos4210IRQGateState *)opaque;
+    const Exynos4210IRQGateState *s = (Exynos4210IRQGateState *)opaque;
     uint32_t i;
 
     assert(irq < s->n_in);

@@ -420,7 +420,7 @@ static void exynos4210_irq_gate_handler(void *opaque, int irq, int level)
 
 static void exynos4210_irq_gate_reset(DeviceState *d)
 {
-    Exynos4210IRQGateState *s = EXYNOS4210_IRQ_GATE(d);
+    const Exynos4210IRQGateState *s = EXYNOS4210_IRQ_GATE(d);
 
     memset(s->level, 0, s->n_in * sizeof(*s->level));
 }

hw/misc/exynos4210_pmu.c +1 -1

index e30dbc7d3d83..cbdfa0614600 100644
@@ -400,7 +400,7 @@ typedef struct Exynos4210PmuState {
 static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
                                     unsigned size)
 {
-    Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
+    const Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
     unsigned i;
     const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
 

In few places the function arguments and local variables are not modifying data passed through pointers so this can be made const for code safeness. Also the static array exynos4210_uart_regs is not being modified.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 hw/char/exynos4210_uart.c     | 10 +++++-----
 hw/intc/exynos4210_combiner.c |  2 +-
 hw/intc/exynos4210_gic.c      |  8 ++++----
 hw/misc/exynos4210_pmu.c      |  2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

hw/char/exynos4210_uart.c +1 -1

index b75f28d473bf..83e1be253255 100644
@@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
     uint32_t            reset_value;
 } Exynos4210UartReg;
 
-static Exynos4210UartReg exynos4210_uart_regs[] = {
+static const Exynos4210UartReg exynos4210_uart_regs[] = {
     {"ULCON",    ULCON,    0x00000000},
     {"UCON",     UCON,     0x00003000},
     {"UFCON",    UFCON,    0x00000000},

hw/char/exynos4210_uart.c +2 -2

-static int fifo_elements_number(Exynos4210UartFIFO *q)
+static int fifo_elements_number(const Exynos4210UartFIFO *q)
 {
     if (q->sp < q->rp) {
         return q->size - q->rp + q->sp;

@@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
     return q->sp - q->rp;
 }
 
-static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
+static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
 {
     return q->size - fifo_elements_number(q);
 }

Quoted 27 lines · Krzysztof Kozlowski

In few places the function arguments and local variables are not modifying data passed through pointers so this can be made const for code safeness. Also the static array exynos4210_uart_regs is not being modified.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 hw/char/exynos4210_uart.c     | 10 +++++-----
 hw/intc/exynos4210_combiner.c |  2 +-
 hw/intc/exynos4210_gic.c      |  8 ++++----
 hw/misc/exynos4210_pmu.c      |  2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

hw/char/exynos4210_uart.c +1 -1

index b75f28d473bf..83e1be253255 100644
@@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
     uint32_t            reset_value;
 } Exynos4210UartReg;
 
-static Exynos4210UartReg exynos4210_uart_regs[] = {
+static const Exynos4210UartReg exynos4210_uart_regs[] = {
     {"ULCON",    ULCON,    0x00000000},
     {"UCON",     UCON,     0x00003000},
     {"UFCON",    UFCON,    0x00000000},

hw/char/exynos4210_uart.c static uint8_t fifo_retrieve(Exynos4210UartFIFO *q) · line 221

Quoted 2 lines

hw/char/exynos4210_uart.c +0 -0

@@ -220,7 +220,7 @@ static uint8_t fifo_retrieve(Exynos4210UartFIFO *q)
     return  ret;
 }

Constifying this sort of thing is OK...

Quoted 18 lines

hw/char/exynos4210_uart.c +2 -2

-static int fifo_elements_number(Exynos4210UartFIFO *q)
+static int fifo_elements_number(const Exynos4210UartFIFO *q)
 {
     if (q->sp < q->rp) {
         return q->size - q->rp + q->sp;

@@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
     return q->sp - q->rp;
 }
 
-static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
+static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
 {
     return q->size - fifo_elements_number(q);
 }

@@ -245,7 +245,7 @@ static void fifo_reset(Exynos4210UartFIFO *q)
     q->rp = 0;
 }
 

hw/char/exynos4210_uart.c static void fifo_reset(Exynos4210UartFIFO *q) · line 251

Quoted 5 lines

hw/char/exynos4210_uart.c +1 -1

-static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
+static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
 {
     uint32_t level = 0;
     uint32_t reg;

...but I disagree here somewhat...

hw/char/exynos4210_uart.c static const MemoryRegionOps exynos4210_uart_ops = { · line 491

Quoted 7 lines

hw/char/exynos4210_uart.c +1 -1

@@ -488,7 +488,7 @@ static const MemoryRegionOps exynos4210_uart_ops = {
 
 static int exynos4210_uart_can_receive(void *opaque)
 {
-    Exynos4210UartState *s = (Exynos4210UartState *)opaque;
+    const Exynos4210UartState *s = (Exynos4210UartState *)opaque;

...and definitely here.

These are effectively method implementations for QEMU objects, and defining the "this" pointer as const in some methods because it happens not to be modifying things just makes the code look out of line with every other method implementation

in the code base (and means somebody will have to drop the 'const' again at some point if the method needs to be updated to modify state in the future).

Short declaration of 'i' was in the middle of declarations with assignments. Make it a little bit more readable. No functional change.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 hw/misc/exynos4210_pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/misc/exynos4210_pmu.c +0 -0

index cbdfa0614600..60d1545c0baa 100644

hw/misc/exynos4210_pmu.c +1 -1

     for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
         if (reg_p->offset == offset) {

@@ -420,8 +420,8 @@ static void exynos4210_pmu_write(void *opaque, hwaddr offset,
                                  uint64_t val, unsigned size)
 {
     Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
-    unsigned i;
     const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
+    unsigned i;
 
     for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
         if (reg_p->offset == offset) {

your change seems OK but while you are here, 'unsigned' is considered harmful since more than a decade.

Quoted 2 lines · Philippe Mathieu-Daudé

your change seems OK but while you are here, 'unsigned' is considered harmful since more than a decade.

Considered harmful by who, and why?

Quoted 4 lines · Peter Maydell

Quoted 2 lines · Philippe Mathieu-Daudé

your change seems OK but while you are here, 'unsigned' is considered harmful since more than a decade.

Quoted 1 line

Considered harmful by who, and why?

It is true that unsigned has all the disadvantages of "int" (it may be smaller than the size of the object" and all the disadvantages of "size_t" (it doesn't optimize as well because the compiler cannot exploit undefined behavior).

I wouldn't call it harmful, but it feels like the worst of both worlds.

Quoted 6 lines · Paolo Bonzini

Quoted 5 lines · Peter Maydell

Quoted 2 lines · Philippe Mathieu-Daudé

your change seems OK but while you are here, 'unsigned' is considered harmful since more than a decade.

Considered harmful by who, and why?

Quoted 5 lines

It is true that unsigned has all the disadvantages of "int" (it may be smaller than the size of the object" and all the disadvantages of "size_t" (it doesn't optimize as well because the compiler cannot exploit undefined behavior).

Quoted 1 line

I wouldn't call it harmful, but it feels like the worst of both worlds.

size_t is a pretty silly choice for a variable that's looping through the number of registers in the device, which is a value that would fit in 16 bits, let alone 32. I would probably have written this with 'int', but use of 'unsigned' doesn't come very high up on my list of things to complain about.

(since we use -fwrapv the compiler can't exploit undefined behaviour for signed loop indexes either.)

Hi Krzysztof,

Quoted 8 lines · Krzysztof Kozlowski

Short declaration of 'i' was in the middle of declarations with assignments. Make it a little bit more readable. No functional change.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 hw/misc/exynos4210_pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/misc/exynos4210_pmu.c static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset, · line 405

Quoted 11 lines

hw/misc/exynos4210_pmu.c +1 -1

index cbdfa0614600..60d1545c0baa 100644
@@ -401,8 +401,8 @@ static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
                                     unsigned size)
 {
     const Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
-    unsigned i;
     const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
+    unsigned i;

your change seems OK but while you are here, 'unsigned' is considered harmful since more than a decade. why not use 'size_t i' since PMU_NUM_OF_REGISTERS is indeed an ARRAY_SIZE()?

error_report() is preferred over fprintf() for logging errors. Also remove square brackets [] and additional new line characters in printed messages.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 hw/arm/exynos4_boards.c   |  6 +++---
 hw/timer/exynos4210_mct.c |  5 +++--
 hw/timer/exynos4210_pwm.c | 11 +++++------
 hw/timer/exynos4210_rtc.c | 16 +++++++---------
 4 files changed, 18 insertions(+), 20 deletions(-)

hw/arm/exynos4_boards.c +1 -0

index 0efa19405409..5cd94d402b52 100644
@@ -22,6 +22,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qemu-common.h"
 #include "cpu.h"
 #include "sysemu/sysemu.h"

hw/arm/exynos4_boards.c +0 -0

     }
 
     exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];

hw/timer/exynos4210_mct.c +1 -0

index 6069116942a4..48041ab036a6 100644
@@ -53,6 +53,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "qemu/main-loop.h"

hw/timer/exynos4210_mct.c +0 -0

         break;
 
     case L0_INT_CSTAT: case L1_INT_CSTAT:

hw/timer/exynos4210_pwm.c +5 -6

index f5765075c720..f8826e24e63d 100644
@@ -21,6 +21,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "qemu-common.h"

@@ -252,9 +253,8 @@ static uint64_t exynos4210_pwm_read(void *opaque, hwaddr offset,
         break;
 
     default:
-        fprintf(stderr,
-                "[exynos4210.pwm: bad read offset " TARGET_FMT_plx "]\n",
-                offset);
+        error_report("exynos4210.pwm: bad read offset " TARGET_FMT_plx,
+                     offset);
         break;
     }
     return value;

@@ -343,9 +343,8 @@ static void exynos4210_pwm_write(void *opaque, hwaddr offset,
         break;
 
     default:
-        fprintf(stderr,
-                "[exynos4210.pwm: bad write offset " TARGET_FMT_plx "]\n",
-                offset);
+        error_report("exynos4210.pwm: bad write offset " TARGET_FMT_plx,
+                     offset);
         break;
 
     }

hw/timer/exynos4210_rtc.c +7 -9

index 1a648c5d9e67..f4548cd555f4 100644
@@ -26,6 +26,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "qemu-common.h"

@@ -370,9 +371,8 @@ static uint64_t exynos4210_rtc_read(void *opaque, hwaddr offset,
         break;
 
     default:
-        fprintf(stderr,
-                "[exynos4210.rtc: bad read offset " TARGET_FMT_plx "]\n",
-                offset);
+        error_report("exynos4210.rtc: bad read offset " TARGET_FMT_plx,
+                     offset);
         break;
     }
     return value;

@@ -433,9 +433,8 @@ static void exynos4210_rtc_write(void *opaque, hwaddr offset,
         if (value > TICNT_THRESHOLD) {
             s->reg_ticcnt = value;
         } else {
-            fprintf(stderr,
-                    "[exynos4210.rtc: bad TICNT value %u ]\n",
-                    (uint32_t)value);
+            error_report("exynos4210.rtc: bad TICNT value %u",
+                         (uint32_t)value);
         }
         break;
 

@@ -500,9 +499,8 @@ static void exynos4210_rtc_write(void *opaque, hwaddr offset,
         break;
 
     default:
-        fprintf(stderr,
-                "[exynos4210.rtc: bad write offset " TARGET_FMT_plx "]\n",
-                offset);
+        error_report("exynos4210.rtc: bad write offset " TARGET_FMT_plx,
+                     offset);
         break;
 
     }

Hi Krzysztof,

Quoted 27 lines · Krzysztof Kozlowski

error_report() is preferred over fprintf() for logging errors. Also remove square brackets [] and additional new line characters in printed messages.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 hw/arm/exynos4_boards.c   |  6 +++---
 hw/timer/exynos4210_mct.c |  5 +++--
 hw/timer/exynos4210_pwm.c | 11 +++++------
 hw/timer/exynos4210_rtc.c | 16 +++++++---------
 4 files changed, 18 insertions(+), 20 deletions(-)

hw/arm/exynos4_boards.c +1 -0

index 0efa19405409..5cd94d402b52 100644
@@ -22,6 +22,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qemu-common.h"
 #include "cpu.h"
 #include "sysemu/sysemu.h"

@@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
     MachineClass *mc = MACHINE_GET_CLASS(machine);
 

hw/arm/exynos4_boards.c static Exynos4210State *exynos4_boards_init_common(MachineState *machine, · line 106

Quoted 6 lines

hw/arm/exynos4_boards.c +2 -3

     if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
-        fprintf(stderr, "%s board supports only %d CPU cores. Ignoring smp_cpus"
-                " value.\n",
-                mc->name, EXYNOS4210_NCPUS);
+        error_report("%s board supports only %d CPU cores. Ignoring smp_cpus value.",
+                     mc->name, EXYNOS4210_NCPUS);

ok, to inform the user

Quoted 17 lines

hw/arm/exynos4_boards.c +0 -0

     }
 
     exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];

hw/timer/exynos4210_mct.c +1 -0

index 6069116942a4..48041ab036a6 100644
@@ -53,6 +53,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "qemu/main-loop.h"

hw/timer/exynos4210_mct.c static void exynos4210_mct_write(void *opaque, hwaddr offset, · line 1369

Quoted 7 lines

hw/timer/exynos4210_mct.c +2 -2

@@ -1364,8 +1365,8 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
     case L0_TCNTO: case L1_TCNTO:
     case L0_ICNTO: case L1_ICNTO:
     case L0_FRCNTO: case L1_FRCNTO:
-        fprintf(stderr, "\n[exynos4210.mct: write to RO register "
-                TARGET_FMT_plx "]\n\n", offset);
+        error_report("exynos4210.mct: write to RO register " TARGET_FMT_plx,
+                     offset);

no need to annoy the user here, it seems better to use qemu_log_mask(LOG_UNIMP, ...

same for following diffs

error_report() is preferred over fprintf() for logging errors. Also remove square brackets [] and additional new line characters in printed messages.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---

Quoted 6 lines · Krzysztof Kozlowski

error_report() is preferred over fprintf() for logging errors. Also remove square brackets [] and additional new line characters in printed messages.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---

I don't see a 0/3 cover letter. When sending multiple patches, it's always best to package them in-reply-to the cover letter ('git config format.coverLetter auto' makes it easy to remember).

Quoted 5 lines

hw/arm/exynos4_boards.c +0 -0

@@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
     MachineClass *mc = MACHINE_GET_CLASS(machine);
 

hw/arm/exynos4_boards.c static Exynos4210State *exynos4_boards_init_common(MachineState *machine, · line 105

Quoted 5 lines

hw/arm/exynos4_boards.c +1 -3

     if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
-        fprintf(stderr, "%s board supports only %d CPU cores. Ignoring smp_cpus"
-                " value.\n",
-                mc->name, EXYNOS4210_NCPUS);
+        error_report("%s board supports only %d CPU cores. Ignoring smp_cpus value.",

Most uses of error_report() avoid trailing dot.

Quoted 3 lines · Eric Blake

I don't see a 0/3 cover letter. When sending multiple patches, it's always best to package them in-reply-to the cover letter ('git config format.coverLetter auto' makes it easy to remember).

The patches are trivial cleanups without any dependencies so the cover letter would be one sentence of "trivial cleanup". :)

If that's the practice in qemu, I can send them, no problem.