| Message ID | 20240208181245.96617-7-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw: Strengthen SysBus & QBus API | expand |
On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote: > We should not wire IRQs on unrealized device. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/misc/macio/macio.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c > index c9f22f8515..db662a2065 100644 > --- a/hw/misc/macio/macio.c > +++ b/hw/misc/macio/macio.c > @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide, > Error **errp) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(ide); > + bool success; > > - sysbus_connect_irq(sbd, 0, irq0); > - sysbus_connect_irq(sbd, 1, irq1); > qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid); > object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma), > &error_abort); > macio_ide_register_dma(ide); > + success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); If realize is unsuccessful can you connect irqs if device may be unrealized? So maybe either the next two lines should be in an if block or drop the success flag and return false here if (!qdev_realize) and true at end of func? Regards, BALATON Zoltan > + sysbus_connect_irq(sbd, 0, irq0); > + sysbus_connect_irq(sbd, 1, irq1); > > - return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); > + return success; > } > > static void macio_oldworld_realize(PCIDevice *d, Error **errp) >
On 8/2/24 19:33, BALATON Zoltan wrote: > On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote: >> We should not wire IRQs on unrealized device. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/misc/macio/macio.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c >> index c9f22f8515..db662a2065 100644 >> --- a/hw/misc/macio/macio.c >> +++ b/hw/misc/macio/macio.c >> @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, >> MACIOIDEState *ide, >> Error **errp) >> { >> SysBusDevice *sbd = SYS_BUS_DEVICE(ide); >> + bool success; >> >> - sysbus_connect_irq(sbd, 0, irq0); >> - sysbus_connect_irq(sbd, 1, irq1); >> qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid); >> object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma), >> &error_abort); >> macio_ide_register_dma(ide); >> + success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); > > If realize is unsuccessful can you connect irqs if device may be > unrealized? So maybe either the next two lines should be in an if block > or drop the success flag and return false here if (!qdev_realize) and > true at end of func? Doh you are right, thanks! >> + sysbus_connect_irq(sbd, 0, irq0); >> + sysbus_connect_irq(sbd, 1, irq1); >> >> - return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); >> + return success; >> } >> >> static void macio_oldworld_realize(PCIDevice *d, Error **errp) >>
On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote: > We should not wire IRQs on unrealized device. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/misc/macio/macio.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c > index c9f22f8515..db662a2065 100644 > --- a/hw/misc/macio/macio.c > +++ b/hw/misc/macio/macio.c > @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide, > Error **errp) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(ide); > + bool success; > > - sysbus_connect_irq(sbd, 0, irq0); > - sysbus_connect_irq(sbd, 1, irq1); > qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid); > object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma), > &error_abort); > macio_ide_register_dma(ide); > + success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); > + sysbus_connect_irq(sbd, 0, irq0); > + sysbus_connect_irq(sbd, 1, irq1); > > - return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); > + return success; > } > > static void macio_oldworld_realize(PCIDevice *d, Error **errp) I see that Zoltan has already commented about checking the success of qdev_realise() before wiring the sysbus IRQs, so with that fixed: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c index c9f22f8515..db662a2065 100644 --- a/hw/misc/macio/macio.c +++ b/hw/misc/macio/macio.c @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide, Error **errp) { SysBusDevice *sbd = SYS_BUS_DEVICE(ide); + bool success; - sysbus_connect_irq(sbd, 0, irq0); - sysbus_connect_irq(sbd, 1, irq1); qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid); object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma), &error_abort); macio_ide_register_dma(ide); + success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); + sysbus_connect_irq(sbd, 0, irq0); + sysbus_connect_irq(sbd, 1, irq1); - return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp); + return success; } static void macio_oldworld_realize(PCIDevice *d, Error **errp)
We should not wire IRQs on unrealized device. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/macio/macio.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)