Patchwork [v3,09/10] isa: Clean up use of cannot_instantiate_with_device_add_yet

login
register
mail settings
Submitter Markus Armbruster
Date Oct. 30, 2013, 4:28 p.m.
Message ID <1383150524-16250-10-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/287286/
State New
Headers show

Comments

Markus Armbruster - Oct. 30, 2013, 4:28 p.m.
From: Markus Armbruster <armbru@redhat.com>

Drop it when there's no obvious reason why device_add could not work.
Else keep and document why.

* isa-fdc: drop

* i8042: drop, even though its I/O base is hardcoded (because you
  could conceivably still add one to a board that has none), and even
  though PC board code wires up the A20 line (because that wiring is
  optional)

* port92: keep because it needs additional wiring by port92_init()

* mc146818rtc: keep because it needs to be wired up by rtc_init()

* m48t59_isa: keep because needs to be wired up by m48t59_init_isa()

* isa-pit, kvm-pit: keep (in their abstract base pic-common) because
  the PIT needs additional wiring by board code, depending on HPET
  presence

* pcspk: keep because of pointer property pit, and because realize
  sets global pcspk_state

* vmmouse: keep because of pointer property ps2_mouse

* vmport: keep because realize sets global port_state

* isa-i8259, kvm-i8259: keep (in their abstract base pic-common),
  because the PICs' IRQ input lines are set up by board code, and the
  wiring of the slave to the master is hard-coded in device model code

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/audio/pcspk.c        | 3 ++-
 hw/block/fdc.c          | 1 -
 hw/i386/pc.c            | 7 ++++++-
 hw/input/pckbd.c        | 1 -
 hw/input/vmmouse.c      | 3 ++-
 hw/intc/i8259_common.c  | 8 +++++++-
 hw/misc/vmport.c        | 3 ++-
 hw/timer/i8254_common.c | 7 ++++++-
 hw/timer/m48t59.c       | 3 ++-
 hw/timer/mc146818rtc.c  | 3 ++-
 10 files changed, 29 insertions(+), 10 deletions(-)
Marcel Apfelbaum - Oct. 31, 2013, 12:44 p.m.
On Wed, 2013-10-30 at 17:28 +0100, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Drop it when there's no obvious reason why device_add could not work.
> Else keep and document why.
> 
> * isa-fdc: drop
> 
> * i8042: drop, even though its I/O base is hardcoded (because you
>   could conceivably still add one to a board that has none), and even
>   though PC board code wires up the A20 line (because that wiring is
>   optional)
> 
> * port92: keep because it needs additional wiring by port92_init()
> 
> * mc146818rtc: keep because it needs to be wired up by rtc_init()
> 
> * m48t59_isa: keep because needs to be wired up by m48t59_init_isa()
> 
> * isa-pit, kvm-pit: keep (in their abstract base pic-common) because
>   the PIT needs additional wiring by board code, depending on HPET
>   presence
> 
> * pcspk: keep because of pointer property pit, and because realize
>   sets global pcspk_state
> 
> * vmmouse: keep because of pointer property ps2_mouse
> 
> * vmport: keep because realize sets global port_state
> 
> * isa-i8259, kvm-i8259: keep (in their abstract base pic-common),
>   because the PICs' IRQ input lines are set up by board code, and the
>   wiring of the slave to the master is hard-coded in device model code

I agree with the intend of this patch, but I am not familiar
with this code, so I am not comfortable reviewing it, anyone else?

