[Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()

[Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()

[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 <krzk@kernel.org>
---
 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(-)

diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index 0efa19405409..5cd94d402b52 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -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);
 
     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);
     }
 
     exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];
diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 6069116942a4..48041ab036a6 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -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"
@@ -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);
         break;
 
     case L0_INT_CSTAT: case L1_INT_CSTAT:
diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
index f5765075c720..f8826e24e63d 100644
--- a/hw/timer/exynos4210_pwm.c
+++ b/hw/timer/exynos4210_pwm.c
@@ -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;
 
     }
diff --git a/hw/timer/exynos4210_rtc.c b/hw/timer/exynos4210_rtc.c
index 1a648c5d9e67..f4548cd555f4 100644
--- a/hw/timer/exynos4210_rtc.c
+++ b/hw/timer/exynos4210_rtc.c
@@ -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;
 
     }
-- 
2.9.3

[Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables

[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 <krzk@kernel.org>
---
 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(-)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index b75f28d473bf..83e1be253255 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -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);
 }
diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
index f19a7062be3a..b057921e0504 100644
--- a/hw/intc/exynos4210_combiner.c
+++ b/hw/intc/exynos4210_combiner.c
@@ -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 */
diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index 2a55817b7660..432b8425d09d 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -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));
 }
diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
index e30dbc7d3d83..cbdfa0614600 100644
--- a/hw/misc/exynos4210_pmu.c
+++ b/hw/misc/exynos4210_pmu.c
@@ -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;
 
-- 
2.9.3

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables

[Philippe Mathieu-Daudé]

On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> 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 <krzk@kernel.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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(-)
>
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index b75f28d473bf..83e1be253255 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -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);
>  }
> diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
> index f19a7062be3a..b057921e0504 100644
> --- a/hw/intc/exynos4210_combiner.c
> +++ b/hw/intc/exynos4210_combiner.c
> @@ -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 */
> diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
> index 2a55817b7660..432b8425d09d 100644
> --- a/hw/intc/exynos4210_gic.c
> +++ b/hw/intc/exynos4210_gic.c
> @@ -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));
>  }
> diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
> index e30dbc7d3d83..cbdfa0614600 100644
> --- a/hw/misc/exynos4210_pmu.c
> +++ b/hw/misc/exynos4210_pmu.c
> @@ -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;
>
>

Re: [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables

[Peter Maydell]

On 5 March 2017 at 21:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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 <krzk@kernel.org>
> ---
>  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(-)
>
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index b75f28d473bf..83e1be253255 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -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;
>  }

Constifying this sort of thing is OK...

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

...but I disagree here somewhat...

> @@ -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).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables

[Krzysztof Kozlowski]

On Mon, Mar 06, 2017 at 09:44:49AM +0000, Peter Maydell wrote:
> On 5 March 2017 at 21:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > 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 <krzk@kernel.org>
> > ---
> >  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(-)
> >
> > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> > index b75f28d473bf..83e1be253255 100644
> > --- a/hw/char/exynos4210_uart.c
> > +++ b/hw/char/exynos4210_uart.c
> > @@ -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;
> >  }
> 
> Constifying this sort of thing is OK...
> 
> > -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;
> 
> ...but I disagree here somewhat...

This is a static function not modifying the state. Const in argument is
a clear indication of that for the readers.

> 
> > @@ -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

I get it, better to keep consistent style.

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

I think the code should be const-correct for current implementation
(following the HACKING guide). If we would have to predict future
changes, then almost no const would be possible to add...

Best regards,
Krzysztof

[Qemu-devel] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability

[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 <krzk@kernel.org>
---
 hw/misc/exynos4210_pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
index cbdfa0614600..60d1545c0baa 100644
--- a/hw/misc/exynos4210_pmu.c
+++ b/hw/misc/exynos4210_pmu.c
@@ -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;
 
     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) {
-- 
2.9.3

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability

[Philippe Mathieu-Daudé]

Hi Krzysztof,

On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> 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 <krzk@kernel.org>
> ---
>  hw/misc/exynos4210_pmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
> index cbdfa0614600..60d1545c0baa 100644
> --- a/hw/misc/exynos4210_pmu.c
> +++ b/hw/misc/exynos4210_pmu.c
> @@ -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()?

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

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability

[Peter Maydell]

On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> your change seems OK but while you are here, 'unsigned' is considered
> harmful since more than a decade.

Considered harmful by who, and why?

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability

[Paolo Bonzini]

On 06/03/2017 10:46, Peter Maydell wrote:
> On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> your change seems OK but while you are here, 'unsigned' is considered
>> harmful since more than a decade.
> 
> 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.

Paolo

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability

[Peter Maydell]

On 6 March 2017 at 13:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/03/2017 10:46, Peter Maydell wrote:
>> On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> your change seems OK but while you are here, 'unsigned' is considered
>>> harmful since more than a decade.
>>
>> 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.

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

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability

[Philippe Mathieu-Daudé]

On 03/06/2017 10:16 AM, Peter Maydell wrote:
> On 6 March 2017 at 13:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 06/03/2017 10:46, Peter Maydell wrote:
>>> On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> your change seems OK but while you are here, 'unsigned' is considered
>>>> harmful since more than a decade.
>>>
>>> Considered harmful by who, and why?

sorry to cry wolf :)

