Thomas Huth <th...@redhat.com> 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 <peter.mayd...@linaro.org>
>> Signed-off-by: Markus Armbruster <arm...@pond.sub.org>
>> Reviewed-by: Peter Maydell <peter.mayd...@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!
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!