Thanks,
Marcel 

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/audio/pcspk.c        | 3 ++-
>  hw/block/fdc.c          | 1 -
>  hw/i386/pc.c            | 7 ++++++-
>  hw/input/pckbd.c        | 1 -
>  hw/input/vmmouse.c      | 3 ++-
>  hw/intc/i8259_common.c  | 8 +++++++-
>  hw/misc/vmport.c        | 3 ++-
>  hw/timer/i8254_common.c | 7 ++++++-
>  hw/timer/m48t59.c       | 3 ++-
>  hw/timer/mc146818rtc.c  | 3 ++-
>  10 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
> index 8e3e178..f980d66 100644
> --- a/hw/audio/pcspk.c
> +++ b/hw/audio/pcspk.c
> @@ -192,8 +192,9 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
>  
>      dc->realize = pcspk_realizefn;
>      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->props = pcspk_properties;
> +    /* Reason: pointer property "pit", realize sets global pcspk_state */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo pcspk_info = {
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 86f4920..592b58f 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2234,7 +2234,6 @@ static void isabus_fdc_class_init(ObjectClass *klass, void *data)
>  
>      dc->realize = isabus_fdc_realize;
>      dc->fw_name = "fdc";
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->reset = fdctrl_external_reset_isa;
>      dc->vmsd = &vmstate_isa_fdc;
>      dc->props = isa_fdc_properties;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fe33843..20402ba 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -544,10 +544,15 @@ static void port92_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->realize = port92_realizefn;
>      dc->reset = port92_reset;
>      dc->vmsd = &vmstate_port92_isa;
> +    /*
> +     * Reason: unlike ordinary ISA devices, this one needs additional
> +     * wiring: its A20 output line needs to be wired up by
> +     * port92_init().
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo port92_info = {
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index dee31a6..655b8c5 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -522,7 +522,6 @@ static void i8042_class_initfn(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = i8042_realizefn;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->vmsd = &vmstate_kbd_isa;
>  }
>  
> diff --git a/hw/input/vmmouse.c b/hw/input/vmmouse.c
> index 600e4a2..6a50533 100644
> --- a/hw/input/vmmouse.c
> +++ b/hw/input/vmmouse.c
> @@ -282,10 +282,11 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = vmmouse_realizefn;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->reset = vmmouse_reset;
>      dc->vmsd = &vmstate_vmmouse;
>      dc->props = vmmouse_properties;
> +    /* Reason: pointer property "ps2_mouse" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo vmmouse_info = {
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index 2acdbfe..9d29399 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
> @@ -135,9 +135,15 @@ static void pic_common_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->vmsd = &vmstate_pic_common;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->props = pic_properties_common;
>      dc->realize = pic_common_realize;
> +    /*
> +     * Reason: unlike ordinary ISA devices, the PICs need additional
> +     * wiring: its IRQ input lines are set up by board code, and the
> +     * wiring of the slave to the master is hard-coded in device model
> +     * code.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo pic_common_type = {
> diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
> index 94ae6ae..cd5716a 100644
> --- a/hw/misc/vmport.c
> +++ b/hw/misc/vmport.c
> @@ -162,7 +162,8 @@ static void vmport_class_initfn(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = vmport_realizefn;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
> +    /* Reason: realize sets global port_state */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo vmport_info = {
> diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
> index dc2196c..9db5c9d 100644
> --- a/hw/timer/i8254_common.c
> +++ b/hw/timer/i8254_common.c
> @@ -282,7 +282,12 @@ static void pit_common_class_init(ObjectClass *klass, void *data)
>  
>      dc->realize = pit_common_realize;
>      dc->vmsd = &vmstate_pit_common;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
> +    /*
> +     * Reason: unlike ordinary ISA devices, the PIT may need to be
> +     * wired to the HPET, and because of that, some wiring is always
> +     * done by board code.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo pit_common_type = {
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index f81cf48..8005503 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -750,9 +750,10 @@ static void m48t59_isa_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = m48t59_isa_realize;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->reset = m48t59_reset_isa;
>      dc->props = m48t59_isa_properties;
> +    /* Reason: needs to be wired up by m48t59_init_isa() */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo m48t59_isa_info = {
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 2f58220..17e1907 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -906,9 +906,10 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = rtc_realizefn;
> -    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
>      dc->vmsd = &vmstate_rtc;
>      dc->props = mc146818rtc_properties;
> +    /* Reason: needs to be wired up by rtc_init() */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo mc146818rtc_info = {

Patch

diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index 8e3e178..f980d66 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -192,8 +192,9 @@  static void pcspk_class_initfn(ObjectClass *klass, void *data)
 
     dc->realize = pcspk_realizefn;
     set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->props = pcspk_properties;
+    /* Reason: pointer property "pit", realize sets global pcspk_state */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo pcspk_info = {
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 86f4920..592b58f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2234,7 +2234,6 @@  static void isabus_fdc_class_init(ObjectClass *klass, void *data)
 
     dc->realize = isabus_fdc_realize;
     dc->fw_name = "fdc";
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->reset = fdctrl_external_reset_isa;
     dc->vmsd = &vmstate_isa_fdc;
     dc->props = isa_fdc_properties;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fe33843..20402ba 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -544,10 +544,15 @@  static void port92_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->realize = port92_realizefn;
     dc->reset = port92_reset;
     dc->vmsd = &vmstate_port92_isa;
+    /*
+     * Reason: unlike ordinary ISA devices, this one needs additional
+     * wiring: its A20 output line needs to be wired up by
+     * port92_init().
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo port92_info = {
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index dee31a6..655b8c5 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -522,7 +522,6 @@  static void i8042_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = i8042_realizefn;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->vmsd = &vmstate_kbd_isa;
 }
 
diff --git a/hw/input/vmmouse.c b/hw/input/vmmouse.c
index 600e4a2..6a50533 100644
--- a/hw/input/vmmouse.c
+++ b/hw/input/vmmouse.c
@@ -282,10 +282,11 @@  static void vmmouse_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = vmmouse_realizefn;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->reset = vmmouse_reset;
     dc->vmsd = &vmstate_vmmouse;
     dc->props = vmmouse_properties;
+    /* Reason: pointer property "ps2_mouse" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo vmmouse_info = {
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index 2acdbfe..9d29399 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -135,9 +135,15 @@  static void pic_common_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_pic_common;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->props = pic_properties_common;
     dc->realize = pic_common_realize;
+    /*
+     * Reason: unlike ordinary ISA devices, the PICs need additional
+     * wiring: its IRQ input lines are set up by board code, and the
+     * wiring of the slave to the master is hard-coded in device model
+     * code.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo pic_common_type = {
diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index 94ae6ae..cd5716a 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -162,7 +162,8 @@  static void vmport_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = vmport_realizefn;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
+    /* Reason: realize sets global port_state */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo vmport_info = {
diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index dc2196c..9db5c9d 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -282,7 +282,12 @@  static void pit_common_class_init(ObjectClass *klass, void *data)
 
     dc->realize = pit_common_realize;
     dc->vmsd = &vmstate_pit_common;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
+    /*
+     * Reason: unlike ordinary ISA devices, the PIT may need to be
+     * wired to the HPET, and because of that, some wiring is always
+     * done by board code.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo pit_common_type = {
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index f81cf48..8005503 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -750,9 +750,10 @@  static void m48t59_isa_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = m48t59_isa_realize;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->reset = m48t59_reset_isa;
     dc->props = m48t59_isa_properties;
+    /* Reason: needs to be wired up by m48t59_init_isa() */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo m48t59_isa_info = {
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 2f58220..17e1907 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -906,9 +906,10 @@  static void rtc_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = rtc_realizefn;
-    dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
     dc->vmsd = &vmstate_rtc;
     dc->props = mc146818rtc_properties;
+    /* Reason: needs to be wired up by rtc_init() */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo mc146818rtc_info = {