From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48924) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9ZaE-0000zm-6g for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:31:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9Za8-000568-4K for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:31:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53044) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9Za7-000563-Sa for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:31:00 -0500 References: <1450354795-31608-1-git-send-email-armbru@redhat.com> <1450354795-31608-3-git-send-email-armbru@redhat.com> From: Thomas Huth Message-ID: <5672C720.7060106@redhat.com> Date: Thu, 17 Dec 2015 15:30:56 +0100 MIME-Version: 1.0 In-Reply-To: <1450354795-31608-3-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 02/13] omap: Don't use hw_error() in device init() methods List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: Peter Maydell , Markus Armbruster On 17/12/15 13:19, Markus Armbruster wrote: > Device init() methods aren't supposed to call hw_error(), they should > report the error and fail cleanly. Do that. > > The errors are all device misconfiguration. All callers use > qdev_init_nofail(), so this patch merely converts hw_error() crashes > into &error_abort crashes. Improvement, because now it crashes closer > to where the misconfiguration bug would be, and a few more bad > examples of hw_error() use are gone. > > Cc: Peter Maydell > Signed-off-by: Markus Armbruster > Reviewed-by: Peter Maydell > --- > 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(-) > > 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) What about omap_gpio_init() ? There is also a hw_error in there! > 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++) { You're now using modulecount here -^ > + 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; ... but it is initialized just here. So I think it is wrong that you moved the "!s->fclk[i]" before that. > 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); > + Unrelated whitespace changes? > 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); > } > + dito. Thomas