>> 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.
>
> 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.

silly would be "typeof(PMU_NUM_OF_REGISTERS) i;" :p

I got your point about the complain priority list.

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

regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability

[Krzysztof Kozlowski]

On Mon, Mar 06, 2017 at 01:06:29AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Krzysztof,
> 
> On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> > 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 <krzk@kernel.org>
> > ---
> >  hw/misc/exynos4210_pmu.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
> > index cbdfa0614600..60d1545c0baa 100644
> > --- a/hw/misc/exynos4210_pmu.c
> > +++ b/hw/misc/exynos4210_pmu.c
> > @@ -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()?

It is a loop counter. I think "unsigned int" for loop counters is the
most common pattern - easy to understand the true meaning of variable.
I can expand it to "unsigned int" but that wouldn't change much...

Let's look at current style:
$ git grep "unsigned int i[,;$]" | wc -l
188

$ git grep "unsigned i[,;$]" | wc -l
92

$ git grep "size_t i[,;$]" | wc -l
114

... where do we go now?



Best regards,
Krzysztof

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()

[Philippe Mathieu-Daudé]

Hi Krzysztof,

On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> 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 <krzk@kernel.org>
> ---
>  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(-)
>
> diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
> index 0efa19405409..5cd94d402b52 100644
> --- a/hw/arm/exynos4_boards.c
> +++ b/hw/arm/exynos4_boards.c
> @@ -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);
>
>      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

>      }
>
>      exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 6069116942a4..48041ab036a6 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -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"
> @@ -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

>          break;
>
>      case L0_INT_CSTAT: case L1_INT_CSTAT:
> diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
> index f5765075c720..f8826e24e63d 100644
> --- a/hw/timer/exynos4210_pwm.c
> +++ b/hw/timer/exynos4210_pwm.c
> @@ -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;
>
>      }
> diff --git a/hw/timer/exynos4210_rtc.c b/hw/timer/exynos4210_rtc.c
> index 1a648c5d9e67..f4548cd555f4 100644
> --- a/hw/timer/exynos4210_rtc.c
> +++ b/hw/timer/exynos4210_rtc.c
> @@ -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;
>
>      }
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()

[Krzysztof Kozlowski]

On Sun, Mar 05, 2017 at 06:56:09PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Krzysztof,
> 
> On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> > 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 <krzk@kernel.org>
> > ---
> >  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(-)
> > 
> > diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
> > index 0efa19405409..5cd94d402b52 100644
> > --- a/hw/arm/exynos4_boards.c
> > +++ b/hw/arm/exynos4_boards.c
> > @@ -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);
> > 
> >      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

By inform you mean error_report or some lower level?

> 
> >      }
> > 
> >      exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];
> > diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> > index 6069116942a4..48041ab036a6 100644
> > --- a/hw/timer/exynos4210_mct.c
> > +++ b/hw/timer/exynos4210_mct.c
> > @@ -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"
> > @@ -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

Sure, I'll fix that as well.

Thanks for review.


Best regards,
Krzysztof

Re: [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()

[Eric Blake]

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

On 03/05/2017 03:48 PM, Krzysztof Kozlowski wrote:
> 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 <krzk@kernel.org>
> ---

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


> @@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>  
>      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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()

[Krzysztof Kozlowski]

On Mon, Mar 06, 2017 at 10:55:12AM -0600, Eric Blake wrote:
> On 03/05/2017 03:48 PM, Krzysztof Kozlowski wrote:
> > 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 <krzk@kernel.org>
> > ---
> 
> 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.

> 
> 
> > @@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
> >      MachineClass *mc = MACHINE_GET_CLASS(machine);
> >  
> >      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.

Sure, I can rephrase it to one sentence without dot.

Best regards,
Krzysztof

Re: [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()

[Peter Maydell]

On 6 March 2017 at 17:07, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Mon, Mar 06, 2017 at 10:55:12AM -0600, Eric Blake wrote:
>> 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.

Yeah, cover letter always for multipatch sets, please. We have
automated tooling that relies on this (and also manual humans).

thanks
-- PMM