diff mbox series

[v3,16/17] hw/display/xlnx_dp: Move problematic code from instance_init to realize

Message ID 1531745974-17187-17-git-send-email-thuth@redhat.com
State New
Headers show
Series Fix crashes with introspection of ARM devices | expand

Commit Message

Thomas Huth July 16, 2018, 12:59 p.m. UTC 
From: Paolo Bonzini <pbonzini@redhat.com>

aux_create_slave() calls qdev_init_nofail() which in turn "realizes"
the corresponding object. This is unlike qdev_create(), and it is wrong
because qdev_init_nofail() must not be called from an instance_init
function.  Move qdev_init_nofail() and the subsequent aux_map_slave into
the caller's realize function.

There are two more bugs that needs to be fixed here, too, where the
objects are created but not added as children.  Therefore when
you call object_unparent on them, nothing happens.

In particular dpcd and edid give you an infinite loop in bus_unparent,
because device_unparent is not called and does not remove them from
the list of devices on the bus.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[thuth: Added Paolo's fixup for the dpcd and edid unparenting]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/xlnx_dp.c     |  8 +++++++-
 hw/misc/auxbus.c         | 18 ++++++++++++------
 include/hw/misc/auxbus.h | 14 +++++++++++++-
 3 files changed, 32 insertions(+), 8 deletions(-)

Comments

Peter Maydell July 16, 2018, 1:56 p.m. UTC | #1 
On 16 July 2018 at 13:59, Thomas Huth <thuth@redhat.com> wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> aux_create_slave() calls qdev_init_nofail() which in turn "realizes"
> the corresponding object. This is unlike qdev_create(), and it is wrong
> because qdev_init_nofail() must not be called from an instance_init
> function.  Move qdev_init_nofail() and the subsequent aux_map_slave into
> the caller's realize function.
>
> There are two more bugs that needs to be fixed here, too, where the
> objects are created but not added as children.  Therefore when
> you call object_unparent on them, nothing happens.
>
> In particular dpcd and edid give you an infinite loop in bus_unparent,
> because device_unparent is not called and does not remove them from
> the list of devices on the bus.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [thuth: Added Paolo's fixup for the dpcd and edid unparenting]
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Alistair Francis July 16, 2018, 10:06 p.m. UTC | #2 
On Mon, Jul 16, 2018 at 5:59 AM, Thomas Huth <thuth@redhat.com> wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
>
> aux_create_slave() calls qdev_init_nofail() which in turn "realizes"
> the corresponding object. This is unlike qdev_create(), and it is wrong
> because qdev_init_nofail() must not be called from an instance_init
> function.  Move qdev_init_nofail() and the subsequent aux_map_slave into
> the caller's realize function.
>
> There are two more bugs that needs to be fixed here, too, where the
> objects are created but not added as children.  Therefore when
> you call object_unparent on them, nothing happens.
>
> In particular dpcd and edid give you an infinite loop in bus_unparent,
> because device_unparent is not called and does not remove them from
> the list of devices on the bus.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [thuth: Added Paolo's fixup for the dpcd and edid unparenting]
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/xlnx_dp.c     |  8 +++++++-
>  hw/misc/auxbus.c         | 18 ++++++++++++------
>  include/hw/misc/auxbus.h | 14 +++++++++++++-
>  3 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 5130122..6439bd0 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1234,9 +1234,12 @@ static void xlnx_dp_init(Object *obj)
>      /*
>       * Initialize DPCD and EDID..
>       */
> -    s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000));
> +    s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd"));
> +    object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd), NULL);
> +
>      s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc"));
>      i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
> +    object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid), NULL);
>
>      fifo8_create(&s->rx_fifo, 16);
>      fifo8_create(&s->tx_fifo, 16);
> @@ -1248,6 +1251,9 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>      DisplaySurface *surface;
>      struct audsettings as;
>
> +    qdev_init_nofail(DEVICE(s->dpcd));
> +    aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
> +
>      s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>      surface = qemu_console_surface(s->console);
>      xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
> index b8a8721..0e56d9a 100644
> --- a/hw/misc/auxbus.c
> +++ b/hw/misc/auxbus.c
> @@ -32,6 +32,7 @@
>  #include "hw/misc/auxbus.h"
>  #include "hw/i2c/i2c.h"
>  #include "monitor/monitor.h"
> +#include "qapi/error.h"
>
>  #ifndef DEBUG_AUX
>  #define DEBUG_AUX 0
> @@ -63,9 +64,14 @@ static void aux_bus_class_init(ObjectClass *klass, void *data)
>  AUXBus *aux_init_bus(DeviceState *parent, const char *name)
>  {
>      AUXBus *bus;
> +    Object *auxtoi2c;
>
>      bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name));
> -    bus->bridge = AUXTOI2C(qdev_create(BUS(bus), TYPE_AUXTOI2C));
> +    auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c",
> +                                     &error_abort, NULL);
> +    qdev_set_parent_bus(DEVICE(auxtoi2c), BUS(bus));
> +
> +    bus->bridge = AUXTOI2C(auxtoi2c);
>
>      /* Memory related. */
>      bus->aux_io = g_malloc(sizeof(*bus->aux_io));
> @@ -74,9 +80,11 @@ AUXBus *aux_init_bus(DeviceState *parent, const char *name)
>      return bus;
>  }
>
> -static void aux_bus_map_device(AUXBus *bus, AUXSlave *dev, hwaddr addr)
> +void aux_map_slave(AUXSlave *aux_dev, hwaddr addr)
>  {
> -    memory_region_add_subregion(bus->aux_io, addr, dev->mmio);
> +    DeviceState *dev = DEVICE(aux_dev);
> +    AUXBus *bus = AUX_BUS(qdev_get_parent_bus(dev));
> +    memory_region_add_subregion(bus->aux_io, addr, aux_dev->mmio);
>  }
>
>  static bool aux_bus_is_bridge(AUXBus *bus, DeviceState *dev)
> @@ -260,15 +268,13 @@ static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int indent)
>                     memory_region_size(s->mmio));
>  }
>
> -DeviceState *aux_create_slave(AUXBus *bus, const char *type, uint32_t addr)
> +DeviceState *aux_create_slave(AUXBus *bus, const char *type)
>  {
>      DeviceState *dev;
>
>      dev = DEVICE(object_new(type));
>      assert(dev);
>      qdev_set_parent_bus(dev, &bus->qbus);
> -    qdev_init_nofail(dev);
> -    aux_bus_map_device(AUX_BUS(qdev_get_parent_bus(dev)), AUX_SLAVE(dev), addr);
>      return dev;
>  }
>
> diff --git a/include/hw/misc/auxbus.h b/include/hw/misc/auxbus.h
> index 68ade8a..c15b444 100644
> --- a/include/hw/misc/auxbus.h
> +++ b/include/hw/misc/auxbus.h
> @@ -123,6 +123,18 @@ I2CBus *aux_get_i2c_bus(AUXBus *bus);
>   */
>  void aux_init_mmio(AUXSlave *aux_slave, MemoryRegion *mmio);
>
> -DeviceState *aux_create_slave(AUXBus *bus, const char *name, uint32_t addr);
> +/* aux_create_slave: Create a new device on an AUX bus
> + *
> + * @bus The AUX bus for the new device.
> + * @name The type of the device to be created.
> + */
> +DeviceState *aux_create_slave(AUXBus *bus, const char *name);
> +
> +/* aux_map_slave: Map the mmio for an AUX slave on the bus.
> + *
> + * @dev The AUX slave.
> + * @addr The address for the slave's mmio.
> + */
> +void aux_map_slave(AUXSlave *dev, hwaddr addr);
>
>  #endif /* HW_MISC_AUXBUS_H */
> --
> 1.8.3.1
>
>
diff mbox series

