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

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> 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 <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Peter Maydell <address@hidden>
>> ---
>>  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!

Missed in the noise... I'll include it in v3.

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

Hand me the brown paperbag...

I still like to move it, because checking properties before modifying
the VM is good practice.  But I do need to move the computation of
modulecount, too.

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

I wouldn't call them unrelated; I'm touching this code.  But if they're
unwelcome, I'm happy to drop them :)

Thanks for catching my mistakes!