Re: [Qemu-devel] [PATCH 2/3] pci: split shpc_cleanup and shpc_free

[Matthew Rosato <mjrosato@linux.vnet.ibm.com>]

On 02/13/2015 09:57 AM, Paolo Bonzini wrote:
> object_unparent should not be called until the parent device is going to be
> destroyed.  Only remove the capability and do memory_region_del_subregion
> at unrealize time.  Freeing the data structures is left in shpc_free, to
> be called from the instance_finalize callback.
> 
> shpc_free follows the same coding style that Alex suggested for VFIO
> (i.e. since a test for NULL is requested, clear the field at end).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

> ---
>  hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++++----
>  hw/pci/shpc.c                  | 11 ++++++++++-
>  include/hw/pci/shpc.h          |  1 +
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 252ea5e..36f73e1 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -97,6 +97,11 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
>      pci_bridge_exitfn(dev);
>  }
> 
> +static void pci_bridge_dev_instance_finalize(Object *obj)
> +{
> +    shpc_free(PCI_DEVICE(obj));
> +}
> +
>  static void pci_bridge_dev_write_config(PCIDevice *d,
>                                          uint32_t address, uint32_t val, int len)
>  {
> @@ -154,10 +159,11 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>  }
> 
>  static const TypeInfo pci_bridge_dev_info = {
> -    .name          = TYPE_PCI_BRIDGE_DEV,
> -    .parent        = TYPE_PCI_BRIDGE,
> -    .instance_size = sizeof(PCIBridgeDev),
> -    .class_init = pci_bridge_dev_class_init,
> +    .name              = TYPE_PCI_BRIDGE_DEV,
> +    .parent            = TYPE_PCI_BRIDGE,
> +    .instance_size     = sizeof(PCIBridgeDev),
> +    .class_init        = pci_bridge_dev_class_init,
> +    .instance_finalize = pci_bridge_dev_instance_finalize,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_HOTPLUG_HANDLER },
>          { }
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 27c496e..5fd7f4b 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -663,13 +663,22 @@ void shpc_cleanup(PCIDevice *d, MemoryRegion *bar)
>      SHPCDevice *shpc = d->shpc;
>      d->cap_present &= ~QEMU_PCI_CAP_SHPC;
>      memory_region_del_subregion(bar, &shpc->mmio);
> -    object_unparent(OBJECT(&shpc->mmio));
>      /* TODO: cleanup config space changes? */
> +}
> +
> +void shpc_free(PCIDevice *d)
> +{
> +    SHPCDevice *shpc = d->shpc;
> +    if (!shpc) {
> +        return;
> +    }
> +    object_unparent(OBJECT(&shpc->mmio));
>      g_free(shpc->config);
>      g_free(shpc->cmask);
>      g_free(shpc->wmask);
>      g_free(shpc->w1cmask);
>      g_free(shpc);
> +    d->shpc = NULL;
>  }
> 
>  void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
> index 025bc5b..9bbea39 100644
> --- a/include/hw/pci/shpc.h
> +++ b/include/hw/pci/shpc.h
> @@ -41,6 +41,7 @@ void shpc_reset(PCIDevice *d);
>  int shpc_bar_size(PCIDevice *dev);
>  int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
>  void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
> +void shpc_free(PCIDevice *dev);
>  void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
> 
>