Patchwork [17/38] e1000: use instance_finalize instead of exit

login
register
mail settings
Submitter Paolo Bonzini
Date Sept. 3, 2013, 12:33 p.m.
Message ID <1378211609-16121-18-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/272264/
State New
Headers show

Comments

Paolo Bonzini - Sept. 3, 2013, 12:33 p.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/e1000.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Michael S. Tsirkin - Sept. 17, 2013, 9:27 a.m.
On Tue, Sep 03, 2013 at 02:33:08PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/net/e1000.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index f5ebed4..55fb062 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1310,9 +1310,9 @@ e1000_cleanup(NetClientState *nc)
>  }
>  
>  static void
> -pci_e1000_uninit(PCIDevice *dev)
> +pci_e1000_instance_finalize(Object *obj)
>  {
> -    E1000State *d = E1000(dev);
> +    E1000State *d = E1000(obj);
>  
>      timer_del(d->autoneg_timer);
>      timer_free(d->autoneg_timer);

So this looks wrong.
This cancels timers after pci device has been destroyed,
so meanwhile timers can run and send interrupts.


> @@ -1394,7 +1394,6 @@ static void e1000_class_init(ObjectClass *klass, void *data)
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
>      k->init = pci_e1000_init;
> -    k->exit = pci_e1000_uninit;
>      k->romfile = "efi-e1000.rom";
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = E1000_DEVID;
> @@ -1412,6 +1411,7 @@ static const TypeInfo e1000_info = {
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(E1000State),
>      .class_init    = e1000_class_init,
> +    .instance_finalize = pci_e1000_instance_finalize,
>  };
>  
>  static void e1000_register_types(void)
> -- 
> 1.8.3.1
>
Paolo Bonzini - Sept. 17, 2013, 10:13 a.m.
Il 17/09/2013 11:27, Michael S. Tsirkin ha scritto:
>> >  static void
>> > -pci_e1000_uninit(PCIDevice *dev)
>> > +pci_e1000_instance_finalize(Object *obj)
>> >  {
>> > -    E1000State *d = E1000(dev);
>> > +    E1000State *d = E1000(obj);
>> >  
>> >      timer_del(d->autoneg_timer);
>> >      timer_free(d->autoneg_timer);
> So this looks wrong.
> This cancels timers after pci device has been destroyed,
> so meanwhile timers can run and send interrupts.

There are definitely cases where the timer deals with pending I/O and
has to run after the device has been removed from guest access.  This is
_not_ yet the point of destruction; the connection to the host backend
still exists in particular (it is only dropped by
object_property_del_all, which is called right after instance_finalize).

It should not be a problem for a device to raise an interrupt after
pci_do_unregister_device; it should go nowhere.  If it is passed to the
guest, it's a bug that we have to fix.

Paolo

Patch

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index f5ebed4..55fb062 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1310,9 +1310,9 @@  e1000_cleanup(NetClientState *nc)
 }
 
 static void
-pci_e1000_uninit(PCIDevice *dev)
+pci_e1000_instance_finalize(Object *obj)
 {
-    E1000State *d = E1000(dev);
+    E1000State *d = E1000(obj);
 
     timer_del(d->autoneg_timer);
     timer_free(d->autoneg_timer);
@@ -1394,7 +1394,6 @@  static void e1000_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pci_e1000_init;
-    k->exit = pci_e1000_uninit;
     k->romfile = "efi-e1000.rom";
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = E1000_DEVID;
@@ -1412,6 +1411,7 @@  static const TypeInfo e1000_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(E1000State),
     .class_init    = e1000_class_init,
+    .instance_finalize = pci_e1000_instance_finalize,
 };
 
 static void e1000_register_types(void)