Re: [Qemu-devel] [PATCH v2 02/13] omap: Don't use hw_error() in device init() methods

[Thomas Huth <thuth@redhat.com>]

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 <peter.maydell@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  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