Patch

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 5130122..6439bd0 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1234,9 +1234,12 @@  static void xlnx_dp_init(Object *obj)
     /*
      * Initialize DPCD and EDID..
      */
-    s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000));
+    s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd"));
+    object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd), NULL);
+
     s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc"));
     i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
+    object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid), NULL);
 
     fifo8_create(&s->rx_fifo, 16);
     fifo8_create(&s->tx_fifo, 16);
@@ -1248,6 +1251,9 @@  static void xlnx_dp_realize(DeviceState *dev, Error **errp)
     DisplaySurface *surface;
     struct audsettings as;
 
+    qdev_init_nofail(DEVICE(s->dpcd));
+    aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
+
     s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
     surface = qemu_console_surface(s->console);
     xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index b8a8721..0e56d9a 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -32,6 +32,7 @@ 
 #include "hw/misc/auxbus.h"
 #include "hw/i2c/i2c.h"
 #include "monitor/monitor.h"
+#include "qapi/error.h"
 
 #ifndef DEBUG_AUX
 #define DEBUG_AUX 0
@@ -63,9 +64,14 @@  static void aux_bus_class_init(ObjectClass *klass, void *data)
 AUXBus *aux_init_bus(DeviceState *parent, const char *name)
 {
     AUXBus *bus;
+    Object *auxtoi2c;
 
     bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name));
