diff mbox

[02/12] omap: Don't use hw_error() in device init() methods

Message ID 1449743372-17169-3-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Dec. 10, 2015, 10:29 a.m. UTC 
Device init() methods aren't supposed to call hw_error(), they should
report the error and fail cleanly.  Do that.

Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/gpio/omap_gpio.c | 19 +++++++++++++++----
 hw/i2c/omap_i2c.c   |  8 ++++++--
 hw/intc/omap_intc.c | 10 +++++++---
 3 files changed, 28 insertions(+), 9 deletions(-)

Comments

Peter Maydell Dec. 10, 2015, 10:42 a.m. UTC | #1 
On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
> Device init() methods aren't supposed to call hw_error(), they should
> report the error and fail cleanly.  Do that.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

These are all really "QEMU bug" error paths -- the only place
that uses omap_i2c is the omap SoC init, you can't create the
device on the command line, and so we'll only get these errors
if there's a bug in a function like omap2420_mpu_init. But
I don't object to the patch in principle.

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

thanks
-- PMM
Markus Armbruster Dec. 10, 2015, 12:44 p.m. UTC | #2 
Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
>> Device init() methods aren't supposed to call hw_error(), they should
>> report the error and fail cleanly.  Do that.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> These are all really "QEMU bug" error paths -- the only place
> that uses omap_i2c is the omap SoC init, you can't create the
> device on the command line, and so we'll only get these errors
> if there's a bug in a function like omap2420_mpu_init. But
> I don't object to the patch in principle.

All callers use qdev_init_nofail(), so this patch merely converts the
hw_error() crash into an &error_abort crash.  Improvement, because now
it crashes closer to where the bug is.  I can spell that out in the
commit message if you like.

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

Thanks!
diff mbox

Patch

diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
index 3c53898..1e0654b 100644
--- a/hw/gpio/omap_gpio.c
+++ b/hw/gpio/omap_gpio.c
@@ -21,6 +21,7 @@ 
 #include "hw/hw.h"
 #include "hw/arm/omap.h"
 #include "hw/sysbus.h"
+#include "qemu/error-report.h"
 
 struct omap_gpio_s {
     qemu_irq irq;
@@ -700,8 +701,17 @@  static int omap2_gpio_init(SysBusDevice *sbd)
     int i;
 
     if (!s->iclk) {
-        hw_error("omap2-gpio: iclk not connected\n");
+        error_report("omap2-gpio: iclk not connected");
+        return -1;
     }
+
+    for (i = 0; i < s->modulecount; i++) {
+        if (!s->fclk[i]) {
+            error_report("omap2-gpio: fclk%d not connected", i);
+            return -1;
+        }
+    }
+
     if (s->mpu_model < omap3430) {
         s->modulecount = (s->mpu_model < omap2430) ? 4 : 5;
         memory_region_init_io(&s->iomem, OBJECT(s), &omap2_gpif_top_ops, s,
@@ -710,15 +720,15 @@  static int omap2_gpio_init(SysBusDevice *sbd)
     } else {
         s->modulecount = 6;
     }
+
     s->modules = g_new0(struct omap2_gpio_s, s->modulecount);
     s->handler = g_new0(qemu_irq, s->modulecount * 32);
     qdev_init_gpio_in(dev, omap2_gpio_set, s->modulecount * 32);
     qdev_init_gpio_out(dev, s->handler, s->modulecount * 32);
+
     for (i = 0; i < s->modulecount; i++) {
         struct omap2_gpio_s *m = &s->modules[i];
-        if (!s->fclk[i]) {
-            hw_error("omap2-gpio: fclk%d not connected\n", i);
-        }
+
         m->revision = (s->mpu_model < omap3430) ? 0x18 : 0x25;
         m->handler = &s->handler[i * 32];
         sysbus_init_irq(sbd, &m->irq[0]); /* mpu irq */
@@ -728,6 +738,7 @@  static int omap2_gpio_init(SysBusDevice *sbd)
                               "omap.gpio-module", 0x1000);
         sysbus_init_mmio(sbd, &m->iomem);
     }
+
     return 0;
 }
 
diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
index b6f544a..8b0b146 100644
--- a/hw/i2c/omap_i2c.c
+++ b/hw/i2c/omap_i2c.c
@@ -20,6 +20,7 @@ 
 #include "hw/i2c/i2c.h"
 #include "hw/arm/omap.h"
 #include "hw/sysbus.h"
+#include "qemu/error-report.h"
 
 #define TYPE_OMAP_I2C "omap_i2c"
 #define OMAP_I2C(obj) OBJECT_CHECK(OMAPI2CState, (obj), TYPE_OMAP_I2C)
@@ -449,12 +450,15 @@  static int omap_i2c_init(SysBusDevice *sbd)
     OMAPI2CState *s = OMAP_I2C(dev);
 
     if (!s->fclk) {
-        hw_error("omap_i2c: fclk not connected\n");
+        error_report("omap_i2c: fclk not connected");
+        return -1;
     }
     if (s->revision >= OMAP2_INTR_REV && !s->iclk) {
         /* Note that OMAP1 doesn't have a separate interface clock */
-        hw_error("omap_i2c: iclk not connected\n");
+        error_report("omap_i2c: iclk not connected");
+        return -1;
     }
+
     sysbus_init_irq(sbd, &s->irq);
     sysbus_init_irq(sbd, &s->drq[0]);
     sysbus_init_irq(sbd, &s->drq[1]);
diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c
index e9b38a3..07b6272 100644
--- a/hw/intc/omap_intc.c
+++ b/hw/intc/omap_intc.c
@@ -20,6 +20,7 @@ 
 #include "hw/hw.h"
 #include "hw/arm/omap.h"
 #include "hw/sysbus.h"
+#include "qemu/error-report.h"
 
 /* Interrupt Handlers */
 struct omap_intr_handler_bank_s {
@@ -367,7 +368,8 @@  static int omap_intc_init(SysBusDevice *sbd)
     struct omap_intr_handler_s *s = OMAP_INTC(dev);
 
     if (!s->iclk) {
-        hw_error("omap-intc: clk not connected\n");
+        error_report("omap-intc: clk not connected");
+        return -1;
     }
     s->nbanks = 1;
     sysbus_init_irq(sbd, &s->parent_intr[0]);
@@ -608,10 +610,12 @@  static int omap2_intc_init(SysBusDevice *sbd)
     struct omap_intr_handler_s *s = OMAP_INTC(dev);
 
     if (!s->iclk) {
-        hw_error("omap2-intc: iclk not connected\n");
+        error_report("omap2-intc: iclk not connected");
+        return -1;
     }
     if (!s->fclk) {
-        hw_error("omap2-intc: fclk not connected\n");
+        error_report("omap2-intc: fclk not connected");
+        return -1;
     }
     s->level_only = 1;
     s->nbanks = 3;