-    bus->bridge = AUXTOI2C(qdev_create(BUS(bus), TYPE_AUXTOI2C));
+    auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c",
+                                     &error_abort, NULL);
+    qdev_set_parent_bus(DEVICE(auxtoi2c), BUS(bus));
+
+    bus->bridge = AUXTOI2C(auxtoi2c);
 
     /* Memory related. */
     bus->aux_io = g_malloc(sizeof(*bus->aux_io));
@@ -74,9 +80,11 @@  AUXBus *aux_init_bus(DeviceState *parent, const char *name)
     return bus;
 }
 
-static void aux_bus_map_device(AUXBus *bus, AUXSlave *dev, hwaddr addr)
+void aux_map_slave(AUXSlave *aux_dev, hwaddr addr)
 {
-    memory_region_add_subregion(bus->aux_io, addr, dev->mmio);
+    DeviceState *dev = DEVICE(aux_dev);
+    AUXBus *bus = AUX_BUS(qdev_get_parent_bus(dev));
+    memory_region_add_subregion(bus->aux_io, addr, aux_dev->mmio);
 }
 
 static bool aux_bus_is_bridge(AUXBus *bus, DeviceState *dev)
@@ -260,15 +268,13 @@  static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int indent)
                    memory_region_size(s->mmio));
 }
 
-DeviceState *aux_create_slave(AUXBus *bus, const char *type, uint32_t addr)
+DeviceState *aux_create_slave(AUXBus *bus, const char *type)
 {
     DeviceState *dev;
 
     dev = DEVICE(object_new(type));
     assert(dev);
     qdev_set_parent_bus(dev, &bus->qbus);
-    qdev_init_nofail(dev);
-    aux_bus_map_device(AUX_BUS(qdev_get_parent_bus(dev)), AUX_SLAVE(dev), addr);
     return dev;
 }
 
diff --git a/include/hw/misc/auxbus.h b/include/hw/misc/auxbus.h
index 68ade8a..c15b444 100644
--- a/include/hw/misc/auxbus.h
+++ b/include/hw/misc/auxbus.h
@@ -123,6 +123,18 @@  I2CBus *aux_get_i2c_bus(AUXBus *bus);
  */
 void aux_init_mmio(AUXSlave *aux_slave, MemoryRegion *mmio);
 
-DeviceState *aux_create_slave(AUXBus *bus, const char *name, uint32_t addr);
+/* aux_create_slave: Create a new device on an AUX bus
+ *
+ * @bus The AUX bus for the new device.
+ * @name The type of the device to be created.
+ */
+DeviceState *aux_create_slave(AUXBus *bus, const char *name);
+
+/* aux_map_slave: Map the mmio for an AUX slave on the bus.
+ *
+ * @dev The AUX slave.
+ * @addr The address for the slave's mmio.
+ */
+void aux_map_slave(AUXSlave *dev, hwaddr addr);
 
 #endif /* HW_MISC_AUXBUS_H */